diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 736d62275b2..4512a5d7e01 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/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 } diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index f2efd16e083..9f34da34808 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/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 diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 4d596830885..62f13cc9d9d 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/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 diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index d57a3dab837..57a339b431f 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/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) + } + } +}