From 9f3560c5633a5fc59bbfde19069c6d66d36be1b8 Mon Sep 17 00:00:00 2001
From: Janet Kuo <chiachenk@google.com>
Date: Wed, 2 Nov 2016 14:43:23 -0700
Subject: [PATCH] Update how we detect overlapping deployments

When looking for overlapping deployments, we should also find other deployments that select current deployment's pods,
not just the ones whose pods are selected by current deployment.
---
 .../deployment/deployment_controller.go       | 10 +++++-----
 .../deployment/util/deployment_util.go        | 19 +++++++++++++++++++
 test/e2e/deployment.go                        |  1 +
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go
index 528858ed883..5ca79eed020 100644
--- a/pkg/controller/deployment/deployment_controller.go
+++ b/pkg/controller/deployment/deployment_controller.go
@@ -382,17 +382,17 @@ func (dc *DeploymentController) syncDeployment(key string) error {
 // the newer overlapping ones (only sync the oldest one). New/old is determined by when the
 // deployment's selector is last updated.
 func (dc *DeploymentController) handleOverlap(d *extensions.Deployment) error {
-	selector, err := unversioned.LabelSelectorAsSelector(d.Spec.Selector)
-	if err != nil {
-		return fmt.Errorf("deployment %s/%s has invalid label selector: %v", d.Namespace, d.Name, err)
-	}
 	deployments, err := dc.dLister.Deployments(d.Namespace).List(labels.Everything())
 	if err != nil {
 		return fmt.Errorf("error listing deployments in namespace %s: %v", d.Namespace, err)
 	}
 	overlapping := false
 	for _, other := range deployments {
-		if !selector.Empty() && selector.Matches(labels.Set(other.Spec.Template.Labels)) && d.UID != other.UID {
+		foundOverlaps, err := util.OverlapsWith(d, other)
+		if err != nil {
+			return err
+		}
+		if foundOverlaps {
 			deploymentCopy, err := util.DeploymentDeepCopy(other)
 			if err != nil {
 				return err
diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go
index 3502e77d8f7..be1513ec2fe 100644
--- a/pkg/controller/deployment/util/deployment_util.go
+++ b/pkg/controller/deployment/util/deployment_util.go
@@ -835,3 +835,22 @@ func (o BySelectorLastUpdateTime) Less(i, j int) bool {
 	}
 	return ti.Before(tj)
 }
+
+// OverlapsWith returns true when two given deployments are different and overlap with each other
+func OverlapsWith(current, other *extensions.Deployment) (bool, error) {
+	if current.UID == other.UID {
+		return false, nil
+	}
+	currentSelector, err := unversioned.LabelSelectorAsSelector(current.Spec.Selector)
+	if err != nil {
+		return false, fmt.Errorf("deployment %s/%s has invalid label selector: %v", current.Namespace, current.Name, err)
+	}
+	otherSelector, err := unversioned.LabelSelectorAsSelector(other.Spec.Selector)
+	if err != nil {
+		// Broken selectors from other deployments shouldn't block current deployment. Just log the error and continue.
+		glog.V(2).Infof("Skip overlapping check: deployment %s/%s has invalid label selector: %v", other.Namespace, other.Name, err)
+		return false, nil
+	}
+	return (!currentSelector.Empty() && currentSelector.Matches(labels.Set(other.Spec.Template.Labels))) ||
+		(!otherSelector.Empty() && otherSelector.Matches(labels.Set(current.Spec.Template.Labels))), nil
+}
diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go
index f1d7677e30e..d48c62c073f 100644
--- a/test/e2e/deployment.go
+++ b/test/e2e/deployment.go
@@ -1232,6 +1232,7 @@ func testOverlappingDeployment(f *framework.Framework) {
 	Expect(err).NotTo(HaveOccurred())
 	deploymentName = "second-deployment"
 	By(fmt.Sprintf("Creating deployment %q with overlapping selector", deploymentName))
+	podLabels["other-label"] = "random-label"
 	d = newDeployment(deploymentName, replicas, podLabels, nginxImageName, nginxImage, extensions.RollingUpdateDeploymentStrategyType, nil)
 	deployOverlapping, err := c.Extensions().Deployments(ns).Create(d)
 	Expect(err).NotTo(HaveOccurred(), "Failed creating the second deployment")