feat: Append job creation timestamp to cronjob annotations #118137
Conversation
|
/ok-to-test |
|
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. |
7bb6b07
to
b981880
Compare
There was a problem hiding this comment.
Some nits and questions around if we want the timestamp from creation or the scheduledtime that gets appended to the job name.
If we want the former, then the test associated with this change is going to fail as the creationTimeStamp of the job and not the scheduledTime passed into the getJobFromTemplate2 function will be different.
|
/lgtm |
@troy0820 Thank you for your review. The annotation will have the original scheduled time, the scheduledTime will be set as the new value for theCreationTimestamp meta, and the user doesn't have a way to get the original/expected scheduled time. |
|
/test pull-kubernetes-e2e-kind-ipv6 |
|
/lgtm |
There was a problem hiding this comment.
Also based on the discussion in #117986 I'm inclined to say that this particular change might also require a KEP, a small one, similar to kubernetes/enhancements#4019 but still would be good to describe the format of the date and ensure this is working as expected.
/hold
Signed-off-by: Heba Elayoty <[email protected]>
| // CronJobScheduledTimestampAnnotation is the scheduled timestamp annotation for the Job. | ||
| // It records the original/expected scheduled timestamp for the running job, represented in RFC3339. | ||
| // The CronJob controller adds this annotation if the CronJobsScheduledAnnotation feature gate (beta in 1.28) is enabled. | ||
| CronJobScheduledTimestampAnnotation = labelPrefix + "cronjob-scheduled-timestamp" |
There was a problem hiding this comment.
this looks good.. open a PR to the dev-1.28 branch of the website to add this doc to https://kubernetes.io/docs/reference/labels-annotations-taints/ as well
There was a problem hiding this comment.
PR is ready already. kubernetes/website#41857
|
/approve /hold @soltysh can lgtm and unhold once that is done |
Signed-off-by: Heba Elayoty <[email protected]>
|
/lgtm |
pkg/controller/cronjob/utils.go
Outdated
| if utilfeature.DefaultFeatureGate.Enabled(features.CronJobsScheduledAnnotation) { | ||
|
|
||
| // Append job creation timestamp to the cronJob annotations. The time will be in RFC3339 form. | ||
| annotations[batchv1.CronJobScheduledTimestampAnnotation] = cj.Spec.JobTemplate.CreationTimestamp.Time.Format(time.RFC3339) |
There was a problem hiding this comment.
This is still wrong, it should be:
annotations[batchv1.CronJobScheduledTimestampAnnotation] = scheduledTime.Format(time.RFC3339).In(timezone) this is because we want to present that annotation in the time that is local to the user, and the information in schedule is transformed to current time zone.
There was a problem hiding this comment.
It's hard to quickly explain, but when we invoke cron.ParseStandard we pass TZ=<time-zone which triggers the library to take into account the timezone when calculating the time, but the date returned from invoking Next on that schedule will give you time relative to your current time zone. In our case that is the time zone local to the kube-controller-manager.
It's best to try it locally with something like this:
sched, _ := cron.ParseStandard("TZ=America/New_York 0 */4 * * *")
next := sched.Next(time.Now())
loc, _ := time.LoadLocation("America/New_York")
fmt.Println("next in TZ", next.In(loc))
fmt.Println("next in local", next)make sure to pick the time zone that is different than your local, the schedule is set to execute every 4h at 0 minutes.
In my case the above will print (it's currently 21:04pm CEST, which is 15:05 EST):
next in TZ 2023-07-06 16:00:00 -0400 EDT
next in local 2023-07-06 22:00:00 +0200 CEST
for the annotation we want to present the user with the time printed in TZ.
There was a problem hiding this comment.
wait... what? we should either make the TZ UTC or the TZ specified in the cronjob, right? the timezone of k-c-m is a weird thing to surface into the API
There was a problem hiding this comment.
the date returned from invoking Next on that schedule will give you time relative to your current time zone
ah, I see, it uses the location of the time passed in to Next()
Signed-off-by: Heba Elayoty <[email protected]>
|
LGTM label has been added. Git tree hash: 4622f664fcee6e781261cca927c8b5398300448e
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: helayoty, 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
CronJobScheduledTimestampAnnotationto the batch API.CronJobsScheduledAnnotation.getJobFromTemplate2func to be able to log the errors.Which issue(s) this PR fixes:
Fixes #108609
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Yes.
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: