Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cron): minimum interval #13365

Closed
wants to merge 1 commit into from
Closed

feat(cron): minimum interval #13365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci-build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ jobs:
run: make wait API=${{matrix.test == 'test-api' || matrix.test == 'test-cli' || matrix.test == 'test-java-sdk' || matrix.test == 'test-python-sdk'}}
timeout-minutes: 5
- name: Run tests ${{matrix.test}}
run: make ${{matrix.test}} E2E_SUITE_TIMEOUT=20m STATIC_FILES=false
run: make ${{matrix.test}} E2E_SUITE_TIMEOUT=25m STATIC_FILES=false

# failure debugging below
- name: Failure debug - describe MinIO/MySQL deployment
Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ K3D_CLUSTER_NAME ?= k3s-default # declares which cluster to import to in ca
# -- test options
E2E_WAIT_TIMEOUT ?= 90s # timeout for wait conditions
E2E_PARALLEL ?= 20
E2E_SUITE_TIMEOUT ?= 15m
E2E_SUITE_TIMEOUT ?= 25m
GOTEST ?= go test -v -p 20

# should we build the static files?
Expand Down
4 changes: 4 additions & 0 deletions api/jsonschema/schema.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions cmd/argo/commands/cron/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ func getCronWorkflowGet(cwf *wfv1.CronWorkflow) string {
if cwf.Spec.ConcurrencyPolicy != "" {
out += fmt.Sprintf(fmtStr, "ConcurrencyPolicy:", cwf.Spec.ConcurrencyPolicy)
}
if cwf.Spec.MinimumInterval != "" {
out += fmt.Sprintf(fmtStr, "MinimumInterval:", cwf.Spec.MinimumInterval)
}
if cwf.Status.LastScheduledTime != nil {
out += fmt.Sprintf(fmtStr, "LastScheduledTime:", humanize.Timestamp(cwf.Status.LastScheduledTime.Time))
}
Expand Down
51 changes: 41 additions & 10 deletions docs/cron-workflows.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,17 @@ You can use `CronWorkflow.spec.workflowMetadata` to add `labels` and `annotation

### `CronWorkflow` Options

| Option Name | Default Value | Description |
|:----------------------------:|:----------------------:|-------------|
| `schedule` | None, must be provided | [Cron schedule](#cron-schedule-syntax) to run `Workflows`. Example: `5 4 * * *` |
| `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 |
| `startingDeadlineSeconds` | `0` | Seconds after [the last scheduled time](#crash-recovery) during which a missed `Workflow` will still be run. |
| `successfulJobsHistoryLimit` | `3` | Number of successful `Workflows` to persist |
| `failedJobsHistoryLimit` | `1` | Number of failed `Workflows` to persist |
| `stopStrategy` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on a condition |
| Option Name | Default Value | Description |
|:----------------------------:|:----------------------:|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
Comment on lines +44 to +45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| `schedule` | None, must be provided | [Cron schedule](#cron-schedule-syntax) to run `Workflows`. Example: `5 4 * * *` |
| `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` |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, TIL. Nice to know that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| `startingDeadlineSeconds` | `0` | Seconds after [the last scheduled time](#crash-recovery) during which a missed `Workflow` will still be run. Should be shorter than `minimumInterval` for that to work as expected. |
| `successfulJobsHistoryLimit` | `3` | Number of successful `Workflows` to persist |
| `failedJobsHistoryLimit` | `1` | Number of failed `Workflows` to persist |
| `stopStrategy` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on a condition |

### Cron Schedule Syntax

Expand Down Expand Up @@ -108,6 +109,36 @@ For example, with `timezone: America/Los_Angeles`:
| | 2 | 2020-11-02 02:01:00 -0800 PST |
| | 3 | 2020-11-03 02:01:00 -0800 PST |

#### 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`.

Comment on lines +112 to +141
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@agilgur5 agilgur5 Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followed up with #13723 (comment)

### Automatically Stopping a `CronWorkflow`

> v3.6 and after
Expand Down
1 change: 1 addition & 0 deletions docs/fields.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,7 @@ CronWorkflowSpec is the specification of a CronWorkflow
|:----------:|:----------:|---------------|
|`concurrencyPolicy`|`string`|ConcurrencyPolicy is the K8s-style concurrency policy that will be used|
|`failedJobsHistoryLimit`|`integer`|FailedJobsHistoryLimit is the number of failed jobs to be kept at a time|
|`minimumInterval`|`string`|v3.6 and after: MinimumInterval is the minimum time between runs of this CronWorkflow. Further runs before MinimumInterval has passed will be suppressed.|
|`schedule`|`string`|Schedule is a schedule to run the Workflow in Cron format|
|`schedules`|`Array< string >`|Schedules is a list of schedules to run the Workflow in Cron format|
|`startingDeadlineSeconds`|`integer`|StartingDeadlineSeconds is the K8s-style deadline that will limit the time a CronWorkflow will be run after its original scheduled time if it is missed.|
Expand Down
2 changes: 2 additions & 0 deletions manifests/base/crds/full/argoproj.io_cronworkflows.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/apis/workflow/v1alpha1/cron_workflow_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ type CronWorkflowSpec struct {
StopStrategy *StopStrategy `json:"stopStrategy,omitempty" protobuf:"bytes,10,opt,name=stopStrategy"`
// Schedules is a list of schedules to run the Workflow in Cron format
Schedules []string `json:"schedules,omitempty" protobuf:"bytes,11,opt,name=schedules"`
// v3.6 and after: MinimumInterval is the minimum time between runs of this CronWorkflow.
// Further runs before MinimumInterval has passed will be suppressed.
MinimumInterval string `json:"minimumInterval,omitempty" protobuf:"bytes,12,name=minimumInterval"`
}

// v3.6 and after: StopStrategy defines if the CronWorkflow should stop scheduling based on a condition
Expand Down
Loading
Loading