KEP 3140: TimeZone support in CronJob #3143
Conversation
k8s-ci-robot
added
the
sig/apps
|
@soltysh: GitHub didn't allow me to request PR reviews from the following users: iterion. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
kind/kep
soltysh
force-pushed
the
cronjob_timezone
branch
from
0ff8d1b to
b64b3bf
Compare
|
/assign @deads2k |
|
|
||
| ### Goals | ||
|
|
||
| - Add the field `.spec.timeZone` which allows specifying a valid TimeZone |
There was a problem hiding this comment.
Is this a name or an offset from UTC?
| <!-- | ||
| What other approaches did you consider, and why did you rule them out? These do | ||
| not need to be as detailed as the proposal, but should include enough | ||
| information to express the idea and why it was not acceptable. | ||
| --> |
There was a problem hiding this comment.
Whichever sense of the .spec.timeZone field we use (name or offset from UTC), let's list the other approach as an alternative.
| # The following PRR answers are required at beta release | ||
| metrics: | ||
| - my_feature_metric |
| The value provided in `TimeZone` field will be validated against the embedded | ||
| golang timezone database. |
There was a problem hiding this comment.
Does this mean that changes to time zone databases need to be applied to the API server and kube-controller-manager in a particular order?
There was a problem hiding this comment.
No need for manual action, just follow the general guideline for updating the cluster: https://kubernetes.io/docs/tasks/administer-cluster/cluster-upgrade/
| The `CronJobSpec` structure is expanded with new `TimeZone` field which allows | ||
| specifying the name of the time zone to be used. Missing or empty value of the | ||
| field indicates the current behavior, which relies on the time zone of the | ||
| kube-controller-manager process. |
There was a problem hiding this comment.
| The `CronJobSpec` structure is expanded with new `TimeZone` field which allows | |
| specifying the name of the time zone to be used. Missing or empty value of the | |
| field indicates the current behavior, which relies on the time zone of the | |
| kube-controller-manager process. | |
| The `.spec` for a CronJob is expanded with new `timeZone` field which allows | |
| specifying the name of the time zone to be used. Missing or empty value of the | |
| field indicates the current behavior, which relies on the time zone of the | |
| kube-controller-manager process. | |
| In the API code, that looks like: |
- Does the kube-controller-manager populate the
.spec.timeZoneonce it detects a CronJob with no timezone set? - Should it?
There was a problem hiding this comment.
- no, it uses the default, to maintain backwards compatibility
- no, again backwards compatibility
janetkuo
changed the title
| on the time zone of the kube-controller-manager process which is hard for a | ||
| regular user to figure out. Exposing an explicit field for setting a time zone | ||
| will allow CronJob authors to simplify the user experience when creating CronJobs. | ||
|
|
There was a problem hiding this comment.
Would it make sense to also introduce a way to set a global default TZ?
There was a problem hiding this comment.
Ah, I had an idea about that.
In my view that is best left for an external controller or for an admission webhook. However, the “external” mechanism could still be official Kubernetes code.
There was a problem hiding this comment.
Yes, no global setting, we don't have it anywhere, and I'd leave it to cluster admin to implement, re-use such setting.
|
|
||
| ## Proposal | ||
|
|
||
| Add the field `.spec.timeZone` to the CronJob resource. The cronjob controller |
There was a problem hiding this comment.
We could make the field a bit more self-documenting
| Add the field `.spec.timeZone` to the CronJob resource. The cronjob controller | |
| Add the field `.spec.timeZoneName` to the CronJob resource. The cronjob controller |
There was a problem hiding this comment.
I think using time zone is in most cases sufficient.
|
|
||
| CronJob controller will use non-nil, non-empty value from `TimeZone` field when | ||
| parsing the schedule and during scheduling the next run time. Additionally the | ||
| time zone will be reflected in the `.status.lastSuccessfulTime` and `.status.lastScheduleTime`. |
There was a problem hiding this comment.
We could also record the applied offset from UTC (from the last invocation) in .status. Maybe after an alpha though?
There was a problem hiding this comment.
lastSuccessfulTime is of metav1.Time type which embeds time.Time which already has time zone information embeded in it, so I'm planning to re-use that.
| Why should this KEP _not_ be implemented? | ||
| --> | ||
|
|
||
| ## Alternatives |
There was a problem hiding this comment.
I suppose the other other alternative is timezone details in the cron time spec (the approach that got merged by accident).
There was a problem hiding this comment.
Rarely, we re-use the current fields, that case you're talking about is more a feature creep not intentional on our side, b/c it exposes implementation details, that were not covered in the original design .
There was a problem hiding this comment.
The inline timezone spec - as mentioned in https://web.archive.org/web/20211007074332/https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/ - is not the design we want. I get that. We should nonetheless list it as a ruled-out alternative.
There was a problem hiding this comment.
I can PR that in after this merges.
soltysh
force-pushed
the
cronjob_timezone
branch
from
8ce2edf to
82eb73a
Compare
There was a problem hiding this comment.
/lgtm
Thank you for working on this @soltysh. This will be really helpful for the community.
k8s-ci-robot
added
the
lgtm
|
|
||
| When downgrading from a release with this feature, to a release without `TimeZone` | ||
| there are a few cases: | ||
| 1. If TimeZone feature gate was enabled and user specified a TimeZone, the newly |
There was a problem hiding this comment.
How will you test to be sure the cronjob controller correctly handles going from timezone aware to non-timezone aware?
There was a problem hiding this comment.
Manual testing of a cronjob with time zone specified, I can't think of a better approach.
There was a problem hiding this comment.
I've mentioned that in the test plan, that we want to enabled with the feature on and off to ensure the controller behaves as expected. As much as possible those tests will be automated in integration, but not everything is possible there.
| This section must be completed when targeting beta to a release. | ||
| --> | ||
|
|
||
| ###### How can a rollout or rollback fail? Can it impact already running workloads? |
There was a problem hiding this comment.
An upgrade flow can be vulnerable to the enable, disable, enable if you have a lease that is acquired by a new KCM, then an old KCM, then a new KCM. The unit and integration tests need to be robust.
There was a problem hiding this comment.
ack, added that here
| Pick one more of these and delete the rest. | ||
| --> | ||
|
|
||
| - [ ] Metrics |
There was a problem hiding this comment.
While not required for alpha, can you think of what metrics you'll watch? If you're able to fill this in now, people running the alpha will know what information to look for.
There was a problem hiding this comment.
We have a pre-existing cronjob_job_creation_skew which ensures that the skew between the schedule and the actual creation is minimal, we could look into that, but that won't give you information about if a cronjob has or not time zone set, although in all honesty from that point of view you only care about timely creation of new jobs.
k8s-ci-robot
removed
the
lgtm
|
|
PRR is complete for alpha. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, janetkuo, ravisantoshgudimetla, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Added link to https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
Added that information, even though the size is negligible since according to my test it's less than 500kB. Both are in the last commit. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
thanks @soltysh ! |
| at the time was that introducing such functionality would require cluster operator | ||
| to manually include TimeZone database since golang did not have one. | ||
| Starting from [golang 1.15](https://go.dev/doc/go1.15) `time/tzdata` package can | ||
| be embedded in the binary thus removing the requirement for external database. |
There was a problem hiding this comment.
I think we should explicitly call out in the KEP intent to embed this package as the approach for keeping tzdata up to date?
(Or else, if we have another approach planned, we should outline it?)
There was a problem hiding this comment.
Hmm. If there is a timezone change (eg a new national law), does that trigger a patch release?
There was a problem hiding this comment.
The approach I initially put forward was to have Kubernetes itself only aware of offsets from UTC.
There was a problem hiding this comment.
The implementing PR appears to be taking the import _ "time/tzdata" approach, operators would have the option to patch their deployments with images or mounts containing a non-embedded on-disk tzdata and go will prefer that.
I'm also not sure that the embedded copy will be used at all in official releases / it's not clear how helpful importing this is, all of our images should be shipping with tzdata on-disk in the image. will x-ref the PR.
EDIT: bringing up here kubernetes/kubernetes#108032 (comment)
There was a problem hiding this comment.
I've already responded to that other comment, but I'll leave the justification also here. The intent why we picked this approach, as a safe net was mentioned here in the motivation section. I'll make sure to add some more details during beta reviews to provide more details around expectations and assumptions for the embedded vs the on-system tz database.
|
|
||
| This problem can be mitigated with a fresh build of kube-controller-manager with | ||
| updated golang version, but it heavily relies on the go community to keep the | ||
| time zone database up-to-date. |
There was a problem hiding this comment.
I feel like expectations around this need fleshing out, also as pointed out in this thread kubernetes/kubernetes#108032 (comment) the actual update affecting most users will likely be in the docker images, so we seem to already have confusion around when / where / how this will be updated.
/assign @ravisantoshgudimetla @janetkuo
/cc @iterion
/sig apps