Merge pull request #39525 from kargakis/update-equality-helper

Automatic merge from submit-queue (batch tested with PRs 39807, 37505, 39844, 39525, 39109)

Update deployment equality helper

@mfojtik @janetkuo this is split out of https://github.com/kubernetes/kubernetes/pull/38714 to reduce the size of that PR, ptal

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;

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{

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
 }
 

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)
 			}

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

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",
 			},