diff --git a/pkg/controller/deployment/rollback.go b/pkg/controller/deployment/rollback.go index 4e0e2ab0a77..89d840f7f3b 100644 --- a/pkg/controller/deployment/rollback.go +++ b/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; diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 4006ee2379e..c208ef18854 100644 --- a/pkg/controller/deployment/sync.go +++ b/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) } diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 62c814cc8e0..e9bd1d6317b 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/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. @@ -567,7 +562,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)) @@ -640,7 +635,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) @@ -651,43 +646,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 { @@ -698,7 +686,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. diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index a26e9dc8894..28e609570e3 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -396,11 +396,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 @@ -463,8 +459,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) } }) } @@ -531,15 +527,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) } }) }