Let the service controller retry when presistUpdate returns a conflict error #68087

grayluck · 2018-08-30T19:24:11Z

What this PR does / why we need it:
If a load balancer is changed while provisioning, it will fall into an error state and will not self-recover.
This PR picks up the conflict error and let serviceController retry in order to get the load balancer out of error state.

Special notes for your reviewer:
/assign @MrHohn @rramkumar1

Release note:

Let service controller retry creating load balancer when persistUpdate failed due to conflict.

MrHohn · 2018-08-30T19:37:54Z

/ok-to-test

MrHohn · 2018-08-30T19:41:30Z

@grayluck Can you update the release note to explain what this fixes? Thanks.

bowei · 2018-08-30T20:11:32Z

where is the test?

neolit123 · 2018-08-30T22:37:40Z

/sig apps

grayluck · 2018-08-31T01:30:50Z

/hold

grayluck · 2018-08-31T21:22:05Z

Unit test added.
/hold cancel

grayluck · 2018-08-31T21:36:58Z

/retest

MrHohn · 2018-08-31T22:57:29Z

Might need to run ./hack/update-bazel.sh to update the BUILD file.

grayluck · 2018-08-31T23:23:58Z

Done. Thanks!

grayluck · 2018-08-31T23:31:14Z

Release note updated.

grayluck · 2018-08-31T23:31:33Z

/milestone v1.12

k8s-ci-robot · 2018-08-31T23:31:34Z

@grayluck: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/milestone v1.12

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.

MrHohn

Thanks! Looks good, minor comments in test.

MrHohn · 2018-08-31T23:57:17Z

pkg/controller/service/service_controller_test.go

+
+	svcCache := controller.cache.getOrCreate(svcName)
+	if err := controller.processServiceUpdate(svcCache, svc, svcName); err == nil {
+		t.Errorf("controller.processServiceUpdate() = nil, want error")


Fatalf here? We won't get event below anyway if this doesn't error out.

MrHohn · 2018-09-01T00:01:49Z

pkg/controller/service/service_controller_test.go

+	retryMsg := "Error creating load balancer (will retry)"
+	if gotEvent := func() bool {
+		events := controller.eventRecorder.(*record.FakeRecorder).Events
+		for len(events) > 0 {


Can we do something similar to

kubernetes/pkg/cloudprovider/providers/gce/gce_loadbalancer_utils_test.go

Lines 385 to 403 in 6ecec23

func checkEvent(t *testing.T, recorder *record.FakeRecorder, expected string, shouldMatch bool) bool {

select {

case received := <-recorder.Events:

if strings.HasPrefix(received, expected) != shouldMatch {

t.Errorf(received)

if shouldMatch {

t.Errorf("Should receive message \"%v\" but got \"%v\".", expected, received)

} else {

t.Errorf("Unexpected event \"%v\".", received)

}

}

return false

case <-time.After(2 * time.Second):

if shouldMatch {

t.Errorf("Should receive message \"%v\" but got timed out.", expected)

}

return true

}

}

?

Seems better to have an timeout on this.

Everything is synchronous in this test. The time processServiceUpdate is returned, all events should be recorded. And "for len(events) > 0" guards that we will not wait for future (and there won't be future events) here.

Makes sense, thanks!

…t error.

grayluck · 2018-09-04T22:35:15Z

Thanks for the review, MrHohn! Commit updated.

MrHohn

/lgtm

MrHohn · 2018-09-04T22:35:27Z

pkg/controller/service/service_controller_test.go

+	retryMsg := "Error creating load balancer (will retry)"
+	if gotEvent := func() bool {
+		events := controller.eventRecorder.(*record.FakeRecorder).Events
+		for len(events) > 0 {


Makes sense, thanks!

k8s-ci-robot · 2018-09-04T22:35:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grayluck, MrHohn

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:

~~pkg/controller/service/OWNERS~~ [MrHohn]

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

bowei · 2018-09-04T23:00:19Z

/milestone v1.12

grayluck · 2018-09-05T01:37:20Z

Thanks Bowei!
/priority important-soon

MrHohn · 2018-09-06T18:01:40Z

/priority critical-urgent

k8s-github-robot · 2018-09-06T20:05:50Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-09-06T21:11:45Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

…87-upstream-release-1.11 Automated cherry pick of #68087

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 30, 2018

k8s-ci-robot assigned MrHohn and rramkumar1 Aug 30, 2018

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 30, 2018

k8s-ci-robot requested review from bowei and thockin August 30, 2018 19:24

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 30, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 30, 2018

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 30, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2018

grayluck force-pushed the refetch branch from e488068 to d9d43c0 Compare August 31, 2018 21:17

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2018

grayluck force-pushed the refetch branch from d9d43c0 to f3ce91b Compare August 31, 2018 21:20

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 31, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2018

grayluck force-pushed the refetch branch 2 times, most recently from 33fd9e6 to 482a499 Compare August 31, 2018 22:20

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 31, 2018

MrHohn approved these changes Sep 1, 2018

View reviewed changes

Let the service controller retry when presistUpdate returns a conflic…

267252d

…t error.

grayluck force-pushed the refetch branch from aed1039 to 267252d Compare September 4, 2018 22:34

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

MrHohn approved these changes Sep 4, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2018

k8s-ci-robot added this to the v1.12 milestone Sep 4, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 5, 2018

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 6, 2018

k8s-github-robot merged commit f85d39a into kubernetes:master Sep 6, 2018

ahmetb mentioned this pull request Sep 7, 2018

GKE LoadBalancer doesn't work with service deployed by Skaffold GoogleContainerTools/skaffold#887

Closed

grayluck mentioned this pull request Sep 7, 2018

Automated cherry pick of #68087 #68414

Merged

balopat mentioned this pull request Sep 7, 2018

Remove service labeling temporarily GoogleContainerTools/skaffold#959

Closed

marpaia mentioned this pull request Sep 20, 2018

Include SIG headers in the duplicate notes section marpaia/release-notes#6

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 4, 2018

Merge pull request #68414 from grayluck/automated-cherry-pick-of-#680…

298ef1f

…87-upstream-release-1.11 Automated cherry pick of #68087

tejal29 mentioned this pull request May 13, 2019

Use Manifest Labeller for helm. GoogleContainerTools/skaffold#2105

Closed

tejal29 mentioned this pull request Jul 30, 2019

Bring back applying labels to services deployed with helm GoogleContainerTools/skaffold#2568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let the service controller retry when presistUpdate returns a conflict error #68087

Let the service controller retry when presistUpdate returns a conflict error #68087

grayluck commented Aug 30, 2018 •

edited

MrHohn commented Aug 30, 2018

MrHohn commented Aug 30, 2018

bowei commented Aug 30, 2018

neolit123 commented Aug 30, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

MrHohn commented Aug 31, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

k8s-ci-robot commented Aug 31, 2018

MrHohn left a comment

MrHohn Aug 31, 2018

grayluck Sep 4, 2018

MrHohn Sep 1, 2018

grayluck Sep 4, 2018

MrHohn Sep 4, 2018

grayluck commented Sep 4, 2018

MrHohn left a comment

MrHohn Sep 4, 2018

k8s-ci-robot commented Sep 4, 2018

bowei commented Sep 4, 2018

grayluck commented Sep 5, 2018

MrHohn commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

	func checkEvent(t testing.T, recorder record.FakeRecorder, expected string, shouldMatch bool) bool {
	select {
	case received := <-recorder.Events:
	if strings.HasPrefix(received, expected) != shouldMatch {
	t.Errorf(received)
	if shouldMatch {
	t.Errorf("Should receive message \"%v\" but got \"%v\".", expected, received)
	} else {
	t.Errorf("Unexpected event \"%v\".", received)
	}
	}
	return false
	case <-time.After(2 * time.Second):
	if shouldMatch {
	t.Errorf("Should receive message \"%v\" but got timed out.", expected)
	}
	return true
	}
	}

Let the service controller retry when presistUpdate returns a conflict error #68087

Let the service controller retry when presistUpdate returns a conflict error #68087

Conversation

grayluck commented Aug 30, 2018 • edited

MrHohn commented Aug 30, 2018

MrHohn commented Aug 30, 2018

bowei commented Aug 30, 2018

neolit123 commented Aug 30, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

MrHohn commented Aug 31, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

grayluck commented Aug 31, 2018

k8s-ci-robot commented Aug 31, 2018

MrHohn left a comment

Choose a reason for hiding this comment

MrHohn Aug 31, 2018

Choose a reason for hiding this comment

grayluck Sep 4, 2018

Choose a reason for hiding this comment

MrHohn Sep 1, 2018

Choose a reason for hiding this comment

grayluck Sep 4, 2018

Choose a reason for hiding this comment

MrHohn Sep 4, 2018

Choose a reason for hiding this comment

grayluck commented Sep 4, 2018

MrHohn left a comment

Choose a reason for hiding this comment

MrHohn Sep 4, 2018

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 4, 2018

bowei commented Sep 4, 2018

grayluck commented Sep 5, 2018

MrHohn commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

k8s-github-robot commented Sep 6, 2018

grayluck commented Aug 30, 2018 •

edited