fix replica set hot loop #33092

m1093782566 · 2016-09-20T14:16:45Z

What this PR does / why we need it:

Fix replicas set hot loop.

Related issue: #30629

This change is

k8s-ci-robot · 2016-09-20T14:16:46Z

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot 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.

m1093782566 · 2016-09-20T14:19:16Z

@deads2k @derekwaynecarr PTAL.

Thanks!

mikedanese

Thanks, this looks good. Some small comments.

mikedanese · 2016-09-20T14:38:48Z

pkg/controller/replicaset/replica_set.go

-				glog.Errorf("Error syncing ReplicaSet: %v", err)
-			}
-		}()
+	for rsc.processNextWorkItem() {


wait.Forever(rsc.processNextWorkItem, 0)

or

for { rsc.processNextWorkItem() }

might read better

Hmm...

for rsc.processNextWorkItem() { }

is not a dead loop. Since processNextWorkItem() may return false and will end the loop.

ah ok, thanks for pointing that out.

mikedanese · 2016-09-20T14:43:14Z

pkg/controller/replicaset/replica_set.go

+		return true
+	}
+
+	utilruntime.HandleError(fmt.Errorf("Sync %v failed with %v", key, err))


use '%q' for key

Okay, will fix.

mikedanese · 2016-09-20T14:43:54Z

pkg/controller/replicaset/replica_set.go

+		if err != nil {
+			return err
+		}
+		default:


This looks not formated?

Okay, will go fmt code

mikedanese · 2016-09-20T14:46:44Z

pkg/controller/replicaset/replica_set.go

 	}
-	return nil
+	// return manageReplicasErr


please delete commented out code

Okay, will do.

m1093782566 · 2016-09-21T02:59:11Z

@mikedanese comments addressed, PTAL.

Thanks!

0xmichalis · 2016-09-21T11:46:35Z

pkg/controller/replicaset/replica_set.go

 		rsc.expectations.DeleteExpectations(key)
 		return nil
 	}
 	if err != nil {
-		glog.Infof("Unable to retrieve ReplicaSet %v from store: %v", key, err)
-		rsc.queue.Add(key)
+		glog.V(4).Infof("Unable to retrieve ReplicaSet %v from store: %v", key, err)


This error will be logged anyway in processNextWorkItem

Ah, yes, will fix.

0xmichalis · 2016-09-21T11:49:32Z

pkg/controller/replicaset/replica_set.go

+	utilruntime.HandleError(fmt.Errorf("Sync %q failed with %v", key, err))
+	rsc.queue.AddRateLimited(key)
+
+	return true
 }

 // manageReplicas checks and updates replicas for the given ReplicaSet.
 // Does NOT modify <filteredPods>.


It's probably worth mentioning that we will requeue the replica set now in case of an error while creating / deleting pods. Seems that we didn't do it prior to this change.

Hmm, I agree with you and I will add some lines comments.

m1093782566 · 2016-09-22T00:20:18Z

@Kargakis comments addressed. PTAL.

0xmichalis · 2016-09-22T09:54:55Z

LGTM, pending a review from @mikedanese

m1093782566 · 2016-09-24T00:50:24Z

pkg/controller/replicaset/replica_set_test.go:1157: possible formatting directive in Fatal call

exit status 1

Makefile:99: recipe for target 'verify' failed

make: *** [verify] Error 1

k8s-ci-robot · 2016-09-24T01:11:53Z

Jenkins GCI Kubemark GCE e2e failed for commit fc0266b. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark gci e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

m1093782566 · 2016-09-24T01:46:58Z

go run hack/e2e.go -v -test --test_args='--ginkgo.focus=Up$'

error running up: exit status 1 from junit_runner.xml

bprashanth · 2016-09-24T01:54:28Z

please verify that replication controller also doesn't suffer from the same issue (I haven't opened the pr, just noticed that files touched didn't include replication_conroller.go)

m1093782566 · 2016-09-24T02:55:20Z

please verify that replication controller also doesn't suffer from the same issue

I think @deads2k has fixed replication controller hot loop?

0xmichalis · 2016-09-24T19:50:00Z

yes

On Sat, Sep 24, 2016 at 4:55 AM, DuJun notifications@github.com wrote:

please verify that replication controller also doesn't suffer from the
same issue

I think @deads2k https://github.com/deads2k has fixed replication
controller hot loop?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33092 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADuFfypeyVZT-KznU5txrB-J8tqEfgUbks5qtJEygaJpZM4KBrjv
.

m1093782566 · 2016-09-25T07:21:02Z

kubernetes-pull-build-test-gci-kubemark-e2e-gce #20

Recent runs
PR m1093782566: fix replica set hot loop
Result FAILURE
Started 2016-09-24 08:57 CST
Elapsed 14m30s
Version v1.5.0-alpha.0.1316+971567d5f6a819
Builder agent-pr-24

@wojtek-t kubemark test failed(Perhaps it's no my fault), please take a look.

m1093782566 · 2016-09-28T01:11:20Z

CI failed by flake issue #33459

Can anyone please ping rebot?

bprashanth · 2016-09-28T01:14:12Z

oh

WARNING: You are creating a legacy network. Using --mode=legacy will be required in future releases.
ERROR: (gcloud.compute.networks.create) Some requests did not succeed:
 - Quota 'NETWORKS' exceeded. Limit: 5.0

I guess we're leaking networks? @kubernetes/goog-testing (networks is a rare leak, I'll clean them up but the source is unknown)

bprashanth · 2016-09-28T01:15:44Z

@k8s-bot test this github issue: #IGNORE

bprashanth · 2016-09-28T02:04:17Z

Ah, this failure looks more familiar: #33617
I think today's just a bad day for flakes. Going to wait a bit before kicking it. If someone doesn't get to debuggin the issue, I probably will at some point, and then we can restart the test.

0xmichalis · 2016-09-29T16:09:58Z

@bprashanth can we move forward with merging the PR since the change seems unrelated to the flake? I would like to have this merged and add Conditions on top for both controllers.

bprashanth · 2016-09-29T16:47:01Z

@k8s-bot test this github issue: #33617

k8s-ci-robot · 2016-09-29T17:31:49Z

Jenkins GKE smoke e2e failed for commit fc0266b. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

0xmichalis · 2016-10-02T22:16:04Z

@k8s-bot test this issue: #33388

0xmichalis · 2016-10-03T09:34:26Z

@k8s-bot gci test this issue: #33388

0xmichalis · 2016-10-03T09:35:44Z

How can we restart the Jenkins GCI Kubemark GCE e2e suite?

0xmichalis · 2016-10-03T09:46:06Z

pkg/controller/replicaset/replica_set.go

 	}
-	return nil
+	return utilerrors.NewAggregate(errlist)


The RC controller is not aggregating errors but instead it will return them separately: there are only two of them anyway, one error representing a replica create/delete failure and one about a failed RC status update. In the first case it will retry forever albeit being rate-limited and in the second case it will only retry once more. There is no reason for not having both controllers being consistent as far as error handling is concerned. I am not sure though if the RC controller is 100% correct.

Actually there is no difference on a second look I had. Still I would prefer to follow the same pattern as the RC controller and not aggregate here.

Thanks for your advice, I will have a deep look at RC controller.

It should be an easy fix: you basically don't aggregate here and return just the manageReplicas error. In the if clause above you return the status update error instead of appending to the list.

Change-Id: I5176eb9350324de8e7b2035686c4e2c2abc5ef3d

m1093782566 · 2016-10-08T08:01:44Z

@Kargakis

Sorry for delay, comments addressed. PTAL.

Thanks!

k8s-ci-robot · 2016-10-08T19:18:23Z

Jenkins GCI GCE e2e failed for commit a76a743. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-10-08T19:25:23Z

Jenkins Kubemark GCE e2e failed for commit a76a743. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-10-08T20:12:49Z

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

k8s-github-robot · 2016-10-08T20:51:34Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 20, 2016

m1093782566 mentioned this pull request Sep 20, 2016

Controllers hotloop on unexpected API server rejections #30629

Closed

8 tasks

k8s-github-robot assigned mikedanese Sep 20, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 20, 2016

mikedanese suggested changes Sep 20, 2016

View reviewed changes

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

m1093782566 force-pushed the m109-fix-rs-hostloop branch 2 times, most recently from 1d063d7 to e80ef7b Compare September 21, 2016 02:58

0xmichalis reviewed Sep 21, 2016

View reviewed changes

m1093782566 force-pushed the m109-fix-rs-hostloop branch from e80ef7b to 6f1cce7 Compare September 22, 2016 00:18

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

mikedanese approved these changes Sep 22, 2016

View reviewed changes

k8s-github-robot mentioned this pull request Sep 22, 2016

[k8s.io] Kubectl client [k8s.io] Kubectl taint should update the taint on a node {Kubernetes e2e suite} #29503

Closed

m1093782566 force-pushed the m109-fix-rs-hostloop branch from 6f1cce7 to fc0266b Compare September 24, 2016 00:56

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2016

k8s-github-robot mentioned this pull request Sep 28, 2016

[k8s.io] Downward API volume should update annotations on modification [Conformance] {E2eNode Suite} #33434

Closed

m1093782566 mentioned this pull request Sep 29, 2016

fix disruption controller hotloop #32850

Merged

0xmichalis reviewed Oct 3, 2016

View reviewed changes

0xmichalis mentioned this pull request Oct 3, 2016

Replica set conditions API #33905

Merged

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

m1093782566 force-pushed the m109-fix-rs-hostloop branch from fc0266b to 41ecaaf Compare October 8, 2016 07:52

fix replica set hot loop

a76a743

Change-Id: I5176eb9350324de8e7b2035686c4e2c2abc5ef3d

m1093782566 force-pushed the m109-fix-rs-hostloop branch from 41ecaaf to a76a743 Compare October 8, 2016 08:00

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

0xmichalis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2016

k8s-github-robot merged commit e10f122 into kubernetes:master Oct 8, 2016

m1093782566 deleted the m109-fix-rs-hostloop branch October 17, 2016 03:27

fix replica set hot loop #33092

fix replica set hot loop #33092

Conversation

m1093782566 commented Sep 20, 2016 • edited Loading

k8s-ci-robot commented Sep 20, 2016

m1093782566 commented Sep 20, 2016

mikedanese left a 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

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 21, 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

m1093782566 commented Sep 22, 2016

0xmichalis commented Sep 22, 2016

m1093782566 commented Sep 24, 2016 • edited Loading

k8s-ci-robot commented Sep 24, 2016

m1093782566 commented Sep 24, 2016 • edited Loading

bprashanth commented Sep 24, 2016

m1093782566 commented Sep 24, 2016

0xmichalis commented Sep 24, 2016

m1093782566 commented Sep 25, 2016 • edited Loading

m1093782566 commented Sep 28, 2016

bprashanth commented Sep 28, 2016

bprashanth commented Sep 28, 2016

bprashanth commented Sep 28, 2016

0xmichalis commented Sep 29, 2016

bprashanth commented Sep 29, 2016

k8s-ci-robot commented Sep 29, 2016

0xmichalis commented Oct 2, 2016

0xmichalis commented Oct 3, 2016

0xmichalis commented Oct 3, 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

m1093782566 commented Oct 8, 2016 • edited Loading

k8s-ci-robot commented Oct 8, 2016

k8s-ci-robot commented Oct 8, 2016

k8s-github-robot commented Oct 8, 2016

k8s-github-robot commented Oct 8, 2016

m1093782566 commented Sep 20, 2016 •

edited

Loading

m1093782566 commented Sep 24, 2016 •

edited

Loading

m1093782566 commented Sep 24, 2016 •

edited

Loading

m1093782566 commented Sep 25, 2016 •

edited

Loading

m1093782566 commented Oct 8, 2016 •

edited

Loading