From 0aae852a3c1a5542e47fed344e12d1132c54964b Mon Sep 17 00:00:00 2001
From: Ryan McNamara <rmcnamara@palantir.com>
Date: Wed, 1 Aug 2018 17:25:37 -0700
Subject: [PATCH] Stably sort controllerrevisions

Fixes https://github.com/kubernetes/kubernetes/issues/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.
---
 pkg/controller/history/controller_history.go  |  9 ++++-
 .../history/controller_history_test.go        | 35 +++++++++++++++----
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/pkg/controller/history/controller_history.go b/pkg/controller/history/controller_history.go
index cc7fc433ce7..85a65eaad02 100644
--- a/pkg/controller/history/controller_history.go
+++ b/pkg/controller/history/controller_history.go
@@ -113,7 +113,7 @@ func HashControllerRevision(revision *apps.ControllerRevision, probe *int32) str
 
 // SortControllerRevisions sorts revisions by their Revision.
 func SortControllerRevisions(revisions []*apps.ControllerRevision) {
-	sort.Sort(byRevision(revisions))
+	sort.Stable(byRevision(revisions))
 }
 
 // EqualRevision returns true if lhs and rhs are either both nil, or both point to non-nil ControllerRevisions that
@@ -162,7 +162,14 @@ func (br byRevision) Len() int {
 	return len(br)
 }
 
+// Less breaks ties first by creation timestamp, then by name
 func (br byRevision) Less(i, j int) bool {
+	if br[i].Revision == br[j].Revision {
+		if br[j].CreationTimestamp.Equal(&br[i].CreationTimestamp) {
+			return br[i].Name < br[j].Name
+		}
+		return br[j].CreationTimestamp.After(br[i].CreationTimestamp.Time)
+	}
 	return br[i].Revision < br[j].Revision
 }
 
diff --git a/pkg/controller/history/controller_history_test.go b/pkg/controller/history/controller_history_test.go
index 13aaaf5d092..dcf6bb609eb 100644
--- a/pkg/controller/history/controller_history_test.go
+++ b/pkg/controller/history/controller_history_test.go
@@ -40,6 +40,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
 
 	core "k8s.io/client-go/testing"
+	"time"
 )
 
 func TestRealHistory_ListControllerRevisions(t *testing.T) {
@@ -1545,12 +1546,14 @@ func TestSortControllerRevisions(t *testing.T) {
 		want      []string
 	}
 	testFn := func(test *testcase, t *testing.T) {
-		SortControllerRevisions(test.revisions)
-		for i := range test.revisions {
-			if test.revisions[i].Name != test.want[i] {
-				t.Errorf("%s: want %s at %d got %s", test.name, test.want[i], i, test.revisions[i].Name)
+		t.Run(test.name, func(t *testing.T) {
+			SortControllerRevisions(test.revisions)
+			for i := range test.revisions {
+				if test.revisions[i].Name != test.want[i] {
+					t.Errorf("%s: want %s at %d got %s", test.name, test.want[i], i, test.revisions[i].Name)
+				}
 			}
-		}
+		})
 	}
 	ss1 := newStatefulSet(3, "ss1", types.UID("ss1"), map[string]string{"foo": "bar"})
 	ss1.Status.CollisionCount = new(int32)
@@ -1559,17 +1562,32 @@ func TestSortControllerRevisions(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+
 	ss1Rev1.Namespace = ss1.Namespace
 	ss1Rev2, err := NewControllerRevision(ss1, parentKind, ss1.Spec.Template.Labels, rawTemplate(&ss1.Spec.Template), 2, ss1.Status.CollisionCount)
 	if err != nil {
 		t.Fatal(err)
 	}
+
 	ss1Rev2.Namespace = ss1.Namespace
 	ss1Rev3, err := NewControllerRevision(ss1, parentKind, ss1.Spec.Template.Labels, rawTemplate(&ss1.Spec.Template), 3, ss1.Status.CollisionCount)
 	if err != nil {
 		t.Fatal(err)
 	}
-	ss1Rev3.Namespace = ss1.Namespace
+
+	ss1Rev3Time2, err := NewControllerRevision(ss1, parentKind, ss1.Spec.Template.Labels, rawTemplate(&ss1.Spec.Template), 3, ss1.Status.CollisionCount)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ss1Rev3Time2.Namespace = ss1.Namespace
+	ss1Rev3Time2.CreationTimestamp = metav1.Time{Time: ss1Rev3.CreationTimestamp.Add(time.Second)}
+
+	ss1Rev3Time2Name2, err := NewControllerRevision(ss1, parentKind, ss1.Spec.Template.Labels, rawTemplate(&ss1.Spec.Template), 3, ss1.Status.CollisionCount)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ss1Rev3Time2Name2.Namespace = ss1.Namespace
+	ss1Rev3Time2Name2.CreationTimestamp = metav1.Time{Time: ss1Rev3.CreationTimestamp.Add(time.Second)}
 
 	tests := []testcase{
 		{
@@ -1587,6 +1605,11 @@ func TestSortControllerRevisions(t *testing.T) {
 			revisions: []*apps.ControllerRevision{ss1Rev3, ss1Rev2, ss1Rev1},
 			want:      []string{ss1Rev1.Name, ss1Rev2.Name, ss1Rev3.Name},
 		},
+		{
+			name:      "with ties",
+			revisions: []*apps.ControllerRevision{ss1Rev3, ss1Rev3Time2, ss1Rev2, ss1Rev1},
+			want:      []string{ss1Rev1.Name, ss1Rev2.Name, ss1Rev3.Name, ss1Rev3Time2.Name, ss1Rev3Time2Name2.Name},
+		},
 		{
 			name:      "empty",
 			revisions: nil,