From 2d1490a024f9f9c22379bb02b33697f876826897 Mon Sep 17 00:00:00 2001
From: Janet Kuo <chiachenk@google.com>
Date: Wed, 3 Aug 2016 11:19:12 -0700
Subject: [PATCH] Fix the map concurrent read/write issue in deployment
 controller

---
 .../deployment/util/deployment_util.go        | 21 ++++++---
 .../deployment/util/deployment_util_test.go   | 45 +++++++++++++------
 2 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go
index e09aea55a68..0b0bc886f4f 100644
--- a/pkg/controller/deployment/util/deployment_util.go
+++ b/pkg/controller/deployment/util/deployment_util.go
@@ -402,17 +402,26 @@ func ListPods(deployment *extensions.Deployment, getPodList podListFunc) (*api.P
 // (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) {
+	// 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(template1.Labels) == 0 || len(template2.Labels) == 0 {
+	if len(labels1) == 0 || len(labels2) == 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
+	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
+		}
+	}
+
+	// Then, compare the templates without comparing their labels
+	template1.Labels, template2.Labels = nil, nil
 	result := api.Semantic.DeepEqual(template1, template2)
-	template1.Labels[extensions.DefaultDeploymentUniqueLabelKey] = hash1
 	return result, nil
 }
 
diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go
index 88564bf9321..c95670ed5ee 100644
--- a/pkg/controller/deployment/util/deployment_util_test.go
+++ b/pkg/controller/deployment/util/deployment_util_test.go
@@ -464,20 +464,39 @@ func TestEqualIgnoreHash(t *testing.T) {
 	}
 
 	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)
+		runTest := func(t1, t2 api.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, err := equalIgnoreHash(t1, t2)
+			// Check
+			if err != nil {
+				t.Errorf("In test case %q%s, expected no error, returned %v", test.test, reverseString, err)
+			}
+			if equal != test.expected {
+				t.Errorf("In test case %q%s, expected %v", test.test, reverseString, test.expected)
+			}
+			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)
+			}
 		}
+		runTest(test.former, test.latter, false)
+		// Test the same case in reverse order
+		runTest(test.latter, test.former, true)
 	}
 }