diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index ff91be56e30..4512a5d7e01 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -73,6 +73,8 @@ type DeploymentController struct { // To allow injection of syncDeployment for testing. syncHandler func(dKey string) error + // used for unit testing + enqueueDeployment func(deployment *extensions.Deployment) // A store of deployments, populated by the dController dLister *cache.StoreToDeploymentLister @@ -134,6 +136,8 @@ func NewDeploymentController(dInformer informers.DeploymentInformer, rsInformer }) dc.syncHandler = dc.syncDeployment + dc.enqueueDeployment = dc.enqueue + dc.dLister = dInformer.Lister() dc.rsLister = rsInformer.Lister() dc.podLister = podInformer.Lister() @@ -343,7 +347,7 @@ func (dc *DeploymentController) deletePod(obj interface{}) { } } -func (dc *DeploymentController) enqueueDeployment(deployment *extensions.Deployment) { +func (dc *DeploymentController) enqueue(deployment *extensions.Deployment) { key, err := controller.KeyFunc(deployment) if err != nil { utilruntime.HandleError(fmt.Errorf("Couldn't get key for object %#v: %v", deployment, err)) @@ -608,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 069ad7499ca..6b2d6fa6aa4 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -19,6 +19,7 @@ package deployment import ( "fmt" "testing" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -30,6 +31,7 @@ import ( "k8s.io/kubernetes/pkg/client/record" "k8s.io/kubernetes/pkg/client/testing/core" "k8s.io/kubernetes/pkg/controller" + "k8s.io/kubernetes/pkg/controller/deployment/util" "k8s.io/kubernetes/pkg/controller/informers" "k8s.io/kubernetes/pkg/util/intstr" "k8s.io/kubernetes/pkg/util/uuid" @@ -67,9 +69,10 @@ func newDeployment(name string, replicas int, revisionHistoryLimit *int32, maxSu d := extensions.Deployment{ TypeMeta: metav1.TypeMeta{APIVersion: api.Registry.GroupOrDie(extensions.GroupName).GroupVersion.String()}, ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), - Name: name, - Namespace: v1.NamespaceDefault, + UID: uuid.NewUUID(), + Name: name, + Namespace: v1.NamespaceDefault, + Annotations: make(map[string]string), }, Spec: extensions.DeploymentSpec{ Strategy: extensions.DeploymentStrategy{ @@ -110,8 +113,10 @@ func newReplicaSet(d *extensions.Deployment, name string, replicas int) *extensi ObjectMeta: v1.ObjectMeta{ Name: name, Namespace: v1.NamespaceDefault, + Labels: d.Spec.Selector.MatchLabels, }, Spec: extensions.ReplicaSetSpec{ + Selector: d.Spec.Selector, Replicas: func() *int32 { i := int32(replicas); return &i }(), Template: d.Spec.Template, }, @@ -142,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" @@ -163,7 +164,7 @@ func newFixture(t *testing.T) *fixture { return f } -func (f *fixture) run(deploymentName string) { +func (f *fixture) newController() (*DeploymentController, informers.SharedInformerFactory) { f.client = fake.NewSimpleClientset(f.objects...) informers := informers.NewSharedInformerFactory(f.client, nil, controller.NoResyncPeriodFunc()) c := NewDeploymentController(informers.Deployments(), informers.ReplicaSets(), informers.Pods(), f.client) @@ -180,6 +181,11 @@ func (f *fixture) run(deploymentName string) { for _, pod := range f.podLister { c.podLister.Indexer.Add(pod) } + return c, informers +} + +func (f *fixture) run(deploymentName string) { + c, informers := f.newController() stopCh := make(chan struct{}) defer close(stopCh) informers.Start(stopCh) @@ -218,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)) @@ -286,3 +292,276 @@ func filterInformerActions(actions []core.Action) []core.Action { return ret } + +// TestOverlappingDeployment ensures that an overlapping deployment will not be synced by +// the controller. +func TestOverlappingDeployment(t *testing.T) { + f := newFixture(t) + now := metav1.Now() + later := metav1.Time{Time: now.Add(time.Minute)} + + foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + foo.CreationTimestamp = now + bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"}) + bar.CreationTimestamp = later + + f.dLister = append(f.dLister, foo, bar) + f.objects = append(f.objects, foo, bar) + + f.expectUpdateDeploymentStatusAction(bar) + f.run(getKey(bar, 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 == "bar" && d.Annotations[util.OverlapAnnotation] != "foo" { + t.Errorf("annotations weren't updated for the overlapping deployment: %v", d.Annotations) + } + } +} + +// TestSyncOverlappedDeployment ensures that from two overlapping deployments, the older +// one will be synced and the newer will be marked as overlapping. Note that in reality it's +// not always the older deployment that is the one that works vs the rest but the one which +// has the selector unchanged for longer time. +func TestSyncOverlappedDeployment(t *testing.T) { + f := newFixture(t) + now := metav1.Now() + later := metav1.Time{Time: now.Add(time.Minute)} + + foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + foo.CreationTimestamp = now + bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"}) + bar.CreationTimestamp = later + + f.dLister = append(f.dLister, foo, bar) + f.objects = append(f.objects, foo, bar) + + f.expectUpdateDeploymentStatusAction(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 == "bar" && d.Annotations[util.OverlapAnnotation] != "foo" { + t.Errorf("annotations weren't updated for the overlapping deployment: %v", d.Annotations) + } + } +} + +// 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) { + f := newFixture(t) + now := metav1.Now() + earlier := metav1.Time{Time: now.Add(-time.Minute)} + later := metav1.Time{Time: now.Add(time.Minute)} + + foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + foo.CreationTimestamp = earlier + foo.DeletionTimestamp = &now + bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + 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.expectUpdateDeploymentStatusAction(bar) + f.expectUpdateDeploymentStatusAction(foo) + f.run(getKey(foo, t)) + + for _, a := range filterInformerActions(f.client.Actions()) { + action, ok := a.(core.UpdateAction) + if !ok { + continue + } + d := action.GetObject().(*extensions.Deployment) + if d.Name != "bar" { + continue + } + + if len(d.Annotations[util.OverlapAnnotation]) > 0 { + t.Errorf("annotations weren't cleaned up for the overlapping deployment: %v", d.Annotations) + } + } +} + +// TestDeletedDeploymentShouldNotCleanupOtherOverlaps ensures that the deletion of +// a deployment will not cleanup deployments that overlap with another deployment. +func TestDeletedDeploymentShouldNotCleanupOtherOverlaps(t *testing.T) { + f := newFixture(t) + now := metav1.Now() + earlier := metav1.Time{Time: now.Add(-time.Minute)} + later := metav1.Time{Time: now.Add(time.Minute)} + + foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + foo.CreationTimestamp = earlier + foo.DeletionTimestamp = &now + bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"bla": "bla"}) + bar.CreationTimestamp = later + // Notice this deployment is overlapping with another deployment + bar.Annotations = map[string]string{util.OverlapAnnotation: "baz"} + + f.dLister = append(f.dLister, foo, bar) + f.objects = append(f.objects, foo, bar) + + f.expectUpdateDeploymentStatusAction(foo) + f.run(getKey(foo, t)) + + for _, a := range filterInformerActions(f.client.Actions()) { + action, ok := a.(core.UpdateAction) + if !ok { + continue + } + d := action.GetObject().(*extensions.Deployment) + if d.Name != "bar" { + continue + } + + if len(d.Annotations[util.OverlapAnnotation]) == 0 { + t.Errorf("overlapping annotation should not be cleaned up for bar: %v", d.Annotations) + } + } +} + +// TestPodDeletionEnqueuesRecreateDeployment ensures that the deletion of a pod +// will requeue a Recreate deployment iff there is no other pod returned from the +// client. +func TestPodDeletionEnqueuesRecreateDeployment(t *testing.T) { + f := newFixture(t) + + foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + foo.Spec.Strategy.Type = extensions.RecreateDeploymentStrategyType + rs := newReplicaSet(foo, "foo-1", 1) + pod := generatePodFromRS(rs) + + f.dLister = append(f.dLister, foo) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, foo, rs) + + c, informers := f.newController() + enqueued := false + c.enqueueDeployment = func(d *extensions.Deployment) { + if d.Name == "foo" { + enqueued = true + } + } + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + c.deletePod(pod) + + if !enqueued { + t.Errorf("expected deployment %q to be queued after pod deletion", foo.Name) + } +} + +// TestPodDeletionDoesntEnqueueRecreateDeployment ensures that the deletion of a pod +// will not requeue a Recreate deployment iff there are other pods returned from the +// client. +func TestPodDeletionDoesntEnqueueRecreateDeployment(t *testing.T) { + f := newFixture(t) + + foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + foo.Spec.Strategy.Type = extensions.RecreateDeploymentStrategyType + rs := newReplicaSet(foo, "foo-1", 1) + pod := generatePodFromRS(rs) + + f.dLister = append(f.dLister, foo) + f.rsLister = append(f.rsLister, rs) + // Let's pretend this is a different pod. The gist is that the pod lister needs to + // return a non-empty list. + f.podLister = append(f.podLister, pod) + + c, informers := f.newController() + enqueued := false + c.enqueueDeployment = func(d *extensions.Deployment) { + if d.Name == "foo" { + enqueued = true + } + } + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + c.deletePod(pod) + + if enqueued { + t.Errorf("expected deployment %q not to be queued after pod deletion", foo.Name) + } +} + +// generatePodFromRS creates a pod, with the input ReplicaSet's selector and its template +func generatePodFromRS(rs *extensions.ReplicaSet) *v1.Pod { + trueVar := true + return &v1.Pod{ + ObjectMeta: v1.ObjectMeta{ + Name: rs.Name + "-pod", + Namespace: rs.Namespace, + Labels: rs.Spec.Selector.MatchLabels, + OwnerReferences: []metav1.OwnerReference{ + {UID: rs.UID, APIVersion: "v1beta1", Kind: "ReplicaSet", Name: rs.Name, Controller: &trueVar}, + }, + }, + Spec: rs.Spec.Template.Spec, + } +} diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 0cb1dc4e641..9fe963a914f 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -984,7 +984,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) @@ -994,7 +994,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 f893c255ff7..666c9b2c0d0 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), @@ -621,7 +622,6 @@ func TestGetReplicaCountForReplicaSets(t *testing.T) { } func TestResolveFenceposts(t *testing.T) { - tests := []struct { maxSurge string maxUnavailable string @@ -1103,3 +1103,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) + } + } +} diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go index 149c58ce7b6..ca7fc740e08 100644 --- a/test/e2e/deployment.go +++ b/test/e2e/deployment.go @@ -1194,27 +1194,27 @@ func testOverlappingDeployment(f *framework.Framework) { podLabels = map[string]string{"name": nginxImageName} By(fmt.Sprintf("Creating deployment %q", deploymentName)) d = newDeployment(deploymentName, replicas, podLabels, nginxImageName, nginxImage, extensions.RollingUpdateDeploymentStrategyType, nil) - deployLater, err := c.Extensions().Deployments(ns).Create(d) + thirdDeployment, err := c.Extensions().Deployments(ns).Create(d) Expect(err).NotTo(HaveOccurred(), "Failed creating the third deployment") // Wait for it to be updated to revision 1 - err = framework.WaitForDeploymentRevisionAndImage(c, ns, deployLater.Name, "1", nginxImage) + err = framework.WaitForDeploymentRevisionAndImage(c, ns, thirdDeployment.Name, "1", nginxImage) Expect(err).NotTo(HaveOccurred(), "The third deployment failed to update to revision 1") // Update the second deployment's selector to make it overlap with the third deployment By(fmt.Sprintf("Updating deployment %q selector to make it overlap with existing one", deployOverlapping.Name)) deployOverlapping, err = framework.UpdateDeploymentWithRetries(c, ns, deployOverlapping.Name, func(update *extensions.Deployment) { - update.Spec.Selector = deployLater.Spec.Selector - update.Spec.Template.Labels = deployLater.Spec.Template.Labels + update.Spec.Selector = thirdDeployment.Spec.Selector + update.Spec.Template.Labels = thirdDeployment.Spec.Template.Labels update.Spec.Template.Spec.Containers[0].Image = redisImage }) Expect(err).NotTo(HaveOccurred()) // Wait for overlapping annotation updated to both deployments - By("Waiting for the third deployment to have the overlapping annotation") - err = framework.WaitForOverlappingAnnotationMatch(c, ns, deployLater.Name, deployOverlapping.Name) - Expect(err).NotTo(HaveOccurred(), "Failed to update the third deployment's overlapping annotation") - err = framework.WaitForOverlappingAnnotationMatch(c, ns, deployOverlapping.Name, "") + By("Waiting for the second deployment to have the overlapping annotation") + err = framework.WaitForOverlappingAnnotationMatch(c, ns, deployOverlapping.Name, thirdDeployment.Name) + Expect(err).NotTo(HaveOccurred(), "Failed to update the second deployment's overlapping annotation") + err = framework.WaitForOverlappingAnnotationMatch(c, ns, thirdDeployment.Name, "") Expect(err).NotTo(HaveOccurred(), "The deployment that holds the oldest selector shouldn't have the overlapping annotation") // The second deployment shouldn't be synced