-
-
Notifications
You must be signed in to change notification settings - Fork 34
Add CRON scheduling functionality to Time Resource #78
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.
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
base: master
Are you sure you want to change the base?
Conversation
This commit implements CRON scheduling as an alternative to interval-based timing in the Time Resource. Key changes include: - Added new `cron` parameter that accepts standard cron expressions - Support for both 5-field (minute, hour, day, month, weekday) and 6-field (with seconds) cron formats - Implemented minimum interval validation to prevent excessive resource usage - Added the gronx library dependency for cron expression parsing - Updated validation to ensure cron and interval configurations are mutually exclusive - Expanded the README with documentation and examples of the new functionality This enhancement gives users more precise control over when builds are triggered, supporting complex scheduling requirements beyond simple intervals. Signed-off-by: Mathias Bogaert <[email protected]>
taylorsilva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about the current implementation not playing out well for users. From reading the code and my manual testing, for crons that trigger at specific date/times (like @yearly) a new version will only be emitted if the check is run within the exact minute specified in the cron. It is impossible to guarantee this level of precision with Concourse's check scheduling logic.
By default, Concourse runs the check for a resource every ~1min. It's not a guaranteed "we will run check every 60s on the dot". You can see this for yourself if you watch a resource's version page. It's quite common for Concourse to overshoot the one minute interval by up to 10-20s, especially on a busy cluster with a lot of resources. Checks can become backlogged and run well past the 1min mark.
A lot of large clusters usually set the default check interval to higher than 1min. Users can always override this with check_every, and based on the current implementation I would recommend setting check_every to 30s. This makes it more likely the check will run every minute, but it's still not 100% guaranteed.
One idea I've had to resolve this would be to have a "trigger window" when using cron. For example, a "5 minute trigger window" would mean the cron would trigger within +/- 5minutes of the cron schedule.
So a daily cron 0 0 * * * would emit a new version between 11:55pm and 12:05am.
You could let users configure this "trigger window", maybe even allow the window to only be forward or backward looking.
Addresses issue where cron expressions like @daily, @Yearly, @5minutes never triggered unless the check ran at the exact cron minute. Root cause: gronx.IsDue() only returns true at the exact scheduled minute, but Concourse checks aren't guaranteed to run with that precision. Changes: time_lord.go: - Check(): Use PrevTickBefore() for first check to find most recent cron time, ensuring initial version is always emitted - Check(): Use Next(previousTime) for subsequent checks to handle late execution (e.g., :31 check for :30 cron still triggers) - Latest(): Use PrevTickBefore() to return the actual cron boundary time, not check time. Also handles multiple missed cron times. - Latest(): Fix to return zero when reference is past stop time for range-based configs without interval - List(): Fix to not include versions when reference is past stop time for range-based configs without interval check_command.go: - Use tl.Latest() for cron version times instead of currentTime, ensuring versions are at cron boundaries (e.g., 3:05pm not 3:07pm) - Handle initial_version with cron using cron boundary time models/models.go: - Fix NextN() to use !next.After(before) instead of next.Before(before) so exact boundary times are included Behavior change: Cron-based resources now emit an initial version immediately when a pipeline is created, using the most recent past cron time. This matches interval behavior and ensures pipelines don't wait until the next cron tick to start. Signed-off-by: Mathias Bogaert <[email protected]>
|
I've addressed your comments @taylorsilva |
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better, but there are still issues with when versions are emitted. From my manual tests, it seems the resource will now emit the last time that would have met the cron schedule, regardless if it's actually close to that scheduled time at all.
This is sometimes fine, like when the cron is set to something that is easily described by interval (every x minutes). But when the cron is set to a specific time, like everyday at 3am, I don't want the resource to emit an initial version of yesterday at 3am. I expect cron to do what you wrote in the README:
initial_version: Optional. When usingstartandstoporcronas a trigger for a job, you will be unable to run the job manually until it reaches the configured time range or cron schedule for the first time (manual runs will work once thetimeresource has produced its first version).
Right now, the resource is emitting a version outside the cron schedule. This is why I previously suggested a "triggering window".
Overall, I think we should caution users about using intervals like that with the cron field. I actually didn't know this, but it seems any "interval-like" cron schedule behaves very differently from interval in this resource.
For example, I was wondering when a cron that runs every 2 days (0 0 */2 * *) would actually trigger, and learned that "every 2 days" to cron actually means "every 2nd day of the month", starting with the 1st of the month, so effectively every odd day of the month. This cron schedule would also trigger every 1st of the month, so it would emit a version for the 31st and 1st of two months, triggering not every 2 days as one might expect. This is fine, it's how cron behaves, but I can see how this would confuse users.
I think we should caution users about this when choosing between cron and interval. In the "every x [minute/hour/day]" cases, interval probably works better for them in the majority of cases. Can we add a section after the source configuration with the following:
### Differences Between `interval` and `cron`
There is a difference between `interval` and `cron` when trying to create
similar schedules. `interval` will trigger regardless of calendar days, while
`cron` will trigger strictly following calendar days. Let's look at an example.
If we want something to run "every 2 days" you can do that in these two ways:
* `interval: 48h` or
* `cron: "0 0 */2 * *"`
When these configurations trigger are very different.
The `interval` configuration will trigger every 48 hours based on when the last
trigger was ran.
The `cron` configuration will trigger every 2 calendar days at midnight. cron
also calculates "every 2 days" to be the 1st of each month and then every 2
days from then. So this cron schedule will trigger on the 1st, 3rd, 5th, 7th,
etc. of every month. This also means if you're in a month with a 31st day, the
resource will emit a version on the 31st and then again on the 1st of the next
month, resulting in a trigger two days in a row.
A similar convention is followed with minutes and hours.
When trying to schedule cron intervals like "every x minute/hour", cron will
actually trigger "every x minute/hour of the hour/day". For example:
* `*/5 * * * *` "Every 5 minutes" is actually "every 5th minute of the hour" (00, 05, 10, 15, etc.)
* `0 */6 * * *` "Every 6 hours" is actually "every 6th hour of the day" (00, 06, 12, 18)
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
|
Fixed both your concerns @taylorsilva |
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Signed-off-by: Mathias Bogaert <[email protected]>
Implement CRON scheduling for Time Resource
Add cron expression support as an alternative to interval-based timing, enabling precise schedule control for complex triggering requirements.
Changes:
cronparameter accepting standard 5-field cron expressions (minute, hour, day, month, weekday)@daily,@hourly,@weekly,@monthly,@yearly,@5minutes,@10minutes,@15minutes,@30minutesL(last day),W(nearest weekday),#(nth weekday)cronandinterval/start/stop/daysconfigurationslocationandstart_aftersupport for cron schedules