diff --git a/pkg/controller/deployment/rollback.go b/pkg/controller/deployment/rollback.go index b04d67026c..2500e57c92 100644 --- a/pkg/controller/deployment/rollback.go +++ b/pkg/controller/deployment/rollback.go @@ -18,7 +18,6 @@ package deployment import ( "fmt" - "reflect" "github.com/golang/glog" "k8s.io/kubernetes/pkg/api/v1" @@ -66,7 +65,7 @@ func (dc *DeploymentController) rollback(deployment *extensions.Deployment, toRe } func (dc *DeploymentController) rollbackToTemplate(deployment *extensions.Deployment, rs *extensions.ReplicaSet) (d *extensions.Deployment, performedRollback bool, err error) { - if !reflect.DeepEqual(deploymentutil.GetNewReplicaSetTemplate(deployment), rs.Spec.Template) { + if !deploymentutil.EqualIgnoreHash(deployment.Spec.Template, rs.Spec.Template) { glog.Infof("Rolling back deployment %s to template spec %+v", deployment.Name, rs.Spec.Template.Spec) deploymentutil.SetFromReplicaSetTemplate(deployment, 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 d341028d00..604cde95f3 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -316,8 +316,9 @@ func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployme // new ReplicaSet does not exist, create one. namespace := deployment.Namespace - podTemplateSpecHash := deploymentutil.GetPodTemplateSpecHash(deployment.Spec.Template) + podTemplateSpecHash := fmt.Sprintf("%d", deploymentutil.GetPodTemplateSpecHash(deployment.Spec.Template)) newRSTemplate := deploymentutil.GetNewReplicaSetTemplate(deployment) + newRSTemplate.Labels = labelsutil.CloneAndAddLabel(deployment.Spec.Template.Labels, extensions.DefaultDeploymentUniqueLabelKey, podTemplateSpecHash) // Add podTemplateHash label to selector. newRSSelector := labelsutil.CloneSelectorAndAddLabel(deployment.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey, podTemplateSpecHash) @@ -325,7 +326,7 @@ func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployme newRS := extensions.ReplicaSet{ ObjectMeta: v1.ObjectMeta{ // Make the name deterministic, to ensure idempotence - Name: deployment.Name + "-" + fmt.Sprintf("%d", podTemplateSpecHash), + Name: deployment.Name + "-" + podTemplateSpecHash, Namespace: namespace, }, Spec: extensions.ReplicaSetSpec{ diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 4d59683088..0cb1dc4e64 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -587,43 +587,32 @@ func ListPods(deployment *extensions.Deployment, getPodList podListFunc) (*v1.Po return getPodList(namespace, options) } -// equalIgnoreHash returns true if two given podTemplateSpec are equal, ignoring the diff in value of Labels[pod-template-hash] +// 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 v1.PodTemplateSpec) (bool, error) { +func EqualIgnoreHash(template1, template2 v1.PodTemplateSpec) bool { // First, compare template.Labels (ignoring hash) labels1, labels2 := template1.Labels, template2.Labels - // 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(labels1) == 0 || len(labels2) == 0 { - return false, fmt.Errorf("Unexpected empty labels found in given template") - } 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, nil + return false } } - // Then, compare the templates without comparing their labels template1.Labels, template2.Labels = nil, nil - result := api.Semantic.DeepEqual(template1, template2) - return result, nil + return api.Semantic.DeepEqual(template1, template2) } // 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 { - equal, err := equalIgnoreHash(rsList[i].Spec.Template, newRSTemplate) - if err != nil { - return nil, err - } - if equal { + if EqualIgnoreHash(rsList[i].Spec.Template, newRSTemplate) { // This is the new ReplicaSet. return rsList[i], nil } @@ -648,11 +637,7 @@ func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions. 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. - equal, err := equalIgnoreHash(rs.Spec.Template, newRSTemplate) - if err != nil { - return nil, nil, err - } - if equal { + if EqualIgnoreHash(rs.Spec.Template, newRSTemplate) { continue } allOldRSs[rs.ObjectMeta.Name] = rs @@ -722,17 +707,13 @@ func LabelPodsWithHash(podList *v1.PodList, c clientset.Interface, podLister *ca } // 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, plus a unique label in some cases. - newRSTemplate := 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, } - newRSTemplate.ObjectMeta.Labels = labelsutil.CloneAndAddLabel( - deployment.Spec.Template.ObjectMeta.Labels, - extensions.DefaultDeploymentUniqueLabelKey, - GetPodTemplateSpecHash(newRSTemplate)) - return newRSTemplate } // TODO: remove the duplicate @@ -746,7 +727,7 @@ func GetNewReplicaSetTemplateInternal(deployment *internalextensions.Deployment) newRSTemplate.ObjectMeta.Labels = labelsutil.CloneAndAddLabel( deployment.Spec.Template.ObjectMeta.Labels, internalextensions.DefaultDeploymentUniqueLabelKey, - GetInternalPodTemplateSpecHash(newRSTemplate)) + fmt.Sprintf("%d", GetInternalPodTemplateSpecHash(newRSTemplate))) return newRSTemplate } diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index d57a3dab83..f893c255ff 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -446,11 +446,7 @@ func TestEqualIgnoreHash(t *testing.T) { reverseString = " (reverse order)" } // Run - equal, err := equalIgnoreHash(*t1, *t2) - // Check - if err != nil { - t.Errorf("In test case %q%s, expected no error, returned %v", test.test, reverseString, err) - } + equal := EqualIgnoreHash(*t1, *t2) if equal != test.expected { t.Errorf("In test case %q%s, expected %v", test.test, reverseString, test.expected) } diff --git a/pkg/util/labels/labels.go b/pkg/util/labels/labels.go index 786ac979ed..0ce48cfb52 100644 --- a/pkg/util/labels/labels.go +++ b/pkg/util/labels/labels.go @@ -17,14 +17,12 @@ limitations under the License. package labels import ( - "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // Clones the given map and returns a new map with the given key and value added. // Returns the given map, if labelKey is empty. -func CloneAndAddLabel(labels map[string]string, labelKey string, labelValue uint32) map[string]string { +func CloneAndAddLabel(labels map[string]string, labelKey, labelValue string) map[string]string { if labelKey == "" { // Don't need to add a label. return labels @@ -34,7 +32,7 @@ func CloneAndAddLabel(labels map[string]string, labelKey string, labelValue uint for key, value := range labels { newLabels[key] = value } - newLabels[labelKey] = fmt.Sprintf("%d", labelValue) + newLabels[labelKey] = labelValue return newLabels } @@ -55,7 +53,7 @@ func CloneAndRemoveLabel(labels map[string]string, labelKey string) map[string]s } // AddLabel returns a map with the given key and value added to the given map. -func AddLabel(labels map[string]string, labelKey string, labelValue string) map[string]string { +func AddLabel(labels map[string]string, labelKey, labelValue string) map[string]string { if labelKey == "" { // Don't need to add a label. return labels @@ -69,7 +67,7 @@ func AddLabel(labels map[string]string, labelKey string, labelValue string) map[ // Clones the given selector and returns a new selector with the given key and value added. // Returns the given selector, if labelKey is empty. -func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey string, labelValue uint32) *metav1.LabelSelector { +func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey, labelValue string) *metav1.LabelSelector { if labelKey == "" { // Don't need to add a label. return selector @@ -85,7 +83,7 @@ func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey string, l newSelector.MatchLabels[key] = val } } - newSelector.MatchLabels[labelKey] = fmt.Sprintf("%d", labelValue) + newSelector.MatchLabels[labelKey] = labelValue if selector.MatchExpressions != nil { newMExps := make([]metav1.LabelSelectorRequirement, len(selector.MatchExpressions)) @@ -108,7 +106,7 @@ func CloneSelectorAndAddLabel(selector *metav1.LabelSelector, labelKey string, l } // AddLabelToSelector returns a selector with the given key and value added to the given selector's MatchLabels. -func AddLabelToSelector(selector *metav1.LabelSelector, labelKey string, labelValue string) *metav1.LabelSelector { +func AddLabelToSelector(selector *metav1.LabelSelector, labelKey, labelValue string) *metav1.LabelSelector { if labelKey == "" { // Don't need to add a label. return selector diff --git a/pkg/util/labels/labels_test.go b/pkg/util/labels/labels_test.go index 6feb9501f1..029977a6df 100644 --- a/pkg/util/labels/labels_test.go +++ b/pkg/util/labels/labels_test.go @@ -33,7 +33,7 @@ func TestCloneAndAddLabel(t *testing.T) { cases := []struct { labels map[string]string labelKey string - labelValue uint32 + labelValue string want map[string]string }{ { @@ -43,7 +43,7 @@ func TestCloneAndAddLabel(t *testing.T) { { labels: labels, labelKey: "foo4", - labelValue: uint32(42), + labelValue: "42", want: map[string]string{ "foo1": "bar1", "foo2": "bar2", @@ -122,7 +122,7 @@ func TestCloneSelectorAndAddLabel(t *testing.T) { cases := []struct { labels map[string]string labelKey string - labelValue uint32 + labelValue string want map[string]string }{ { @@ -132,7 +132,7 @@ func TestCloneSelectorAndAddLabel(t *testing.T) { { labels: labels, labelKey: "foo4", - labelValue: 89, + labelValue: "89", want: map[string]string{ "foo1": "bar1", "foo2": "bar2", @@ -143,7 +143,7 @@ func TestCloneSelectorAndAddLabel(t *testing.T) { { labels: nil, labelKey: "foo4", - labelValue: 12, + labelValue: "12", want: map[string]string{ "foo4": "12", },