feat: allow multiple schedules in a cron workflow #12616

eduardodbr · 2024-02-04T17:55:21Z

Motivation

This PR introduces a new CronWorkflow spec field Schedules, a list of cron schedules for a workflow to execute. It maintains backward compatibility with Spec.Schedule and adds a validation that ensures that both fields cannot be configured simultaneously.

Modifications

Controller

1 - This implementation tries to maintain the same behavior for the controller and operator. This biggest change was made to the cronFacadewhere a key can hold multiple cron entries instead of only one. This involved changing the signature for the method Load to return an array of cronWfOperationCtx since a key can hold multiple entries:

 func (f *cronFacade) Load(key string) ([]*cronWfOperationCtx, error).

This method doesn't seem to be used anywhere and cronFacede is not exposed so I don't think there is any issue with this change. Let me know if you think otherwise.

2 - The controller is currently using the label cronworkflows.argoproj.io/last-used-schedule to store the current cron expression. This label is only used to check whether the cron expression has been updated since the last time the WF ran, so this PR changes it to be a comma-separated list of the schedules (if applicable). Whenever any of the schedules is updated the label will be changed.

Possible Issues

If two cron expressions overlap (for example the expressions */2 * * * * and */3 * * * * will overlap at minute 6,12,18,etc) only one WF will be executed because the second will be seen as a duplicate submission. I don't think there is an issue with this, but let me know if you think otherwise. This makes it a bit harder to create an E2E test that is deterministic. Because of that, I decided to not add an E2E test for this feature.

CLI

argo cron list was updated to show all schedules:

argo cron get was updated:

UI

NOTE: this is my first time ever doing react /typescript it is expected that the code lacks quality. Please suggest changes.

Cron Workflow list was updated to show all schedules:

Cron Workflow status viewer was also updated:

Cron Spec editor:

Verification

This change can be tested with the following cron workflow:

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  name: test-cron
spec:
  schedules:
  - "*/2 * * * *"
  - "*/3 * * * *"
  concurrencyPolicy: "Allow"
  startingDeadlineSeconds: 0
  workflowSpec:
    entrypoint: whalesay
    templates:
    - name: whalesay
      container:
        image: alpine:3.6
        command: [sh, -c]
        args: ["date; sleep 1"]

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

eduardodbr · 2024-02-04T23:43:46Z

CI is failing because of #12596

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

tooptoop4 · 2024-02-13T10:27:11Z

terrytangyuan

Great work!

cmd/argo/commands/cron/util.go

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

terrytangyuan

LGTM once conflicts are resolved

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

agilgur5

NOTE: this is my first time ever doing react /typescript it is expected that the code lacks quality. Please suggest changes.

This seems to have not gotten a UI SME before merge. It's mostly good, but I left a few comments on improvements below. In particular, the possible unnecessary new line when schedules contains only a single schedule is something to check

agilgur5 · 2024-02-20T06:34:53Z

ui/src/app/cron-workflows/components/cron-workflow-list/cron-workflow-list.tsx

+                                                        <>
+                                                            <PrettySchedule schedule={schedule} />
+                                                            <br />
+                                                        </>


I'm pretty sure that most of these instances can be simplified with a <p> paragraph element instead of a <> fragment + a <br /> line break

Suggested change

<>

<PrettySchedule schedule={schedule} />

<br />

</>

<p><PrettySchedule schedule={schedule} /></p>

Also how does this look when schedules only contains a single schedule? It seems like there may be an unnecessary new line in that case since it is currently treated differently from schedule

I tested this suggestion and it seems that replacing <> <br /> </> with:

<p> <PrettySchedule schedule={schedule} /> </p>

seems to be adding an unnecessary new line after the last schedule, as you can see here:

while the current behavior is shown as:

no new line

Hmm we use <p> elements in the new markdown title & description feature. Might need to add similar CSS here (line-height, display, and vertical-align). That and it might not work properly if one column uses <p> while the other uses <br />.

Try that; if it doesn't work, I can take a look on my side

agilgur5 · 2024-02-20T06:42:32Z

ui/src/app/cron-workflows/components/cron-workflow-list/cron-workflow-list.tsx

@@ -183,3 +199,18 @@ export function CronWorkflowList({match, location, history}: RouteComponentProps
        </Page>
    );
 }
+
+function getCronNextScheduledTime(spec: CronWorkflowSpec): Date {


this naming is quite confusing -- getNextScheduledTime already operates on cron schedules (and uses cron-parser directly and is imported from shared/cron.ts) so adding a function with Cron in it sounds identical.

It looks like you wrote "Cron" because it operates on the CronWorkflowSpec -- "Spec" would be better naming in that case.
Or "Multi" can differentiate it from the other one, since this operates on multiple schedules

Suggested change

function getCronNextScheduledTime(spec: CronWorkflowSpec): Date {

function getSpecNextScheduledTime(spec: CronWorkflowSpec): Date {

This func calculates the next scheduled time for the combination of cron expressions configured in the cron workflow spec, it handles both"schedule" and "schedules". Changing it to getSpecNextScheduledTime seems fine to me.

Yea the word "Cron" was ambiguous in this case, whereas "Spec" is very specific

agilgur5 · 2024-02-20T06:43:19Z

ui/src/models/cron-workflows.ts

@@ -21,6 +21,7 @@ export interface CronWorkflowSpec {
    successfulJobsHistoryLimit?: number;
    failedJobsHistoryLimit?: number;
    timezone?: string;
+    schedules?: string[];


this makes more sense on line 18 right below schedule

agilgur5 · 2024-02-20T06:48:49Z

ui/src/app/cron-workflows/components/cron-workflow-list/cron-workflow-list.tsx

+                                                      <>
+                                                          {schedule}
+                                                          <br />
+                                                      </>


Suggested change

<>

{schedule}

<br />

</>

<p>{schedule}</p>

same as above

agilgur5 · 2024-02-20T06:51:18Z

ui/src/app/cron-workflows/components/cron-workflow-status-viewer.tsx

+                                        <>
+                                            <code>{schedule}</code> <PrettySchedule schedule={schedule} />
+                                            <br />
+                                        </>


Suggested change

<>

<code>{schedule}</code> <PrettySchedule schedule={schedule} />

<br />

</>

<p><code>{schedule}</code> <PrettySchedule schedule={schedule} /><p/>

same as above

agilgur5

Also clients (CLI and UI) should probably have updated labels: Schedule -> Schedules.

agilgur5 · 2024-02-20T06:58:46Z

ui/src/app/cron-workflows/components/cron-workflow-status-viewer.tsx

@@ -19,7 +19,18 @@ export function CronWorkflowStatusViewer({spec, status}: {spec: CronWorkflowSpec
                        title: 'Schedule',


this should probably be plural now, no?

Suggested change

title: 'Schedule',

title: 'Schedules',

I don't have a strong opinion here. It can be a schedule or multiple schedules, depending on the configuration. If you configure schedule instead of multiple schedules it will show a single cron expression:

Schedules: */2 * * * *

which is also weird. Do you think it makes sense to show schedule or schedules based on how may cron expressions were configured?

Yes I considered that being the case. My thought process included:

we might eventually deprecate and remove schedule if we have schedules, to simplify the code / remove tech debt etc.

Argo and k8s already have fields that are plural even if there is only 1 element in the list, for instance: labels, volumes, volumeMounts, sidecars, templates, tasks, steps, nodes, etc

"Schedule(s)" would be an in between option, although it is inconsistent with other 1 element lists, per above

agilgur5 · 2024-02-20T06:59:50Z

ui/src/app/cron-workflows/components/cron-workflow-spec-editior.tsx

@@ -12,8 +12,22 @@ export function CronWorkflowSpecEditor({onChange, spec}: {spec: CronWorkflowSpec
                <div className='row white-box__details-row'>
                    <div className='columns small-3'>Schedule</div>


Same as above on plurality

Suggested change

<div className='columns small-3'>Schedule</div>

<div className='columns small-3'>Schedules</div>

agilgur5 · 2024-02-20T07:00:34Z

ui/src/app/cron-workflows/components/cron-workflow-list/cron-workflow-list.tsx

Line 138 should probably also be Schedules now.

I can't comment on the line since GH does not allow commenting on unchanged lines that are not close to a diff

agilgur5 · 2024-02-20T07:02:59Z

cmd/argo/commands/cron/list.go

Similar on line 69 below, SCHEDULE -> SCHEDULES

agilgur5 · 2024-02-20T07:03:28Z

cmd/argo/commands/cron/get.go

@@ -73,7 +73,7 @@ func getCronWorkflowGet(cwf *wfv1.CronWorkflow) string {
 	out += fmt.Sprintf(fmtStr, "Name:", cwf.ObjectMeta.Name)
 	out += fmt.Sprintf(fmtStr, "Namespace:", cwf.ObjectMeta.Namespace)
 	out += fmt.Sprintf(fmtStr, "Created:", humanize.Timestamp(cwf.ObjectMeta.CreationTimestamp.Time))
-	out += fmt.Sprintf(fmtStr, "Schedule:", cwf.Spec.Schedule)
+	out += fmt.Sprintf(fmtStr, "Schedule:", cwf.Spec.GetScheduleString())


Suggested change

out += fmt.Sprintf(fmtStr, "Schedule:", cwf.Spec.GetScheduleString())

out += fmt.Sprintf(fmtStr, "Schedules:", cwf.Spec.GetScheduleString())

similar to above

agilgur5

The CronWorkflows docs also need updating around multiple schedules, with version specifiers (see #12581 for examples)

agilgur5 · 2024-02-20T07:06:19Z

pkg/apis/workflow/v1alpha1/cron_workflow_types.go

@@ -61,6 +63,8 @@ type CronWorkflowSpec struct {
 	WorkflowMetadata *metav1.ObjectMeta `json:"workflowMetadata,omitempty" protobuf:"bytes,9,opt,name=workflowMeta"`
 	// StopStrategy defines if the cron workflow will stop being triggered once a certain condition has been reached, involving a number of runs of the workflow
 	StopStrategy *StopStrategy `json:"stopStrategy,omitempty" protobuf:"bytes,10,opt,name=stopStrategy"`
+	// Schedules is a list of schedules to run the Workflow in Cron format


Suggested change

// Schedules is a list of schedules to run the Workflow in Cron format

// v3.6 and after: Schedules is a list of schedules to run the Workflow in Cron format

This should ideally have a version specifier

eduardodbr added 6 commits February 4, 2024 17:11

feat: allow multiple schedules in a cron workflow

ab4753f

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

feat: merge master

f8f8c4b

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

fix: unit test

f641eb1

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

fix: e2e CronTest

6154eca

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

test: add CLI e2e test with multiple schedules

c51e877

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

fix: blank commit to retrigger ci

c510e54

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

eduardodbr marked this pull request as ready for review February 4, 2024 23:43

eduardodbr requested review from terrytangyuan and sarabala1979 as code owners February 4, 2024 23:43

eduardodbr added area/cron-workflows area/cli The `argo` CLI area/ui labels Feb 5, 2024

chore: merge master

179ffd1

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

isubasinghe self-assigned this Feb 8, 2024

tooptoop4 approved these changes Feb 13, 2024

View reviewed changes

terrytangyuan reviewed Feb 14, 2024

View reviewed changes

cmd/argo/commands/cron/util.go Show resolved Hide resolved

test: add test for GetNextRuntime with multiple schedules

1e16721

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

terrytangyuan approved these changes Feb 18, 2024

View reviewed changes

chore: merge master

156967d

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>

terrytangyuan merged commit 986b069 into argoproj:main Feb 19, 2024
27 checks passed

agilgur5 mentioned this pull request Feb 20, 2024

refactor(ui): flatten directory structures #12539

Merged

agilgur5 reviewed Feb 20, 2024

View reviewed changes

agilgur5 mentioned this pull request Feb 25, 2024

feat: add stopStrategy to cron workflows #12305

Merged

agilgur5 added this to the v3.6.0 milestone Apr 12, 2024

eduardodbr mentioned this pull request Jul 2, 2024

docs: full copy-edit of CronWorkflows page #13292

Merged

Joibel mentioned this pull request Jul 18, 2024

fix(cron): schedules with timezone #13363

Open

eduardodbr mentioned this pull request Jul 19, 2024

fix(cli): argo cron list and get to show schedules without timezone #13369

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow multiple schedules in a cron workflow #12616

feat: allow multiple schedules in a cron workflow #12616

eduardodbr commented Feb 4, 2024 •

edited by agilgur5

Loading

eduardodbr commented Feb 4, 2024

tooptoop4 commented Feb 13, 2024

terrytangyuan left a comment

terrytangyuan left a comment

agilgur5 left a comment •

edited

Loading

agilgur5 Feb 20, 2024

agilgur5 Feb 20, 2024

eduardodbr Feb 23, 2024 •

edited by agilgur5

Loading

agilgur5 Feb 25, 2024 •

edited

Loading

agilgur5 Feb 20, 2024

eduardodbr Feb 23, 2024

agilgur5 Feb 25, 2024

agilgur5 Feb 20, 2024

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 left a comment

agilgur5 Feb 20, 2024

eduardodbr Feb 23, 2024

agilgur5 Feb 25, 2024

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Feb 20, 2024

	function getCronNextScheduledTime(spec: CronWorkflowSpec): Date {
	function getSpecNextScheduledTime(spec: CronWorkflowSpec): Date {

		@@ -19,7 +19,18 @@ export function CronWorkflowStatusViewer({spec, status}: {spec: CronWorkflowSpec
		title: 'Schedule',

		@@ -12,8 +12,22 @@ export function CronWorkflowSpecEditor({onChange, spec}: {spec: CronWorkflowSpec
		<div className='row white-box__details-row'>
		<div className='columns small-3'>Schedule</div>

	out += fmt.Sprintf(fmtStr, "Schedule:", cwf.Spec.GetScheduleString())
	out += fmt.Sprintf(fmtStr, "Schedules:", cwf.Spec.GetScheduleString())

	// Schedules is a list of schedules to run the Workflow in Cron format
	// v3.6 and after: Schedules is a list of schedules to run the Workflow in Cron format

feat: allow multiple schedules in a cron workflow #12616

feat: allow multiple schedules in a cron workflow #12616

Conversation

eduardodbr commented Feb 4, 2024 • edited by agilgur5 Loading

Motivation

Modifications

Controller

Possible Issues

CLI

UI

Verification

eduardodbr commented Feb 4, 2024

tooptoop4 commented Feb 13, 2024

terrytangyuan left a comment

Choose a reason for hiding this comment

terrytangyuan left a comment

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardodbr Feb 23, 2024 • edited by agilgur5 Loading

Choose a reason for hiding this comment

agilgur5 Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eduardodbr commented Feb 4, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

eduardodbr Feb 23, 2024 •

edited by agilgur5

Loading

agilgur5 Feb 25, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 Feb 20, 2024 •

edited

Loading

agilgur5 left a comment •

edited

Loading