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

Add timezone support to CronWorkflows #1931

Closed
simster7 opened this issue Jan 10, 2020 · 5 comments · Fixed by #1986
Closed

Add timezone support to CronWorkflows #1931

simster7 opened this issue Jan 10, 2020 · 5 comments · Fixed by #1986
Assignees
Labels
type/feature Feature request

Comments

@simster7
Copy link
Member

simster7 commented Jan 10, 2020

Users have requested timezone support to CronWorkflows

Use Case: Being able to schedule something periodically during, say business hours M-F using simple cron ranges when your business hours stradle UTC midnight. The patterns would be much simpler if you could evaluate them in the context of a specific timezone
That said, it's pretty straightforward schedule a workflow to run every period, but start with a step workflow whose sole job can checks if the time is in a local timezone window and whose result is a conditional for continuing the workflow.
It's not as efficient as being able to evaluate cron expressions in the context of a timezone, and results in a lot of noop workflows, but it works, as a workaround.

Originally posted by @evaneaston in #1758 (comment)

Indeed, we can find plenty of workarounds, but it seems to be cumbersome and moreover, playing with infrastructure that can scale from 0 to X, it is sad to say "Ok let's run the workflow, wait for a node to span (GKE autoscale feature), the first step says that we should not go forward and abort the workflow, wait for the cluster to scale down" = pay extra bucks because CRON does not support timezones.
By the way, I am nowhere near to know how "hard" it would be to implement on CronWorkflows, but I really thought this new feature would come bundled with timezones support and I would be able to drop all those K8S CronJobs

Originally posted by @rdigiorgio in #1758 (comment)

@simster7
Copy link
Member Author

Update: I've come up with a design to add timezone support and will implement it after the upcoming API Server release

@rdigiorgio
Copy link
Contributor

Hey @simster7 , am I right thinking this is quite easy to implement ? Being more and more involved into K8S stuff, I am willing to get my hands dirty with some work using Golang, and why not this feature implementation. What do you think about it ?

If you agree, I would be glad to hear about the design you came up with.

@simster7
Copy link
Member Author

Sure! New contributors are always welcome. The design is simple:

The library we use to dispatch cron events already has in-line timezone support by prepending a CRON_TZ env variable before the schedule, e.g. "CRON_TZ=Asia/Tokyo 0 6 * * *". This technically means that timezone support is actually available now if users would just prepend their schedule string with CRON_TZ. However, since we want to abstract away the library in case the library changes, or we want to use our own mechanism, I suggest we add a timezone field to the CronWorkflow.spec that accepts strings. Then, at the CronWorkflow creation point we could simply prepend the CRON_TZ to the schedule if necessary.

Docs: https://godoc.org/github.com/robfig/cron#hdr-Time_zones

P.S. I suggest starting this after #1882 is merged in, since that also contains some changes to cron (unless you are comfortable dealing with merge conflicts)

@simster7
Copy link
Member Author

@rdigiorgio Sorry, I had to take over this PR since I realized that more involved changes would be needed (such as adding timezone binaries to our images). If you're still interested in contributing feel free to checkout our help wanted and good first issue tags! I'll be adding more items to these soon

@simster7 simster7 assigned simster7 and unassigned rdigiorgio Jan 14, 2020
@rdigiorgio
Copy link
Contributor

Ok @simster7 , I started working on it, but I do not think it will help you much making a PR for you to see the changes as it really was simple

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature Feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants