Merge pull request #39088 from kargakis/unit-tests-for-the-d-controller

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
 				}

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)
+		}
+	}
+}

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