Browse code

Replace custom/flaky pod comparison with DeepEqual

Replace the old custom flaky hash based pod comparison in the deployment
code with Semantic.DeepEqual. This is far more reliable, and fixes a long
standing bug where Pod resource changes don't trigger deployments. It also
ensures future Pod API changes will be correctly handled when detecting
changes.

Dan Mace authored on 2015/05/29 23:26:33
Showing 4 changed files
... ...
@@ -61,6 +61,9 @@ func (c *DeploymentConfigChangeController) Handle(config *deployapi.DeploymentCo
61 61
 	latestDeploymentName := deployutil.LatestDeploymentNameForConfig(config)
62 62
 	deployment, err := c.changeStrategy.getDeployment(config.Namespace, latestDeploymentName)
63 63
 	if err != nil {
64
+		// If there's no deployment for the latest config, we have no basis of
65
+		// comparison. It's the responsibility of the deployment config controller
66
+		// to make the deployment for the config, so return early.
64 67
 		if kerrors.IsNotFound(err) {
65 68
 			glog.V(2).Infof("Ignoring change for DeploymentConfig %s; no existing Deployment found", deployutil.LabelForDeploymentConfig(config))
66 69
 			return nil
... ...
@@ -73,7 +76,8 @@ func (c *DeploymentConfigChangeController) Handle(config *deployapi.DeploymentCo
73 73
 		return fatalError(fmt.Sprintf("error decoding DeploymentConfig from Deployment %s for DeploymentConfig %s: %v", deployutil.LabelForDeployment(deployment), deployutil.LabelForDeploymentConfig(config), err))
74 74
 	}
75 75
 
76
-	if deployutil.PodSpecsEqual(config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec) {
76
+	newSpec, oldSpec := config.Template.ControllerTemplate.Template.Spec, deployedConfig.Template.ControllerTemplate.Template.Spec
77
+	if kapi.Semantic.DeepEqual(oldSpec, newSpec) {
77 78
 		glog.V(2).Infof("Ignoring DeploymentConfig change for %s (latestVersion=%d); same as Deployment %s", deployutil.LabelForDeploymentConfig(config), config.LatestVersion, deployutil.LabelForDeployment(deployment))
78 79
 		return nil
79 80
 	}
... ...
@@ -7,7 +7,6 @@ import (
7 7
 
8 8
 	deployapi "github.com/openshift/origin/pkg/deploy/api"
9 9
 	deploytest "github.com/openshift/origin/pkg/deploy/api/test"
10
-	deployutil "github.com/openshift/origin/pkg/deploy/util"
11 10
 )
12 11
 
13 12
 func TestGeneration(t *testing.T) {
... ...
@@ -110,5 +109,6 @@ func hasReplicationMetaDiff(a, b *deployapi.DeploymentConfig) bool {
110 110
 }
111 111
 
112 112
 func hasPodTemplateDiff(a, b *deployapi.DeploymentConfig) bool {
113
-	return !deployutil.PodSpecsEqual(a.Template.ControllerTemplate.Template.Spec, b.Template.ControllerTemplate.Template.Spec)
113
+	specA, specB := a.Template.ControllerTemplate.Template.Spec, b.Template.ControllerTemplate.Template.Spec
114
+	return !kapi.Semantic.DeepEqual(specA, specB)
114 115
 }
... ...
@@ -1,14 +1,10 @@
1 1
 package util
2 2
 
3 3
 import (
4
-	"encoding/json"
5 4
 	"fmt"
6
-	"hash/adler32"
7 5
 	"strconv"
8 6
 	"strings"
9 7
 
10
-	"github.com/golang/glog"
11
-
12 8
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
13 9
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
14 10
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
... ...
@@ -76,30 +72,6 @@ func LabelForDeploymentConfig(config *deployapi.DeploymentConfig) string {
76 76
 	return fmt.Sprintf("%s/%s:%d", config.Namespace, config.Name, config.LatestVersion)
77 77
 }
78 78
 
79
-// HashPodSpec hashes a PodSpec into a uint64.
80
-// TODO: Resources are currently ignored due to the formats not surviving encoding/decoding
81
-// in a consistent manner (e.g. 0 is represented sometimes as 0.000)
82
-func HashPodSpec(t api.PodSpec) uint64 {
83
-	// Ignore resources by making them uniformly empty
84
-	for i := range t.Containers {
85
-		t.Containers[i].Resources = api.ResourceRequirements{}
86
-	}
87
-
88
-	jsonString, err := json.Marshal(t)
89
-	if err != nil {
90
-		glog.Errorf("An error occurred marshalling pod state: %v", err)
91
-		return 0
92
-	}
93
-	hash := adler32.New()
94
-	fmt.Fprintf(hash, "%s", jsonString)
95
-	return uint64(hash.Sum32())
96
-}
97
-
98
-// PodSpecsEqual returns true if the given PodSpecs are the same.
99
-func PodSpecsEqual(a, b api.PodSpec) bool {
100
-	return HashPodSpec(a) == HashPodSpec(b)
101
-}
102
-
103 79
 // DecodeDeploymentConfig decodes a DeploymentConfig from controller using codec. An error is returned
104 80
 // if the controller doesn't contain an encoded config.
105 81
 func DecodeDeploymentConfig(controller *api.ReplicationController, codec runtime.Codec) (*deployapi.DeploymentConfig, error) {
... ...
@@ -57,38 +57,6 @@ func TestPodName(t *testing.T) {
57 57
 	}
58 58
 }
59 59
 
60
-func TestPodSpecsEqualTrue(t *testing.T) {
61
-	result := PodSpecsEqual(podTemplateA().Spec, podTemplateA().Spec)
62
-
63
-	if !result {
64
-		t.Fatalf("Unexpected false result for PodSpecsEqual")
65
-	}
66
-}
67
-
68
-func TestPodSpecsJustLabelDiff(t *testing.T) {
69
-	result := PodSpecsEqual(podTemplateA().Spec, podTemplateB().Spec)
70
-
71
-	if !result {
72
-		t.Fatalf("Unexpected false result for PodSpecsEqual")
73
-	}
74
-}
75
-
76
-func TestPodSpecsEqualContainerImageChange(t *testing.T) {
77
-	result := PodSpecsEqual(podTemplateA().Spec, podTemplateC().Spec)
78
-
79
-	if result {
80
-		t.Fatalf("Unexpected true result for PodSpecsEqual")
81
-	}
82
-}
83
-
84
-func TestPodSpecsEqualAdditionalContainerInManifest(t *testing.T) {
85
-	result := PodSpecsEqual(podTemplateA().Spec, podTemplateD().Spec)
86
-
87
-	if result {
88
-		t.Fatalf("Unexpected true result for PodSpecsEqual")
89
-	}
90
-}
91
-
92 60
 func TestMakeDeploymentOk(t *testing.T) {
93 61
 	config := deploytest.OkDeploymentConfig(1)
94 62
 	deployment, err := MakeDeployment(config, kapi.Codec)