Stably sort controllerrevisions #66882

ryanmcnamara · 2018-08-02T00:41:57Z

Fixes #61998

There are times when multiple "equal" controllerrevisions are created with
the same revision number. When this happens and this is the case for the
largest revision number, the statefulset controller will periodically
select one of the maximal controllerrevisions to be the target of the
underlying statefulset. The selection happens here: /~https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L212.
Prior to this change this selection was random as the sort was not
stable, which caused the pods of a stable set to continually roll.

What this PR does / why we need it:

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 #

Special notes for your reviewer:

Release note:

Stably sort controllerrevisions. This can prevent pods of statefulsets from continually rolling.

ryanmcnamara · 2018-08-02T00:42:59Z

/assign @smarterclayton

ryanmcnamara · 2018-08-02T00:44:18Z

This is somewhat related to this bug: #63282
Regardless of how the discussion finishes there, this will still cause a problem as there can be multiple equal controller revisions at the end of the sorted array that can be selected as mentioned above, and without the stable sort they are selected randomly

tanshanshan · 2018-08-13T06:43:57Z

/ok-to-test

BenTheElder · 2018-09-07T09:16:26Z

/retest

BenTheElder · 2018-09-07T09:18:23Z

/sig apps

BenTheElder · 2018-09-07T10:16:57Z

Verify is failing on a gofmt check, can you please run hack/update-gofmt.sh?

BenTheElder · 2018-09-08T00:51:45Z

one more format issue:

diff -u ./pkg/controller/history/controller_history_test.go.orig ./pkg/controller/history/controller_history_test.go
--- ./pkg/controller/history/controller_history_test.go.orig	2018-09-07 09:43:31.562334296 +0000
+++ ./pkg/controller/history/controller_history_test.go	2018-09-07 09:43:31.563334384 +0000
@@ -1545,7 +1545,7 @@
 		want      []string
 	}
 	testFn := func(test *testcase, t *testing.T) {
-		t.Run(test.name,func(t *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			SortControllerRevisions(test.revisions)
 			for i := range test.revisions {
 				if test.revisions[i].Name != test.want[i] {

Run ./hack/update-gofmt.sh

ryanmcnamara · 2018-09-08T02:09:13Z

/retest

ryanmcnamara · 2018-09-08T02:12:21Z

pull-kubernetes-verify — Job succeeded.
so I think format's good, not sure if the other failures were a flake or what

BenTheElder · 2018-09-08T02:26:00Z

Those do look like flakes, I've also mentioned this PR in #sig-apps on slack seeing if I can get someone appropriate to take a look. 👍

janetkuo · 2018-09-11T00:42:33Z

pkg/controller/history/controller_history.go

-	sort.Sort(byRevision(revisions))
+	// Stably sort first by name to break revision number ties by name
+	sort.Stable(byName(revisions))
+	sort.Stable(byRevision(revisions))


You don't need to sort it twice. In the Less function of byRevision, use a tie-breaker there when two revisions are equal. I'd prefer using something like CreationTimestamp as a tie-breaker.

We can use Name as another tie-breaker if both Revision and CreationTimestamp are equal.

BenTheElder · 2018-09-21T06:44:56Z

gentle poke @ryanmcnamara :-) #66882 (comment)

ryanmcnamara · 2018-09-24T02:21:33Z

/assign @janetkuo

BenTheElder · 2018-09-26T03:17:43Z

Thanks for keeping after this! Poke @janetkuo :-)

ryanmcnamara · 2018-10-17T03:28:16Z

@janetkuo any update?

ryanmcnamara · 2018-10-31T22:18:49Z

Ping @BenTheElder @janetkuo

BenTheElder

/lgtm

ryanmcnamara · 2018-11-06T00:03:19Z

@janetkuo done!
Out of curiosity does the squash and merge github UI button not work? Or is there some additional process in merging prs for kubernetes?

ryanmcnamara · 2018-11-06T01:06:01Z

/retest

BenTheElder · 2018-11-07T19:10:05Z

Out of curiosity does the squash and merge github UI button not work? Or is there some additional process in merging prs for kubernetes?

We have a merge robot that ~ merges approved + lgtm'd PRs once they are passing tests against the target branch.

BenTheElder

/lgtm
sorry for the huge delay!

BenTheElder · 2018-11-07T19:11:50Z

Thanks for working on this! Merge details (tide) here. This PR still needs approval from @janetkuo

ryanmcnamara · 2018-12-18T01:01:25Z

ping @janetkuo @BenTheElder

BenTheElder · 2018-12-18T01:03:48Z

/test pull-kubernetes-godeps
Will need Janet's approval still, or someone else in the OWNERS /~https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/history/OWNERS

janetkuo · 2018-12-18T01:29:11Z

/approve

k8s-ci-robot · 2018-12-18T01:29:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, ryanmcnamara

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~~ [janetkuo]

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

janetkuo · 2018-12-18T01:29:56Z

/test pull-kubernetes-godeps

BenTheElder · 2018-12-18T01:35:14Z

godep: error downloading dep (bitbucket.org/ww/goautoneg): exit status 255

ah, we've had issues with hg + bitbucket, there's a fix being worked on. I don't think we have a tracking issue yet.

ryanmcnamara · 2018-12-18T02:14:47Z

/retest

ryanmcnamara · 2018-12-18T02:51:57Z

/test pull-kubernetes-godeps

fejta-bot · 2018-12-18T05:33:18Z

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

Review the full test history for this PR.