Simplify some deployment utilities

Sponsored by the recent refactoring that removed errors
from deep copies.

Signed-off-by: Michalis Kargakis <mkargaki@redhat.com>

pkg/controller/deployment/rollback.go

@@ -73,11 +73,7 @@ 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
-	isEqual, err := deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template)
-	if err != nil {
-		return false, err
-	}
-	if !isEqual {
+	if !deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template) {
 		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

@@ -122,10 +122,7 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *extensions.D
 	if err != nil {
 		return nil, nil, fmt.Errorf("error labeling replica sets and pods with pod-template-hash: %v", err)
 	}
-	_, allOldRSs, err := deploymentutil.FindOldReplicaSets(d, rsList)
-	if err != nil {
-		return nil, nil, err
-	}
+	_, allOldRSs := deploymentutil.FindOldReplicaSets(d, rsList)
 
 	// Get new replica set with the updated revision number
 	newRS, err := dc.getNewReplicaSet(d, rsList, allOldRSs, createIfNotExisted)
@@ -235,10 +232,7 @@ func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet,
 // 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(d *extensions.Deployment, rsList, oldRSs []*extensions.ReplicaSet, createIfNotExisted bool) (*extensions.ReplicaSet, error) {
-	existingNewRS, err := deploymentutil.FindNewReplicaSet(d, rsList)
-	if err != nil {
-		return nil, err
-	}
+	existingNewRS := deploymentutil.FindNewReplicaSet(d, rsList)
 
 	// Calculate the max revision number among all old RSes
 	maxOldRevision := deploymentutil.MaxRevision(oldRSs)
@@ -274,6 +268,7 @@ func (dc *DeploymentController) getNewReplicaSet(d *extensions.Deployment, rsLis
 		}
 
 		if needsUpdate {
+			var err error
 			if d, err = dc.client.Extensions().Deployments(d.Namespace).UpdateStatus(d); err != nil {
 				return nil, err
 			}
@@ -333,13 +328,9 @@ func (dc *DeploymentController) getNewReplicaSet(d *extensions.Deployment, rsLis
 		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 !deploymentutil.EqualIgnoreHash(&d.Spec.Template, &rs.Spec.Template) {
 			if d.Status.CollisionCount == nil {
 				d.Status.CollisionCount = new(int32)
 			}

pkg/controller/deployment/util/deployment_util.go

@@ -502,14 +502,8 @@ func GetAllReplicaSets(deployment *extensions.Deployment, c extensionsv1beta1.Ex
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-	newRS, err := FindNewReplicaSet(deployment, rsList)
-	if err != nil {
-		return nil, nil, nil, err
-	}
+	oldRSes, allOldRSes := FindOldReplicaSets(deployment, rsList)
+	newRS := FindNewReplicaSet(deployment, rsList)
 	return oldRSes, allOldRSes, newRS, nil
 }
 
@@ -520,7 +514,8 @@ func GetOldReplicaSets(deployment *extensions.Deployment, c extensionsv1beta1.Ex
 	if err != nil {
 		return nil, nil, err
 	}
-	return FindOldReplicaSets(deployment, rsList)
+	oldRSes, allOldRSes := FindOldReplicaSets(deployment, rsList)
+	return oldRSes, allOldRSes, nil
 }
 
 // GetNewReplicaSet returns a replica set that matches the intent of the given deployment; get ReplicaSetList from client interface.
@@ -530,7 +525,7 @@ func GetNewReplicaSet(deployment *extensions.Deployment, c extensionsv1beta1.Ext
 	if err != nil {
 		return nil, err
 	}
-	return FindNewReplicaSet(deployment, rsList)
+	return FindNewReplicaSet(deployment, rsList), nil
 }
 
 // RsListFromClient returns an rsListFunc that wraps the given client.
@@ -574,7 +569,7 @@ func ListReplicaSets(deployment *extensions.Deployment, getRSList RsListFunc) ([
 	options := metav1.ListOptions{LabelSelector: selector.String()}
 	all, err := getRSList(namespace, options)
 	if err != nil {
-		return all, err
+		return nil, err
 	}
 	// Only include those whose ControllerRef matches the Deployment.
 	owned := make([]*extensions.ReplicaSet, 0, len(all))
@@ -647,7 +642,7 @@ 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, error) {
+func EqualIgnoreHash(template1, template2 *v1.PodTemplateSpec) bool {
 	t1Copy := template1.DeepCopy()
 	t2Copy := template2.DeepCopy()
 	// First, compare template.Labels (ignoring hash)
@@ -658,43 +653,36 @@ func EqualIgnoreHash(template1, template2 *v1.PodTemplateSpec) (bool, error) {
 	// We make sure len(labels2) >= len(labels1)
 	for k, v := range labels2 {
 		if labels1[k] != v && k != extensions.DefaultDeploymentUniqueLabelKey {
-			return false, nil
+			return false
 		}
 	}
 	// Then, compare the templates without comparing their labels
 	t1Copy.Labels, t2Copy.Labels = nil, nil
-	return apiequality.Semantic.DeepEqual(t1Copy, t2Copy), nil
+	return apiequality.Semantic.DeepEqual(t1Copy, t2Copy)
 }
 
 // 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) {
+func FindNewReplicaSet(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) *extensions.ReplicaSet {
 	sort.Sort(controller.ReplicaSetsByCreationTimestamp(rsList))
 	for i := range rsList {
-		equal, err := EqualIgnoreHash(&rsList[i].Spec.Template, &deployment.Spec.Template)
-		if err != nil {
-			return nil, err
-		}
-		if equal {
+		if EqualIgnoreHash(&rsList[i].Spec.Template, &deployment.Spec.Template) {
 			// 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
 			// We deterministically choose the oldest new ReplicaSet.
-			return rsList[i], nil
+			return rsList[i]
 		}
 	}
 	// new ReplicaSet does not exist.
-	return nil, nil
+	return nil
 }
 
 // FindOldReplicaSets returns the old replica sets targeted by the given Deployment, with the given slice of RSes.
 // Note that the first set of old replica sets doesn't include the ones with no pods, and the second set of old replica sets include all old replica sets.
-func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, error) {
+func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet) {
 	var requiredRSs []*extensions.ReplicaSet
 	var allRSs []*extensions.ReplicaSet
-	newRS, err := FindNewReplicaSet(deployment, rsList)
-	if err != nil {
-		return nil, nil, err
-	}
+	newRS := FindNewReplicaSet(deployment, rsList)
 	for _, rs := range rsList {
 		// Filter out new replica set
 		if newRS != nil && rs.UID == newRS.UID {
@@ -705,7 +693,7 @@ func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions.
 			requiredRSs = append(requiredRSs, rs)
 		}
 	}
-	return requiredRSs, allRSs, nil
+	return requiredRSs, allRSs
 }
 
 // WaitForReplicaSetUpdated polls the replica set until it is updated.

pkg/controller/deployment/util/deployment_util_test.go

@@ -454,11 +454,7 @@ func TestEqualIgnoreHash(t *testing.T) {
 					reverseString = " (reverse order)"
 				}
 				// Run
-				equal, err := EqualIgnoreHash(t1, t2)
-				if err != nil {
-					t.Errorf("%s: unexpected error: %v", err, test.Name)
-					return
-				}
+				equal := EqualIgnoreHash(t1, t2)
 				if equal != test.expected {
 					t.Errorf("%q%s: expected %v", test.Name, reverseString, test.expected)
 					return
@@ -521,8 +517,8 @@ func TestFindNewReplicaSet(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.Name, func(t *testing.T) {
-			if rs, err := FindNewReplicaSet(&test.deployment, test.rsList); !reflect.DeepEqual(rs, test.expected) || err != nil {
-				t.Errorf("In test case %q, expected %#v, got %#v: %v", test.Name, test.expected, rs, err)
+			if rs := FindNewReplicaSet(&test.deployment, test.rsList); !reflect.DeepEqual(rs, test.expected) {
+				t.Errorf("In test case %q, expected %#v, got %#v", test.Name, test.expected, rs)
 			}
 		})
 	}
@@ -589,15 +585,15 @@ func TestFindOldReplicaSets(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.Name, func(t *testing.T) {
-			requireRS, allRS, err := FindOldReplicaSets(&test.deployment, test.rsList)
+			requireRS, allRS := FindOldReplicaSets(&test.deployment, test.rsList)
 			sort.Sort(controller.ReplicaSetsByCreationTimestamp(allRS))
 			sort.Sort(controller.ReplicaSetsByCreationTimestamp(test.expected))
-			if !reflect.DeepEqual(allRS, test.expected) || err != nil {
-				t.Errorf("In test case %q, expected %#v, got %#v: %v", test.Name, test.expected, allRS, err)
+			if !reflect.DeepEqual(allRS, test.expected) {
+				t.Errorf("In test case %q, expected %#v, got %#v", test.Name, test.expected, allRS)
 			}
 			// RSs are getting filtered correctly by rs.spec.replicas
-			if !reflect.DeepEqual(requireRS, test.expectedRequire) || err != nil {
-				t.Errorf("In test case %q, expected %#v, got %#v: %v", test.Name, test.expectedRequire, requireRS, err)
+			if !reflect.DeepEqual(requireRS, test.expectedRequire) {
+				t.Errorf("In test case %q, expected %#v, got %#v", test.Name, test.expectedRequire, requireRS)
 			}
 		})
 	}