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
Promote CronJobTimeZone to beta #111435
Promote CronJobTimeZone to beta #111435
Conversation
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. |
/lgtm |
/cc @Jefftree |
@@ -3542,7 +3542,7 @@ | |||
"type": "boolean" | |||
}, | |||
"timeZone": { | |||
"description": "The time zone for the given schedule, see https://en.wikipedia.org/wiki/List_of_tz_database_time_zones. If not specified, this will rely on the time zone of the kube-controller-manager process. ALPHA: This field is in alpha and must be enabled via the `CronJobTimeZone` feature gate.", | |||
"description": "The time zone for the given schedule, see https://en.wikipedia.org/wiki/List_of_tz_database_time_zones. If not specified, this will rely on the time zone of the kube-controller-manager process. This is beta field and must be enabled via the `CronJobTimeZone` feature gate.", |
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.
nit: I am not sure if saying must be enabled
isn't too strong considering it is on by default.
examples what we use in other places>
// This is beta field and enabled/disabled by DaemonSetUpdateSurge feature gate.
// This is a beta field and requires enabling GRPCContainerProbe feature gate.
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 copied that from some other place, so I guess we use both interchangeably 😉
// inproperly recognized. | ||
// See https://github.com/golang/go/issues/21512 for more details. | ||
func validateTimeZoneCase(timeZone string) error { | ||
if runtime.GOOS == "darwin" { |
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.
wouldn't it be better to compare the lowercase strings and have a case for darwin in the for loop?
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.
not needed the values are guaranteed by the language https://pkg.go.dev/runtime#pkg-constants
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 meant comparing the timezones by lowercase for darwin
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.
Yeah, but our tests explicitly verify the case correctness, as you see in validation_tests.go
so the whole point is to make sure that the case exactly matches what we have in the database file. If you check https://go.dev/src/time/zoneinfo_unix.go, specifically this fragment:
var zoneSources = []string{
"/usr/share/zoneinfo/",
"/usr/share/lib/zoneinfo/",
"/usr/lib/locale/TZ/",
runtime.GOROOT() + "/lib/time/zoneinfo.zip",
}
you'll notice that the mechanism first checks OS-provided time zone database, which is just a list of directories with files, where the name matches exactly the name of the time zone. The problem is that for MacOS with case insensitive file system America/NewYork
is equal to AMERICA/Newyork
which causes a problem for our validation tests. Thus to make sure the case is exactly we fallback to verifying the golang provided database which is a zip file, thus we can rely on casing for that. I hope that makes more sense now 🤓
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.
Thanks for explanation :)
return allErrs | ||
} | ||
|
||
// validateTimeZoneCase compares the timeZone with golang's builtin database | ||
// to ensure the caseing is correctly recognized. This only applies to MacOS |
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.
maybe I am missing something, but I am a bit confused when I read this description.
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've improved the description a bit.
// systems with case insensitive filesystem, where timezone can be | ||
// inproperly recognized. | ||
// See https://github.com/golang/go/issues/21512 for more details. | ||
func validateTimeZoneCase(timeZone string) error { |
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.
would it make sense to have some unit tests for this function and passing the OS as a parameter?
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.
Seems like an overkill, but I could.
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.
ack, I will let it up to you
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.
Wait, don't all operating systems have the potential to have case insensitive filesystems?
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.
Yeah, after some research it turns out that all can, but linux usually defaults to case sensitive, whereas mac insensitive (https://en.wikipedia.org/wiki/Case_sensitivity#In_filesystems).
1b28d5d
to
e7a4e37
Compare
return allErrs | ||
} | ||
|
||
// validateTimeZoneCase compares the timeZone with golang's builtin database | ||
// to ensure the case of the timeZone matches exactly. This only applies to MacOS |
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.
Sorry for nitpicking, but the following is not still clear to me. I mean This
( from a sentence This only applies to MacOS
) seems to reference ensure the case of the timeZone matches exactly
, but we do the opposite in the function.
// inproperly recognized. | ||
// See https://github.com/golang/go/issues/21512 for more details. | ||
func validateTimeZoneCase(timeZone string) error { | ||
if runtime.GOOS == "darwin" { |
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 meant comparing the timezones by lowercase for darwin
// systems with case insensitive filesystem, where timezone can be | ||
// inproperly recognized. | ||
// See https://github.com/golang/go/issues/21512 for more details. | ||
func validateTimeZoneCase(timeZone string) error { |
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.
ack, I will let it up to you
1697059
to
c9a6cd9
Compare
/priority important-longterm |
@@ -384,9 +386,40 @@ func validateTimeZone(timeZone *string, fldPath *field.Path) field.ErrorList { | |||
allErrs = append(allErrs, field.Invalid(fldPath, timeZone, err.Error())) | |||
} | |||
|
|||
if err := validateTimeZoneCase(*timeZone); err != nil { |
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.
Is the reason we have the MacOS guard because we believe on other systems there may be a valid usecase for having case-insensitive timezone changes?
If all OS's can have case-insensitive filesystems, then this doesn't make sense.
Also, this is validation from the server process... that feels... very hacky, to put that check in here, since we don't typically have OS specific checks in server side code.
This also has the side effect of forcing the server process to only accept timezones in the GOROOT, not the ones in the system. That feels wrong, and I am very uncomfortable having server code having different behavior across OSes.
What is the alternate fix for this that doesn't alter the validation path? Or makes this a "symptom based" detection, not a "IF general environment THEN workaround" detection?
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 to fulfill the requirement we found after merging alpha, see https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/3140-TimeZone-support-in-CronJob/README.md#beta which points to both #109218 which added skip for a case-sensitive TZ on a mac, and golang/go#21512 explaining why golang doesn't care about case in TZ database.
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'm saying the requirement shouldn't be solved like this.
At a first read this code doesn't belong here (it's mac specific, not "insensitive filesystem specific", and it works around a problem by identifying the environment, rather than resolving the problem). If you're on a Mac, and you provide "foo" to CronJob timezone, and the system database says "i've got foo", given how we've defined the field, we must accept that input.
The linked test is wrong - based on the defined behavior of the field, the database could have AMERICA/new_york
in it, which means our validation automatically accepts whatever values are in that file. In effect, we are not validating the locale name, we're consulting a random third party (the host fs) for info that usually, but not always, is consistent.
I don't think this is something that should be fixed in validation. This is instead something we try to verify via our tests - that a third party TZ data entry can be loaded based on the current environment of the process. I.e. create a synthetic file on Mac, try to load it. Create a synthetic timezone DB on Linux, set the process lookup so that time.LoadLocation
checks that, verify we find the arbitrary thing we look for.
/test pull-kubernetes-e2e-gce-alpha-features |
c9a6cd9
to
0f29daf
Compare
@smarterclayton updated per your requests, ptal |
0f29daf
to
e3723b1
Compare
@@ -105,8 +105,15 @@ type CronJobSpec struct { | |||
Schedule string `json:"schedule" protobuf:"bytes,1,opt,name=schedule"` | |||
|
|||
// The time zone for the given schedule, see https://en.wikipedia.org/wiki/List_of_tz_database_time_zones. |
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 tweaked this to suggest The time zone name
(since we're specifying a name, not an offset etc. Minor clarity point.
@@ -882,6 +886,19 @@ func TestValidateCronJob(t *testing.T) { | |||
validPodTemplateSpec := getValidPodTemplateSpecForGenerated(getValidGeneratedSelector()) | |||
validPodTemplateSpec.Labels = map[string]string{} | |||
|
|||
// make sure to revert previous ZONEINFO value | |||
previousTimeZone := os.Getenv("ZONEINFO") | |||
defer os.Setenv("ZONEINFO", previousTimeZone) |
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.
Use https://pkg.go.dev/testing#T.Setenv instead (in go1.17)
provided TZ database and fail on any other values
e3723b1
to
cb8eabe
Compare
Final comments addressed. |
/test pull-kubernetes-e2e-gce-alpha-features |
/approve LGTM, waiting to see green on the tests. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton, 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 |
Edit: I see that you were verifying alpha didn't run cronjob anymore. |
/lgtm Sorry for the delay, was busy writing docs (for once). |
What type of PR is this?
/kind feature
/kind api-change
/sig apps
/area batch
What this PR does / why we need it:
This PR promotes CronJob TimeZone support to Beta.
2nd commit (Add additional checks for MacOS to ensure the time zone case is exactly matched
) resolves the Beta requirements as outlined in https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/3140-TimeZone-support-in-CronJob/README.md#beta2nd commit (
Change validation tests such that they accept valid values from a provided TZ database and fail on any other values
) resolves the Beta requirement as outlined in https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/3140-TimeZone-support-in-CronJob/README.md#beta by ensuring we allow every possible value from the database and nothing which does not exist in it.Special notes for your reviewer:
/assign @atiratree @deejross
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: