Fix Daylight saving transition handling

[notnoop]

This fixes handling of daylight saving transitions. It does so in the following way:

• If the cronexpr isn't satisfied on a day due to a leap forward, the day will be skipped from the periodic job.

  • e.g. a periodic job specifying 30 2 * * * will not run on 2019-03-10 in the US/New_York timezone

• If an hour is "repeated" due to daylight savings backward transition, the job may run twice that day

  • 30 1 * * * will run twice on 2019-11-03 US/New_York - once at 1:30am EDT and then again at 1:30am EST.

If a job sets a cron expression that runs every second/minute/hour, the job will run as expected during the entire time and not miss any period.

Prior to the change, the job may run at somewhat arbitrary time and may cause a job to be running uncontrollably and causing a denial of service.

This approach is relatively simple easy to reason approach. Operators are strongly encouraged to use UTC, or have their nightly jobs run in times that aren't affected by day light saving changes (e.g. 12.30am or 3.30am ET) .

Implementation Notes

I have forked github.com/gorhill/cronexpr in https://github.com/hashicorp/cronexpr . I have updated it so that it is daylight saving aware. The relevant PR is hashicorp/cronexpr#1 . It includes the tests here and more, specifically for US (representing the normal case where DST is hour offset) and Lord Howe Island, Australia (where daylight saving has a 30 minute offset).

Also, this also adds checking if a cron expression is valid. We'd better report if a cron expression is invalid. Previously, we'd ignore the periodic job silently.

Abandoned ideas

I have considered an alternative approach that's closer to how some cron distributions manage daylight saving changes. Namely, running skipped jobs immediately when time leaps forward. Also, considered ensuring that jobs that are expected to run once a day run only once backward transition period.

However, doing so will complicate our periodic job handling - specially considering it's a stateless processor now. Also, deciding whether a job is expected to run only once during the backward transition period isn't trivial and will probably may surprise uses just as much (e.g. how should * */2 * * * be handled? ).

Fixes #7289
Fixes #5410

[shoenig]

Since we own the library now, can we semver it?

[shoenig]

Should we add a note on the time_zone parameter about skipping jobs for times that do not exist?

[notnoop]

Should we add a note on the time_zone parameter about skipping jobs for times that do not exist?

Yeah, I can update the documentation and upgrade guide in a follow up after getting 👍 on this approach.

[shoenig]

FYI one of the conditions of the APLv2 from cronexpr is that we explicitly document the changes made. Since we forked it, we should probably update the changed files with a notice.

You must cause any modified files to carry prominent notices
          stating that You changed the files; and [...]

[schmichael]

• If the cronexpr isn't satisfied on a day due to a leap forward, the day will be skipped from the periodic job.
  • e.g. a periodic job specifying 30 2 * * * will not run on 2019-03-10 in the US/New_York timezone

I'm a little sad the two DST "directions" seem to cause 2 different behaviors: 1 skips, the other duplicates.

That being said I think due to the stateless nature of this code, and the fact that people have to explicitly opt into a timezone with DST, it's a fine tradeoff.

Let's make sure to very explicitly document this behavior both for the time_zone parameter, and on the Upgrade Specific Notes page.

I'm tempted to say we should emit a Job.Validation Warning for any TZ other than UTC, but it seems strange to offer a feature that always emits a warning when used. If it's that big of a footgun we probably shouldn't leave it lying around.

My main concern is that a lot of people probably use this as a convenience to not have to mentally translate their jobspecs to UTC. "I just want this to run nightly at low-load times." is quick and easy to define in your local TZ but will break twice a year.

[notnoop]

Thanks @schmichael . I'll merge this to be in 0.11.2 - and follow up with docs in a separate PR.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.