diff --git a/pkg/controller/deployment/rollback.go b/pkg/controller/deployment/rollback.go index 703cf682b19..acc2bc45980 100644 --- a/pkg/controller/deployment/rollback.go +++ b/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; diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 1355d3eda05..ea6ff569a99 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/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 diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index b9e9c10d30f..d3d0b591077 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -31,7 +31,6 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" core "k8s.io/client-go/testing" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/fake" @@ -444,29 +443,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)