diff --git a/pkg/util/deployment/deployment.go b/pkg/util/deployment/deployment.go index 0c348657401..192648bf8cf 100644 --- a/pkg/util/deployment/deployment.go +++ b/pkg/util/deployment/deployment.go @@ -140,11 +140,34 @@ func ListPods(deployment *extensions.Deployment, getPodList podListFunc) (*api.P return getPodList(namespace, options) } +// equalIgnoreHash returns true if two given podTemplateSpec are equal, ignoring the diff in value of Labels[pod-template-hash] +// 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 api.PodTemplateSpec) (bool, error) { + // The podTemplateSpec must have a non-empty label so that label selectors can find them. + // This is checked by validation (of resources contain a podTemplateSpec). + if len(template1.Labels) == 0 || len(template2.Labels) == 0 { + return false, fmt.Errorf("Unexpected empty labels found in given template") + } + hash1 := template1.Labels[extensions.DefaultDeploymentUniqueLabelKey] + hash2 := template2.Labels[extensions.DefaultDeploymentUniqueLabelKey] + // compare equality ignoring pod-template-hash + template1.Labels[extensions.DefaultDeploymentUniqueLabelKey] = hash2 + result := api.Semantic.DeepEqual(template1, template2) + template1.Labels[extensions.DefaultDeploymentUniqueLabelKey] = hash1 + return result, 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) for i := range rsList { - if api.Semantic.DeepEqual(rsList[i].Spec.Template, newRSTemplate) { + equal, err := equalIgnoreHash(rsList[i].Spec.Template, newRSTemplate) + if err != nil { + return nil, err + } + if equal { // This is the new ReplicaSet. return &rsList[i], nil } @@ -169,7 +192,11 @@ func FindOldReplicaSets(deployment *extensions.Deployment, rsList []extensions.R return nil, nil, fmt.Errorf("invalid label selector: %v", err) } // Filter out replica set that has the same pod template spec as the deployment - that is the new replica set. - if api.Semantic.DeepEqual(rs.Spec.Template, newRSTemplate) { + equal, err := equalIgnoreHash(rs.Spec.Template, newRSTemplate) + if err != nil { + return nil, nil, err + } + if equal { continue } allOldRSs[rs.ObjectMeta.Name] = rs diff --git a/pkg/util/deployment/deployment_test.go b/pkg/util/deployment/deployment_test.go index 31d23d2c18b..61d3a163114 100644 --- a/pkg/util/deployment/deployment_test.go +++ b/pkg/util/deployment/deployment_test.go @@ -168,6 +168,9 @@ func generateRSWithLabel(labels map[string]string, image string) extensions.Repl Replicas: 1, Selector: &unversioned.LabelSelector{MatchLabels: labels}, Template: api.PodTemplateSpec{ + ObjectMeta: api.ObjectMeta{ + Labels: labels, + }, Spec: api.PodSpec{ Containers: []api.Container{ { @@ -388,6 +391,190 @@ func TestGetOldRCs(t *testing.T) { } } +func generatePodTemplateSpec(name, nodeName string, annotations, labels map[string]string) api.PodTemplateSpec { + return api.PodTemplateSpec{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Annotations: annotations, + Labels: labels, + }, + Spec: api.PodSpec{ + NodeName: nodeName, + }, + } +} + +func TestEqualIgnoreHash(t *testing.T) { + tests := []struct { + test string + former, latter api.PodTemplateSpec + expected bool + }{ + { + "Same spec, same labels", + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + true, + }, + { + "Same spec, only pod-template-hash label value is different", + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-2", "something": "else"}), + true, + }, + { + "Same spec, the former doesn't have pod-template-hash label", + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{"something": "else"}), + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-2", "something": "else"}), + true, + }, + { + "Same spec, the label is different, and the pod-template-hash label value is the same", + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1"}), + generatePodTemplateSpec("foo", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + false, + }, + { + "Different spec, same labels", + generatePodTemplateSpec("foo", "foo-node", map[string]string{"former": "value"}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + generatePodTemplateSpec("foo", "foo-node", map[string]string{"latter": "value"}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + false, + }, + { + "Different spec, different pod-template-hash label value", + generatePodTemplateSpec("foo-1", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-1", "something": "else"}), + generatePodTemplateSpec("foo-2", "foo-node", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-2", "something": "else"}), + false, + }, + { + "Different spec, the former doesn't have pod-template-hash label", + generatePodTemplateSpec("foo-1", "foo-node-1", map[string]string{}, map[string]string{"something": "else"}), + generatePodTemplateSpec("foo-2", "foo-node-2", map[string]string{}, map[string]string{extensions.DefaultDeploymentUniqueLabelKey: "value-2", "something": "else"}), + false, + }, + { + "Different spec, different labels", + generatePodTemplateSpec("foo", "foo-node-1", map[string]string{}, map[string]string{"something": "else"}), + generatePodTemplateSpec("foo", "foo-node-2", map[string]string{}, map[string]string{"nothing": "else"}), + false, + }, + } + + for _, test := range tests { + equal, err := equalIgnoreHash(test.former, test.latter) + if err != nil { + t.Errorf("In test case %q, returned unexpected error %v", test.test, err) + } + if equal != test.expected { + t.Errorf("In test case %q, expected %v", test.test, test.expected) + } + equal, err = equalIgnoreHash(test.latter, test.former) + if err != nil { + t.Errorf("In test case %q (reverse order), returned unexpected error %v", test.test, err) + } + if equal != test.expected { + t.Errorf("In test case %q (reverse order), expected %v", test.test, test.expected) + } + } +} + +func TestFindNewReplicaSet(t *testing.T) { + deployment := generateDeployment("nginx") + newRS := generateRS(deployment) + newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "different-hash" + oldDeployment := generateDeployment("nginx") + oldDeployment.Spec.Template.Spec.Containers[0].Name = "nginx-old-1" + oldRS := generateRS(oldDeployment) + oldRS.Status.FullyLabeledReplicas = oldRS.Spec.Replicas + + tests := []struct { + test string + deployment extensions.Deployment + rsList []extensions.ReplicaSet + expected *extensions.ReplicaSet + }{ + { + test: "Get new ReplicaSet with the same spec but different pod-template-hash value", + deployment: deployment, + rsList: []extensions.ReplicaSet{newRS, oldRS}, + expected: &newRS, + }, + { + test: "Get nil new ReplicaSet", + deployment: deployment, + rsList: []extensions.ReplicaSet{oldRS}, + expected: nil, + }, + } + + for _, test := range tests { + 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.test, test.expected, rs, err) + } + } +} + +func TestFindOldReplicaSets(t *testing.T) { + deployment := generateDeployment("nginx") + newRS := generateRS(deployment) + newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "different-hash" + oldDeployment := generateDeployment("nginx") + oldDeployment.Spec.Template.Spec.Containers[0].Name = "nginx-old-1" + oldRS := generateRS(oldDeployment) + oldRS.Status.FullyLabeledReplicas = oldRS.Spec.Replicas + newPod := generatePodFromRS(newRS) + oldPod := generatePodFromRS(oldRS) + + tests := []struct { + test string + deployment extensions.Deployment + rsList []extensions.ReplicaSet + podList *api.PodList + expected []*extensions.ReplicaSet + }{ + { + test: "Get old ReplicaSets", + deployment: deployment, + rsList: []extensions.ReplicaSet{newRS, oldRS}, + podList: &api.PodList{ + Items: []api.Pod{ + newPod, + oldPod, + }, + }, + expected: []*extensions.ReplicaSet{&oldRS}, + }, + { + test: "Get old ReplicaSets with no new ReplicaSet", + deployment: deployment, + rsList: []extensions.ReplicaSet{oldRS}, + podList: &api.PodList{ + Items: []api.Pod{ + oldPod, + }, + }, + expected: []*extensions.ReplicaSet{&oldRS}, + }, + { + test: "Get empty old ReplicaSets", + deployment: deployment, + rsList: []extensions.ReplicaSet{newRS}, + podList: &api.PodList{ + Items: []api.Pod{ + newPod, + }, + }, + expected: []*extensions.ReplicaSet{}, + }, + } + + for _, test := range tests { + if old, _, err := FindOldReplicaSets(&test.deployment, test.rsList, test.podList); !reflect.DeepEqual(old, test.expected) || err != nil { + t.Errorf("In test case %q, expected %#v, got %#v: %v", test.test, test.expected, old, err) + } + } +} + // equal compares the equality of two ReplicaSet slices regardless of their ordering func equal(rss1, rss2 []*extensions.ReplicaSet) bool { if reflect.DeepEqual(rss1, rss2) { diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 0eb5caf9317..e57a13e2a24 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -62,6 +62,7 @@ import ( "k8s.io/kubernetes/pkg/types" "k8s.io/kubernetes/pkg/util" deploymentutil "k8s.io/kubernetes/pkg/util/deployment" + labelsutil "k8s.io/kubernetes/pkg/util/labels" "k8s.io/kubernetes/pkg/util/sets" "k8s.io/kubernetes/pkg/util/wait" utilyaml "k8s.io/kubernetes/pkg/util/yaml" @@ -2873,6 +2874,12 @@ func WaitForDeploymentStatus(c clientset.Interface, ns, deploymentName string, d return false, nil } allRSs = append(oldRSs, newRS) + // The old/new ReplicaSets need to contain the pod-template-hash label + for i := range allRSs { + if !labelsutil.SelectorHasLabel(allRSs[i].Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey) { + return false, nil + } + } totalCreated := deploymentutil.GetReplicaCountForReplicaSets(allRSs) totalAvailable, err := deploymentutil.GetAvailablePodsForReplicaSets(c, allRSs, minReadySeconds) if err != nil { @@ -2958,8 +2965,9 @@ func WaitForDeploymentRevisionAndImage(c clientset.Interface, ns, deploymentName if err != nil { return false, err } + // The new ReplicaSet needs to be non-nil and contain the pod-template-hash label newRS, err = deploymentutil.GetNewReplicaSet(deployment, c) - if err != nil || newRS == nil { + if err != nil || newRS == nil || !labelsutil.SelectorHasLabel(newRS.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey) { return false, err } // Check revision of this deployment, and of the new replica set of this deployment