diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 4512a5d7e01..ff91be56e30 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -73,8 +73,6 @@ 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 @@ -136,8 +134,6 @@ 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() @@ -347,7 +343,7 @@ func (dc *DeploymentController) deletePod(obj interface{}) { } } -func (dc *DeploymentController) enqueue(deployment *extensions.Deployment) { +func (dc *DeploymentController) enqueueDeployment(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)) @@ -612,27 +608,14 @@ 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 currentOverlapsWith == otherCopy.Name || util.SelectorUpdatedBefore(otherCopy, d) { + if 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 6913c8ddd8b..12f9312022a 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -19,7 +19,6 @@ package deployment import ( "fmt" "testing" - "time" "k8s.io/apimachinery/pkg/apimachinery/registered" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -31,7 +30,6 @@ 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" @@ -69,10 +67,9 @@ func newDeployment(name string, replicas int, revisionHistoryLimit *int32, maxSu d := extensions.Deployment{ TypeMeta: metav1.TypeMeta{APIVersion: registered.GroupOrDie(extensions.GroupName).GroupVersion.String()}, ObjectMeta: v1.ObjectMeta{ - UID: uuid.NewUUID(), - Name: name, - Namespace: v1.NamespaceDefault, - Annotations: make(map[string]string), + UID: uuid.NewUUID(), + Name: name, + Namespace: v1.NamespaceDefault, }, Spec: extensions.DeploymentSpec{ Strategy: extensions.DeploymentStrategy{ @@ -113,10 +110,8 @@ 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, }, @@ -147,6 +142,10 @@ 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" @@ -164,7 +163,7 @@ func newFixture(t *testing.T) *fixture { return f } -func (f *fixture) newController() (*DeploymentController, informers.SharedInformerFactory) { +func (f *fixture) run(deploymentName string) { f.client = fake.NewSimpleClientset(f.objects...) informers := informers.NewSharedInformerFactory(f.client, nil, controller.NoResyncPeriodFunc()) c := NewDeploymentController(informers.Deployments(), informers.ReplicaSets(), informers.Pods(), f.client) @@ -181,11 +180,6 @@ func (f *fixture) newController() (*DeploymentController, informers.SharedInform 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) @@ -224,7 +218,7 @@ func TestSyncDeploymentCreatesReplicaSet(t *testing.T) { rs := newReplicaSet(d, "deploymentrs-4186632231", 1) f.expectCreateRSAction(rs) - f.expectUpdateDeploymentStatusAction(d) + f.expectUpdateDeploymentAction(d) f.expectUpdateDeploymentStatusAction(d) f.run(getKey(d, t)) @@ -292,260 +286,3 @@ 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)) - - actions := f.client.Actions() - d := actions[0].(core.UpdateAction).GetObject().(*extensions.Deployment) - if len(d.Annotations[util.OverlapAnnotation]) == 0 { - 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)) - - actions := f.client.Actions() - d := actions[0].(core.UpdateAction).GetObject().(*extensions.Deployment) - if 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 62f13cc9d9d..4d596830885 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(time.RFC3339, t) + parsedTime, err := time.Parse(t, time.RFC3339) // 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 57a339b431f..d57a3dab837 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -177,8 +177,7 @@ func generateDeployment(image string) extensions.Deployment { terminationSec := int64(30) return extensions.Deployment{ ObjectMeta: v1.ObjectMeta{ - Name: image, - Annotations: make(map[string]string), + Name: image, }, Spec: extensions.DeploymentSpec{ Replicas: func(i int32) *int32 { return &i }(1), @@ -626,6 +625,7 @@ func TestGetReplicaCountForReplicaSets(t *testing.T) { } func TestResolveFenceposts(t *testing.T) { + tests := []struct { maxSurge string maxUnavailable string @@ -1107,111 +1107,3 @@ 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 22197abfb64..87e8b61f656 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) - thirdDeployment, err := c.Extensions().Deployments(ns).Create(d) + deployLater, 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, thirdDeployment.Name, "1", nginxImage) + err = framework.WaitForDeploymentRevisionAndImage(c, ns, deployLater.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 = thirdDeployment.Spec.Selector - update.Spec.Template.Labels = thirdDeployment.Spec.Template.Labels + update.Spec.Selector = deployLater.Spec.Selector + update.Spec.Template.Labels = deployLater.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 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, "") + 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, "") Expect(err).NotTo(HaveOccurred(), "The deployment that holds the oldest selector shouldn't have the overlapping annotation") // The second deployment shouldn't be synced