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

KEP 3140: TimeZone support in CronJob #3143

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Jan 14, 2022

/assign @ravisantoshgudimetla @janetkuo
/cc @iterion

/sig apps

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jan 14, 2022
@k8s-ci-robot
Copy link
Contributor

@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:

/assign @ravisantoshgudimetla @janetkuo
/cc @iterion

/sig apps

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 14, 2022
@soltysh soltysh mentioned this pull request Jan 14, 2022
13 tasks
@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jan 14, 2022
@soltysh
Copy link
Contributor Author

soltysh commented Jan 14, 2022

/assign @deads2k


### Goals

- Add the field `.spec.timeZone` which allows specifying a valid TimeZone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a name or an offset from UTC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name - updated

Comment on lines +492 to +496
<!--
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.
-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whichever sense of the .spec.timeZone field we use (name or offset from UTC), let's list the other approach as an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 42 to 44
# The following PRR answers are required at beta release
metrics:
- my_feature_metric
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe comment this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 146 to 147
The value provided in `TimeZone` field will be validated against the embedded
golang timezone database.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for manual action, just follow the general guideline for updating the cluster: https://kubernetes.io/docs/tasks/administer-cluster/cluster-upgrade/

Comment on lines 128 to 131
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:
  1. Does the kube-controller-manager populate the .spec.timeZone once it detects a CronJob with no timezone set?
  2. Should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. no, it uses the default, to maintain backwards compatibility
  2. no, again backwards compatibility

@janetkuo janetkuo changed the title KEP 3140: TimeZone suppot in CronJob KEP 3140: TimeZone support in CronJob Jan 14, 2022
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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to also introduce a way to set a global default TZ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make the field a bit more self-documenting

Suggested change
Add the field `.spec.timeZone` to the CronJob resource. The cronjob controller
Add the field `.spec.timeZoneName` to the CronJob resource. The cronjob controller

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also record the applied offset from UTC (from the last invocation) in .status. Maybe after an alpha though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the other other alternative is timezone details in the cron time spec (the approach that got merged by accident).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can PR that in after this merges.

@soltysh
Copy link
Contributor Author

soltysh commented Jan 31, 2022

Copy link
Contributor

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thank you for working on this @soltysh. This will be really helpful for the community.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2022

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will you test to be sure the cronjob controller correctly handles going from timezone aware to non-timezone aware?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual testing of a cronjob with time zone specified, I can't think of a better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, added that here

Pick one more of these and delete the rest.
-->

- [ ] Metrics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2022
@dims
Copy link
Member

dims commented Feb 1, 2022

@soltysh

@deads2k
Copy link
Contributor

deads2k commented Feb 1, 2022

PRR is complete for alpha.

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2022
@soltysh
Copy link
Contributor Author

soltysh commented Feb 2, 2022

@dims

  • can you please add examples of "timezone name"?

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.

@atiratree
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit ef0923b into kubernetes:master Feb 2, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Feb 2, 2022
@soltysh soltysh deleted the cronjob_timezone branch February 2, 2022 17:27
@dims
Copy link
Member

dims commented Feb 2, 2022

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. If there is a timezone change (eg a new national law), does that trigger a patch release?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach I initially put forward was to have Kubernetes itself only aware of offsets from UTC.

Copy link
Member

@BenTheElder BenTheElder Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants