Use CollisionCount for collision avoidance in StatefulSet controller

pkg/controller/history/controller_history.go

@@ -54,15 +54,17 @@ func ControllerRevisionName(prefix string, hash uint32) string {
 	return fmt.Sprintf("%s-%d", prefix, hash)
 }
 
-// NewControllerRevision returns the a ControllerRevision with a ControllerRef pointing parent and indicating that
+// NewControllerRevision returns a ControllerRevision with a ControllerRef pointing to parent and indicating that
 // parent is of parentKind. The ControllerRevision has labels matching selector, contains Data equal to data, and
-// has a Revision equal to revision. If the returned error is nil, the returned ControllerRevision is valid. If the
+// has a Revision equal to revision. The collisionCount is used when creating the name of the ControllerRevision
+// so the name is likely unique. If the returned error is nil, the returned ControllerRevision is valid. If the
 // returned error is not nil, the returned ControllerRevision is invalid for use.
 func NewControllerRevision(parent metav1.Object,
 	parentKind schema.GroupVersionKind,
 	selector labels.Selector,
 	data runtime.RawExtension,
-	revision int64) (*apps.ControllerRevision, error) {
+	revision int64,
+	collisionCount *int32) (*apps.ControllerRevision, error) {
 	labelMap, err := labels.ConvertSelectorToLabelsMap(selector.String())
 	if err != nil {
 		return nil, err
@@ -86,7 +88,7 @@ func NewControllerRevision(parent metav1.Object,
 		Data:     data,
 		Revision: revision,
 	}
-	hash := HashControllerRevision(cr, nil)
+	hash := HashControllerRevision(cr, collisionCount)
 	cr.Name = ControllerRevisionName(parent.GetName(), hash)
 	cr.Labels[ControllerRevisionHashLabel] = strconv.FormatInt(int64(hash), 10)
 	return cr, nil
@@ -94,7 +96,7 @@ func NewControllerRevision(parent metav1.Object,
 
 // HashControllerRevision hashes the contents of revision's Data using FNV hashing. If probe is not nil, the byte value
 // of probe is added written to the hash as well.
-func HashControllerRevision(revision *apps.ControllerRevision, probe *uint32) uint32 {
+func HashControllerRevision(revision *apps.ControllerRevision, probe *int32) uint32 {
 	hf := fnv.New32()
 	if len(revision.Data.Raw) > 0 {
 		hf.Write(revision.Data.Raw)
@@ -177,11 +179,13 @@ type Interface interface {
 	// returned error is not nil, the returned slice is not valid.
 	ListControllerRevisions(parent metav1.Object, selector labels.Selector) ([]*apps.ControllerRevision, error)
 	// CreateControllerRevision attempts to create the revision as owned by parent via a ControllerRef. If name
-	// collision occurs, a unique identifier is added to the hash of the revision and it is renamed using
-	// ControllerRevisionName. Implementations may cease to attempt to retry creation after some number of attempts
-	// and return an error. If the returned error is not nil, creation failed. If the returned error is nil, the
-	// returned ControllerRevision has been created.
-	CreateControllerRevision(parent metav1.Object, revision *apps.ControllerRevision) (*apps.ControllerRevision, error)
+	// collision occurs, collisionCount (incremented each time collision occurs except for the first time) is
+	// added to the hash of the revision and it is renamed using ControllerRevisionName. Implementations may
+	// cease to attempt to retry creation after some number of attempts and return an error. If the returned
+	// error is not nil, creation failed. If the returned error is nil, the returned ControllerRevision has been
+	// created.
+	// Callers must make sure that collisionCount is not nil. An error is returned if it is.
+	CreateControllerRevision(parent metav1.Object, revision *apps.ControllerRevision, collisionCount *int32) (*apps.ControllerRevision, error)
 	// DeleteControllerRevision attempts to delete revision. If the returned error is not nil, deletion has failed.
 	DeleteControllerRevision(revision *apps.ControllerRevision) error
 	// UpdateControllerRevision updates revision such that its Revision is equal to newRevision. Implementations
@@ -233,9 +237,10 @@ func (rh *realHistory) ListControllerRevisions(parent metav1.Object, selector la
 	return owned, err
 }
 
-func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *apps.ControllerRevision) (*apps.ControllerRevision, error) {
-	// Initialize the probe to 0
-	probe := uint32(0)
+func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *apps.ControllerRevision, collisionCount *int32) (*apps.ControllerRevision, error) {
+	if collisionCount == nil {
+		return nil, fmt.Errorf("collisionCount should not be nil")
+	}
 
 	// Clone the input
 	any, err := scheme.Scheme.DeepCopy(revision)
@@ -246,18 +251,12 @@ func (rh *realHistory) CreateControllerRevision(parent metav1.Object, revision *
 
 	// Continue to attempt to create the revision updating the name with a new hash on each iteration
 	for {
-		var hash uint32
-		// The first attempt uses no probe to resolve collisions
-		if probe > 0 {
-			hash = HashControllerRevision(revision, &probe)
-		} else {
-			hash = HashControllerRevision(revision, nil)
-		}
+		hash := HashControllerRevision(revision, collisionCount)
 		// Update the revisions name and labels
 		clone.Name = ControllerRevisionName(parent.GetName(), hash)
 		created, err := rh.client.AppsV1beta1().ControllerRevisions(parent.GetNamespace()).Create(clone)
 		if errors.IsAlreadyExists(err) {
-			probe++
+			*collisionCount++
 			continue
 		}
 		return created, err
@@ -370,9 +369,10 @@ func (fh *fakeHistory) addRevision(revision *apps.ControllerRevision) (*apps.Con
 	return revision, fh.indexer.Update(revision)
 }
 
-func (fh *fakeHistory) CreateControllerRevision(parent metav1.Object, revision *apps.ControllerRevision) (*apps.ControllerRevision, error) {
-	// Initialize the probe to 0
-	probe := uint32(0)
+func (fh *fakeHistory) CreateControllerRevision(parent metav1.Object, revision *apps.ControllerRevision, collisionCount *int32) (*apps.ControllerRevision, error) {
+	if collisionCount == nil {
+		return nil, fmt.Errorf("collisionCount should not be nil")
+	}
 
 	// Clone the input
 	any, err := scheme.Scheme.DeepCopy(revision)
@@ -384,18 +384,12 @@ func (fh *fakeHistory) CreateControllerRevision(parent metav1.Object, revision *
 
 	// Continue to attempt to create the revision updating the name with a new hash on each iteration
 	for {
-		var hash uint32
-		// The first attempt uses no probe to resolve collisions
-		if probe > 0 {
-			hash = HashControllerRevision(revision, &probe)
-		} else {
-			hash = HashControllerRevision(revision, nil)
-		}
+		hash := HashControllerRevision(revision, collisionCount)
 		// Update the revisions name and labels
 		clone.Name = ControllerRevisionName(parent.GetName(), hash)
 		created, err := fh.addRevision(clone)
 		if errors.IsAlreadyExists(err) {
-			probe++
+			*collisionCount++
 			continue
 		}
 		return created, err

pkg/controller/statefulset/stateful_set_control.go

@@ -80,13 +80,13 @@ func (ssc *defaultStatefulSetControl) UpdateStatefulSet(set *apps.StatefulSet, p
 	history.SortControllerRevisions(revisions)
 
 	// get the current, and update revisions
-	currentRevision, updateRevision, err := ssc.getStatefulSetRevisions(set, revisions)
+	currentRevision, updateRevision, collisionCount, err := ssc.getStatefulSetRevisions(set, revisions)
 	if err != nil {
 		return err
 	}
 
 	// perform the main update function and get the status
-	status, err := ssc.updateStatefulSet(set, currentRevision, updateRevision, pods)
+	status, err := ssc.updateStatefulSet(set, currentRevision, updateRevision, collisionCount, pods)
 	if err != nil {
 		return err
 	}
@@ -174,21 +174,31 @@ func (ssc *defaultStatefulSetControl) truncateHistory(
 	return nil
 }
 
-// getStatefulSetRevisions returns the current and update ControllerRevisions for set. This method may create a new revision,
-// or modify the Revision of an existing revision if an update to set is detected. This method expects that revisions
-// is sorted when supplied.
+// getStatefulSetRevisions returns the current and update ControllerRevisions for set. It also
+// returns a collision count that records the number of name collisions set saw when creating
+// new ControllerRevisions. This count is incremented on every name collision and is used in
+// building the ControllerRevision names for name collision avoidance. This method may create
+// a new revision, or modify the Revision of an existing revision if an update to set is detected.
+// This method expects that revisions is sorted when supplied.
 func (ssc *defaultStatefulSetControl) getStatefulSetRevisions(
 	set *apps.StatefulSet,
-	revisions []*apps.ControllerRevision) (*apps.ControllerRevision, *apps.ControllerRevision, error) {
+	revisions []*apps.ControllerRevision) (*apps.ControllerRevision, *apps.ControllerRevision, int32, error) {
 	var currentRevision, updateRevision *apps.ControllerRevision
 
 	revisionCount := len(revisions)
 	history.SortControllerRevisions(revisions)
 
+	// Use a local copy of set.Status.CollisionCount to avoid modifying set.Status directly.
+	// This copy is returned so the value gets carried over to set.Status in updateStatefulSet.
+	var collisionCount int32
+	if set.Status.CollisionCount != nil {
+		collisionCount = *set.Status.CollisionCount
+	}
+
 	// create a new revision from the current set
-	updateRevision, err := newRevision(set, nextRevision(revisions))
+	updateRevision, err := newRevision(set, nextRevision(revisions), &collisionCount)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, collisionCount, err
 	}
 
 	// find any equivalent revisions
@@ -205,13 +215,13 @@ func (ssc *defaultStatefulSetControl) getStatefulSetRevisions(
 			equalRevisions[equalCount-1],
 			updateRevision.Revision)
 		if err != nil {
-			return nil, nil, err
+			return nil, nil, collisionCount, err
 		}
 	} else {
 		//if there is no equivalent revision we create a new one
-		updateRevision, err = ssc.controllerHistory.CreateControllerRevision(set, updateRevision)
+		updateRevision, err = ssc.controllerHistory.CreateControllerRevision(set, updateRevision, &collisionCount)
 		if err != nil {
-			return nil, nil, err
+			return nil, nil, collisionCount, err
 		}
 	}
 
@@ -227,7 +237,7 @@ func (ssc *defaultStatefulSetControl) getStatefulSetRevisions(
 		currentRevision = updateRevision
 	}
 
-	return currentRevision, updateRevision, nil
+	return currentRevision, updateRevision, collisionCount, nil
 }
 
 // updateStatefulSet performs the update function for a StatefulSet. This method creates, updates, and deletes Pods in
@@ -243,6 +253,7 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
 	set *apps.StatefulSet,
 	currentRevision *apps.ControllerRevision,
 	updateRevision *apps.ControllerRevision,
+	collisionCount int32,
 	pods []*v1.Pod) (*apps.StatefulSetStatus, error) {
 	// get the current and update revisions of the set.
 	currentSet, err := applyRevision(set, currentRevision)
@@ -260,6 +271,8 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
 	*status.ObservedGeneration = set.Generation
 	status.CurrentRevision = currentRevision.Name
 	status.UpdateRevision = updateRevision.Name
+	status.CollisionCount = new(int32)
+	*status.CollisionCount = collisionCount
 
 	replicaCount := int(*set.Spec.Replicas)
 	// slice that will contain all Pods such that 0 <= getOrdinal(pod) < set.Spec.Replicas

pkg/controller/statefulset/stateful_set_control_test.go

@@ -465,14 +465,15 @@ func TestStatefulSetControl_getSetRevisions(t *testing.T) {
 			informerFactory.Core().V1().Pods().Informer().HasSynced,
 			informerFactory.Apps().V1beta1().ControllerRevisions().Informer().HasSynced,
 		)
+		test.set.Status.CollisionCount = new(int32)
 		for i := range test.existing {
-			ssc.controllerHistory.CreateControllerRevision(test.set, test.existing[i])
+			ssc.controllerHistory.CreateControllerRevision(test.set, test.existing[i], test.set.Status.CollisionCount)
 		}
 		revisions, err := ssc.ListRevisions(test.set)
 		if err != nil {
 			t.Fatal(err)
 		}
-		current, update, err := ssc.getStatefulSetRevisions(test.set, revisions)
+		current, update, _, err := ssc.getStatefulSetRevisions(test.set, revisions)
 		revisions, err = ssc.ListRevisions(test.set)
 		if err != nil {
 			t.Fatal(err)
@@ -480,20 +481,20 @@ func TestStatefulSetControl_getSetRevisions(t *testing.T) {
 		if len(revisions) != test.expectedCount {
 			t.Errorf("%s: want %d revisions got %d", test.name, test.expectedCount, len(revisions))
 		}
-		if test.err && err != nil {
+		if test.err && err == nil {
 			t.Errorf("%s: expected error", test.name)
 		}
 		if !test.err && !history.EqualRevision(current, test.expectedCurrent) {
 			t.Errorf("%s: for current want %v got %v", test.name, test.expectedCurrent, current)
 		}
 		if !test.err && !history.EqualRevision(update, test.expectedUpdate) {
-			t.Errorf("%s: for current want %v got %v", test.name, test.expectedUpdate, update)
+			t.Errorf("%s: for update want %v got %v", test.name, test.expectedUpdate, update)
 		}
 		if !test.err && test.expectedCurrent != nil && current != nil && test.expectedCurrent.Revision != current.Revision {
 			t.Errorf("%s: for current revision want %d got %d", test.name, test.expectedCurrent.Revision, current.Revision)
 		}
 		if !test.err && test.expectedUpdate != nil && update != nil && test.expectedUpdate.Revision != update.Revision {
-			t.Errorf("%s: for current revision want %d got %d", test.name, test.expectedUpdate.Revision, update.Revision)
+			t.Errorf("%s: for update revision want %d got %d", test.name, test.expectedUpdate.Revision, update.Revision)
 		}
 	}
 
@@ -508,14 +509,17 @@ func TestStatefulSetControl_getSetRevisions(t *testing.T) {
 	}
 
 	set := newStatefulSet(3)
+	set.Status.CollisionCount = new(int32)
 	rev0 := newRevisionOrDie(set, 1)
 	set1 := copySet(set)
 	set1.Spec.Template.Spec.Containers[0].Image = "foo"
 	set1.Status.CurrentRevision = rev0.Name
+	set1.Status.CollisionCount = new(int32)
 	rev1 := newRevisionOrDie(set1, 2)
 	set2 := copySet(set1)
 	set2.Spec.Template.Labels["new"] = "label"
 	set2.Status.CurrentRevision = rev0.Name
+	set2.Status.CollisionCount = new(int32)
 	rev2 := newRevisionOrDie(set2, 3)
 	tests := []testcase{
 		{
@@ -2097,7 +2101,7 @@ func updateStatefulSetControl(set *apps.StatefulSet,
 }
 
 func newRevisionOrDie(set *apps.StatefulSet, revision int64) *apps.ControllerRevision {
-	rev, err := newRevision(set, revision)
+	rev, err := newRevision(set, revision, set.Status.CollisionCount)
 	if err != nil {
 		panic(err)
 	}

pkg/controller/statefulset/stateful_set_utils.go

@@ -288,7 +288,7 @@ func getPatch(set *apps.StatefulSet) ([]byte, error) {
 // The Revision of the returned ControllerRevision is set to revision. If the returned error is nil, the returned
 // ControllerRevision is valid. StatefulSet revisions are stored as patches that re-apply the current state of set
 // to a new StatefulSet using a strategic merge patch to replace the saved state of the new StatefulSet.
-func newRevision(set *apps.StatefulSet, revision int64) (*apps.ControllerRevision, error) {
+func newRevision(set *apps.StatefulSet, revision int64, collisionCount *int32) (*apps.ControllerRevision, error) {
 	patch, err := getPatch(set)
 	if err != nil {
 		return nil, err
@@ -301,7 +301,8 @@ func newRevision(set *apps.StatefulSet, revision int64) (*apps.ControllerRevisio
 		controllerKind,
 		selector,
 		runtime.RawExtension{Raw: patch},
-		revision)
+		revision,
+		collisionCount)
 	if err != nil {
 		return nil, err
 	}

pkg/controller/statefulset/stateful_set_utils_test.go

@@ -274,7 +274,8 @@ func TestNewPodControllerRef(t *testing.T) {
 
 func TestCreateApplyRevision(t *testing.T) {
 	set := newStatefulSet(1)
-	revision, err := newRevision(set, 1)
+	set.Status.CollisionCount = new(int32)
+	revision, err := newRevision(set, 1, set.Status.CollisionCount)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -289,7 +290,7 @@ func TestCreateApplyRevision(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	restoredRevision, err := newRevision(restoredSet, 2)
+	restoredRevision, err := newRevision(restoredSet, 2, restoredSet.Status.CollisionCount)
 	if err != nil {
 		t.Fatal(err)
 	}