Merge pull request #36094 from janetkuo/overlapping-deployment-select

Automatic merge from submit-queue

Update how we detect overlapping deployments

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://github.com/kubernetes/kubernetes/blob/master/CONTRIBUTING.md and developer guide https://github.com/kubernetes/kubernetes/blob/master/docs/devel/development.md
2. If you want *faster* PR reviews, read how: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/faster_reviews.md
3. Follow the instructions for writing a release note: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes
-->

**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #24152 

**Special notes for your reviewer**: cc @kubernetes/deployment 

**Release note**:
<!--  Steps to write your release note:
1. Use the release-note-* labels to set the release note state (if you have access) 
2. Enter your extended release note in the below block; leaving it blank means using the PR title as the release note. If no release note is required, just write `NONE`. 
-->
```release-note
NONE
```

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.

pkg/controller/deployment/deployment_controller.go

@@ -397,17 +397,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

pkg/controller/deployment/util/deployment_util.go

@@ -988,3 +988,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
+}

test/e2e/deployment.go

@@ -1212,6 +1212,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")