Fix and tests for SelectorUpdatedBefore

pkg/controller/deployment/deployment_controller.go

@@ -612,14 +612,27 @@ func (dc *DeploymentController) handleOverlap(d *extensions.Deployment, deployme
 		// deployments if this one has been marked deleted, we only update its status as long
 		// as it is not actually deleted.
 		if foundOverlaps && d.DeletionTimestamp == nil {
+			overlapping = true
+			// Look at the overlapping annotation in both deployments. If one of them has it and points
+			// to the other one then we don't need to compare their timestamps.
+			otherOverlapsWith := otherD.Annotations[util.OverlapAnnotation]
+			currentOverlapsWith := d.Annotations[util.OverlapAnnotation]
+			// The other deployment is already marked as overlapping with the current one.
+			if otherOverlapsWith == d.Name {
+				var err error
+				if d, err = dc.clearDeploymentOverlap(d, otherD.Name); err != nil {
+					errs = append(errs, err)
+				}
+				continue
+			}
+
 			otherCopy, err := util.DeploymentDeepCopy(otherD)
 			if err != nil {
 				return false, err
 			}
-			overlapping = true
 
 			// Skip syncing this one if older overlapping one is found.
-			if util.SelectorUpdatedBefore(otherCopy, d) {
+			if currentOverlapsWith == otherCopy.Name || util.SelectorUpdatedBefore(otherCopy, d) {
 				if _, err = dc.markDeploymentOverlap(d, otherCopy.Name); err != nil {
 					return false, err
 				}

pkg/controller/deployment/deployment_controller_test.go

@@ -147,10 +147,6 @@ type fixture struct {
 	objects []runtime.Object
 }
 
-func (f *fixture) expectUpdateDeploymentAction(d *extensions.Deployment) {
-	f.actions = append(f.actions, core.NewUpdateAction(schema.GroupVersionResource{Resource: "deployments"}, d.Namespace, d))
-}
-
 func (f *fixture) expectUpdateDeploymentStatusAction(d *extensions.Deployment) {
 	action := core.NewUpdateAction(schema.GroupVersionResource{Resource: "deployments"}, d.Namespace, d)
 	action.Subresource = "status"
@@ -228,7 +224,7 @@ func TestSyncDeploymentCreatesReplicaSet(t *testing.T) {
 	rs := newReplicaSet(d, "deploymentrs-4186632231", 1)
 
 	f.expectCreateRSAction(rs)
-	f.expectUpdateDeploymentAction(d)
+	f.expectUpdateDeploymentStatusAction(d)
 	f.expectUpdateDeploymentStatusAction(d)
 
 	f.run(getKey(d, t))
@@ -349,7 +345,7 @@ func TestSyncOverlappedDeployment(t *testing.T) {
 
 	f.expectUpdateDeploymentStatusAction(bar)
 	f.expectCreateRSAction(newReplicaSet(foo, "foo-rs", 1))
-	f.expectUpdateDeploymentAction(foo)
+	f.expectUpdateDeploymentStatusAction(foo)
 	f.expectUpdateDeploymentStatusAction(foo)
 	f.run(getKey(foo, t))
 
@@ -368,6 +364,48 @@ func TestSyncOverlappedDeployment(t *testing.T) {
 	}
 }
 
+// TestSelectorUpdate ensures that from two overlapping deployments, the one that is working won't
+// be marked as overlapping if its selector is updated but still overlaps with the other one.
+func TestSelectorUpdate(t *testing.T) {
+	f := newFixture(t)
+	now := metav1.Now()
+	later := metav1.Time{Time: now.Add(time.Minute)}
+	selectorUpdated := metav1.Time{Time: later.Add(time.Minute)}
+
+	foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"})
+	foo.CreationTimestamp = now
+	foo.Annotations = map[string]string{util.SelectorUpdateAnnotation: selectorUpdated.Format(time.RFC3339)}
+	bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"})
+	bar.CreationTimestamp = later
+	bar.Annotations = map[string]string{util.OverlapAnnotation: "foo"}
+
+	f.dLister = append(f.dLister, foo, bar)
+	f.objects = append(f.objects, foo, bar)
+
+	f.expectCreateRSAction(newReplicaSet(foo, "foo-rs", 1))
+	f.expectUpdateDeploymentStatusAction(foo)
+	f.expectUpdateDeploymentStatusAction(foo)
+	f.run(getKey(foo, t))
+
+	for _, a := range filterInformerActions(f.client.Actions()) {
+		action, ok := a.(core.UpdateAction)
+		if !ok {
+			continue
+		}
+		d, ok := action.GetObject().(*extensions.Deployment)
+		if !ok {
+			continue
+		}
+
+		if d.Name == "foo" && len(d.Annotations[util.OverlapAnnotation]) > 0 {
+			t.Errorf("deployment %q should not have the overlapping annotation", d.Name)
+		}
+		if d.Name == "bar" && len(d.Annotations[util.OverlapAnnotation]) == 0 {
+			t.Errorf("deployment %q should have the overlapping annotation", d.Name)
+		}
+	}
+}
+
 // TestDeletedDeploymentShouldCleanupOverlaps ensures that the deletion of a deployment
 // will cleanup any deployments that overlap with it.
 func TestDeletedDeploymentShouldCleanupOverlaps(t *testing.T) {
@@ -390,7 +428,7 @@ func TestDeletedDeploymentShouldCleanupOverlaps(t *testing.T) {
 	f.expectUpdateDeploymentStatusAction(foo)
 	f.run(getKey(foo, t))
 
-	for _, a := range f.client.Actions() {
+	for _, a := range filterInformerActions(f.client.Actions()) {
 		action, ok := a.(core.UpdateAction)
 		if !ok {
 			continue
@@ -428,7 +466,7 @@ func TestDeletedDeploymentShouldNotCleanupOtherOverlaps(t *testing.T) {
 	f.expectUpdateDeploymentStatusAction(foo)
 	f.run(getKey(foo, t))
 
-	for _, a := range f.client.Actions() {
+	for _, a := range filterInformerActions(f.client.Actions()) {
 		action, ok := a.(core.UpdateAction)
 		if !ok {
 			continue

pkg/controller/deployment/util/deployment_util.go

@@ -1003,7 +1003,7 @@ func DeploymentDeepCopy(deployment *extensions.Deployment) (*extensions.Deployme
 }
 
 // SelectorUpdatedBefore returns true if the former deployment's selector
-// is updated before the latter, false otherwise
+// is updated before the latter, false otherwise.
 func SelectorUpdatedBefore(d1, d2 *extensions.Deployment) bool {
 	t1, t2 := LastSelectorUpdate(d1), LastSelectorUpdate(d2)
 	return t1.Before(t2)
@@ -1013,7 +1013,7 @@ func SelectorUpdatedBefore(d1, d2 *extensions.Deployment) bool {
 func LastSelectorUpdate(d *extensions.Deployment) metav1.Time {
 	t := d.Annotations[SelectorUpdateAnnotation]
 	if len(t) > 0 {
-		parsedTime, err := time.Parse(t, time.RFC3339)
+		parsedTime, err := time.Parse(time.RFC3339, t)
 		// If failed to parse the time, use creation timestamp instead
 		if err != nil {
 			return d.CreationTimestamp

pkg/controller/deployment/util/deployment_util_test.go

@@ -177,7 +177,8 @@ func generateDeployment(image string) extensions.Deployment {
 	terminationSec := int64(30)
 	return extensions.Deployment{
 		ObjectMeta: v1.ObjectMeta{
-			Name: image,
+			Name:        image,
+			Annotations: make(map[string]string),
 		},
 		Spec: extensions.DeploymentSpec{
 			Replicas: func(i int32) *int32 { return &i }(1),
@@ -625,7 +626,6 @@ func TestGetReplicaCountForReplicaSets(t *testing.T) {
 }
 
 func TestResolveFenceposts(t *testing.T) {
-
 	tests := []struct {
 		maxSurge          string
 		maxUnavailable    string
@@ -1107,3 +1107,111 @@ func TestDeploymentTimedOut(t *testing.T) {
 		}
 	}
 }
+
+func TestSelectorUpdatedBefore(t *testing.T) {
+	now := metav1.Now()
+	later := metav1.Time{Time: now.Add(time.Minute)}
+	selectorUpdated := metav1.Time{Time: later.Add(time.Minute)}
+	selectorUpdatedLater := metav1.Time{Time: selectorUpdated.Add(time.Minute)}
+
+	tests := []struct {
+		name string
+
+		d1                 extensions.Deployment
+		creationTimestamp1 *metav1.Time
+		selectorUpdated1   *metav1.Time
+
+		d2                 extensions.Deployment
+		creationTimestamp2 *metav1.Time
+		selectorUpdated2   *metav1.Time
+
+		expected bool
+	}{
+		{
+			name: "d1 created before d2",
+
+			d1:                 generateDeployment("foo"),
+			creationTimestamp1: &now,
+
+			d2:                 generateDeployment("bar"),
+			creationTimestamp2: &later,
+
+			expected: true,
+		},
+		{
+			name: "d1 created after d2",
+
+			d1:                 generateDeployment("foo"),
+			creationTimestamp1: &later,
+
+			d2:                 generateDeployment("bar"),
+			creationTimestamp2: &now,
+
+			expected: false,
+		},
+		{
+			// Think of the following scenario:
+			// d1 is created first, d2 is created after and its selector overlaps
+			// with d1. d2 is marked as overlapping correctly. If d1's selector is
+			// updated and continues to overlap with the selector of d2 then d1 is
+			// now marked overlapping and d2 is cleaned up. Proved by the following
+			// test case. Callers of SelectorUpdatedBefore should first check for
+			// the existence of the overlapping annotation in any of the two deployments
+			// prior to comparing their timestamps and as a matter of fact this is
+			// now handled in `(dc *DeploymentController) handleOverlap`.
+			name: "d1 created before d2 but updated its selector afterwards",
+
+			d1:                 generateDeployment("foo"),
+			creationTimestamp1: &now,
+			selectorUpdated1:   &selectorUpdated,
+
+			d2:                 generateDeployment("bar"),
+			creationTimestamp2: &later,
+
+			expected: false,
+		},
+		{
+			name: "d1 selector is older than d2",
+
+			d1:               generateDeployment("foo"),
+			selectorUpdated1: &selectorUpdated,
+
+			d2:               generateDeployment("bar"),
+			selectorUpdated2: &selectorUpdatedLater,
+
+			expected: true,
+		},
+		{
+			name: "d1 selector is younger than d2",
+
+			d1:               generateDeployment("foo"),
+			selectorUpdated1: &selectorUpdatedLater,
+
+			d2:               generateDeployment("bar"),
+			selectorUpdated2: &selectorUpdated,
+
+			expected: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Logf("running scenario %q", test.name)
+
+		if test.creationTimestamp1 != nil {
+			test.d1.CreationTimestamp = *test.creationTimestamp1
+		}
+		if test.creationTimestamp2 != nil {
+			test.d2.CreationTimestamp = *test.creationTimestamp2
+		}
+		if test.selectorUpdated1 != nil {
+			test.d1.Annotations[SelectorUpdateAnnotation] = test.selectorUpdated1.Format(time.RFC3339)
+		}
+		if test.selectorUpdated2 != nil {
+			test.d2.Annotations[SelectorUpdateAnnotation] = test.selectorUpdated2.Format(time.RFC3339)
+		}
+
+		if got := SelectorUpdatedBefore(&test.d1, &test.d2); got != test.expected {
+			t.Errorf("expected d1 selector to be updated before d2: %t, got: %t", test.expected, got)
+		}
+	}
+}