-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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 for CronJob #108032
KEP 3140: TimeZone support for CronJob #108032
Conversation
@deejross: The label(s) 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. |
Hi @deejross. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @soltysh |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
/ok-to-test |
We triaged this one, feels it's mostly SIG Apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor style issues but mostly ok. Thank you for the pr @deejross. Please address them. You also need to generate proto and open-api spec. Can you run them and let us know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/priority important-longterm
8370313
to
37ccd1f
Compare
37ccd1f
to
a750962
Compare
/retest |
test failure looks related:
|
Yup, it looks like your new test here https://github.com/kubernetes/kubernetes/pull/108032/files#diff-6b48a7bb6b4f570f1f2b46f0ad7e4952f1f8aff5a54b42094a0f1f1bcb216981R244 should expect 1, not 0 since you're passing invalid time zone. |
a750962
to
32ac89e
Compare
CI failures related with |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deejross, liggitt, 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 |
32ac89e
to
d26e6cc
Compare
/lgtm |
@@ -20,6 +20,7 @@ package main | |||
|
|||
import ( | |||
"os" | |||
_ "time/tzdata" // for timeZone support in CronJob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only <500kb, but I want to note that we're likely not using this copy anyhow, since the images we publish will have tzdata in them (and .. pretty much any docker base image short of literal FROM SCRATCH
) and if the binary is run directly on the host (which seems unusual) host environments generally will as well.
Note from package docs:
Package tzdata provides an embedded copy of the timezone database. If this package is imported anywhere in the program, then if the time package cannot find tzdata files on the system, it will use this embedded information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that it will probably import the tzdata from the image, but I think it's also important to spell out dependencies when possible. Also, I've had trouble in the past with Alpine and other minimal images not including that data and ultimately breaking things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that it will probably import the tzdata from the image, but I think it's also important to spell out dependencies when possible.
It's not clear to me that embedded tzdata is a real dependency that we desire, it will increase the size of our images (granted not drastically) for zero gain since all of our base images contain tzdata, even "distroless", AFAICT there is no scenario in the entire Kubernetes organization + subprojects where this data will actually be used.
Also, I've had trouble in the past with Alpine and other minimal images not including that data and ultimately breaking things.
Yes, but we don't ship alpine based images in Kubernetes, and anyone doing their own custom images is liable to break via failing to include any number of packages already (e.g. cacerts) and should probably just install tzdata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also means we have another place where Kubernetes should be trying to keep tzdata patched in the releases we ship, I'm not sure this is desirable vs just using tzdata from the docker images, not to mention [tzdata in the images] being decoupled from go version lifecycle [vs this is not] ... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to get this PR in before the 1.24 freeze, and if we want to remove this import (which I'm fine with if we're confident we don't need it), then I can open a separate PR after this one gets merged to remove it, if that's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if we want to put it in a build-flag gated file so someone can exclude it if they know they don't need it, I wouldn't object to that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct what Jordan wrote above, last time we discussed and rejected further implementation of this feature was because we did not want to force users to install tzdata explicitly. Having this mechanism as a fallback is what allowed us to proceed. This was explicitly laid out in the motivation section of the KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-apps/3140-TimeZone-support-in-CronJob#motivation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further more, since this tzdata is embedded inside golang, every time we patch golang we get appropriate patches with it, so I'm not worried about it, since it does not require separate release cadence. Also, as you mentioned the 500kb barely makes a difference in the size of 100MB+ binaries such as kube-apisever and kube-controller-manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golang patch releases do not necessarily happen when the tzdb updates, and go releases are not supported with new patches after some time so users depending on this will be on their own.
I still think it would be reasonable to require tzdb to run the apiserver binaries we ship anyhow, we don't even test it without running in our images which contain it and we require other reasonable system packages to function.
If we make the expectations clear that this will not receive special patching vs regular golang upgrade cadence, I think it is reasonable though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this deserves a build flag for exclusion fwiw. I just don't want any expectation that outdated bundled tzdata will lead to urgent golang updates + k8s releases and I don't see this spelled out anywhere. If we didn't provide the fallback the stance is clearer that users are responsible for having an updated tzdb in their local install. I also don't think we exercise this fallback in CI FWIW though we can probably trust golang there.
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This adds optional support for time zones to CronJobs
Which issue(s) this PR fixes:
Fixes # https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/3140-TimeZone-support-in-CronJob/README.md
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: