Fix for duplicate revisions created by StatefulSet #67039

mortent · 2018-08-06T23:04:01Z

What this PR does / why we need it: This PR replaces PR #65038 as a fix to issue #55159. The statefulset controller can in some situations create more controller revisions than necessary and this change makes sure the controller checks with the API server and only create new revision if the raw data is different.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #55159

Special notes for your reviewer:

Release note:

Avoid creating new controller revisions for statefulsets when cache is stale

neolit123

/sig apps
/kind bug

neolit123 · 2018-08-06T23:09:35Z

pkg/controller/history/controller_history_test.go

@@ -258,8 +258,8 @@ func TestRealHistory_CreateControllerRevision(t *testing.T) {
 		history := NewHistory(client, informer.Lister())

 		var collisionCount int32
-		for i := range test.existing {
-			_, err := history.CreateControllerRevision(test.existing[i].parent, test.existing[i].revision, &collisionCount)
+		for _, exst := range test.existing {


exst -> exists

Renamed the iteration variable to item. I think exists just sounds too much like a boolean variable and the name is also used by another variable in the function.

neolit123 · 2018-08-06T23:09:53Z

pkg/controller/history/controller_history.go

+				return nil, err
+			}
+			if bytes.Equal(existed.Data.Raw, clone.Data.Raw) {
+				return existed, err


existed, err -> existed, nil to make an explicit statement here.
or exists, nil as per the above comment about tense.

neolit123 · 2018-08-06T23:10:43Z

pkg/controller/history/controller_history.go

@@ -252,6 +252,13 @@ func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *
 		clone.Name = ControllerRevisionName(parent.GetName(), hash)
 		created, err := rh.client.AppsV1().ControllerRevisions(parent.GetNamespace()).Create(clone)
 		if errors.IsAlreadyExists(err) {
+			existed, err := rh.client.AppsV1().ControllerRevisions(parent.GetNamespace()).Get(clone.Name, metav1.GetOptions{})


existed -> exists present tense?

neolit123 · 2018-08-06T23:13:30Z

thank you for your first PR @mortent 👍
added a couple of review comments.

@kubernetes/sig-apps-pr-reviews

mortent · 2018-08-06T23:26:05Z

@neolit123 Thanks for the comments. I have updated the PR.

mortent · 2018-08-06T23:26:26Z

/assign @kow3ns

kow3ns · 2018-08-07T19:49:00Z

/ok-to-test

kow3ns · 2018-08-07T20:22:41Z

/approve

mortent · 2018-08-07T21:49:26Z

/retest

crimsonfaith91 · 2018-08-12T02:18:08Z

pkg/controller/history/controller_history.go

@@ -252,6 +252,13 @@ func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *
 		clone.Name = ControllerRevisionName(parent.GetName(), hash)
 		created, err := rh.client.AppsV1().ControllerRevisions(parent.GetNamespace()).Create(clone)
 		if errors.IsAlreadyExists(err) {
+			exists, err := rh.client.AppsV1().ControllerRevisions(parent.GetNamespace()).Get(clone.Name, metav1.GetOptions{})


nit: set parent.GetNamespace to a variable, and reuse it.

crimsonfaith91 · 2018-08-12T02:43:31Z

pkg/controller/history/controller_history_test.go

+	modTemplate := ss1.Spec.Template.DeepCopy()
+	modTemplate.Labels["foo"] = "not_bar"
+	ss1Rev2, err := NewControllerRevision(ss1, parentKind, ss1.Spec.Template.Labels, rawTemplate(modTemplate), 2, ss1.Status.CollisionCount)
+	ss1Rev2.Name = ss1Rev1.Name


By modifying labels of modTemplate, the hash for populating name and labels fields will change, so I think it is incorrect to set ss1Rev2.Name = ss1Rev1.Name (same with .Labels below).

The labels are being manipulated in this way in order to set up the scenario where there are two controller revisions with the same name and hash label, but which are not identical. This is done by creating a new controller revision which is slightly different than an existing one (change the value of a label) and then give it the same name and label as the existing one. This will create a controller revision where the name and hash are "incorrect" for the content, but it works for setting up the data for the test. Normally this would be the result of a hash collision, we don't have a way to create those in the test.

Noted. As this is a unit test, I think this can be made clearer by creating the 2nd ControllerRevision (with the same name and labels) before modifying it's labels.

As sync by person, ControllerRevision is immutable once created, so this "hack" is necessary. I will LGTM again once comments are added. Thanks for the work!

crimsonfaith91 · 2018-08-13T03:58:30Z

/lgtm

Other than the nit comments, everything looks good to me.

fejta-bot · 2018-08-13T06:34:21Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-08-13T09:01:21Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-08-13T16:01:21Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

crimsonfaith91 · 2018-08-13T16:54:14Z

/lgtm

k8s-ci-robot · 2018-08-13T16:54:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crimsonfaith91, kow3ns, mortent

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/history/OWNERS~~ [kow3ns]

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

crimsonfaith91 · 2018-08-14T04:39:38Z

/retest

fejta-bot · 2018-08-14T08:28:21Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-08-14T14:04:22Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

mortent · 2018-08-14T20:38:24Z

/retest

k8s-github-robot · 2018-08-15T05:43:27Z

Automatic merge from submit-queue (batch tested with PRs 67071, 66906, 66722, 67276, 67039). If you want to cherry-pick this change to another branch, please follow the instructions here.

…67039-upstream-release-1.10 Automated cherry pick of #67039: Fix for duplicate revisions created by StatefulSet

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2018

k8s-ci-robot requested review from smarterclayton and tnozicka August 6, 2018 23:04

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 6, 2018

neolit123 reviewed Aug 6, 2018

View reviewed changes

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. kind/bug Categorizes issue or PR as related to a bug. labels Aug 6, 2018

mortent force-pushed the AvoidDuplicateRevisionsForStatefulSet branch from 9d08e46 to 343a209 Compare August 6, 2018 23:23

k8s-ci-robot assigned kow3ns Aug 6, 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 7, 2018

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

crimsonfaith91 reviewed Aug 12, 2018

View reviewed changes

k8s-ci-robot assigned crimsonfaith91 Aug 13, 2018

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

Fix for duplicate revisions created by StatefulSet

31f1972

mortent force-pushed the AvoidDuplicateRevisionsForStatefulSet branch from 343a209 to 31f1972 Compare August 13, 2018 16:43

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 13, 2018

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

k8s-github-robot merged commit cfb4a5e into kubernetes:master Aug 15, 2018

tanshanshan mentioned this pull request Aug 18, 2018

Automated cherry pick of #67039: Fix for duplicate revisions created by StatefulSet #67560

Merged

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

Merge pull request #67560 from tanshanshan/automated-cherry-pick-of-#…

384ed37

…67039-upstream-release-1.10 Automated cherry pick of #67039: Fix for duplicate revisions created by StatefulSet

mortent mentioned this pull request Oct 3, 2018

REQUEST: New membership for mortent kubernetes/org#136

Closed

6 tasks

fahadullah mentioned this pull request Jan 9, 2019

Fix for duplicate revisions created by StatefulSet igneous-systems/kubernetes#7

Merged

wangxiaoq mentioned this pull request Jul 29, 2019

[Controller Manager][StatefulSet Controller] Different ControllerRevisions with the same Revision #80698

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for duplicate revisions created by StatefulSet #67039

Fix for duplicate revisions created by StatefulSet #67039

mortent commented Aug 6, 2018

neolit123 left a comment

neolit123 Aug 6, 2018

mortent Aug 6, 2018

neolit123 Aug 6, 2018

neolit123 Aug 6, 2018

neolit123 commented Aug 6, 2018 •

edited

Loading

mortent commented Aug 6, 2018

mortent commented Aug 6, 2018

kow3ns commented Aug 7, 2018

kow3ns commented Aug 7, 2018

mortent commented Aug 7, 2018

crimsonfaith91 Aug 12, 2018

crimsonfaith91 Aug 12, 2018

mortent Aug 13, 2018

crimsonfaith91 Aug 13, 2018

crimsonfaith91 Aug 13, 2018

crimsonfaith91 commented Aug 13, 2018

fejta-bot commented Aug 13, 2018

fejta-bot commented Aug 13, 2018

fejta-bot commented Aug 13, 2018

crimsonfaith91 commented Aug 13, 2018

k8s-ci-robot commented Aug 13, 2018

crimsonfaith91 commented Aug 14, 2018

fejta-bot commented Aug 14, 2018

fejta-bot commented Aug 14, 2018

mortent commented Aug 14, 2018

k8s-github-robot commented Aug 15, 2018

Fix for duplicate revisions created by StatefulSet #67039

Fix for duplicate revisions created by StatefulSet #67039

Conversation

mortent commented Aug 6, 2018

neolit123 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

neolit123 commented Aug 6, 2018 • edited Loading

mortent commented Aug 6, 2018

mortent commented Aug 6, 2018

kow3ns commented Aug 7, 2018

kow3ns commented Aug 7, 2018

mortent commented Aug 7, 2018

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

crimsonfaith91 commented Aug 13, 2018

fejta-bot commented Aug 13, 2018

fejta-bot commented Aug 13, 2018

fejta-bot commented Aug 13, 2018

crimsonfaith91 commented Aug 13, 2018

k8s-ci-robot commented Aug 13, 2018

crimsonfaith91 commented Aug 14, 2018

fejta-bot commented Aug 14, 2018

fejta-bot commented Aug 14, 2018

mortent commented Aug 14, 2018

k8s-github-robot commented Aug 15, 2018

neolit123 commented Aug 6, 2018 •

edited

Loading