KEP for Graduating CronJob to GA

[barney-s]

• KEP for graduating CronJob to GA

[k8s-ci-robot]

Hi @barney-s. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[justaugustus]

/ok-to-test
/assign @kow3ns @liggitt

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: barney-s
To complete the pull request process, please assign kow3ns
You can assign the PR to them by writing /assign @kow3ns in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-apps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dims]

@barney-s please see kubernetes/kubernetes#82659 for list of things that needs to be fixed.

[sftim]

CronJobs use the TZ of the controller-manager (master).

The trouble is, someone with just API access has no idea what that TZ is. Sometimes cluster operators too, who run the controller manager in a container with a UTC timezone on a host with a non-UTC timezone.

The main part of my concern I have is people with just API access who want to predict when their job will run. Making the API stable is a promise that the snags are sorted (or sorted enough), so I want to make sure people who are deciding on this can have regard to this particular snag. It's a thing that Kubernetes users often seem to run into.

I'd like to address that challenge before GA, by adding a timezone field to each CronJob.

and then, for the initial implementation, constrain the value for timezone to either:

• ["UTC"]
  • easier to implement, but annoying for people who explicitly want a different timezone for their CronJobs
• any valid entry from https://www.iana.org/time-zones
  • convenient, because it lets you mix CronJobs with a UTC zone with CronJobs that take account of local daylight-savings changes etc.

I can make a separate PR against the KEP if other people feel this is an improvement worth doing.

[barney-s]

The trouble is, someone with just API access has no idea what that TZ is. Sometimes cluster operators too, who run the controller manager in a container with a UTC timezone on a host with a non-UTC timezone.

The main part of my concern I have is people with just API access who want to predict when their job will run. Making the API stable is a promise that the snags are sorted (or sorted enough), so I want to make sure people who are deciding on this can have regard to this particular snag. It's a thing that Kubernetes users often seem to run into.

I'd like to address that challenge before GA, by adding a timezone field to each CronJob.

I see 2 issues.

1. User with just API access don't know the timezone of the master.
2. Clusters across different env have different timezones. There is no known recommendation or conformance test to define timezone for the master. Different distributions may have different behavior.

For 1, Can it be fixed by asking the administrator for the timezone of the master component ?

For 2, TBH i don't know if we have a conformance recommendation for timezone of a master. This causes the same set of cronjobs to behave differently across clusters from different providers.

Iam thinking per CronJob timezone may be an overkill. Instead it should be a cluster level config or a recommendation.

That being said i will reach out to some more folks.

@liggitt - Would you have any thoughts on this ?

[fredsted]

Was asked to comment from a PR over at the main repo. After reading the KEP, my impression that this initiative is mostly geared towards scaling, performance and monitoring, which is of course welcome. However, I'm not seeing initiatives that fixes the "100 missed start times" limit. Therefore I would suggest that the aforementioned PR is added to the list of "Fix applicable open issues" list, or at least that some attention is given to it. It's the only issue I've had with Kubernetes CronJobs so far.

[sftim]

asking the administrator for the timezone of the master component

Question I'd ask is: once this is declared stable, does that make it harder to remove that requirement for an out-of-band conversation?
If it's easy to fix even after the feature is stable, without an impact on cluster operators etc, then it's OK to defer the fix.
On the other hand if a post-stabilization fix is hard then it should land before GA.

[barney-s]

@sftim please recheck. i have added timezone field.

[sftim]

@barney-s thanks, that's addressed my concerns. It means that someone who wants to can write an admission controller to default to UTC or to reject CronJobs that don't specify a whitelisted timezone.

[vllry]

There is a critical problem with the current implementation that I would like to call out.

When determining if a cron should be scheduled, getMissedSchedules() is used to return the most recent missed schedule time (and a count of missed start times, in the form of extra items in the returned slice). https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/cronjob/utils.go#L92

This code uses a 3rd party module that has a Next() function for schedules, but not a Previous(). Rather than looking back at now.Previous(), the code repeatedly calls Next(), until hitting reaching the current time... or hitting a hard cap. I have explored 3 ways to remove the hard cap, ranked in decreasing order of appeal as I understand them:

1. Implement a Previous function, by working around the 3rd party library, or by outright replacing it.
2. Use a binary search with the Next() function, to avoid iterating over every start time in the checked time window. For any subwindow, if there is a start time in the second half of the window, the most recent missed start time is at least at that time.
3. Remove the concept of "which start time" the job is launched on behalf of. If we only need to know that the Cron Job is due for a run (and is within its startingDeadlineSeconds), we can avoid this pattern. This seems the least ideal due to the decrease in clear status.

We are implementing option 2 at Lyft (as it's far easier to test and have fast confidence in than option 1). 100 missed start times is nowhere near long enough to support our production Cron users, given the myriad of ways that code or the CronJob/Kubernetes machinery can break.

[barney-s]

@vllry - Thanks for this feedback. Are you using a custom version of CronJob or a separate controller ? Is that public ?

We are interested in the myriad ways the machinery can break.

[vllry]

We are patching the upstream controller (I can open an open source PR soon if you're interested, but it's not yet deployed/proven internally). We considered writing a new controller but are hoping this effort moves forward soon.

We have ~250 CronJobs at present, which entails hitting (a) a lot of infrequent edge cases, and (b) being extremely susceptible to the performance issues outlined in the KEP and my specific comment.

[vllry]

Suggestion: this may be better suited for a v2. This feels easy to get wrong, as a number of other commenters have suggested. It would be unfortunate to rush a design to v1 while trying to promote the other improvements.

[sftim]

It would be unfortunate to rush a design to v1 while trying to promote the other improvements.

If rushing is a concern, I'd prefer to extend the beta. Add .spec.timezone along with other tidying, and try that out for a release cycle (or more).

[barney-s]

I do see a use case for this. Especially with multi cluster scenarios. Having a schedule with local timezone or UTC (depending on master VM/pod) would be good for most use cases. Having a deterministic schedule may be preferable for others.

But i do agree we need to get some consensus on the priority and when it needs to be done.

[wojtek-t]

I would suggest moving it to a separate KEP. I agree it's potentially controversial, while the overall idea behind this KEP isn't. I don't want to have this KEP stuck for too long (it's already hanging too long).

[wojtek-t]

LGTM from the scalability perspective.

[wojtek-t]

I would suggest moving it to a separate KEP. I agree it's potentially controversial, while the overall idea behind this KEP isn't. I don't want to have this KEP stuck for too long (it's already hanging too long).

[wojtek-t]

Putting my "production readiness review" hat, you will also need to fill in the PRR questionaire:
#1620

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[riking]

Still needs work to chase up loose ends.

[soltysh]

@barney-s are you still working on this one @alaypatel07 and I are working on updating the controller currently, I'd like to take over this KEP and update and finish it, if you don't mind.

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[soltysh]

/remove-lifecycle rotten

[krmayankk]

/assign

[wojtek-t]

Closing in favor of #1996