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.

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

jobs,backup,vendor: switch from gorhill/cronexpr to robfig/cron #74881

Merged
merged 1 commit into from
Jan 16, 2022

Conversation

dt
Copy link
Member

@dt dt commented Jan 14, 2022

gorhill/cronexpr is abandoned, and we recently found a known panic that was
crashing nodes that upstream considered known behavior when an invalid expression
was provided. While robfig/cron is a full scheduled job execution framework, we can
use its parser separately as well, which is what this change does. When presented with
the same malformed expression as before, CREATE SCHEDULE now returns an error.

Release note (bug fix): certain malformed backup schedule expressions no longer cause the node to crash

@dt dt requested review from adityamaru, miretskiy and a team January 14, 2022 23:39
@dt dt requested a review from a team as a code owner January 14, 2022 23:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt force-pushed the cron branch 2 times, most recently from d10239f to b7b6696 Compare January 15, 2022 03:52
@dt
Copy link
Member Author

dt commented Jan 15, 2022

Updated release note to identify this as a backward-incompatible change, since it eliminates support for two non-standard expression forms (those with 6 or 7 field, to specify seconds and years).

@shermanCRL
Copy link
Contributor

Updated release note to identify this as a backward-incompatible change, since it eliminates support for two non-standard expression forms (those with 6 or 7 field, to specify seconds and years).

Hmm, think we should have a fallback to the old path for existing cron jobs that may be using those expressions? Seems like this could be a surprise after upgrade.

@dt
Copy link
Member Author

dt commented Jan 15, 2022

I checked our docs on schedule expressions and they just link to Wikipedia for what's supported. The formats the article describes as standard are supported by both the old and new libraries; Seconds and specific years were never standard. So I don't think we're regressing in documented functionality but I just wanted to call out the change explicitly all the same. Either way I'm not sure that the change confined to those undocumented and non-standard expressions is enough to keep using a library that is abandoned, unmaintained and has severe bugs.

@shermanCRL
Copy link
Contributor

OK, let’s make sure it ends up under “breaking changes” in release notes nonetheless. We’ve had issues in the past which were correctness improvements, but also breaking changes, and ended up blocking a customer upgrade.

gorhill/cronexpr is abandoned, and we recently found a known panic that was
crashing nodes that upstream considered known behavior when an invalid expression
was provided. While robfig/cron is a full scheduled job execution framework, we can
use its parser separately as well, which is what this change does.

Release note (bug fix): Certain malformed backup schedule expressions no longer cause the node to crash.
Release note (backward-incompatible change): Non-standard cron expressions that specify seconds or year fields are no longer supported.
@dt
Copy link
Member Author

dt commented Jan 16, 2022

ends up under “breaking changes”

Yep, that's what the Release note (backward-incompatible change) is there for.

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 16, 2022

Build succeeded:

@craig craig bot merged commit 4b41789 into cockroachdb:master Jan 16, 2022
@shermanCRL
Copy link
Contributor

Thank you @robfig.

@dt dt deleted the cron branch January 16, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants