From d19a1109e2b383bd5bf12fb19e5c3a2e890dfd88 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Mon, 5 Dec 2016 03:34:51 +0100 Subject: [PATCH 1/2] controller: sync deployments once they don't overlap anymore --- .../deployment/deployment_controller.go | 169 ++++++++++++++---- .../deployment/util/deployment_util.go | 4 +- 2 files changed, 136 insertions(+), 37 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index ae5fb644731..a6e277e9db1 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -39,6 +39,7 @@ import ( "k8s.io/kubernetes/pkg/controller/deployment/util" "k8s.io/kubernetes/pkg/controller/informers" "k8s.io/kubernetes/pkg/labels" + utilerrors "k8s.io/kubernetes/pkg/util/errors" "k8s.io/kubernetes/pkg/util/metrics" utilruntime "k8s.io/kubernetes/pkg/util/runtime" "k8s.io/kubernetes/pkg/util/wait" @@ -160,9 +161,31 @@ func (dc *DeploymentController) addDeployment(obj interface{}) { func (dc *DeploymentController) updateDeployment(old, cur interface{}) { oldD := old.(*extensions.Deployment) + curD := cur.(*extensions.Deployment) glog.V(4).Infof("Updating deployment %s", oldD.Name) - // Resync on deployment object relist. - dc.enqueueDeployment(cur.(*extensions.Deployment)) + dc.enqueueDeployment(curD) + // If the selector of the current deployment just changed, we need to requeue any old + // overlapping deployments. If the new selector steps on another deployment, the current + // deployment will get denied during the resync loop. + if !reflect.DeepEqual(curD.Spec.Selector, oldD.Spec.Selector) { + deployments, err := dc.dLister.Deployments(curD.Namespace).List(labels.Everything()) + if err != nil { + utilruntime.HandleError(fmt.Errorf("error listing deployments in namespace %s: %v", curD.Namespace, err)) + return + } + // Trigger cleanup of any old overlapping deployments; we don't care about any error + // returned here. + for i := range deployments { + otherD := deployments[i] + + oldOverlaps, oldErr := util.OverlapsWith(oldD, otherD) + curOverlaps, curErr := util.OverlapsWith(curD, otherD) + // Enqueue otherD so it gets cleaned up + if oldErr == nil && curErr == nil && oldOverlaps && !curOverlaps { + dc.enqueueDeployment(otherD) + } + } + } } func (dc *DeploymentController) deleteDeployment(obj interface{}) { @@ -181,6 +204,22 @@ func (dc *DeploymentController) deleteDeployment(obj interface{}) { } glog.V(4).Infof("Deleting deployment %s", d.Name) dc.enqueueDeployment(d) + deployments, err := dc.dLister.Deployments(d.Namespace).List(labels.Everything()) + if err != nil { + utilruntime.HandleError(fmt.Errorf("error listing deployments in namespace %s: %v", d.Namespace, err)) + return + } + // Trigger cleanup of any old overlapping deployments; we don't care about any error + // returned here. + for i := range deployments { + otherD := deployments[i] + + overlaps, err := util.OverlapsWith(d, otherD) + // Enqueue otherD so it gets cleaned up + if err == nil && overlaps { + dc.enqueueDeployment(otherD) + } + } } // addReplicaSet enqueues the deployment that manages a ReplicaSet when the ReplicaSet is created. @@ -426,14 +465,25 @@ func (dc *DeploymentController) syncDeployment(key string) error { return nil } - if d.DeletionTimestamp != nil { - return dc.syncStatusOnly(d) + 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) } // Handle overlapping deployments by deterministically avoid syncing deployments that fight over ReplicaSets. - if err = dc.handleOverlap(d); err != nil { - dc.eventRecorder.Eventf(d, v1.EventTypeWarning, "SelectorOverlap", err.Error()) - return nil + overlaps, err := dc.handleOverlap(d, deployments) + if err != nil { + if overlaps { + // Emit an event and return a nil error for overlapping deployments so we won't resync them again. + dc.eventRecorder.Eventf(d, v1.EventTypeWarning, "SelectorOverlap", err.Error()) + return nil + } + // For any other failure, we should retry the deployment. + return err + } + + if d.DeletionTimestamp != nil { + return dc.syncStatusOnly(d) } // Update deployment conditions with an Unknown condition when pausing/resuming @@ -479,42 +529,88 @@ func (dc *DeploymentController) syncDeployment(key string) error { return fmt.Errorf("unexpected deployment strategy type: %s", d.Spec.Strategy.Type) } -// handleOverlap relists all deployment in the same namespace for overlaps, and avoid syncing -// 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 { - 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) - } +// handleOverlap will avoid syncing 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, deployments []*extensions.Deployment) (bool, error) { overlapping := false - for _, other := range deployments { - foundOverlaps, err := util.OverlapsWith(d, other) - if err != nil { - return err + var errs []error + + for i := range deployments { + otherD := deployments[i] + + if d.Name == otherD.Name { + continue } - if foundOverlaps { - deploymentCopy, err := util.DeploymentDeepCopy(other) + + // Error is already checked during validation + foundOverlaps, _ := util.OverlapsWith(d, otherD) + + // If the otherD deployment overlaps with the current we need to identify which one + // holds the set longer and mark the other as overlapping. Requeue the overlapping + // 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 { + otherCopy, err := util.DeploymentDeepCopy(otherD) if err != nil { - return err + return false, err } overlapping = true + // Skip syncing this one if older overlapping one is found. - if util.SelectorUpdatedBefore(deploymentCopy, d) { - // 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) + if util.SelectorUpdatedBefore(otherCopy, d) { + if _, err = dc.markDeploymentOverlap(d, otherCopy.Name); err != nil { + return false, err + } + if _, err = dc.clearDeploymentOverlap(otherCopy, d.Name); err != nil { + return false, err + } + return true, fmt.Errorf("deployment %s/%s has overlapping selector with an older deployment %s/%s, skip syncing it", d.Namespace, d.Name, otherCopy.Namespace, otherCopy.Name) + } + + // TODO: We need to support annotations in deployments that overlap with multiple other + // deployments. + if _, err = dc.markDeploymentOverlap(otherCopy, d.Name); err != nil { + errs = append(errs, err) + } + // This is going to get some deployments into update hotlooping if we remove the overlapping + // annotation unconditionally. + // + // Scenario: + // --> Deployment foo with label selector A=A is created. + // --> Deployment bar with label selector A=A,B=B is created. Marked as overlapping since it + // overlaps with foo. + // --> Deployment baz with label selector B=B is created. Marked as overlapping, since it + // overlaps with bar, bar overlapping annotation is cleaned up. Next sync loop marks bar + // as overlapping and it gets in an update hotloop. + if d, err = dc.clearDeploymentOverlap(d, otherCopy.Name); err != nil { + errs = append(errs, err) + } + continue + } + + // If the otherD deployment does not overlap with the current deployment *anymore* + // we need to cleanup otherD from the overlapping annotation so it can be synced by + // the deployment controller. + dName, hasOverlappingAnnotation := otherD.Annotations[util.OverlapAnnotation] + if hasOverlappingAnnotation && dName == d.Name { + otherCopy, err := util.DeploymentDeepCopy(otherD) + if err != nil { + return false, err + } + if _, err = dc.clearDeploymentOverlap(otherCopy, d.Name); err != nil { + errs = append(errs, err) } - dc.markDeploymentOverlap(deploymentCopy, d.Name) - d, _ = dc.clearDeploymentOverlap(d) } } + if !overlapping { - // We don't care if the overlapping annotation update failed or not (we don't make decision on it) - d, _ = dc.clearDeploymentOverlap(d) + var err error + if d, err = dc.clearDeploymentOverlap(d, ""); err != nil { + errs = append(errs, err) + } } - return nil + + return false, utilerrors.NewAggregate(errs) } func (dc *DeploymentController) markDeploymentOverlap(deployment *extensions.Deployment, withDeployment string) (*extensions.Deployment, error) { @@ -530,8 +626,13 @@ func (dc *DeploymentController) markDeploymentOverlap(deployment *extensions.Dep 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 { +func (dc *DeploymentController) clearDeploymentOverlap(deployment *extensions.Deployment, otherName string) (*extensions.Deployment, error) { + overlapsWith := deployment.Annotations[util.OverlapAnnotation] + if len(overlapsWith) == 0 { + return deployment, nil + } + // This is not the deployment found in the annotation - do not remove the annotation. + if len(otherName) > 0 && otherName != overlapsWith { return deployment, nil } delete(deployment.Annotations, util.OverlapAnnotation) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index ffe5118afea..d6ea22d5cfa 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -1041,9 +1041,7 @@ func OverlapsWith(current, other *extensions.Deployment) (bool, error) { } otherSelector, err := metav1.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 false, fmt.Errorf("deployment %s/%s has invalid label selector: %v", other.Namespace, other.Name, err) } return (!currentSelector.Empty() && currentSelector.Matches(labels.Set(other.Spec.Template.Labels))) || (!otherSelector.Empty() && otherSelector.Matches(labels.Set(current.Spec.Template.Labels))), nil From 04c6fecbc7da54b4fae29e2f0fd24a0a49ba75f9 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Thu, 15 Dec 2016 16:52:36 +0100 Subject: [PATCH 2/2] controller: use defaultResync for the deployment controller --- pkg/controller/informers/extensions.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/controller/informers/extensions.go b/pkg/controller/informers/extensions.go index a5650cb5275..f98c7360a1d 100644 --- a/pkg/controller/informers/extensions.go +++ b/pkg/controller/informers/extensions.go @@ -18,7 +18,6 @@ package informers import ( "reflect" - "time" "k8s.io/kubernetes/pkg/api/v1" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" @@ -99,9 +98,7 @@ func (f *deploymentInformer) Informer() cache.SharedIndexInformer { }, }, &extensions.Deployment{}, - // TODO remove this. It is hardcoded so that "Waiting for the second deployment to clear overlapping annotation" in - // "overlapping deployment should not fight with each other" will work since it requires a full resync to work properly. - 30*time.Second, + f.defaultResync, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, ) f.informers[informerType] = informer