Merge pull request #44774 from kargakis/uniquifier

Automatic merge from submit-queue

Switch Deployments to new hashing algo w/ collision avoidance mechanism

Implements https://github.com/kubernetes/community/pull/477

@kubernetes/sig-apps-api-reviews @kubernetes/sig-apps-pr-reviews 

Fixes https://github.com/kubernetes/kubernetes/issues/29735
Fixes https://github.com/kubernetes/kubernetes/issues/43948

```release-note
Deployments are updated to use (1) a more stable hashing algorithm (fnv) than the previous one (adler) and (2) a hashing collision avoidance mechanism that will ensure new rollouts will not block on hashing collisions anymore.
```

pkg/controller/deployment/deployment_controller.go

@@ -574,7 +574,6 @@ func (dc *DeploymentController) syncDeployment(key string) error {
 		return nil
 	}
 	if err != nil {
-		utilruntime.HandleError(fmt.Errorf("Unable to retrieve deployment %v from store: %v", key, err))
 		return err
 	}
 

pkg/controller/deployment/rollback.go

@@ -73,7 +73,11 @@ func (dc *DeploymentController) rollback(d *extensions.Deployment, rsList []*ext
 // cleans up the rollback spec so subsequent requeues of the deployment won't end up in here.
 func (dc *DeploymentController) rollbackToTemplate(d *extensions.Deployment, rs *extensions.ReplicaSet) (bool, error) {
 	performedRollback := false
-	if !deploymentutil.EqualIgnoreHash(d.Spec.Template, rs.Spec.Template) {
+	isEqual, err := deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template)
+	if err != nil {
+		return false, err
+	}
+	if !isEqual {
 		glog.V(4).Infof("Rolling back deployment %q to template spec %+v", d.Name, rs.Spec.Template.Spec)
 		deploymentutil.SetFromReplicaSetTemplate(d, rs.Spec.Template)
 		// set RS (the old RS we'll rolling back to) annotations back to the deployment;

pkg/controller/deployment/sync.go

@@ -140,7 +140,7 @@ func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deploym
 		// Add pod-template-hash information if it's not in the RS.
 		// Otherwise, new RS produced by Deployment will overlap with pre-existing ones
 		// that aren't constrained by the pod-template-hash.
-		syncedRS, err := dc.addHashKeyToRSAndPods(rs, podMap[rs.UID])
+		syncedRS, err := dc.addHashKeyToRSAndPods(rs, podMap[rs.UID], d.Status.CollisionCount)
 		if err != nil {
 			return nil, err
 		}
@@ -153,12 +153,15 @@ func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deploym
 // 1. Add hash label to the rs's pod template, and make sure the controller sees this update so that no orphaned pods will be created
 // 2. Add hash label to all pods this rs owns, wait until replicaset controller reports rs.Status.FullyLabeledReplicas equal to the desired number of replicas
 // 3. Add hash label to the rs's label and selector
-func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet, podList *v1.PodList) (*extensions.ReplicaSet, error) {
+func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet, podList *v1.PodList, collisionCount *int64) (*extensions.ReplicaSet, error) {
 	// If the rs already has the new hash label in its selector, it's done syncing
 	if labelsutil.SelectorHasLabel(rs.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey) {
 		return rs, nil
 	}
-	hash := deploymentutil.GetReplicaSetHash(rs)
+	hash, err := deploymentutil.GetReplicaSetHash(rs, collisionCount)
+	if err != nil {
+		return nil, err
+	}
 	// 1. Add hash template label to the rs. This ensures that any newly created pods will have the new label.
 	updatedRS, err := deploymentutil.UpdateRSWithRetries(dc.client.Extensions().ReplicaSets(rs.Namespace), dc.rsLister, rs.Namespace, rs.Name,
 		func(updated *extensions.ReplicaSet) error {
@@ -224,8 +227,8 @@ func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet,
 // 2. If there's existing new RS, update its revision number if it's smaller than (maxOldRevision + 1), where maxOldRevision is the max revision number among all old RSes.
 // 3. If there's no existing new RS and createIfNotExisted is true, create one with appropriate revision number (maxOldRevision + 1) and replicas.
 // Note that the pod-template-hash will be added to adopted RSes and pods.
-func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployment, rsList, oldRSs []*extensions.ReplicaSet, createIfNotExisted bool) (*extensions.ReplicaSet, error) {
-	existingNewRS, err := deploymentutil.FindNewReplicaSet(deployment, rsList)
+func (dc *DeploymentController) getNewReplicaSet(d *extensions.Deployment, rsList, oldRSs []*extensions.ReplicaSet, createIfNotExisted bool) (*extensions.ReplicaSet, error) {
+	existingNewRS, err := deploymentutil.FindNewReplicaSet(d, rsList)
 	if err != nil {
 		return nil, err
 	}
@@ -247,28 +250,28 @@ func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployme
 		rsCopy := objCopy.(*extensions.ReplicaSet)
 
 		// Set existing new replica set's annotation
-		annotationsUpdated := deploymentutil.SetNewReplicaSetAnnotations(deployment, rsCopy, newRevision, true)
-		minReadySecondsNeedsUpdate := rsCopy.Spec.MinReadySeconds != deployment.Spec.MinReadySeconds
+		annotationsUpdated := deploymentutil.SetNewReplicaSetAnnotations(d, rsCopy, newRevision, true)
+		minReadySecondsNeedsUpdate := rsCopy.Spec.MinReadySeconds != d.Spec.MinReadySeconds
 		if annotationsUpdated || minReadySecondsNeedsUpdate {
-			rsCopy.Spec.MinReadySeconds = deployment.Spec.MinReadySeconds
+			rsCopy.Spec.MinReadySeconds = d.Spec.MinReadySeconds
 			return dc.client.Extensions().ReplicaSets(rsCopy.ObjectMeta.Namespace).Update(rsCopy)
 		}
 
 		// Should use the revision in existingNewRS's annotation, since it set by before
-		updateConditions := deploymentutil.SetDeploymentRevision(deployment, rsCopy.Annotations[deploymentutil.RevisionAnnotation])
+		needsUpdate := deploymentutil.SetDeploymentRevision(d, rsCopy.Annotations[deploymentutil.RevisionAnnotation])
 		// If no other Progressing condition has been recorded and we need to estimate the progress
 		// of this deployment then it is likely that old users started caring about progress. In that
 		// case we need to take into account the first time we noticed their new replica set.
-		cond := deploymentutil.GetDeploymentCondition(deployment.Status, extensions.DeploymentProgressing)
-		if deployment.Spec.ProgressDeadlineSeconds != nil && cond == nil {
+		cond := deploymentutil.GetDeploymentCondition(d.Status, extensions.DeploymentProgressing)
+		if d.Spec.ProgressDeadlineSeconds != nil && cond == nil {
 			msg := fmt.Sprintf("Found new replica set %q", rsCopy.Name)
 			condition := deploymentutil.NewDeploymentCondition(extensions.DeploymentProgressing, v1.ConditionTrue, deploymentutil.FoundNewRSReason, msg)
-			deploymentutil.SetDeploymentCondition(&deployment.Status, *condition)
-			updateConditions = true
+			deploymentutil.SetDeploymentCondition(&d.Status, *condition)
+			needsUpdate = true
 		}
 
-		if updateConditions {
-			if deployment, err = dc.client.Extensions().Deployments(deployment.Namespace).UpdateStatus(deployment); err != nil {
+		if needsUpdate {
+			if d, err = dc.client.Extensions().Deployments(d.Namespace).UpdateStatus(d); err != nil {
 				return nil, err
 			}
 		}
@@ -280,72 +283,107 @@ func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployme
 	}
 
 	// new ReplicaSet does not exist, create one.
-	namespace := deployment.Namespace
-	podTemplateSpecHash := fmt.Sprintf("%d", deploymentutil.GetPodTemplateSpecHash(deployment.Spec.Template))
-	newRSTemplate := deploymentutil.GetNewReplicaSetTemplate(deployment)
-	newRSTemplate.Labels = labelsutil.CloneAndAddLabel(deployment.Spec.Template.Labels, extensions.DefaultDeploymentUniqueLabelKey, podTemplateSpecHash)
+	templateCopy, err := api.Scheme.DeepCopy(d.Spec.Template)
+	if err != nil {
+		return nil, err
+	}
+	newRSTemplate := templateCopy.(v1.PodTemplateSpec)
+	podTemplateSpecHash := fmt.Sprintf("%d", deploymentutil.GetPodTemplateSpecHash(&newRSTemplate, d.Status.CollisionCount))
+	newRSTemplate.Labels = labelsutil.CloneAndAddLabel(d.Spec.Template.Labels, extensions.DefaultDeploymentUniqueLabelKey, podTemplateSpecHash)
 	// Add podTemplateHash label to selector.
-	newRSSelector := labelsutil.CloneSelectorAndAddLabel(deployment.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey, podTemplateSpecHash)
+	newRSSelector := labelsutil.CloneSelectorAndAddLabel(d.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey, podTemplateSpecHash)
 
 	// Create new ReplicaSet
 	newRS := extensions.ReplicaSet{
 		ObjectMeta: metav1.ObjectMeta{
 			// Make the name deterministic, to ensure idempotence
-			Name:            deployment.Name + "-" + podTemplateSpecHash,
-			Namespace:       namespace,
-			OwnerReferences: []metav1.OwnerReference{*newControllerRef(deployment)},
+			Name:            d.Name + "-" + podTemplateSpecHash,
+			Namespace:       d.Namespace,
+			OwnerReferences: []metav1.OwnerReference{*newControllerRef(d)},
 		},
 		Spec: extensions.ReplicaSetSpec{
 			Replicas:        new(int32),
-			MinReadySeconds: deployment.Spec.MinReadySeconds,
+			MinReadySeconds: d.Spec.MinReadySeconds,
 			Selector:        newRSSelector,
 			Template:        newRSTemplate,
 		},
 	}
 	allRSs := append(oldRSs, &newRS)
-	newReplicasCount, err := deploymentutil.NewRSNewReplicas(deployment, allRSs, &newRS)
+	newReplicasCount, err := deploymentutil.NewRSNewReplicas(d, allRSs, &newRS)
 	if err != nil {
 		return nil, err
 	}
 
 	*(newRS.Spec.Replicas) = newReplicasCount
 	// Set new replica set's annotation
-	deploymentutil.SetNewReplicaSetAnnotations(deployment, &newRS, newRevision, false)
-	createdRS, err := dc.client.Extensions().ReplicaSets(namespace).Create(&newRS)
+	deploymentutil.SetNewReplicaSetAnnotations(d, &newRS, newRevision, false)
+	// Create the new ReplicaSet. If it already exists, then we need to check for possible
+	// hash collisions. If there is any other error, we need to report it in the status of
+	// the Deployment.
+	alreadyExists := false
+	createdRS, err := dc.client.Extensions().ReplicaSets(d.Namespace).Create(&newRS)
 	switch {
-	// We may end up hitting this due to a slow cache or a fast resync of the deployment.
-	// TODO: Restore once https://github.com/kubernetes/kubernetes/issues/29735 is fixed
-	// ie. we start using a new hashing algorithm.
+	// We may end up hitting this due to a slow cache or a fast resync of the Deployment.
+	// Fetch a copy of the ReplicaSet. If its PodTemplateSpec is semantically deep equal
+	// with the PodTemplateSpec of the Deployment, then that is our new ReplicaSet. Otherwise,
+	// this is a hash collision and we need to increment the collisionCount field in the
+	// status of the Deployment and try the creation again.
 	case errors.IsAlreadyExists(err):
-		return nil, err
-	//	return dc.rsLister.ReplicaSets(namespace).Get(newRS.Name)
+		alreadyExists = true
+		rs, rsErr := dc.rsLister.ReplicaSets(newRS.Namespace).Get(newRS.Name)
+		if rsErr != nil {
+			return nil, rsErr
+		}
+		isEqual, equalErr := deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template)
+		if equalErr != nil {
+			return nil, equalErr
+		}
+		// Matching ReplicaSet is not equal - increment the collisionCount in the DeploymentStatus
+		// and requeue the Deployment.
+		if !isEqual {
+			if d.Status.CollisionCount == nil {
+				d.Status.CollisionCount = new(int64)
+			}
+			preCollisionCount := *d.Status.CollisionCount
+			*d.Status.CollisionCount++
+			// Update the collisionCount for the Deployment and let it requeue by returning the original
+			// error.
+			_, dErr := dc.client.Extensions().Deployments(d.Namespace).UpdateStatus(d)
+			if dErr == nil {
+				glog.V(2).Infof("Found a hash collision for deployment %q - bumping collisionCount (%d->%d) to resolve it", d.Name, preCollisionCount, *d.Status.CollisionCount)
+			}
+			return nil, err
+		}
+		// Pass through the matching ReplicaSet as the new ReplicaSet.
+		createdRS = rs
+		err = nil
 	case err != nil:
 		msg := fmt.Sprintf("Failed to create new replica set %q: %v", newRS.Name, err)
-		if deployment.Spec.ProgressDeadlineSeconds != nil {
+		if d.Spec.ProgressDeadlineSeconds != nil {
 			cond := deploymentutil.NewDeploymentCondition(extensions.DeploymentProgressing, v1.ConditionFalse, deploymentutil.FailedRSCreateReason, msg)
-			deploymentutil.SetDeploymentCondition(&deployment.Status, *cond)
+			deploymentutil.SetDeploymentCondition(&d.Status, *cond)
 			// We don't really care about this error at this point, since we have a bigger issue to report.
-			// TODO: Update the rest of the Deployment status, too. We may need to do this every time we
-			// error out in all other places in the controller so that we let users know that their deployments
-			// have been noticed by the controller, albeit with errors.
 			// TODO: Identify which errors are permanent and switch DeploymentIsFailed to take into account
 			// these reasons as well. Related issue: https://github.com/kubernetes/kubernetes/issues/18568
-			_, _ = dc.client.Extensions().Deployments(deployment.ObjectMeta.Namespace).UpdateStatus(deployment)
+			_, _ = dc.client.Extensions().Deployments(d.Namespace).UpdateStatus(d)
 		}
-		dc.eventRecorder.Eventf(deployment, v1.EventTypeWarning, deploymentutil.FailedRSCreateReason, msg)
+		dc.eventRecorder.Eventf(d, v1.EventTypeWarning, deploymentutil.FailedRSCreateReason, msg)
 		return nil, err
 	}
-	if newReplicasCount > 0 {
-		dc.eventRecorder.Eventf(deployment, v1.EventTypeNormal, "ScalingReplicaSet", "Scaled up replica set %s to %d", createdRS.Name, newReplicasCount)
+	if !alreadyExists && newReplicasCount > 0 {
+		dc.eventRecorder.Eventf(d, v1.EventTypeNormal, "ScalingReplicaSet", "Scaled up replica set %s to %d", createdRS.Name, newReplicasCount)
 	}
 
-	deploymentutil.SetDeploymentRevision(deployment, newRevision)
-	if deployment.Spec.ProgressDeadlineSeconds != nil {
+	needsUpdate := deploymentutil.SetDeploymentRevision(d, newRevision)
+	if !alreadyExists && d.Spec.ProgressDeadlineSeconds != nil {
 		msg := fmt.Sprintf("Created new replica set %q", createdRS.Name)
 		condition := deploymentutil.NewDeploymentCondition(extensions.DeploymentProgressing, v1.ConditionTrue, deploymentutil.NewReplicaSetReason, msg)
-		deploymentutil.SetDeploymentCondition(&deployment.Status, *condition)
+		deploymentutil.SetDeploymentCondition(&d.Status, *condition)
+		needsUpdate = true
+	}
+	if needsUpdate {
+		_, err = dc.client.Extensions().Deployments(d.Namespace).UpdateStatus(d)
 	}
-	_, err = dc.client.Extensions().Deployments(deployment.Namespace).UpdateStatus(deployment)
 	return createdRS, err
 }
 
@@ -564,6 +602,7 @@ func calculateStatus(allRSs []*extensions.ReplicaSet, newRS *extensions.ReplicaS
 		ReadyReplicas:       deploymentutil.GetReadyReplicaCountForReplicaSets(allRSs),
 		AvailableReplicas:   availableReplicas,
 		UnavailableReplicas: unavailableReplicas,
+		CollisionCount:      deployment.Status.CollisionCount,
 	}
 
 	// Copy conditions one by one so we won't mutate the original object.

pkg/controller/deployment/util/BUILD

@@ -48,6 +48,7 @@ go_test(
     srcs = [
         "deployment_util_test.go",
         "hash_test.go",
+        "pod_util_test.go",
     ],
     library = ":go_default_library",
     tags = ["automanaged"],
@@ -57,6 +58,7 @@ go_test(
         "//pkg/apis/extensions/v1beta1:go_default_library",
         "//pkg/client/clientset_generated/clientset/fake:go_default_library",
         "//pkg/controller:go_default_library",
+        "//pkg/util/hash:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",

pkg/controller/deployment/util/deployment_util.go

@@ -640,29 +640,42 @@ func ListPods(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet
 // We ignore pod-template-hash because the hash result would be different upon podTemplateSpec API changes
 // (e.g. the addition of a new field will cause the hash code to change)
 // Note that we assume input podTemplateSpecs contain non-empty labels
-func EqualIgnoreHash(template1, template2 v1.PodTemplateSpec) bool {
+func EqualIgnoreHash(template1, template2 *v1.PodTemplateSpec) (bool, error) {
+	cp, err := api.Scheme.DeepCopy(template1)
+	if err != nil {
+		return false, err
+	}
+	t1Copy := cp.(*v1.PodTemplateSpec)
+	cp, err = api.Scheme.DeepCopy(template2)
+	if err != nil {
+		return false, err
+	}
+	t2Copy := cp.(*v1.PodTemplateSpec)
 	// First, compare template.Labels (ignoring hash)
-	labels1, labels2 := template1.Labels, template2.Labels
+	labels1, labels2 := t1Copy.Labels, t2Copy.Labels
 	if len(labels1) > len(labels2) {
 		labels1, labels2 = labels2, labels1
 	}
 	// We make sure len(labels2) >= len(labels1)
 	for k, v := range labels2 {
 		if labels1[k] != v && k != extensions.DefaultDeploymentUniqueLabelKey {
-			return false
+			return false, nil
 		}
 	}
 	// Then, compare the templates without comparing their labels
-	template1.Labels, template2.Labels = nil, nil
-	return apiequality.Semantic.DeepEqual(template1, template2)
+	t1Copy.Labels, t2Copy.Labels = nil, nil
+	return apiequality.Semantic.DeepEqual(t1Copy, t2Copy), nil
 }
 
 // FindNewReplicaSet returns the new RS this given deployment targets (the one with the same pod template).
 func FindNewReplicaSet(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) (*extensions.ReplicaSet, error) {
-	newRSTemplate := GetNewReplicaSetTemplate(deployment)
 	sort.Sort(controller.ReplicaSetsByCreationTimestamp(rsList))
 	for i := range rsList {
-		if EqualIgnoreHash(rsList[i].Spec.Template, newRSTemplate) {
+		equal, err := EqualIgnoreHash(&rsList[i].Spec.Template, &deployment.Spec.Template)
+		if err != nil {
+			return nil, err
+		}
+		if equal {
 			// In rare cases, such as after cluster upgrades, Deployment may end up with
 			// having more than one new ReplicaSets that have the same template as its template,
 			// see https://github.com/kubernetes/kubernetes/issues/40415
@@ -746,31 +759,6 @@ func LabelPodsWithHash(podList *v1.PodList, c clientset.Interface, podLister cor
 	return nil
 }
 
-// GetNewReplicaSetTemplate returns the desired PodTemplateSpec for the new ReplicaSet corresponding to the given ReplicaSet.
-// Callers of this helper need to set the DefaultDeploymentUniqueLabelKey k/v pair.
-func GetNewReplicaSetTemplate(deployment *extensions.Deployment) v1.PodTemplateSpec {
-	// newRS will have the same template as in deployment spec.
-	return v1.PodTemplateSpec{
-		ObjectMeta: deployment.Spec.Template.ObjectMeta,
-		Spec:       deployment.Spec.Template.Spec,
-	}
-}
-
-// TODO: remove the duplicate
-// GetNewReplicaSetTemplateInternal returns the desired PodTemplateSpec for the new ReplicaSet corresponding to the given ReplicaSet.
-func GetNewReplicaSetTemplateInternal(deployment *internalextensions.Deployment) api.PodTemplateSpec {
-	// newRS will have the same template as in deployment spec, plus a unique label in some cases.
-	newRSTemplate := api.PodTemplateSpec{
-		ObjectMeta: deployment.Spec.Template.ObjectMeta,
-		Spec:       deployment.Spec.Template.Spec,
-	}
-	newRSTemplate.ObjectMeta.Labels = labelsutil.CloneAndAddLabel(
-		deployment.Spec.Template.ObjectMeta.Labels,
-		internalextensions.DefaultDeploymentUniqueLabelKey,
-		fmt.Sprintf("%d", GetInternalPodTemplateSpecHash(newRSTemplate)))
-	return newRSTemplate
-}
-
 // SetFromReplicaSetTemplate sets the desired PodTemplateSpec from a replica set template to the given deployment.
 func SetFromReplicaSetTemplate(deployment *extensions.Deployment, template v1.PodTemplateSpec) *extensions.Deployment {
 	deployment.Spec.Template.ObjectMeta = template.ObjectMeta

pkg/controller/deployment/util/deployment_util_test.go

@@ -184,7 +184,8 @@ func newDControllerRef(d *extensions.Deployment) *metav1.OwnerReference {
 
 // generateRS creates a replica set, with the input deployment's template as its template
 func generateRS(deployment extensions.Deployment) extensions.ReplicaSet {
-	template := GetNewReplicaSetTemplate(&deployment)
+	cp, _ := api.Scheme.DeepCopy(deployment.Spec.Template)
+	template := cp.(v1.PodTemplateSpec)
 	return extensions.ReplicaSet{
 		ObjectMeta: metav1.ObjectMeta{
 			UID:             randomUID(),
@@ -193,7 +194,7 @@ func generateRS(deployment extensions.Deployment) extensions.ReplicaSet {
 			OwnerReferences: []metav1.OwnerReference{*newDControllerRef(&deployment)},
 		},
 		Spec: extensions.ReplicaSetSpec{
-			Replicas: func() *int32 { i := int32(0); return &i }(),
+			Replicas: new(int32),
 			Template: template,
 			Selector: &metav1.LabelSelector{MatchLabels: template.Labels},
 		},
@@ -444,29 +445,22 @@ func TestEqualIgnoreHash(t *testing.T) {
 
 	for _, test := range tests {
 		runTest := func(t1, t2 *v1.PodTemplateSpec, reversed bool) {
-			// Set up
-			t1Copy, err := api.Scheme.DeepCopy(t1)
-			if err != nil {
-				t.Errorf("Failed setting up the test: %v", err)
-			}
-			t2Copy, err := api.Scheme.DeepCopy(t2)
-			if err != nil {
-				t.Errorf("Failed setting up the test: %v", err)
-			}
 			reverseString := ""
 			if reversed {
 				reverseString = " (reverse order)"
 			}
 			// Run
-			equal := EqualIgnoreHash(*t1, *t2)
+			equal, err := EqualIgnoreHash(t1, t2)
+			if err != nil {
+				t.Errorf("%s: unexpected error: %v", err, test.test)
+				return
+			}
 			if equal != test.expected {
-				t.Errorf("In test case %q%s, expected %v", test.test, reverseString, test.expected)
+				t.Errorf("%q%s: expected %v", test.test, reverseString, test.expected)
+				return
 			}
 			if t1.Labels == nil || t2.Labels == nil {
-				t.Errorf("In test case %q%s, unexpected labels becomes nil", test.test, reverseString)
-			}
-			if !reflect.DeepEqual(t1, t1Copy) || !reflect.DeepEqual(t2, t2Copy) {
-				t.Errorf("In test case %q%s, unexpected input template modified", test.test, reverseString)
+				t.Errorf("%q%s: unexpected labels becomes nil", test.test, reverseString)
 			}
 		}
 		runTest(&test.former, &test.latter, false)

pkg/controller/deployment/util/hash_test.go

@@ -18,11 +18,13 @@ package util
 
 import (
 	"encoding/json"
+	"hash/adler32"
 	"strconv"
 	"strings"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api/v1"
+	hashutil "k8s.io/kubernetes/pkg/util/hash"
 )
 
 var podSpec string = `
@@ -103,34 +105,12 @@ var podSpec string = `
 
 func TestPodTemplateSpecHash(t *testing.T) {
 	seenHashes := make(map[uint32]int)
-	broken := false
 
 	for i := 0; i < 1000; i++ {
 		specJson := strings.Replace(podSpec, "@@VERSION@@", strconv.Itoa(i), 1)
 		spec := v1.PodTemplateSpec{}
 		json.Unmarshal([]byte(specJson), &spec)
-		hash := GetPodTemplateSpecHash(spec)
-		if v, ok := seenHashes[hash]; ok {
-			broken = true
-			t.Logf("Hash collision, old: %d new: %d", v, i)
-			break
-		}
-		seenHashes[hash] = i
-	}
-
-	if !broken {
-		t.Errorf("expected adler to break but it didn't")
-	}
-}
-
-func TestPodTemplateSpecHashFnv(t *testing.T) {
-	seenHashes := make(map[uint32]int)
-
-	for i := 0; i < 1000; i++ {
-		specJson := strings.Replace(podSpec, "@@VERSION@@", strconv.Itoa(i), 1)
-		spec := v1.PodTemplateSpec{}
-		json.Unmarshal([]byte(specJson), &spec)
-		hash := GetPodTemplateSpecHashFnv(spec)
+		hash := GetPodTemplateSpecHash(&spec, nil)
 		if v, ok := seenHashes[hash]; ok {
 			t.Errorf("Hash collision, old: %d new: %d", v, i)
 			break
@@ -144,15 +124,21 @@ func BenchmarkAdler(b *testing.B) {
 	json.Unmarshal([]byte(podSpec), &spec)
 
 	for i := 0; i < b.N; i++ {
-		GetPodTemplateSpecHash(spec)
+		getPodTemplateSpecOldHash(spec)
 	}
 }
 
+func getPodTemplateSpecOldHash(template v1.PodTemplateSpec) uint32 {
+	podTemplateSpecHasher := adler32.New()
+	hashutil.DeepHashObject(podTemplateSpecHasher, template)
+	return podTemplateSpecHasher.Sum32()
+}
+
 func BenchmarkFnv(b *testing.B) {
 	spec := v1.PodTemplateSpec{}
 	json.Unmarshal([]byte(podSpec), &spec)
 
 	for i := 0; i < b.N; i++ {
-		GetPodTemplateSpecHashFnv(spec)
+		GetPodTemplateSpecHash(&spec, nil)
 	}
 }

pkg/controller/deployment/util/pod_util.go

@@ -17,7 +17,7 @@ limitations under the License.
 package util
 
 import (
-	"hash/adler32"
+	"encoding/binary"
 	"hash/fnv"
 
 	"github.com/golang/glog"
@@ -31,22 +31,17 @@ import (
 	hashutil "k8s.io/kubernetes/pkg/util/hash"
 )
 
-func GetPodTemplateSpecHash(template v1.PodTemplateSpec) uint32 {
-	podTemplateSpecHasher := adler32.New()
-	hashutil.DeepHashObject(podTemplateSpecHasher, template)
-	return podTemplateSpecHasher.Sum32()
-}
-
-// TODO: remove the duplicate
-func GetInternalPodTemplateSpecHash(template api.PodTemplateSpec) uint32 {
-	podTemplateSpecHasher := adler32.New()
-	hashutil.DeepHashObject(podTemplateSpecHasher, template)
-	return podTemplateSpecHasher.Sum32()
-}
-
-func GetPodTemplateSpecHashFnv(template v1.PodTemplateSpec) uint32 {
+func GetPodTemplateSpecHash(template *v1.PodTemplateSpec, uniquifier *int64) uint32 {
 	podTemplateSpecHasher := fnv.New32a()
-	hashutil.DeepHashObject(podTemplateSpecHasher, template)
+	hashutil.DeepHashObject(podTemplateSpecHasher, *template)
+
+	// Add uniquifier in the hash if it exists.
+	if uniquifier != nil {
+		uniquifierBytes := make([]byte, 8)
+		binary.LittleEndian.PutUint64(uniquifierBytes, uint64(*uniquifier))
+		podTemplateSpecHasher.Write(uniquifierBytes)
+	}
+
 	return podTemplateSpecHasher.Sum32()
 }
 

pkg/controller/deployment/util/pod_util_test.go

@@ -0,0 +1,59 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"math"
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api/v1"
+)
+
+func int64P(num int64) *int64 {
+	return &num
+}
+
+func TestGetPodTemplateSpecHash(t *testing.T) {
+	tests := []struct {
+		name                string
+		template            *v1.PodTemplateSpec
+		collisionCount      *int64
+		otherCollisionCount *int64
+	}{
+		{
+			name:                "simple",
+			template:            &v1.PodTemplateSpec{},
+			collisionCount:      int64P(1),
+			otherCollisionCount: int64P(2),
+		},
+		{
+			name:                "using math.MaxInt64",
+			template:            &v1.PodTemplateSpec{},
+			collisionCount:      nil,
+			otherCollisionCount: int64P(int64(math.MaxInt64)),
+		},
+	}
+
+	for _, test := range tests {
+		hash := GetPodTemplateSpecHash(test.template, test.collisionCount)
+		otherHash := GetPodTemplateSpecHash(test.template, test.otherCollisionCount)
+
+		if hash == otherHash {
+			t.Errorf("expected different hashes but got the same: %d", hash)
+		}
+	}
+}

pkg/controller/deployment/util/replicaset_util.go

@@ -69,21 +69,12 @@ func UpdateRSWithRetries(rsClient unversionedextensions.ReplicaSetInterface, rsL
 }
 
 // GetReplicaSetHash returns the pod template hash of a ReplicaSet's pod template space
-func GetReplicaSetHash(rs *extensions.ReplicaSet) string {
-	meta := rs.Spec.Template.ObjectMeta
-	meta.Labels = labelsutil.CloneAndRemoveLabel(meta.Labels, extensions.DefaultDeploymentUniqueLabelKey)
-	return fmt.Sprintf("%d", GetPodTemplateSpecHash(v1.PodTemplateSpec{
-		ObjectMeta: meta,
-		Spec:       rs.Spec.Template.Spec,
-	}))
-}
-
-// GetReplicaSetHashFnv returns the pod template hash of a ReplicaSet's pod template spec.
-func GetReplicaSetHashFnv(rs *extensions.ReplicaSet) string {
-	meta := rs.Spec.Template.ObjectMeta
-	meta.Labels = labelsutil.CloneAndRemoveLabel(meta.Labels, extensions.DefaultDeploymentUniqueLabelKey)
-	return fmt.Sprintf("%d", GetPodTemplateSpecHashFnv(v1.PodTemplateSpec{
-		ObjectMeta: meta,
-		Spec:       rs.Spec.Template.Spec,
-	}))
+func GetReplicaSetHash(rs *extensions.ReplicaSet, uniquifier *int64) (string, error) {
+	template, err := api.Scheme.DeepCopy(rs.Spec.Template)
+	if err != nil {
+		return "", err
+	}
+	rsTemplate := template.(v1.PodTemplateSpec)
+	rsTemplate.Labels = labelsutil.CloneAndRemoveLabel(rsTemplate.Labels, extensions.DefaultDeploymentUniqueLabelKey)
+	return fmt.Sprintf("%d", GetPodTemplateSpecHash(&rsTemplate, uniquifier)), nil
 }