[Controller Manager] Fix job controller hot loop #32785

m1093782566 · 2016-09-15T13:15:39Z

What this PR does / why we need it:

Fix Job controller hotloop on unexpected API server rejections.

Which issue this PR fixes

Related issue is #30629

Special notes for your reviewer:

@deads2k @derekwaynecarr PTAL.

This change is

k8s-ci-robot · 2016-09-15T13:15:40Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "ok to test" on its own line.

Regular contributors should join the org to skip this step.

While we transition away from the Jenkins GitHub PR Builder plugin, "ok to test" commenters will need to be on the admin list defined in this file.

deads2k · 2016-09-15T19:55:28Z

pkg/controller/job/jobcontroller.go

@@ -167,7 +173,7 @@ func (jm *JobController) getPodJob(pod *api.Pod) *batch.Job {
 		return nil
 	}
 	if len(jobs) > 1 {
-		glog.Errorf("user error! more than one job is selecting pods with labels: %+v", pod.Labels)
+		utilruntime.HandleError(fmt.Errorf("user error! more than one job is selecting pods with labels: %+v", pod.Labels))


@soltysh why do we have a message here that only a cluster-admin can see, but only a project-admin can fix?

This is for the manual label selector which will soon be gone when I remove extensions.Job. Backwards compatibility, mainly.

Should I keep the error message here or just remove it?

Keep it for now, extensions.Jobs are not removed, yet.

deads2k · 2016-09-15T19:56:05Z

pkg/controller/job/jobcontroller.go

@@ -271,27 +277,35 @@ func (jm *JobController) enqueueController(obj interface{}) {
 	// all controllers there will still be some replica instability. One way to handle this is
 	// by querying the store for all controllers that this rc overlaps, as well as all
 	// controllers that overlap this rc, and sorting them.
-	jm.queue.Add(key)
+	jm.queue.AddRateLimited(key)


This shouldn't be ratelimited. This is a primary add

Yes, you are right, will fix.

deads2k · 2016-09-15T19:56:51Z

pkg/controller/job/jobcontroller.go

 	obj, exists, err := jm.jobStore.Store.GetByKey(key)
 	if !exists {
 		glog.V(4).Infof("Job has been deleted: %v", key)
 		jm.expectations.DeleteExpectations(key)
 		return nil
 	}
 	if err != nil {
-		glog.Errorf("Unable to retrieve job %v from store: %v", key, err)
-		jm.queue.Add(key)
+		utilruntime.HandleError(fmt.Errorf("Unable to retrieve job %v from store: %v", key, err))


Don't print the error twice. Just return the error from here and let the higher level logic report it.

Okay, fixed.

deads2k · 2016-09-15T19:57:06Z

pkg/controller/job/jobcontroller.go

-		glog.Errorf("Couldn't get key for job %#v: %v", job, err)
-		return err
+		utilruntime.HandleError(fmt.Errorf("Couldn't get key for job %#v: %v", job, err))
+		// We explicitly return nil to avoid re-enqueue the bad key


Good comment

deads2k · 2016-09-15T19:57:12Z

pkg/controller/job/jobcontroller.go

 	}
 	jobNeedsSync := jm.expectations.SatisfiedExpectations(jobKey)
 	selector, _ := unversioned.LabelSelectorAsSelector(job.Spec.Selector)
 	pods, err := jm.podStore.Pods(job.Namespace).List(selector)
 	if err != nil {
-		glog.Errorf("Error getting pods for job %q: %v", key, err)
-		jm.queue.Add(key)
+		utilruntime.HandleError(fmt.Errorf("Error getting pods for job %q: %v", key, err))


Don't print the error twice. Just return the error from here and let the higher level logic report it.

Okay, fixed.

deads2k · 2016-09-15T19:57:43Z

pkg/controller/job/jobcontroller.go

@@ -419,7 +424,7 @@ func (jm *JobController) syncJob(key string) error {
 		job.Status.Failed = failed

 		if err := jm.updateHandler(&job); err != nil {
-			glog.Errorf("Failed to update job %v, requeuing.  Error: %v", job.Name, err)
+			utilruntime.HandleError(fmt.Errorf("Failed to update job %v, requeuing.  Error: %v", job.Name, err))


You should return the error here and let the requeuing be done by the error handling

Yes, but I wonder if key is same with jobKey? Since jm.queue.AddRateLimited(jKey) will enqueue the former.

If the same, why we do controller.keyFunc(&job) here?

key and jobKey look to be the same.

Hmmm... I've noticed we're copying this pattern in most (if not all) our controllers. Seems ok to remove.

@soltysh If you agree, I will remove these lines and simply use key we passed to syncJob(key) to replace jobKey inside the function. Then, I will simply use return err to replace these lines

Go for it. Actually while fixing other controllers look up similar lines and remove them if possible, as well. Thanks!

Okay, will roll back :)

deads2k · 2016-09-19T14:06:17Z

pkg/controller/job/jobcontroller.go

+}
+
+func (jm *JobController) processNextWorkItem() bool {
+	jKey, quit := jm.queue.Get()


deads2k · 2016-09-19T14:06:30Z

nit on name, then lgtm. Go ahead and squash.

derekwaynecarr · 2016-09-19T14:38:00Z

agree with @deads2k comments, thanks for picking this up.

derekwaynecarr · 2016-09-19T14:39:33Z

pkg/controller/job/jobcontroller.go

@@ -40,6 +40,7 @@ import (
 	"k8s.io/kubernetes/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/util/workqueue"
 	"k8s.io/kubernetes/pkg/watch"
+	"fmt"


nit: we group imports. this should be in lines 20-23 range. do you use goimports?

ok, yeah, just this one nit and david's comments, then this is lgtm.

m1093782566 · 2016-09-20T03:26:46Z

@deads2k @derekwaynecarr Comments addressed.

Additional, I modify a UT to pass CI and modify copyright year to 2016. PTAL again.

soltysh · 2016-09-20T09:56:03Z

pkg/controller/endpoint/endpoints_controller.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2014 The Kubernetes Authors.
+Copyright 2016 The Kubernetes Authors.


No need to change the year. That should stay as is.

Hmmm... Okay, but I wonder why?

Hmmm... Okay, but I wonder why?

I think this file was originally authored (under a different name) in 2014.

I'd suspect to reflect the initial time this was committed. We never update those.

Change-Id: I55ce706381f1494e5cd2064177b938f56d9c356a

m1093782566 · 2016-09-20T14:27:08Z

@soltysh comments addressed. PTAL.

Thanks!

soltysh

LGTM, will wait for @derekwaynecarr final signoff.

soltysh · 2016-09-20T15:18:05Z

@k8s-bot ok to test

derekwaynecarr · 2016-09-20T17:06:22Z

LGTM

k8s-github-robot · 2016-09-20T20:12:59Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-09-20T20:52:44Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 15, 2016

m1093782566 mentioned this pull request Sep 15, 2016

Controllers hotloop on unexpected API server rejections #30629

Closed

8 tasks

k8s-github-robot assigned derekwaynecarr Sep 15, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 15, 2016

deads2k self-assigned this Sep 15, 2016

deads2k reviewed Sep 15, 2016

View reviewed changes

deads2k reviewed Sep 19, 2016

View reviewed changes

derekwaynecarr added this to the v1.5 milestone Sep 19, 2016

derekwaynecarr added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Sep 19, 2016

derekwaynecarr suggested changes Sep 19, 2016

View reviewed changes

m1093782566 force-pushed the m109-job-controller-hot-loop branch 3 times, most recently from 1a50c4c to 8b18703 Compare September 20, 2016 03:24

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2016

m1093782566 force-pushed the m109-job-controller-hot-loop branch from 8b18703 to b047496 Compare September 20, 2016 03:31

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 20, 2016

m1093782566 force-pushed the m109-job-controller-hot-loop branch 2 times, most recently from ce39e45 to 9b65522 Compare September 20, 2016 03:58

soltysh requested changes Sep 20, 2016

View reviewed changes

m1093782566 force-pushed the m109-job-controller-hot-loop branch from 9b65522 to 28a0acc Compare September 20, 2016 14:23

fix job controller hot loop

27cc90c

Change-Id: I55ce706381f1494e5cd2064177b938f56d9c356a

m1093782566 force-pushed the m109-job-controller-hot-loop branch from 28a0acc to 27cc90c Compare September 20, 2016 14:25

soltysh approved these changes Sep 20, 2016

View reviewed changes

derekwaynecarr approved these changes Sep 20, 2016

View reviewed changes

derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2016

k8s-github-robot merged commit ad7ba62 into kubernetes:master Sep 20, 2016

m1093782566 deleted the m109-job-controller-hot-loop branch September 21, 2016 00:14

foxish mentioned this pull request Oct 12, 2016

'kubectl delete job <jobname>' hangs for ten minutes, then times out #28977

Closed

[Controller Manager] Fix job controller hot loop #32785

[Controller Manager] Fix job controller hot loop #32785

Conversation

m1093782566 commented Sep 15, 2016 • edited Loading

k8s-ci-robot commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 19, 2016

derekwaynecarr commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 commented Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 commented Sep 20, 2016

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Sep 20, 2016

derekwaynecarr commented Sep 20, 2016

k8s-github-robot commented Sep 20, 2016

k8s-github-robot commented Sep 20, 2016

m1093782566 commented Sep 15, 2016 •

edited

Loading

m1093782566 Sep 16, 2016 •

edited

Loading

m1093782566 Sep 16, 2016 •

edited

Loading

m1093782566 Sep 16, 2016 •

edited

Loading

m1093782566 commented Sep 20, 2016 •

edited

Loading