feat(cron): minimum interval #13365

Joibel · 2024-07-19T07:26:35Z

A new field in CronWorkflows minimumInterval which specifies a minimum monotonic time between workflow creations.

Motivation

This has two main uses I can think of

allow CronWorkflows which on first glance will run once a day to do so even in a daylight saving observing timezone.
allow "run every interval" workflows which run rapidly on first injection into a cluster and as soon as possible again if interval has passed after downtime of the controller.

This is also possible a generically useful change now that we have schedules which allow for much more complex scheduling.

Modifications

If we attempt to schedule inside minimum interval do not do so.

Verification

New e2e test

Tested both of the next DST changes forward and backward against the CronWorkflows in the documentation.

A new field in CronWorkflows `minimumInterval` which specifies a minimum monotonic time between workflow creations. This has two main uses I can think of * allow CronWorkflows which on first glance will run once a day to do so even in a daylight saving observing timezone. * allow "run every interval" workflows which run rapidly on first injection into a cluster and as soon as possible again if interval has passed after downtime of the controller. This is also possible a generically useful change now that we have `schedules` which allow for much more complex scheduling. If we attempt to schedule inside minimum interval do not do so. New e2e test Signed-off-by: Alan Clucas <[email protected]>

agilgur5

The usefulness, esp in the DST example, makes sense. However, I'm wondering if there is precedent for this in other cron implementations? Since I imagine this has been handled before

Joibel · 2024-07-19T12:34:35Z

I did do some fairly extensive research into alternative golang schedulers, and didn't find any. Quartz (not implemented for go) didn't seem to do it either.

We could adopt/fork a scheduler and add it. But

implementing it either requires persistence (which is what I leverage here) or very complex algorithms in an already hard to implement interface.
I don't think we should do this for maintenance reasons anyway.

I thought about adopting gronx as an alternative cron scheduler. The user would have a choice per workflow. It would be relatively clean to do. But doesn't solve either problem this does.

If kubernetes ever move Cronjobs from robfig/cron we'd be best placed if we were able to follow them.

agilgur5 · 2024-07-19T12:43:34Z

Maybe a different question then, how do people currently work around this issue in existing schedulers? (other than, well, not using a TZ or time affected by DST)

Joibel · 2024-07-19T12:57:55Z

Apart from those, I have no evidence of anyone doing it. It's a hard thing to research outside of cron, but no solutions for cron based scheduling anyway.

agilgur5

A few docs/spec suggestions

agilgur5 · 2024-07-19T12:31:06Z

docs/cron-workflows.md

+| `timezone`                   | Machine timezone       | [IANA Timezone](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones) to run `Workflows`. Example: `America/Los_Angeles`                                                           |
+| `suspend`                    | `false`                | If `true` Workflow scheduling will not occur. Can be set from the CLI, GitOps, or directly                                                                                                 |
+| `concurrencyPolicy`          | `Allow`                | What to do if multiple `Workflows` are scheduled at the same time. `Allow`: allow all, `Replace`: remove all old before scheduling new, `Forbid`: do not allow any new while there are old |
+| `minimumInterval`            | None                   | The minimum interval between executions of this workflow. Examples: `90s`, `10m`, `2h`                                                                                                     |


We use durations in a few places already, but I learned recently that apparently it's k8s convention to not expose them in fields since they are Go specific: https://github.com/kubernetes/community/blob/fb55d44/contributors/devel/sig-architecture/api-conventions.md#units

It also aligns with the startingDeadlineSeconds that's used for CronWorkflows already (and inherits from upstream k8s)

Ah, TIL. Nice to know that.

agilgur5 · 2024-07-19T13:21:40Z

docs/cron-workflows.md

+| Option Name                  | Default Value          | Description                                                                                                                                                                                |
+|:----------------------------:|:----------------------:|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|


I'd prefer if we didn't change the formatting for every row. In #13292 I specifically addressed that and tried to use a formatting that wouldn't require changing every other row if only one description changed

agilgur5 · 2024-07-19T13:38:34Z

docs/cron-workflows.md

+#### Skip forward
+
+You can use `minimumInterval` to schedule once per day, even if the time you want is in a daylight saving skip forward period where it would otherwise be scheduled twice.
+
+An example 02:30:00 schedule
+
+```yaml
+schedules:
+  - 30 2 * * *
+  - 0 3 * * *
+minimumInterval: 60m
+```
+
+The 3:00 run of the schedule will not be scheduled every day of the year except on the day when the clock leaps forward over 2:30.
+In that case the 3:00 run will run.
+
+#### Skip backwards (duplication)
+
+You can use `minimumInterval` to schedule once per day, even if the time you want is in a daylight saving skip backwards period where it would otherwise not be scheduled.
+
+An example 01:30:00 schedule
+
+```yaml
+schedule: 30 1 * * *
+minimumInterval: 120m
+```
+
+This will schedule at the first 01:30 on a skip backwards change.
+The second will not run because of `minimumInterval`.
+


Suggested change

#### Skip forward

You can use `minimumInterval` to schedule once per day, even if the time you want is in a daylight saving skip forward period where it would otherwise be scheduled twice.

An example 02:30:00 schedule

```yaml

schedules:

- 30 2 * * *

- 0 3 * * *

minimumInterval: 60m

```

The 3:00 run of the schedule will not be scheduled every day of the year except on the day when the clock leaps forward over 2:30.

In that case the 3:00 run will run.

#### Skip backwards (duplication)

You can use `minimumInterval` to schedule once per day, even if the time you want is in a daylight saving skip backwards period where it would otherwise not be scheduled.

An example 01:30:00 schedule

```yaml

schedule: 30 1 * * *

minimumInterval: 120m

```

This will schedule at the first 01:30 on a skip backwards change.

The second will not run because of `minimumInterval`.

#### `minimumInterval`

You can set a minimum interval between runs.

For example, if you want to ensure you only run once a day during a DST leap forward:

```yaml

schedules:

- 30 2 * * *

- 0 3 * * *

minimumInterval: 60m

\```

The 3:00 schedule will never run except on the day the clock leaps forward over 2:30.

In that case, only the 3:00 schedule will run and the 2:30 will not.

Another example, if you want to ensure you only run once a day during a DST leap backward:

```yaml

schedule: 30 1 * * *

minimumInterval: 120m

\```

The first run will be at 01:30.

On the day the clock leaps backward, the second will not run because of `minimumInterval`.

I think we can simplify this a bit and combine the two sections as two examples. We can also reduce the verbal descriptions as the examples are a lot clearer than them (I found the initial descriptions to be a bit confusing and verbose. The descriptions afterward are good, though I simplified those as well)

Use consistent language with "runs" vs. "schedule", and "leap" vs "skip"

"leap" has a less technical terminology and is used often for DST (and infrequently for non-DST), so I think works a bit better as specific to DST

I didn't like making this a single document. In the skim reading case you MUST only care about one or the other per cronworkflow, so have kept the two items separate.

I have also kept the description verbose as they are complex to understand I believe it's helpful for users to understand how these work rather than just use them as magic words.

In the skim reading case

I have also kept the description verbose as they are complex to understand I believe it's helpful for users to understand how these work

These two are mutually exclusive scenarios. Either you can skim read it or it's verbose, and therefore you cannot skim read it. To understand how it works, you also would not be skim reading.

In the skim reading case

In the skim reading case, you'd just search "DST" rather than which transition.
But I don't think we should be tailoring docs to skim readers in any case, that's not a good way of building depth, and as per above, you can really only choose one or the other.

I have also kept the description verbose as they are complex to understand I believe it's helpful for users to understand how these work

I don't think they're complex to understand IMO, and the verbosity actually makes it harder to understand. More words is not necessarily a good thing, especially when the words are inconsistent. The style guide says to use simplicity and directness for a reason after all. And exceptions to the style guide lead to bikeshedding ad infinitum.

rather than just use them as magic words.

There's a Wikipedia link provided. We don't define the term "leap", it already exists. The same way we don't define the term "Pod" and rather link to the k8s docs if needed

Followed up with #13723 (comment)

agilgur5 · 2024-07-19T13:44:43Z

docs/cron-workflows.md

+| `timezone`                   | Machine timezone       | [IANA Timezone](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones) to run `Workflows`. Example: `America/Los_Angeles`                                                           |
+| `suspend`                    | `false`                | If `true` Workflow scheduling will not occur. Can be set from the CLI, GitOps, or directly                                                                                                 |
+| `concurrencyPolicy`          | `Allow`                | What to do if multiple `Workflows` are scheduled at the same time. `Allow`: allow all, `Replace`: remove all old before scheduling new, `Forbid`: do not allow any new while there are old |
+| `minimumInterval`            | None                   | The minimum interval between executions of this workflow. Examples: `90s`, `10m`, `2h`                                                                                                     |


Suggested change

| `minimumInterval` | None | The minimum interval between executions of this workflow. Examples: `90s`, `10m`, `2h` |

| `minimumInterval` | None | The minimum time between runs. Examples: `90s`, `10m`, `2h` |

simplify the description and consistently use "runs". Also use "time" (vs "interval") like the field description, which also indicates the units.

agilgur5 · 2024-07-22T02:25:08Z

I'm also wondering if we perhaps want to add some expression "conditional" of some sort. Since minimumInterval and other such features could open a lot of flood gates to supporting things outside of the cron spec/scheduler. We already use expressions in a lot of places and they can be quite flexible so that would open up more possibilities into user-land instead of requiring core features.

Off the top of my head and pure pseudo-code, minimumInterval could be expressed as something like when: schedule[1].nextScheduledTime - schedule[0].nextScheduledTime > 120m.

Oh, and now I'm realizing that that could potentially be done on the Workflow itself already as a workaround is similar to existing ones like #8348, #8347 etc. Hmmm... maybe leaving that with user-land workaround is better than adding this to core...

Joibel · 2024-07-26T12:34:05Z

I think we should support this without starting a workflow. It pushes the decision left, and reduces load considerably. I'm aware of users with tens of thousands of cron workflows, where the overhead of an in workflow decision would be unusable.

I'm also in favour of having well tested patterns for this stuff. It's hard work to test a scheduled workflow, and the cost of having your schedule go wrong can be high. This is why I'd quite like to support gronx as an alternative cron implementation, it would be simpler to get some of the other behaviours you've linked to with that, and more likely to work consistently and be easier to provide support.

I like the idea of this being an expression, I'm sure we can come up with other uses for it. I would instead go for now - cronworkflow.lastScheduledTime >120m as a way of implementing minimumInterval as it has the other properties I was after.

Exposing the schedule would be possible I guess.

Then it comes down to whether this is something we'd consider reliable. I'll experiment with implementing it this way.

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This patch is just the automated fixes excluding the removal of the t.Parallel from `cron_test.go`. Some fixes like ```go assert.True(t, len(pods.Items) > 0, "pod was not created successfully") ``` went to ```go assert.Positive(t, pods.Items, "pod was not created successfully") ``` and I have manually converted these to ```go assert.NotEmpty(t, pods.Items, "pod was not created successfully") ``` Signed-off-by: Alan Clucas <[email protected]>

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This patch is just the automated fixes excluding the removal of the t.Parallel from `cron_test.go`. Some fixes like ```go assert.True(t, len(pods.Items) > 0, "pod was not created successfully") ``` went to ```go assert.Positive(t, pods.Items, "pod was not created successfully") ``` and I have manually converted these to ```go assert.NotEmpty(t, pods.Items, "pod was not created successfully") ``` `test/e2e/agent_test.go` also needed `0`->`time.Duration(0)` for one `assert.Equal`. Signed-off-by: Alan Clucas <[email protected]>

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment) In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <[email protected]>

…ory) This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment) In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <[email protected]>

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment) In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <[email protected]>

…ory) This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment) In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <[email protected]>

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the part of the codebase, as per #13270 (comment) In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <[email protected]>

isubasinghe · 2024-08-13T02:25:32Z

I generally like @agilgur5 's suggestion here.
A when clause or something similar would be excellent.

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the rest of the code base. In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made.

This is part of a series of test tidies started by #13365. The aim is to enable the testifylint golangci-lint checker. This commit converts assert.Error checks into require.Error for the rest of the code base. In some places checks have been coaleced - in particular the pattern ```go if assert.Error() { assert.Contains(..., "message") } ``` is now ```go require.ErrorContains(..., "message") ``` Getting this wrong and missing the Contains is still valid go, so that's a mistake I may have made. Signed-off-by: Alan Clucas <[email protected]>

Joibel · 2024-08-13T12:27:28Z

@isubasinghe is going to implement when clauses for CronWorkflows instead of this.

agilgur5 · 2024-08-13T15:01:39Z

I think we should support this without starting a workflow. It pushes the decision left, and reduces load considerably. I'm aware of users with tens of thousands of cron workflows, where the overhead of an in workflow decision would be unusable.

I don't necessarily disagree with this, but it raises the question of how much we should put into the core if it can be done in user-land. Same reason I've suggested DAG transforms (#12694) and would like to get more plugins supported.
Expressions are broad enough that they can support quite a lot of use-cases, so they are somewhat similar to plugins in that respect though.

I would instead go for now - cronworkflow.lastScheduledTime >120m as a way of implementing minimumInterval as it has the other properties I was after.

Yea that makes more sense, my suggestion was very back of the napkin.

I'm also in favour of having well tested patterns for this stuff. It's hard work to test a scheduled workflow, and the cost of having your schedule go wrong can be high.

Agree

This is why I'd quite like to support gronx as an alternative cron implementation, it would be simpler to get some of the other behaviours you've linked to with that, and more likely to work consistently and be easier to provide support.

I thought about adopting gronx as an alternative cron scheduler. The user would have a choice per workflow. It would be relatively clean to do. But doesn't solve either problem this does.

I hadn't thought about giving an option and running multiple schedulers, but I suppose we could do that too.
Though I do wonder at what point that should be a plugin too -- i.e. "plug-in your favorite cron scheduler". To be fair, most of them are very similar, but have some small but important differences 😕
And I do think defaulting to k8s's cron scheduler helps for consistency too, as you mentioned as well.

Joibel · 2024-08-15T13:57:58Z

#13474 is the replacement PR

Joibel · 2024-10-07T14:29:37Z

#13723 contains the DST skip forward/backwards documentation from this. Comments should be made there from now on please.

Joibel force-pushed the cron-minimum-interval branch from 0143637 to 82f0976 Compare July 19, 2024 07:51

Joibel force-pushed the cron-minimum-interval branch from be1e19f to e432a31 Compare July 19, 2024 10:58

agilgur5 added the area/cron-workflows label Jul 19, 2024

agilgur5 reviewed Jul 19, 2024

View reviewed changes

agilgur5 changed the title ~~feat: minimum interval for CronWorkflows~~ feat(cron): minimum interval Jul 19, 2024

agilgur5 added the area/spec Changes to the workflow specification. label Jul 19, 2024

agilgur5 reviewed Jul 19, 2024

View reviewed changes

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): testifylint automated fixes #13396

Merged

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): assert.Error -> require.Error (cmd/) #13397

Merged

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): assert.Error -> require.Error (pkg/) #13398

Merged

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): assert.Error -> require.Error (other unit tests) #13399

Merged

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): assert.Error -> require.Error (util/) #13400

Merged

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): assert.Error -> require.Error (workflow/controller/) #13401

Merged

Joibel mentioned this pull request Jul 26, 2024

refactor(tests): assert.Error -> require.Error (other workflow/ dirs) #13402

Merged

Joibel mentioned this pull request Aug 12, 2024

refactor(tests): testifylint don't use Equal for floating point tests #13459

Merged

Joibel mentioned this pull request Aug 13, 2024

refactor(tests): assert.Error->require.Error (remainder) #13462

Merged

Joibel closed this Aug 13, 2024

agilgur5 added this to the v3.6.0 milestone Aug 15, 2024

agilgur5 added the solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) label Aug 15, 2024

isubasinghe mentioned this pull request Aug 16, 2024

feat(cron): cronworkflows when clause #13474

Merged

Joibel mentioned this pull request Oct 7, 2024

docs(cron): add when to table examples for DST leap handling #13723

Merged

Joibel deleted the cron-minimum-interval branch October 7, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cron): minimum interval #13365

feat(cron): minimum interval #13365

Joibel commented Jul 19, 2024

agilgur5 left a comment

Joibel commented Jul 19, 2024 •

edited by agilgur5

Loading

agilgur5 commented Jul 19, 2024

Joibel commented Jul 19, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Jul 19, 2024

Joibel Jul 19, 2024

agilgur5 Jul 19, 2024

agilgur5 Jul 19, 2024

Joibel Oct 7, 2024

agilgur5 Oct 7, 2024 •

edited

Loading

agilgur5 Oct 18, 2024

agilgur5 Jul 19, 2024

agilgur5 commented Jul 22, 2024 •

edited

Loading

Joibel commented Jul 26, 2024 •

edited by agilgur5

Loading

isubasinghe commented Aug 13, 2024

Joibel commented Aug 13, 2024

agilgur5 commented Aug 13, 2024 •

edited

Loading

Joibel commented Aug 15, 2024

Joibel commented Oct 7, 2024

		\| Option Name \| Default Value \| Description \|
		\|:----------------------------:\|:----------------------:\|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------\|

	\| `minimumInterval` \| None \| The minimum interval between executions of this workflow. Examples: `90s`, `10m`, `2h` \|
	\| `minimumInterval` \| None \| The minimum time between runs. Examples: `90s`, `10m`, `2h` \|

feat(cron): minimum interval #13365

feat(cron): minimum interval #13365

Conversation

Joibel commented Jul 19, 2024

Motivation

Modifications

Verification

agilgur5 left a comment

Choose a reason for hiding this comment

Joibel commented Jul 19, 2024 • edited by agilgur5 Loading

agilgur5 commented Jul 19, 2024

Joibel commented Jul 19, 2024 • edited by agilgur5 Loading

agilgur5 left a comment • edited Loading

Choose a reason for hiding this comment

agilgur5 Jul 19, 2024

Choose a reason for hiding this comment

Joibel Jul 19, 2024

Choose a reason for hiding this comment

agilgur5 Jul 19, 2024

Choose a reason for hiding this comment

agilgur5 Jul 19, 2024

Choose a reason for hiding this comment

Joibel Oct 7, 2024

Choose a reason for hiding this comment

agilgur5 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

agilgur5 Oct 18, 2024

Choose a reason for hiding this comment

agilgur5 Jul 19, 2024

Choose a reason for hiding this comment

agilgur5 commented Jul 22, 2024 • edited Loading

Joibel commented Jul 26, 2024 • edited by agilgur5 Loading

isubasinghe commented Aug 13, 2024

Joibel commented Aug 13, 2024

agilgur5 commented Aug 13, 2024 • edited Loading

Joibel commented Aug 15, 2024

Joibel commented Oct 7, 2024

Joibel commented Jul 19, 2024 •

edited by agilgur5

Loading

Joibel commented Jul 19, 2024 •

edited by agilgur5

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Oct 7, 2024 •

edited

Loading

agilgur5 commented Jul 22, 2024 •

edited

Loading

Joibel commented Jul 26, 2024 •

edited by agilgur5

Loading

agilgur5 commented Aug 13, 2024 •

edited

Loading