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.

Sign up for GitHub

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

Jump to bottom

Add cronjob controller v2 #93370

Merged

k8s-ci-robot merged 2 commits into kubernetes:master from alaypatel07:add-new-cronjob-controller

Nov 11, 2020

Contributor

alaypatel07 commented Jul 23, 2020 •

edited by soltysh

Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
We need the new controller because it is built using informers instead of polling. This re-work will help with the scale and performance issue of the old controller.

Special notes for your reviewer:
/assign @soltysh @wojtek-t

Does this PR introduce a user-facing change?:

Users can try the cronjob controller v2 using the feature gate. This will be the default controller in future releases.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP] - https://github.com/kubernetes/enhancements/blob/master/keps/sig-apps/19-Graduate-CronJob-to-Stable/README.md

k8s-ci-robot added do-not-merge/work-in-progress kind/cleanup size/XXL do-not-merge/release-note-label-needed cncf-cla: yes needs-sig needs-priority needs-ok-to-test labels

Contributor

k8s-ci-robot commented Jul 23, 2020

Hi @alaypatel07. 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.

k8s-ci-robot added kind/api-change sig/api-machinery sig/apps triage/needs-information and removed needs-sig labels

k8s-ci-robot requested review from a-robinson and stewart-yu

July 23, 2020 00:42

Contributor Author

alaypatel07 commented Jul 23, 2020

/assign @soltysh

k8s-ci-robot assigned soltysh

alaypatel07 force-pushed the add-new-cronjob-controller branch from 8ac7628 to a9606ea Compare

July 23, 2020 02:03

thehackercat mentioned this pull request

cronjobs do not honor defined schedules #86647

Closed

sftim reviewed

View reviewed changes

Contributor

sftim left a comment

Some early feedback

staging/src/k8s.io/kube-controller-manager/features/kube_controller_manager_features.go Outdated Show resolved Hide resolved

Contributor

fedebongio commented Jul 23, 2020

/assign @cheftako
keeping the label until is not WIP anymore

k8s-ci-robot assigned cheftako

soltysh requested changes

View reviewed changes

Contributor

soltysh left a comment

I did a first pass, I'll look into this more when I'm back from my PTO.

cmd/kube-controller-manager/app/batch.go

    
              	if features.DefaultMutableFeatureGate.Enabled(features.CronjobController2) {

              		cj2c, err := cronjob.NewController2(ctx.InformerFactory.Batch().V1().Jobs(),

              			ctx.InformerFactory.Batch().V1beta1().CronJobs(),

              			ctx.ClientBuilder.ClientOrDie("cronjob-controller"),

Contributor

soltysh Jul 26, 2020

nit: cronjob-controllerv2 to make it explicit in the metrics.

Contributor Author

alaypatel07 Jul 26, 2020

+1, modified

Contributor

soltysh Oct 26, 2020

For other readers, we went with the same name, so that we can re-use the SA for this controller.

Contributor

deads2k Nov 5, 2020

For other readers, we went with the same name, so that we can re-use the SA for this controller.

Please create a new SA and tie the existing rolebinding to it so we can figure out what is being used. This can be a followup (still in 1.120) to avoid making this PR larger.

cmd/kube-controller-manager/app/batch.go Outdated Show resolved Hide resolved

pkg/controller/cronjob/utils.go Outdated Show resolved Hide resolved

pkg/controller/cronjob/cronjob_controller2.go Outdated

    
              		return nil, nil

              	default:

              		// multiple unmet start times, start the last one.

              		klog.V(4).Infof("Multiple unmet start times for %s so only starting last one", nameForLog)

Contributor

soltysh Jul 26, 2020

This won't work as you wish to, since getRecentUnmetScheduleTimes2 returns error when 100+ missed times. You need to react differently or change that function.

staging/src/k8s.io/kube-controller-manager/features/kube_controller_manager_features.go Outdated Show resolved Hide resolved

pkg/controller/cronjob/cronjob_controller2.go Outdated Show resolved Hide resolved

pkg/controller/cronjob/cronjob_controller2.go Outdated Show resolved Hide resolved

pkg/controller/cronjob/cronjob_controller2.go Outdated

    
              		recorder.Eventf(cj, v1.EventTypeWarning, "UnparseableSchedule", "unparseable schedule: %s : %s", cj.Spec.Schedule, err)

              		return nil, nil

              	}

              	times, err := getRecentUnmetScheduleTimes2(*cj, now, sched)

Contributor

soltysh Jul 26, 2020

Reading through this more, I don't think we need the separate method for this calculation.

Contributor Author

alaypatel07 Jul 26, 2020

addressed this, PTAL, thanks

pkg/controller/cronjob/cronjob_controller2.go Outdated

    
              	}

              	times, err := getRecentUnmetScheduleTimes2(*cj, now, sched)

              	switch {

              	case err != nil && len(times) == 0:

Contributor

soltysh Jul 26, 2020

All the error situations in getRecentUnmetScheduleTimes2 are returning error and an empty array, so all errors will pick this condition and not the others. You need to change this switch.

Contributor Author

alaypatel07 Jul 26, 2020

yes, but with getRecentUnmetScheduleTimes2 the only place error is not nil is when too many missed start time occurs, so functionally, I am thinking this should give us desired output

pkg/controller/cronjob/cronjob_controller2.go Outdated Show resolved Hide resolved

alaypatel07 force-pushed the add-new-cronjob-controller branch from 2e936e7 to c6e0314 Compare

July 26, 2020 21:45

alaypatel07 closed this

alaypatel07 reopened this

alaypatel07 force-pushed the add-new-cronjob-controller branch 2 times, most recently from 971d9c7 to e74eea6 Compare

July 26, 2020 21:53

Contributor Author

alaypatel07 commented Jul 26, 2020

/retitle [WIP]: Add cronjob controller v2

Contributor

soltysh commented Nov 10, 2020

/retest

alaypatel07 added 2 commits

November 10, 2020 17:32


          add cronjob_controllerv2.go

8d7dd44


          update violation_exceptions.list and make generated

38bb535

alaypatel07 force-pushed the add-new-cronjob-controller branch from 7f669ff to 38bb535 Compare

November 10, 2020 22:32

soltysh approved these changes

View reviewed changes

Contributor

soltysh left a comment

/lgtm

k8s-ci-robot added the lgtm label

Contributor Author

alaypatel07 commented Nov 11, 2020

/test pull-kubernetes-e2e-gce-alpha-features

alaypatel07 mentioned this pull request

handle slow cronjob lister in cronjob controller v2 and improve memory footprint #96443

Merged

Contributor Author

alaypatel07 commented Nov 11, 2020

/test pull-kubernetes-e2e-gce-ubuntu-containerd

Contributor

deads2k commented Nov 11, 2020

/lgtm
/approve

k8s-ci-robot assigned deads2k

deads2k added this to the v1.20 milestone

Member

liggitt commented Nov 11, 2020

/approve
for config API change

Contributor

k8s-ci-robot commented Nov 11, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alaypatel07, deads2k, 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:

~~api/OWNERS~~ [liggitt]
~~cmd/kube-controller-manager/OWNERS~~ [deads2k,liggitt]
~~hack/OWNERS~~ [deads2k,liggitt,soltysh]
~~pkg/controller/apis/config/OWNERS~~ [deads2k,liggitt,soltysh]
~~pkg/controller/cronjob/OWNERS~~ [deads2k,liggitt,soltysh]
~~pkg/features/OWNERS~~ [deads2k,liggitt,soltysh]
~~staging/src/k8s.io/kube-controller-manager/config/OWNERS~~ [liggitt]
~~test/e2e/apps/OWNERS~~ [deads2k,liggitt,soltysh]

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

k8s-ci-robot added the approved label

Contributor Author

alaypatel07 commented Nov 11, 2020

test/e2e/network/service.go:2587
Nov 11 17:37:26.272: failed to create replication controller with headless service:  in the namespace: services-6960
Unexpected error:
    <*errors.errorString | 0xc001b18c60>: {
        s: "1 containers failed which is more than allowed 0",
    }
    1 containers failed which is more than allowed 0
occurred
test/e2e/network/service.go:2602

seems to be unrelated to the PR

Contributor Author

alaypatel07 commented Nov 11, 2020

/test pull-kubernetes-e2e-gce-ubuntu-containerd

Contributor Author

alaypatel07 commented Nov 11, 2020

/retest

k8s-ci-robot merged commit 667d1c2 into kubernetes:master

github-actions bot mentioned this pull request

Week Ending November 15, 2020 dev-obs/actus#273

Open

2rs2ts mentioned this pull request

Fix CronJob missed start time handling #81557

Closed

github-actions bot mentioned this pull request

Week Ending November 22, 2020 dev-obs/actus#280

Open

tengqm reviewed

View reviewed changes

pkg/features/kube_features.go

+              	// CronJobControllerV2 controls whether the controller manager starts old cronjob
+              	// controller or new one which is implemented with informers and delaying queue
+              	//
+              	// This feature is deprecated, and will be removed in v1.22.

Contributor

tengqm Nov 25, 2020

???

Contributor

soltysh Nov 25, 2020

We'll fix that in follow-ups 😉

This was referenced Nov 26, 2020

Do not use NumReques in job controller to count backoff #64787

Closed

Stopping cluster overnight prevents scheduled jobs from running after cluster startup. #42649

Closed

WIP: Remove the "100 missed start times" CronJob limit #89397

Closed

alaypatel07 mentioned this pull request

REQUEST: New membership for alaypatel07 kubernetes/org#2615

Closed

6 tasks

d-honeybadger mentioned this pull request

Fix cronjob status reconciliation when job template labels change #107997

Merged

b0b0haha commented Mar 14, 2022

Why the v2 cronjob controller can get the fix? It still includes the 100 missed start times limit 'feature' from the v1 controller.
https://github.com/alaypatel07/kubernetes/blob/8d7dd4415e28bded77667ce857e1c58016f9ab3a/pkg/controller/cronjob/utils.go#L194-L214

Contributor

soltysh commented Apr 4, 2022

Why the v2 cronjob controller can get the fix? It still includes the 100 missed start times limit 'feature' from the v1 controller. https://github.com/alaypatel07/kubernetes/blob/8d7dd4415e28bded77667ce857e1c58016f9ab3a/pkg/controller/cronjob/utils.go#L194-L214

@b0b0haha ff you look carefully through

kubernetes/pkg/controller/cronjob/utils.go

Lines 93 to 113 in 92a1d0f

    
           if numberOfMissedSchedules > 100 { 
        
           	// An object might miss several starts. For example, if 
        
           	// controller gets wedged on friday at 5:01pm when everyone has 
        
           	// gone home, and someone comes in on tuesday AM and discovers 
        
           	// the problem and restarts the controller, then all the hourly 
        
           	// jobs, more than 80 of them for one hourly cronJob, should 
        
           	// all start running with no further intervention (if the cronJob 
        
           	// allows concurrency and late starts). 
        
           	// 
        
           	// However, if there is a bug somewhere, or incorrect clock 
        
           	// on controller's server or apiservers (for setting creationTimestamp) 
        
           	// then there could be so many missed start times (it could be off 
        
           	// by decades or more), that it would eat up all the CPU and memory 
        
           	// of this controller. In that case, we want to not try to list 
        
           	// all the missed start times. 
        
           	// 
        
           	// I've somewhat arbitrarily picked 100, as more than 80, 
        
           	// but less than "lots". 
        
           	recorder.Eventf(&cj, corev1.EventTypeWarning, "TooManyMissedTimes", "too many missed start times: %d. Set or decrease .spec.startingDeadlineSeconds or check clock skew", numberOfMissedSchedules) 
        
           	klog.InfoS("too many missed times", "cronjob", klog.KRef(cj.GetNamespace(), cj.GetName()), "missed times", numberOfMissedSchedules) 
        
           }

you'll notice that 100+ missed start times only logs the problem and reports warning event, but still proceeds picking the most recent time to create a job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

praseodym praseodym left review comments

soltysh soltysh approved these changes

tengqm tengqm left review comments

deads2k deads2k left review comments

wojtek-t wojtek-t left review comments

krmayankk krmayankk left review comments

sftim sftim left review comments

a-robinson Awaiting requested review from a-robinson

stewart-yu Awaiting requested review from stewart-yu

Labels

approved area/apiserver area/code-generation area/test cncf-cla: yes kind/api-change kind/cleanup lgtm ok-to-test priority/backlog release-note sig/api-machinery sig/apps sig/auth sig/testing size/XXL triage/needs-information