diff --git a/hack/make-rules/test-cmd.sh b/hack/make-rules/test-cmd.sh index e08f50b5b2a..e488bd7951d 100755 --- a/hack/make-rules/test-cmd.sh +++ b/hack/make-rules/test-cmd.sh @@ -2268,6 +2268,13 @@ __EOF__ kubectl get rs nginx-618515232 -o yaml | grep "deployment.kubernetes.io/revision-history: 1,3" # Check that trying to watch the status of a superseded revision returns an error ! kubectl rollout status deployment/nginx --revision=3 + cat hack/testdata/deployment-revision1.yaml | $SED "s/name: nginx$/name: nginx2/" | kubectl create -f - "${kube_flags[@]}" + # Newest deployment should be marked as overlapping + kubectl get deployment nginx2 -o yaml "${kube_flags[@]}" | grep "deployment.kubernetes.io/error-selector-overlapping-with" + # Oldest deployment should not be marked as overlapping + ! kubectl get deployment nginx -o yaml "${kube_flags[@]}" | grep "deployment.kubernetes.io/error-selector-overlapping-with" + # Deletion of both deployments should not be blocked + kubectl delete deployment nginx2 "${kube_flags[@]}" # Clean up kubectl delete deployment nginx "${kube_flags[@]}" diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index f04c872ed2f..c5341e08e6f 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -265,25 +265,6 @@ func (dc *DeploymentController) enqueueDeployment(deployment *extensions.Deploym dc.queue.Add(key) } -func (dc *DeploymentController) markDeploymentOverlap(deployment *extensions.Deployment, withDeployment string) (*extensions.Deployment, error) { - if deployment.Annotations[util.OverlapAnnotation] == withDeployment { - return deployment, nil - } - if deployment.Annotations == nil { - deployment.Annotations = make(map[string]string) - } - deployment.Annotations[util.OverlapAnnotation] = withDeployment - return dc.client.Extensions().Deployments(deployment.Namespace).Update(deployment) -} - -func (dc *DeploymentController) clearDeploymentOverlap(deployment *extensions.Deployment) (*extensions.Deployment, error) { - if len(deployment.Annotations[util.OverlapAnnotation]) == 0 { - return deployment, nil - } - delete(deployment.Annotations, util.OverlapAnnotation) - return dc.client.Extensions().Deployments(deployment.Namespace).Update(deployment) -} - // worker runs a worker thread that just dequeues items, processes them, and marks them done. // It enforces that the syncHandler is never invoked concurrently with the same key. func (dc *DeploymentController) worker() { @@ -342,12 +323,6 @@ func (dc *DeploymentController) syncDeployment(key string) error { } deployment := obj.(*extensions.Deployment) - everything := unversioned.LabelSelector{} - if reflect.DeepEqual(deployment.Spec.Selector, &everything) { - dc.eventRecorder.Eventf(deployment, api.EventTypeWarning, "SelectingAll", "This deployment is selecting all pods. A non-empty selector is required.") - return nil - } - // Deep-copy otherwise we are mutating our cache. // TODO: Deep-copy only when needed. d, err := util.DeploymentDeepCopy(deployment) @@ -355,13 +330,23 @@ func (dc *DeploymentController) syncDeployment(key string) error { return err } + everything := unversioned.LabelSelector{} + if reflect.DeepEqual(d.Spec.Selector, &everything) { + dc.eventRecorder.Eventf(d, api.EventTypeWarning, "SelectingAll", "This deployment is selecting all pods. A non-empty selector is required.") + if d.Status.ObservedGeneration < d.Generation { + d.Status.ObservedGeneration = d.Generation + dc.client.Extensions().Deployments(d.Namespace).UpdateStatus(d) + } + return nil + } + if d.DeletionTimestamp != nil { return dc.syncStatusOnly(d) } // Handle overlapping deployments by deterministically avoid syncing deployments that fight over ReplicaSets. if err = dc.handleOverlap(d); err != nil { - dc.eventRecorder.Eventf(deployment, api.EventTypeWarning, "SelectorOverlap", err.Error()) + dc.eventRecorder.Eventf(d, api.EventTypeWarning, "SelectorOverlap", err.Error()) return nil } @@ -413,17 +398,15 @@ func (dc *DeploymentController) handleOverlap(d *extensions.Deployment) error { return err } overlapping = true - // We don't care if the overlapping annotation update failed or not (we don't make decision on it) - d, _ = dc.markDeploymentOverlap(d, other.Name) - deploymentCopy, _ = dc.markDeploymentOverlap(deploymentCopy, d.Name) - // Skip syncing this one if older overlapping one is found - // TODO: figure out a better way to determine which deployment to skip, - // either with controller reference, or with validation. - // Using oldest active replica set to determine which deployment to skip wouldn't make much difference, - // since new replica set hasn't been created after selector update + // Skip syncing this one if older overlapping one is found. if util.SelectorUpdatedBefore(deploymentCopy, d) { - return fmt.Errorf("found deployment %s/%s has overlapping selector with an older deployment %s/%s, skip syncing it", d.Namespace, d.Name, other.Namespace, other.Name) + // We don't care if the overlapping annotation update failed or not (we don't make decision on it) + dc.markDeploymentOverlap(d, deploymentCopy.Name) + dc.clearDeploymentOverlap(deploymentCopy) + return fmt.Errorf("found deployment %s/%s has overlapping selector with an older deployment %s/%s, skip syncing it", d.Namespace, d.Name, deploymentCopy.Namespace, deploymentCopy.Name) } + dc.markDeploymentOverlap(deploymentCopy, d.Name) + d, _ = dc.clearDeploymentOverlap(d) } } if !overlapping { @@ -432,3 +415,24 @@ func (dc *DeploymentController) handleOverlap(d *extensions.Deployment) error { } return nil } + +func (dc *DeploymentController) markDeploymentOverlap(deployment *extensions.Deployment, withDeployment string) (*extensions.Deployment, error) { + if deployment.Annotations[util.OverlapAnnotation] == withDeployment && deployment.Status.ObservedGeneration >= deployment.Generation { + return deployment, nil + } + if deployment.Annotations == nil { + deployment.Annotations = make(map[string]string) + } + // Update observedGeneration for overlapping deployments so that their deletion won't be blocked. + deployment.Status.ObservedGeneration = deployment.Generation + deployment.Annotations[util.OverlapAnnotation] = withDeployment + return dc.client.Extensions().Deployments(deployment.Namespace).UpdateStatus(deployment) +} + +func (dc *DeploymentController) clearDeploymentOverlap(deployment *extensions.Deployment) (*extensions.Deployment, error) { + if len(deployment.Annotations[util.OverlapAnnotation]) == 0 { + return deployment, nil + } + delete(deployment.Annotations, util.OverlapAnnotation) + return dc.client.Extensions().Deployments(deployment.Namespace).UpdateStatus(deployment) +} diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 90eea0c9cbc..3553618778b 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -435,6 +435,11 @@ func (reaper *DeploymentReaper) Stop(namespace, name string, timeout time.Durati return err } + // Do not cascade deletion for overlapping deployments. + if len(deployment.Annotations[deploymentutil.OverlapAnnotation]) > 0 { + return deployments.Delete(name, nil) + } + // Stop all replica sets. selector, err := unversioned.LabelSelectorAsSelector(deployment.Spec.Selector) if err != nil { diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go index 5150142a14c..34ab20e4fdb 100644 --- a/test/e2e/deployment.go +++ b/test/e2e/deployment.go @@ -1239,11 +1239,11 @@ func testOverlappingDeployment(f *framework.Framework) { Expect(err).NotTo(HaveOccurred(), "Failed creating the second deployment") // Wait for overlapping annotation updated to both deployments - By("Waiting for both deployments to have overlapping annotations") - err = framework.WaitForOverlappingAnnotationMatch(c, ns, deploy.Name, deployOverlapping.Name) - Expect(err).NotTo(HaveOccurred(), "Failed to update the first deployment's overlapping annotation") + By("Waiting for the overlapping deployment to have overlapping annotation") err = framework.WaitForOverlappingAnnotationMatch(c, ns, deployOverlapping.Name, deploy.Name) Expect(err).NotTo(HaveOccurred(), "Failed to update the second deployment's overlapping annotation") + err = framework.WaitForOverlappingAnnotationMatch(c, ns, deploy.Name, "") + Expect(err).NotTo(HaveOccurred(), "The deployment that holds the oldest selector shouldn't have the overlapping annotation") // Only the first deployment is synced By("Checking only the first overlapping deployment is synced") @@ -1295,11 +1295,11 @@ func testOverlappingDeployment(f *framework.Framework) { Expect(err).NotTo(HaveOccurred()) // Wait for overlapping annotation updated to both deployments - By("Waiting for both deployments to have overlapping annotations") - err = framework.WaitForOverlappingAnnotationMatch(c, ns, deployOverlapping.Name, deployLater.Name) - Expect(err).NotTo(HaveOccurred(), "Failed to update the second deployment's overlapping annotation") + 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 By("Checking the second deployment is not synced")