feat(cron): allow multiple schedules

[eduardodbr]

Fixes: #12493
Fixes: #9734

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"]

[eduardodbr]

CI is failing because of #12596

[tooptoop4]

[terrytangyuan]

Great work!

[terrytangyuan]

LGTM once conflicts are resolved

[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]

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>

[agilgur5]

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

[eduardodbr]

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

[agilgur5]

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]

Suggested change

	<>
	{schedule}
	<br />
	</>
	<p>{schedule}</p>

same as above

[agilgur5]

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]

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