Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
soltysh committed Jan 31, 2022
1 parent b64b3bf commit 82eb73a
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 14 deletions.
31 changes: 18 additions & 13 deletions keps/sig-apps/3140-TimeZone-support-in-CronJob/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Items marked with (R) are required *prior to targeting to a milestone / release*
## Summary

CronJob creates Jobs based on the schedule specified by the author, but the Time
Zone used during the creation depens on where kube-controller-manager is running.
Zone used during the creation depends on where kube-controller-manager is running.
This proposal aims to extend CronJob resource with the ability for a user to
define the TimeZone when a Job should be created.

Expand All @@ -76,13 +76,13 @@ at the time was that introducing such functionality would require cluster operat
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.
At that time the majority of the focus was towards [moving CronJob to GA](https://github.com/kubernetes/enhancements/issues/19)
At that time, the majority of the focus was towards [moving CronJob to GA](https://github.com/kubernetes/enhancements/issues/19),
so the effort to support TimeZone was again delayed. Now that we have CronJob
fully GA-ed it's time to satisfy the original request.

### Goals

- Add the field `.spec.timeZone` which allows specifying a valid TimeZone
- Add the field `.spec.timeZone` which allows specifying a valid TimeZone name

### Non-Goals

Expand All @@ -95,14 +95,14 @@ and make progress.

Add the field `.spec.timeZone` to the CronJob resource. The cronjob controller
will take the field into account when scheduling the next Job run. In case the
field is not specified or is empty the controller will maitain the current
behavior which is to rely on the time zone of the kube-controller-manager
field is not specified or is empty, the controller will maintain the current
behavior, which is to rely on the time zone of the kube-controller-manager
process.

### Notes/Constraints/Caveats (Optional)

The current mechanism, which will still be the default behavior heavily relies
on the time zone of the kube-controller-manager process which is hard for a
The current mechanism, which will still be the default behavior, heavily relies
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.

Expand All @@ -125,20 +125,22 @@ CronJobs can be created per user.

### CronJob API

The `CronJobSpec` structure is expanded with new `TimeZone` field which allows
The `.spec` for a CronJob is expanded with a 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:

```golang

type CronJobSpec struct {

// The schedule in Cron format, see https://en.wikipedia.org/wiki/Cron.
Schedule string
// The schedule in Cron format, see https://en.wikipedia.org/wiki/Cron.
Schedule string

// Time zone for the above schedule
TimeZone *string
// Time zone for the above schedule
TimeZone *string

}
```
Expand All @@ -149,7 +151,7 @@ golang timezone database.
### CronJob controller

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
parsing the schedule and during scheduling the next run time. Additionally, the
time zone will be reflected in the `.status.lastSuccessfulTime` and `.status.lastScheduleTime`.
In all other cases the controller will maintain the current behavior.

Expand Down Expand Up @@ -489,6 +491,9 @@ Why should this KEP _not_ be implemented?

## Alternatives

Another approach was to specify time zone as an offset to UTC, but using the
name instead seems more user friendly.

<!--
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
Expand Down
3 changes: 2 additions & 1 deletion keps/sig-apps/3140-TimeZone-support-in-CronJob/kep.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ status: implementable
creation-date: 2022-01-14
reviewers:
- "@ravig"
- "@atiratree"
- "@iterion"
approvers:
- "@janetkuo"
Expand Down Expand Up @@ -41,4 +42,4 @@ disable-supported: true

# The following PRR answers are required at beta release
metrics:
- my_feature_metric
# - my_feature_metric

0 comments on commit 82eb73a

Please sign in to comment.