From 92d75cbb2338c27a1404a556a2c3b92f52041941 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Sun, 26 Feb 2017 10:53:45 -0800 Subject: [PATCH 01/12] Deployment: Use ControllerRef to list controlled objects. Although Deployment already applied its ControllerRef to adopt matching ReplicaSets, it actually still used label selectors to list objects that it controls. That meant it didn't actually follow the rules of ControllerRef, so it could still fight with other controller types. This should mean that the special handling for overlapping Deployments is no longer necessary, since each Deployment will only see objects that it owns (via ControllerRef). --- pkg/controller/deployment/BUILD | 2 +- .../deployment/deployment_controller.go | 279 ++++--------- .../deployment/deployment_controller_test.go | 367 +++++++++--------- pkg/controller/deployment/progress.go | 5 +- pkg/controller/deployment/recreate.go | 30 +- pkg/controller/deployment/rollback.go | 5 +- pkg/controller/deployment/rolling.go | 16 +- pkg/controller/deployment/sync.go | 132 +++---- 8 files changed, 334 insertions(+), 502 deletions(-) diff --git a/pkg/controller/deployment/BUILD b/pkg/controller/deployment/BUILD index c5607e18ead..f0cb68a73fd 100644 --- a/pkg/controller/deployment/BUILD +++ b/pkg/controller/deployment/BUILD @@ -37,7 +37,7 @@ go_library( "//vendor:k8s.io/apimachinery/pkg/api/errors", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/labels", - "//vendor:k8s.io/apimachinery/pkg/runtime/schema", + "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apimachinery/pkg/util/errors", "//vendor:k8s.io/apimachinery/pkg/util/runtime", "//vendor:k8s.io/apimachinery/pkg/util/wait", diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index ac637347257..3e31684b274 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -31,8 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/runtime/schema" - utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" v1core "k8s.io/client-go/kubernetes/typed/core/v1" @@ -58,9 +57,8 @@ const ( maxRetries = 5 ) -func getDeploymentKind() schema.GroupVersionKind { - return extensions.SchemeGroupVersion.WithKind("Deployment") -} +// controllerKind contains the schema.GroupVersionKind for this controller type. +var controllerKind = extensions.SchemeGroupVersion.WithKind("Deployment") // DeploymentController is responsible for synchronizing Deployment objects stored // in the system with actual running replica sets and pods. @@ -174,28 +172,6 @@ func (dc *DeploymentController) updateDeployment(old, cur interface{}) { curD := cur.(*extensions.Deployment) glog.V(4).Infof("Updating deployment %s", oldD.Name) 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{}) { @@ -214,22 +190,6 @@ 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. @@ -336,8 +296,20 @@ func (dc *DeploymentController) deletePod(obj interface{}) { } glog.V(4).Infof("Pod %s deleted.", pod.Name) if d := dc.getDeploymentForPod(pod); d != nil && d.Spec.Strategy.Type == extensions.RecreateDeploymentStrategyType { - podList, err := dc.listPods(d) - if err == nil && len(podList.Items) == 0 { + // Sync if this Deployment now has no more Pods. + rsList, err := dc.getReplicaSetsForDeployment(d) + if err != nil { + return + } + podMap, err := dc.getPodMapForReplicaSets(d.Namespace, rsList) + if err != nil { + return + } + numPods := 0 + for _, podList := range podMap { + numPods += len(podList.Items) + } + if numPods == 0 { dc.enqueueDeployment(d) } } @@ -447,23 +419,52 @@ func (dc *DeploymentController) handleErr(err error, key interface{}) { dc.queue.Forget(key) } -// claimReplicaSets uses NewReplicaSetControllerRefManager to classify ReplicaSets -// and adopts them if their labels match the Deployment but are missing the reference. -// It also removes the controllerRef for ReplicaSets, whose labels no longer matches -// the deployment. -func (dc *DeploymentController) claimReplicaSets(deployment *extensions.Deployment) error { - rsList, err := dc.rsLister.ReplicaSets(deployment.Namespace).List(labels.Everything()) +// getReplicaSetsForDeployment uses ControllerRefManager to reconcile +// ControllerRef by adopting and orphaning. +// It returns the list of ReplicaSets that this Deployment should manage. +func (dc *DeploymentController) getReplicaSetsForDeployment(d *extensions.Deployment) ([]*extensions.ReplicaSet, error) { + // List all ReplicaSets to find those we own but that no longer match our + // selector. They will be orphaned by ClaimReplicaSets(). + rsList, err := dc.rsLister.ReplicaSets(d.Namespace).List(labels.Everything()) if err != nil { - return err + return nil, err } + deploymentSelector, err := metav1.LabelSelectorAsSelector(d.Spec.Selector) + if err != nil { + return nil, fmt.Errorf("deployment %s/%s has invalid label selector: %v", d.Namespace, d.Name, err) + } + cm := controller.NewReplicaSetControllerRefManager(dc.rsControl, d, deploymentSelector, controllerKind) + return cm.ClaimReplicaSets(rsList) +} - deploymentSelector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) +// getPodMapForReplicaSets scans the list of all Pods and returns a map from +// RS UID to Pods controlled by that RS, based on the Pod's ControllerRef. +func (dc *DeploymentController) getPodMapForReplicaSets(namespace string, rsList []*extensions.ReplicaSet) (map[types.UID]*v1.PodList, error) { + // List all Pods. + pods, err := dc.podLister.Pods(namespace).List(labels.Everything()) if err != nil { - return fmt.Errorf("deployment %s/%s has invalid label selector: %v", deployment.Namespace, deployment.Name, err) + return nil, err } - cm := controller.NewReplicaSetControllerRefManager(dc.rsControl, deployment, deploymentSelector, getDeploymentKind()) - _, err = cm.ClaimReplicaSets(rsList) - return err + // Group Pods by their controller (if it's in rsList). + podMap := make(map[types.UID]*v1.PodList, len(rsList)) + for _, rs := range rsList { + podMap[rs.UID] = &v1.PodList{} + } + for _, pod := range pods { + // Ignore inactive Pods since that's what ReplicaSet does. + if !controller.IsPodActive(pod) { + continue + } + controllerRef := controller.GetControllerOf(pod) + if controllerRef == nil { + continue + } + // Only append if we care about this UID. + if podList, ok := podMap[controllerRef.UID]; ok { + podList.Items = append(podList.Items, *pod) + } + } + return podMap, nil } // syncDeployment will sync the deployment with the given key. @@ -506,25 +507,21 @@ func (dc *DeploymentController) syncDeployment(key string) error { return nil } - deployments, err := dc.dLister.Deployments(d.Namespace).List(labels.Everything()) + // List ReplicaSets owned by this Deployment, while reconciling ControllerRef + // through adoption/orphaning. + rsList, err := dc.getReplicaSetsForDeployment(d) if err != nil { - return fmt.Errorf("error listing deployments in namespace %s: %v", d.Namespace, err) + return err } - - // Handle overlapping deployments by deterministically avoid syncing deployments that fight over ReplicaSets. - overlaps, err := dc.handleOverlap(d, deployments) + // List all Pods owned by this Deployment, grouped by their ReplicaSet. + // This is expensive, so do it once and pass it along to subroutines. + podMap, err := dc.getPodMapForReplicaSets(d.Namespace, rsList) 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) + return dc.syncStatusOnly(d, rsList, podMap) } // Why run the cleanup policy only when there is no rollback request? @@ -536,7 +533,7 @@ func (dc *DeploymentController) syncDeployment(key string) error { // (and chances are higher that they will work again as opposed to others that didn't) for candidates to // automatically roll back to (#23211) and the cleanup policy should help. if d.Spec.RollbackTo == nil { - _, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, false) + _, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { return err } @@ -546,11 +543,6 @@ func (dc *DeploymentController) syncDeployment(key string) error { dc.cleanupDeployment(oldRSs, d) } - err = dc.claimReplicaSets(d) - if err != nil { - return err - } - // Update deployment conditions with an Unknown condition when pausing/resuming // a deployment. In this way, we can be sure that we won't timeout when a user // resumes a Deployment with a set progressDeadlineSeconds. @@ -558,7 +550,7 @@ func (dc *DeploymentController) syncDeployment(key string) error { return err } - _, err = dc.hasFailed(d) + _, err = dc.hasFailed(d, rsList, podMap) if err != nil { return err } @@ -567,152 +559,29 @@ func (dc *DeploymentController) syncDeployment(key string) error { // See https://github.com/kubernetes/kubernetes/issues/23211. if d.Spec.Paused { - return dc.sync(d) + return dc.sync(d, rsList, podMap) } // rollback is not re-entrant in case the underlying replica sets are updated with a new // revision so we should ensure that we won't proceed to update replica sets until we // make sure that the deployment has cleaned up its rollback spec in subsequent enqueues. if d.Spec.RollbackTo != nil { - return dc.rollback(d) + return dc.rollback(d, rsList, podMap) } - scalingEvent, err := dc.isScalingEvent(d) + scalingEvent, err := dc.isScalingEvent(d, rsList, podMap) if err != nil { return err } if scalingEvent { - return dc.sync(d) + return dc.sync(d, rsList, podMap) } switch d.Spec.Strategy.Type { case extensions.RecreateDeploymentStrategyType: - return dc.rolloutRecreate(d) + return dc.rolloutRecreate(d, rsList, podMap) case extensions.RollingUpdateDeploymentStrategyType: - return dc.rolloutRolling(d) + return dc.rolloutRolling(d, rsList, podMap) } return fmt.Errorf("unexpected deployment strategy type: %s", d.Spec.Strategy.Type) } - -// 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 - var errs []error - - for i := range deployments { - otherD := deployments[i] - - if d.Name == otherD.Name { - continue - } - - // 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 { - 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 - } - - // Skip syncing this one if older overlapping one is found. - if currentOverlapsWith == otherCopy.Name || 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) - } - } - } - - if !overlapping { - var err error - if d, err = dc.clearDeploymentOverlap(d, ""); err != nil { - errs = append(errs, err) - } - } - - return false, utilerrors.NewAggregate(errs) -} - -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, 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) - return dc.client.Extensions().Deployments(deployment.Namespace).UpdateStatus(deployment) -} diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index 7e63cf02915..dff3d54b8af 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -18,7 +18,6 @@ package deployment import ( "testing" - "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -108,13 +107,13 @@ func newDeployment(name string, replicas int, revisionHistoryLimit *int32, maxSu } func newReplicaSet(d *extensions.Deployment, name string, replicas int) *extensions.ReplicaSet { - control := true return &extensions.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{ Name: name, + UID: uuid.NewUUID(), Namespace: metav1.NamespaceDefault, Labels: d.Spec.Selector.MatchLabels, - OwnerReferences: []metav1.OwnerReference{{APIVersion: getDeploymentKind().GroupVersion().Version, Kind: getDeploymentKind().Kind, Name: d.Name, UID: d.UID, Controller: &control}}, + OwnerReferences: []metav1.OwnerReference{*newControllerRef(d)}, }, Spec: extensions.ReplicaSetSpec{ Selector: d.Spec.Selector, @@ -311,195 +310,6 @@ func TestReentrantRollback(t *testing.T) { f.run(getKey(d, t)) } -// TestOverlappingDeployment ensures that an overlapping deployment will not be synced by -// the controller. -func TestOverlappingDeployment(t *testing.T) { - f := newFixture(t) - now := metav1.Now() - later := metav1.Time{Time: now.Add(time.Minute)} - - foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - foo.CreationTimestamp = now - bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"}) - bar.CreationTimestamp = later - - f.dLister = append(f.dLister, foo, bar) - f.objects = append(f.objects, foo, bar) - - f.expectUpdateDeploymentStatusAction(bar) - f.run(getKey(bar, t)) - - for _, a := range filterInformerActions(f.client.Actions()) { - action, ok := a.(core.UpdateAction) - if !ok { - continue - } - d, ok := action.GetObject().(*extensions.Deployment) - if !ok { - continue - } - if d.Name == "bar" && d.Annotations[util.OverlapAnnotation] != "foo" { - t.Errorf("annotations weren't updated for the overlapping deployment: %v", d.Annotations) - } - } -} - -// TestSyncOverlappedDeployment ensures that from two overlapping deployments, the older -// one will be synced and the newer will be marked as overlapping. Note that in reality it's -// not always the older deployment that is the one that works vs the rest but the one which -// has the selector unchanged for longer time. -func TestSyncOverlappedDeployment(t *testing.T) { - f := newFixture(t) - now := metav1.Now() - later := metav1.Time{Time: now.Add(time.Minute)} - - foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - foo.CreationTimestamp = now - bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"}) - bar.CreationTimestamp = later - - f.dLister = append(f.dLister, foo, bar) - f.objects = append(f.objects, foo, bar) - - f.expectUpdateDeploymentStatusAction(bar) - f.expectCreateRSAction(newReplicaSet(foo, "foo-rs", 1)) - f.expectUpdateDeploymentStatusAction(foo) - f.expectUpdateDeploymentStatusAction(foo) - f.run(getKey(foo, t)) - - for _, a := range filterInformerActions(f.client.Actions()) { - action, ok := a.(core.UpdateAction) - if !ok { - continue - } - d, ok := action.GetObject().(*extensions.Deployment) - if !ok { - continue - } - if d.Name == "bar" && d.Annotations[util.OverlapAnnotation] != "foo" { - t.Errorf("annotations weren't updated for the overlapping deployment: %v", d.Annotations) - } - } -} - -// TestSelectorUpdate ensures that from two overlapping deployments, the one that is working won't -// be marked as overlapping if its selector is updated but still overlaps with the other one. -func TestSelectorUpdate(t *testing.T) { - f := newFixture(t) - now := metav1.Now() - later := metav1.Time{Time: now.Add(time.Minute)} - selectorUpdated := metav1.Time{Time: later.Add(time.Minute)} - - foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - foo.CreationTimestamp = now - foo.Annotations = map[string]string{util.SelectorUpdateAnnotation: selectorUpdated.Format(time.RFC3339)} - bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar", "app": "baz"}) - bar.CreationTimestamp = later - bar.Annotations = map[string]string{util.OverlapAnnotation: "foo"} - - f.dLister = append(f.dLister, foo, bar) - f.objects = append(f.objects, foo, bar) - - f.expectCreateRSAction(newReplicaSet(foo, "foo-rs", 1)) - f.expectUpdateDeploymentStatusAction(foo) - f.expectUpdateDeploymentStatusAction(foo) - f.run(getKey(foo, t)) - - for _, a := range filterInformerActions(f.client.Actions()) { - action, ok := a.(core.UpdateAction) - if !ok { - continue - } - d, ok := action.GetObject().(*extensions.Deployment) - if !ok { - continue - } - - if d.Name == "foo" && len(d.Annotations[util.OverlapAnnotation]) > 0 { - t.Errorf("deployment %q should not have the overlapping annotation", d.Name) - } - if d.Name == "bar" && len(d.Annotations[util.OverlapAnnotation]) == 0 { - t.Errorf("deployment %q should have the overlapping annotation", d.Name) - } - } -} - -// TestDeletedDeploymentShouldCleanupOverlaps ensures that the deletion of a deployment -// will cleanup any deployments that overlap with it. -func TestDeletedDeploymentShouldCleanupOverlaps(t *testing.T) { - f := newFixture(t) - now := metav1.Now() - earlier := metav1.Time{Time: now.Add(-time.Minute)} - later := metav1.Time{Time: now.Add(time.Minute)} - - foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - foo.CreationTimestamp = earlier - foo.DeletionTimestamp = &now - bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - bar.CreationTimestamp = later - bar.Annotations = map[string]string{util.OverlapAnnotation: "foo"} - - f.dLister = append(f.dLister, foo, bar) - f.objects = append(f.objects, foo, bar) - - f.expectUpdateDeploymentStatusAction(bar) - f.expectUpdateDeploymentStatusAction(foo) - f.run(getKey(foo, t)) - - for _, a := range filterInformerActions(f.client.Actions()) { - action, ok := a.(core.UpdateAction) - if !ok { - continue - } - d := action.GetObject().(*extensions.Deployment) - if d.Name != "bar" { - continue - } - - if len(d.Annotations[util.OverlapAnnotation]) > 0 { - t.Errorf("annotations weren't cleaned up for the overlapping deployment: %v", d.Annotations) - } - } -} - -// TestDeletedDeploymentShouldNotCleanupOtherOverlaps ensures that the deletion of -// a deployment will not cleanup deployments that overlap with another deployment. -func TestDeletedDeploymentShouldNotCleanupOtherOverlaps(t *testing.T) { - f := newFixture(t) - now := metav1.Now() - earlier := metav1.Time{Time: now.Add(-time.Minute)} - later := metav1.Time{Time: now.Add(time.Minute)} - - foo := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - foo.CreationTimestamp = earlier - foo.DeletionTimestamp = &now - bar := newDeployment("bar", 1, nil, nil, nil, map[string]string{"bla": "bla"}) - bar.CreationTimestamp = later - // Notice this deployment is overlapping with another deployment - bar.Annotations = map[string]string{util.OverlapAnnotation: "baz"} - - f.dLister = append(f.dLister, foo, bar) - f.objects = append(f.objects, foo, bar) - - f.expectUpdateDeploymentStatusAction(foo) - f.run(getKey(foo, t)) - - for _, a := range filterInformerActions(f.client.Actions()) { - action, ok := a.(core.UpdateAction) - if !ok { - continue - } - d := action.GetObject().(*extensions.Deployment) - if d.Name != "bar" { - continue - } - - if len(d.Annotations[util.OverlapAnnotation]) == 0 { - t.Errorf("overlapping annotation should not be cleaned up for bar: %v", d.Annotations) - } - } -} - // TestPodDeletionEnqueuesRecreateDeployment ensures that the deletion of a pod // will requeue a Recreate deployment iff there is no other pod returned from the // client. @@ -562,6 +372,179 @@ func TestPodDeletionDoesntEnqueueRecreateDeployment(t *testing.T) { } } +func TestGetReplicaSetsForDeployment(t *testing.T) { + f := newFixture(t) + + // Two Deployments with same labels. + d1 := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("bar", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // Two ReplicaSets that match labels for both Deployments, + // but have ControllerRefs to make ownership explicit. + rs1 := newReplicaSet(d1, "rs1", 1) + rs2 := newReplicaSet(d2, "rs2", 1) + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs1, rs2) + f.objects = append(f.objects, d1, d2, rs1, rs2) + + // Start the fixture. + c, informers := f.newController() + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + rsList, err := c.getReplicaSetsForDeployment(d1) + if err != nil { + t.Fatalf("getReplicaSetsForDeployment() error: %v", err) + } + rsNames := []string{} + for _, rs := range rsList { + rsNames = append(rsNames, rs.Name) + } + if len(rsNames) != 1 || rsNames[0] != rs1.Name { + t.Errorf("getReplicaSetsForDeployment() = %v, want [%v]", rsNames, rs1.Name) + } + + rsList, err = c.getReplicaSetsForDeployment(d2) + if err != nil { + t.Fatalf("getReplicaSetsForDeployment() error: %v", err) + } + rsNames = []string{} + for _, rs := range rsList { + rsNames = append(rsNames, rs.Name) + } + if len(rsNames) != 1 || rsNames[0] != rs2.Name { + t.Errorf("getReplicaSetsForDeployment() = %v, want [%v]", rsNames, rs2.Name) + } +} + +func TestGetReplicaSetsForDeploymentAdopt(t *testing.T) { + f := newFixture(t) + + d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // RS with matching labels, but orphaned. Should be adopted and returned. + rs := newReplicaSet(d, "rs", 1) + rs.OwnerReferences = nil + + f.dLister = append(f.dLister, d) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, d, rs) + + // Start the fixture. + c, informers := f.newController() + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + rsList, err := c.getReplicaSetsForDeployment(d) + if err != nil { + t.Fatalf("getReplicaSetsForDeployment() error: %v", err) + } + rsNames := []string{} + for _, rs := range rsList { + rsNames = append(rsNames, rs.Name) + } + if len(rsNames) != 1 || rsNames[0] != rs.Name { + t.Errorf("getReplicaSetsForDeployment() = %v, want [%v]", rsNames, rs.Name) + } +} + +func TestGetReplicaSetsForDeploymentRelease(t *testing.T) { + f := newFixture(t) + + d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // RS with matching ControllerRef, but wrong labels. Should be released. + rs := newReplicaSet(d, "rs", 1) + rs.Labels = map[string]string{"foo": "notbar"} + + f.dLister = append(f.dLister, d) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, d, rs) + + // Start the fixture. + c, informers := f.newController() + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + rsList, err := c.getReplicaSetsForDeployment(d) + if err != nil { + t.Fatalf("getReplicaSetsForDeployment() error: %v", err) + } + rsNames := []string{} + for _, rs := range rsList { + rsNames = append(rsNames, rs.Name) + } + if len(rsNames) != 0 { + t.Errorf("getReplicaSetsForDeployment() = %v, want []", rsNames) + } +} + +func TestGetPodMapForReplicaSets(t *testing.T) { + f := newFixture(t) + + d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // Two ReplicaSets that match labels for both Deployments, + // but have ControllerRefs to make ownership explicit. + rs1 := newReplicaSet(d, "rs1", 1) + rs2 := newReplicaSet(d, "rs2", 1) + + // Add a Pod for each ReplicaSet. + pod1 := generatePodFromRS(rs1) + pod2 := generatePodFromRS(rs2) + // Add a Pod that has matching labels, but no ControllerRef. + pod3 := generatePodFromRS(rs1) + pod3.Name = "pod3" + pod3.OwnerReferences = nil + // Add a Pod that has matching labels and ControllerRef, but is inactive. + pod4 := generatePodFromRS(rs1) + pod4.Name = "pod4" + pod4.Status.Phase = v1.PodFailed + + f.dLister = append(f.dLister, d) + f.rsLister = append(f.rsLister, rs1, rs2) + f.podLister = append(f.podLister, pod1, pod2, pod3, pod4) + f.objects = append(f.objects, d, rs1, rs2, pod1, pod2, pod3, pod4) + + // Start the fixture. + c, informers := f.newController() + stopCh := make(chan struct{}) + defer close(stopCh) + informers.Start(stopCh) + + podMap, err := c.getPodMapForReplicaSets(d.Namespace, f.rsLister) + if err != nil { + t.Fatalf("getPodMapForReplicaSets() error: %v", err) + } + podCount := 0 + for _, podList := range podMap { + podCount += len(podList.Items) + } + if got, want := podCount, 2; got != want { + t.Errorf("podCount = %v, want %v", got, want) + } + + if got, want := len(podMap), 2; got != want { + t.Errorf("len(podMap) = %v, want %v", got, want) + } + if got, want := len(podMap[rs1.UID].Items), 1; got != want { + t.Errorf("len(podMap[rs1]) = %v, want %v", got, want) + } + if got, want := podMap[rs1.UID].Items[0].Name, "rs1-pod"; got != want { + t.Errorf("podMap[rs1] = [%v], want [%v]", got, want) + } + if got, want := len(podMap[rs2.UID].Items), 1; got != want { + t.Errorf("len(podMap[rs2]) = %v, want %v", got, want) + } + if got, want := podMap[rs2.UID].Items[0].Name, "rs2-pod"; got != want { + t.Errorf("podMap[rs2] = [%v], want [%v]", got, want) + } +} + // generatePodFromRS creates a pod, with the input ReplicaSet's selector and its template func generatePodFromRS(rs *extensions.ReplicaSet) *v1.Pod { trueVar := true diff --git a/pkg/controller/deployment/progress.go b/pkg/controller/deployment/progress.go index 7363da98e16..9c02f2f669f 100644 --- a/pkg/controller/deployment/progress.go +++ b/pkg/controller/deployment/progress.go @@ -23,6 +23,7 @@ import ( "github.com/golang/glog" + "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/api/v1" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" "k8s.io/kubernetes/pkg/controller/deployment/util" @@ -33,12 +34,12 @@ import ( // and when new pods scale up or old pods scale down. Progress is not estimated for paused // deployments or when users don't really care about it ie. progressDeadlineSeconds is not // specified. -func (dc *DeploymentController) hasFailed(d *extensions.Deployment) (bool, error) { +func (dc *DeploymentController) hasFailed(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) (bool, error) { if d.Spec.ProgressDeadlineSeconds == nil || d.Spec.RollbackTo != nil || d.Spec.Paused { return false, nil } - newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, false) + newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { return false, err } diff --git a/pkg/controller/deployment/recreate.go b/pkg/controller/deployment/recreate.go index 3c3be85481e..9499cc37dd6 100644 --- a/pkg/controller/deployment/recreate.go +++ b/pkg/controller/deployment/recreate.go @@ -17,14 +17,16 @@ limitations under the License. package deployment import ( + "k8s.io/apimachinery/pkg/types" + "k8s.io/kubernetes/pkg/api/v1" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" "k8s.io/kubernetes/pkg/controller" ) // rolloutRecreate implements the logic for recreating a replica set. -func (dc *DeploymentController) rolloutRecreate(deployment *extensions.Deployment) error { +func (dc *DeploymentController) rolloutRecreate(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) error { // Don't create a new RS if not already existed, so that we avoid scaling up before scaling down - newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(deployment, false) + newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { return err } @@ -32,31 +34,29 @@ func (dc *DeploymentController) rolloutRecreate(deployment *extensions.Deploymen activeOldRSs := controller.FilterActiveReplicaSets(oldRSs) // scale down old replica sets - scaledDown, err := dc.scaleDownOldReplicaSetsForRecreate(activeOldRSs, deployment) + scaledDown, err := dc.scaleDownOldReplicaSetsForRecreate(activeOldRSs, d) if err != nil { return err } if scaledDown { // Update DeploymentStatus - return dc.syncRolloutStatus(allRSs, newRS, deployment) + return dc.syncRolloutStatus(allRSs, newRS, d) } - newStatus := calculateStatus(allRSs, newRS, deployment) + newStatus := calculateStatus(allRSs, newRS, d) // Do not process a deployment when it has old pods running. if newStatus.UpdatedReplicas == 0 { - podList, err := dc.listPods(deployment) - if err != nil { - return err - } - if len(podList.Items) > 0 { - return dc.syncRolloutStatus(allRSs, newRS, deployment) + for _, podList := range podMap { + if len(podList.Items) > 0 { + return dc.syncRolloutStatus(allRSs, newRS, d) + } } } // If we need to create a new RS, create it now // TODO: Create a new RS without re-listing all RSs. if newRS == nil { - newRS, oldRSs, err = dc.getAllReplicaSetsAndSyncRevision(deployment, true) + newRS, oldRSs, err = dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, true) if err != nil { return err } @@ -64,17 +64,17 @@ func (dc *DeploymentController) rolloutRecreate(deployment *extensions.Deploymen } // scale up new replica set - scaledUp, err := dc.scaleUpNewReplicaSetForRecreate(newRS, deployment) + scaledUp, err := dc.scaleUpNewReplicaSetForRecreate(newRS, d) if err != nil { return err } if scaledUp { // Update DeploymentStatus - return dc.syncRolloutStatus(allRSs, newRS, deployment) + return dc.syncRolloutStatus(allRSs, newRS, d) } // Sync deployment status - return dc.syncRolloutStatus(allRSs, newRS, deployment) + return dc.syncRolloutStatus(allRSs, newRS, d) } // scaleDownOldReplicaSetsForRecreate scales down old replica sets when deployment strategy is "Recreate" diff --git a/pkg/controller/deployment/rollback.go b/pkg/controller/deployment/rollback.go index 68cec72f569..703cf682b19 100644 --- a/pkg/controller/deployment/rollback.go +++ b/pkg/controller/deployment/rollback.go @@ -21,14 +21,15 @@ import ( "github.com/golang/glog" + "k8s.io/apimachinery/pkg/types" "k8s.io/kubernetes/pkg/api/v1" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util" ) // rollback the deployment to the specified revision. In any case cleanup the rollback spec. -func (dc *DeploymentController) rollback(d *extensions.Deployment) error { - newRS, allOldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, true) +func (dc *DeploymentController) rollback(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) error { + newRS, allOldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, true) if err != nil { return err } diff --git a/pkg/controller/deployment/rolling.go b/pkg/controller/deployment/rolling.go index c783cb048b9..6c979b6786a 100644 --- a/pkg/controller/deployment/rolling.go +++ b/pkg/controller/deployment/rolling.go @@ -21,42 +21,44 @@ import ( "sort" "github.com/golang/glog" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/integer" + "k8s.io/kubernetes/pkg/api/v1" extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1" "k8s.io/kubernetes/pkg/controller" deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util" ) // rolloutRolling implements the logic for rolling a new replica set. -func (dc *DeploymentController) rolloutRolling(deployment *extensions.Deployment) error { - newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(deployment, true) +func (dc *DeploymentController) rolloutRolling(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) error { + newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, true) if err != nil { return err } allRSs := append(oldRSs, newRS) // Scale up, if we can. - scaledUp, err := dc.reconcileNewReplicaSet(allRSs, newRS, deployment) + scaledUp, err := dc.reconcileNewReplicaSet(allRSs, newRS, d) if err != nil { return err } if scaledUp { // Update DeploymentStatus - return dc.syncRolloutStatus(allRSs, newRS, deployment) + return dc.syncRolloutStatus(allRSs, newRS, d) } // Scale down, if we can. - scaledDown, err := dc.reconcileOldReplicaSets(allRSs, controller.FilterActiveReplicaSets(oldRSs), newRS, deployment) + scaledDown, err := dc.reconcileOldReplicaSets(allRSs, controller.FilterActiveReplicaSets(oldRSs), newRS, d) if err != nil { return err } if scaledDown { // Update DeploymentStatus - return dc.syncRolloutStatus(allRSs, newRS, deployment) + return dc.syncRolloutStatus(allRSs, newRS, d) } // Sync deployment status - return dc.syncRolloutStatus(allRSs, newRS, deployment) + return dc.syncRolloutStatus(allRSs, newRS, d) } func (dc *DeploymentController) reconcileNewReplicaSet(allRSs []*extensions.ReplicaSet, newRS *extensions.ReplicaSet, deployment *extensions.Deployment) (bool, error) { diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index cf855939b1a..4571e4a4345 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -25,7 +25,7 @@ import ( "github.com/golang/glog" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/v1" @@ -36,31 +36,31 @@ import ( ) // syncStatusOnly only updates Deployments Status and doesn't take any mutating actions. -func (dc *DeploymentController) syncStatusOnly(deployment *extensions.Deployment) error { - newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(deployment, false) +func (dc *DeploymentController) syncStatusOnly(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) error { + newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { return err } allRSs := append(oldRSs, newRS) - return dc.syncDeploymentStatus(allRSs, newRS, deployment) + return dc.syncDeploymentStatus(allRSs, newRS, d) } // sync is responsible for reconciling deployments on scaling events or when they // are paused. -func (dc *DeploymentController) sync(deployment *extensions.Deployment) error { - newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(deployment, false) +func (dc *DeploymentController) sync(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) error { + newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { return err } - if err := dc.scale(deployment, newRS, oldRSs); err != nil { + if err := dc.scale(d, newRS, oldRSs); err != nil { // If we get an error while trying to scale, the deployment will be requeued // so we can abort this resync return err } allRSs := append(oldRSs, newRS) - return dc.syncDeploymentStatus(allRSs, newRS, deployment) + return dc.syncDeploymentStatus(allRSs, newRS, d) } // checkPausedConditions checks if the given deployment is paused or not and adds an appropriate condition. @@ -98,25 +98,31 @@ func (dc *DeploymentController) checkPausedConditions(d *extensions.Deployment) } // getAllReplicaSetsAndSyncRevision returns all the replica sets for the provided deployment (new and all old), with new RS's and deployment's revision updated. +// +// rsList should come from getReplicaSetsForDeployment(d). +// podMap should come from getPodMapForReplicaSets(rsList). +// These are passed around to avoid repeating expensive API calls. +// // 1. Get all old RSes this deployment targets, and calculate the max revision number among them (maxOldV). // 2. Get new RS this deployment targets (whose pod template matches deployment's), and update new RS's revision number to (maxOldV + 1), // only if its revision number is smaller than (maxOldV + 1). If this step failed, we'll update it in the next deployment sync loop. // 3. Copy new RS's revision number to deployment (update deployment's revision). If this step failed, we'll update it in the next deployment sync loop. +// // Note that currently the deployment controller is using caches to avoid querying the server for reads. // This may lead to stale reads of replica sets, thus incorrect deployment status. -func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(deployment *extensions.Deployment, createIfNotExisted bool) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { +func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList, createIfNotExisted bool) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { // List the deployment's RSes & Pods and apply pod-template-hash info to deployment's adopted RSes/Pods - rsList, podList, err := dc.rsAndPodsWithHashKeySynced(deployment) + rsList, podList, err := dc.rsAndPodsWithHashKeySynced(d, rsList, podMap) if err != nil { return nil, nil, fmt.Errorf("error labeling replica sets and pods with pod-template-hash: %v", err) } - _, allOldRSs, err := deploymentutil.FindOldReplicaSets(deployment, rsList, podList) + _, allOldRSs, err := deploymentutil.FindOldReplicaSets(d, rsList, podList) if err != nil { return nil, nil, err } // Get new replica set with the updated revision number - newRS, err := dc.getNewReplicaSet(deployment, rsList, allOldRSs, createIfNotExisted) + newRS, err := dc.getNewReplicaSet(d, rsList, allOldRSs, createIfNotExisted) if err != nil { return nil, nil, err } @@ -124,33 +130,28 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(deployment *ext return newRS, allOldRSs, nil } -// rsAndPodsWithHashKeySynced returns the RSes and pods the given deployment targets, with pod-template-hash information synced. -func (dc *DeploymentController) rsAndPodsWithHashKeySynced(deployment *extensions.Deployment) ([]*extensions.ReplicaSet, *v1.PodList, error) { - rsList, err := deploymentutil.ListReplicaSets(deployment, - func(namespace string, options metav1.ListOptions) ([]*extensions.ReplicaSet, error) { - parsed, err := labels.Parse(options.LabelSelector) - if err != nil { - return nil, err - } - return dc.rsLister.ReplicaSets(namespace).List(parsed) - }) - if err != nil { - return nil, nil, fmt.Errorf("error listing ReplicaSets: %v", err) - } +// rsAndPodsWithHashKeySynced returns the RSes and pods the given deployment +// targets, with pod-template-hash information synced. +// +// rsList should come from getReplicaSetsForDeployment(d). +// podMap should come from getPodMapForReplicaSets(rsList). +// These are passed around to avoid repeating expensive API calls. +func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) ([]*extensions.ReplicaSet, *v1.PodList, error) { syncedRSList := []*extensions.ReplicaSet{} for _, rs := range rsList { // Add pod-template-hash information if it's not in the RS. // Otherwise, new RS produced by Deployment will overlap with pre-existing ones // that aren't constrained by the pod-template-hash. - syncedRS, err := dc.addHashKeyToRSAndPods(rs) + syncedRS, err := dc.addHashKeyToRSAndPods(rs, podMap[rs.UID]) if err != nil { return nil, nil, err } syncedRSList = append(syncedRSList, syncedRS) } - syncedPodList, err := dc.listPods(deployment) - if err != nil { - return nil, nil, err + // Put all Pods from podMap into one list. + syncedPodList := &v1.PodList{} + for _, podList := range podMap { + syncedPodList.Items = append(syncedPodList.Items, podList.Items...) } return syncedRSList, syncedPodList, nil } @@ -159,7 +160,7 @@ func (dc *DeploymentController) rsAndPodsWithHashKeySynced(deployment *extension // 1. Add hash label to the rs's pod template, and make sure the controller sees this update so that no orphaned pods will be created // 2. Add hash label to all pods this rs owns, wait until replicaset controller reports rs.Status.FullyLabeledReplicas equal to the desired number of replicas // 3. Add hash label to the rs's label and selector -func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet) (*extensions.ReplicaSet, error) { +func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet, podList *v1.PodList) (*extensions.ReplicaSet, error) { // If the rs already has the new hash label in its selector, it's done syncing if labelsutil.SelectorHasLabel(rs.Spec.Selector, extensions.DefaultDeploymentUniqueLabelKey) { return rs, nil @@ -189,24 +190,7 @@ func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet) } // 2. Update all pods managed by the rs to have the new hash label, so they will be correctly adopted. - selector, err := metav1.LabelSelectorAsSelector(updatedRS.Spec.Selector) - if err != nil { - return nil, fmt.Errorf("error in converting selector to label selector for replica set %s: %s", updatedRS.Name, err) - } - options := metav1.ListOptions{LabelSelector: selector.String()} - parsed, err := labels.Parse(options.LabelSelector) - if err != nil { - return nil, err - } - pods, err := dc.podLister.Pods(updatedRS.Namespace).List(parsed) - if err != nil { - return nil, fmt.Errorf("error in getting pod list for namespace %s and list options %+v: %s", rs.Namespace, options, err) - } - podList := v1.PodList{Items: make([]v1.Pod, 0, len(pods))} - for i := range pods { - podList.Items = append(podList.Items, *pods[i]) - } - if err := deploymentutil.LabelPodsWithHash(&podList, dc.client, dc.podLister, rs.Namespace, rs.Name, hash); err != nil { + if err := deploymentutil.LabelPodsWithHash(podList, dc.client, dc.podLister, rs.Namespace, rs.Name, hash); err != nil { return nil, fmt.Errorf("error in adding template hash label %s to pods %+v: %s", hash, podList, err) } @@ -242,22 +226,6 @@ func (dc *DeploymentController) addHashKeyToRSAndPods(rs *extensions.ReplicaSet) return updatedRS, nil } -func (dc *DeploymentController) listPods(deployment *extensions.Deployment) (*v1.PodList, error) { - return deploymentutil.ListPods(deployment, - func(namespace string, options metav1.ListOptions) (*v1.PodList, error) { - parsed, err := labels.Parse(options.LabelSelector) - if err != nil { - return nil, err - } - pods, err := dc.podLister.Pods(namespace).List(parsed) - result := v1.PodList{Items: make([]v1.Pod, 0, len(pods))} - for i := range pods { - result.Items = append(result.Items, *pods[i]) - } - return &result, err - }) -} - // Returns a replica set that matches the intent of the given deployment. Returns nil if the new replica set doesn't exist yet. // 1. Get existing new RS (the RS that the given deployment targets, whose pod template is the same as deployment's). // 2. If there's existing new RS, update its revision number if it's smaller than (maxOldRevision + 1), where maxOldRevision is the max revision number among all old RSes. @@ -329,25 +297,17 @@ func (dc *DeploymentController) getNewReplicaSet(deployment *extensions.Deployme newRS := extensions.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{ // Make the name deterministic, to ensure idempotence - Name: deployment.Name + "-" + podTemplateSpecHash, - Namespace: namespace, + Name: deployment.Name + "-" + podTemplateSpecHash, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{*newControllerRef(deployment)}, }, Spec: extensions.ReplicaSetSpec{ - Replicas: func(i int32) *int32 { return &i }(0), + Replicas: new(int32), MinReadySeconds: deployment.Spec.MinReadySeconds, Selector: newRSSelector, Template: newRSTemplate, }, } - var trueVar = true - controllerRef := &metav1.OwnerReference{ - APIVersion: getDeploymentKind().GroupVersion().String(), - Kind: getDeploymentKind().Kind, - Name: deployment.Name, - UID: deployment.UID, - Controller: &trueVar, - } - newRS.OwnerReferences = append(newRS.OwnerReferences, *controllerRef) allRSs := append(oldRSs, &newRS) newReplicasCount, err := deploymentutil.NewRSNewReplicas(deployment, allRSs, &newRS) if err != nil { @@ -632,8 +592,12 @@ func calculateStatus(allRSs []*extensions.ReplicaSet, newRS *extensions.ReplicaS // isScalingEvent checks whether the provided deployment has been updated with a scaling event // by looking at the desired-replicas annotation in the active replica sets of the deployment. -func (dc *DeploymentController) isScalingEvent(d *extensions.Deployment) (bool, error) { - newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, false) +// +// rsList should come from getReplicaSetsForDeployment(d). +// podMap should come from getPodMapForReplicaSets(rsList). +// These are passed around to avoid repeating expensive API calls. +func (dc *DeploymentController) isScalingEvent(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) (bool, error) { + newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { return false, err } @@ -649,3 +613,15 @@ func (dc *DeploymentController) isScalingEvent(d *extensions.Deployment) (bool, } return false, nil } + +// newControllerRef returns a ControllerRef pointing to the deployment. +func newControllerRef(d *extensions.Deployment) *metav1.OwnerReference { + isController := true + return &metav1.OwnerReference{ + APIVersion: controllerKind.GroupVersion().String(), + Kind: controllerKind.Kind, + Name: d.Name, + UID: d.UID, + Controller: &isController, + } +} From 887acb07eae1c6e87a2b84b5727043d738e3fccf Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Sun, 26 Feb 2017 15:26:33 -0800 Subject: [PATCH 02/12] Deployment/util: Filter by ControllerRef. The list functions in deployment/util are used outside the Deployment controller itself. Therefore, they don't do actual adoption/orphaning. However, they still need to avoid listing things that don't belong. --- pkg/controller/deployment/util/BUILD | 1 + .../deployment/util/deployment_util.go | 56 ++++++++++++++++--- .../deployment/util/deployment_util_test.go | 37 ++++++++++-- test/e2e/framework/util.go | 17 +++++- 4 files changed, 95 insertions(+), 16 deletions(-) diff --git a/pkg/controller/deployment/util/BUILD b/pkg/controller/deployment/util/BUILD index 17aac73be64..7b4e638136b 100644 --- a/pkg/controller/deployment/util/BUILD +++ b/pkg/controller/deployment/util/BUILD @@ -37,6 +37,7 @@ go_library( "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/labels", "//vendor:k8s.io/apimachinery/pkg/runtime", + "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apimachinery/pkg/util/errors", "//vendor:k8s.io/apimachinery/pkg/util/intstr", "//vendor:k8s.io/apimachinery/pkg/util/wait", diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 7ccb7b6102e..b0fcfcb0d37 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -30,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/errors" intstrutil "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/wait" @@ -507,7 +508,7 @@ func GetAllReplicaSets(deployment *extensions.Deployment, c clientset.Interface) if err != nil { return nil, nil, nil, err } - podList, err := listPods(deployment, c) + podList, err := listPods(deployment, rsList, c) if err != nil { return nil, nil, nil, err } @@ -529,7 +530,7 @@ func GetOldReplicaSets(deployment *extensions.Deployment, c clientset.Interface) if err != nil { return nil, nil, err } - podList, err := listPods(deployment, c) + podList, err := listPods(deployment, rsList, c) if err != nil { return nil, nil, err } @@ -563,8 +564,8 @@ func listReplicaSets(deployment *extensions.Deployment, c clientset.Interface) ( } // listReplicaSets lists all Pods the given deployment targets with the given client interface. -func listPods(deployment *extensions.Deployment, c clientset.Interface) (*v1.PodList, error) { - return ListPods(deployment, +func listPods(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet, c clientset.Interface) (*v1.PodList, error) { + return ListPods(deployment, rsList, func(namespace string, options metav1.ListOptions) (*v1.PodList, error) { return c.Core().Pods(namespace).List(options) }) @@ -575,28 +576,65 @@ type rsListFunc func(string, metav1.ListOptions) ([]*extensions.ReplicaSet, erro type podListFunc func(string, metav1.ListOptions) (*v1.PodList, error) // ListReplicaSets returns a slice of RSes the given deployment targets. +// Note that this does NOT attempt to reconcile ControllerRef (adopt/orphan), +// because only the controller itself should do that. +// However, it does filter out anything whose ControllerRef doesn't match. func ListReplicaSets(deployment *extensions.Deployment, getRSList rsListFunc) ([]*extensions.ReplicaSet, error) { // TODO: Right now we list replica sets by their labels. We should list them by selector, i.e. the replica set's selector - // should be a superset of the deployment's selector, see https://github.com/kubernetes/kubernetes/issues/19830; - // or use controllerRef, see https://github.com/kubernetes/kubernetes/issues/2210 + // should be a superset of the deployment's selector, see https://github.com/kubernetes/kubernetes/issues/19830. namespace := deployment.Namespace selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) if err != nil { return nil, err } options := metav1.ListOptions{LabelSelector: selector.String()} - return getRSList(namespace, options) + all, err := getRSList(namespace, options) + if err != nil { + return all, err + } + // Only include those whose ControllerRef matches the Deployment. + owned := make([]*extensions.ReplicaSet, 0, len(all)) + for _, rs := range all { + controllerRef := controller.GetControllerOf(rs) + if controllerRef != nil && controllerRef.UID == deployment.UID { + owned = append(owned, rs) + } + } + return owned, nil } // ListPods returns a list of pods the given deployment targets. -func ListPods(deployment *extensions.Deployment, getPodList podListFunc) (*v1.PodList, error) { +// This needs a list of ReplicaSets for the Deployment, +// which can be found with ListReplicaSets(). +// Note that this does NOT attempt to reconcile ControllerRef (adopt/orphan), +// because only the controller itself should do that. +// However, it does filter out anything whose ControllerRef doesn't match. +func ListPods(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet, getPodList podListFunc) (*v1.PodList, error) { namespace := deployment.Namespace selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) if err != nil { return nil, err } options := metav1.ListOptions{LabelSelector: selector.String()} - return getPodList(namespace, options) + all, err := getPodList(namespace, options) + if err != nil { + return all, err + } + // Only include those whose ControllerRef points to a ReplicaSet that is in + // turn owned by this Deployment. + rsMap := make(map[types.UID]bool, len(rsList)) + for _, rs := range rsList { + rsMap[rs.UID] = true + } + owned := &v1.PodList{Items: make([]v1.Pod, 0, len(all.Items))} + for i := range all.Items { + pod := &all.Items[i] + controllerRef := controller.GetControllerOf(pod) + if controllerRef != nil && rsMap[controllerRef.UID] { + owned.Items = append(owned.Items, *pod) + } + } + return owned, nil } // EqualIgnoreHash returns true if two given podTemplateSpec are equal, ignoring the diff in value of Labels[pod-template-hash] diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 7bca36b9d20..add606ce043 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -105,11 +105,23 @@ func newPod(now time.Time, ready bool, beforeSec int) v1.Pod { } } +func newRSControllerRef(rs *extensions.ReplicaSet) *metav1.OwnerReference { + isController := true + return &metav1.OwnerReference{ + APIVersion: "extensions/v1beta1", + Kind: "ReplicaSet", + Name: rs.GetName(), + UID: rs.GetUID(), + Controller: &isController, + } +} + // generatePodFromRS creates a pod, with the input ReplicaSet's selector and its template func generatePodFromRS(rs extensions.ReplicaSet) v1.Pod { return v1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Labels: rs.Labels, + Labels: rs.Labels, + OwnerReferences: []metav1.OwnerReference{*newRSControllerRef(&rs)}, }, Spec: rs.Spec.Template.Spec, } @@ -161,14 +173,26 @@ func generateRSWithLabel(labels map[string]string, image string) extensions.Repl } } +func newDControllerRef(d *extensions.Deployment) *metav1.OwnerReference { + isController := true + return &metav1.OwnerReference{ + APIVersion: "extensions/v1beta1", + Kind: "Deployment", + Name: d.GetName(), + UID: d.GetUID(), + Controller: &isController, + } +} + // generateRS creates a replica set, with the input deployment's template as its template func generateRS(deployment extensions.Deployment) extensions.ReplicaSet { template := GetNewReplicaSetTemplate(&deployment) return extensions.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{ - UID: randomUID(), - Name: v1.SimpleNameGenerator.GenerateName("replicaset"), - Labels: template.Labels, + UID: randomUID(), + Name: v1.SimpleNameGenerator.GenerateName("replicaset"), + Labels: template.Labels, + OwnerReferences: []metav1.OwnerReference{*newDControllerRef(&deployment)}, }, Spec: extensions.ReplicaSetSpec{ Replicas: func() *int32 { i := int32(0); return &i }(), @@ -291,7 +315,8 @@ func TestGetOldRCs(t *testing.T) { oldRS2.Status.FullyLabeledReplicas = *(oldRS2.Spec.Replicas) oldPod2 := generatePodFromRS(oldRS2) - // create 1 ReplicaSet that existed before the deployment, with the same labels as the deployment + // create 1 ReplicaSet that existed before the deployment, + // with the same labels as the deployment, but no ControllerRef. existedPod := generatePod(newDeployment.Spec.Template.Labels, "foo") existedRS := generateRSWithLabel(newDeployment.Spec.Template.Labels, "foo") existedRS.Status.FullyLabeledReplicas = *(existedRS.Spec.Replicas) @@ -345,7 +370,7 @@ func TestGetOldRCs(t *testing.T) { }, }, }, - []*extensions.ReplicaSet{&oldRS, &oldRS2, &existedRS}, + []*extensions.ReplicaSet{&oldRS, &oldRS2}, }, } diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index b172c0d8874..bcde917991d 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -3515,7 +3515,22 @@ func WaitForDeploymentWithCondition(c clientset.Interface, ns, deploymentName, r func logPodsOfDeployment(c clientset.Interface, deployment *extensions.Deployment) { minReadySeconds := deployment.Spec.MinReadySeconds - podList, err := deploymentutil.ListPods(deployment, + rsList, err := deploymentutil.ListReplicaSets(deployment, + func(namespace string, options metav1.ListOptions) ([]*extensions.ReplicaSet, error) { + rsList, err := c.Extensions().ReplicaSets(namespace).List(options) + if err != nil { + return nil, err + } + ret := make([]*extensions.ReplicaSet, 0, len(rsList.Items)) + for i := range rsList.Items { + ret = append(ret, &rsList.Items[i]) + } + return ret, nil + }) + if err != nil { + Logf("Failed to list ReplicaSets of Deployment %s: %v", deployment.Name, err) + } + podList, err := deploymentutil.ListPods(deployment, rsList, func(namespace string, options metav1.ListOptions) (*v1.PodList, error) { return c.Core().Pods(namespace).List(options) }) From 0d9c9bfee0e54fc8b521ab259f58b8a3b27769f9 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Sun, 26 Feb 2017 16:25:21 -0800 Subject: [PATCH 03/12] Deployment: Use ControllerRef to route watch events. This is part of the completion of ControllerRef, as described here: https://github.com/kubernetes/community/blob/master/contributors/design-proposals/controller-ref.md#watches --- pkg/controller/deployment/BUILD | 1 - .../deployment/deployment_controller.go | 163 +++++++--- .../deployment/deployment_controller_test.go | 279 +++++++++++++++++- 3 files changed, 393 insertions(+), 50 deletions(-) diff --git a/pkg/controller/deployment/BUILD b/pkg/controller/deployment/BUILD index f0cb68a73fd..f6b6ec4cfb0 100644 --- a/pkg/controller/deployment/BUILD +++ b/pkg/controller/deployment/BUILD @@ -33,7 +33,6 @@ go_library( "//pkg/util/labels:go_default_library", "//pkg/util/metrics:go_default_library", "//vendor:github.com/golang/glog", - "//vendor:k8s.io/apimachinery/pkg/api/equality", "//vendor:k8s.io/apimachinery/pkg/api/errors", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/labels", diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 3e31684b274..be51c9aabb1 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -23,11 +23,9 @@ package deployment import ( "fmt" "reflect" - "sort" "time" "github.com/golang/glog" - apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -195,17 +193,46 @@ func (dc *DeploymentController) deleteDeployment(obj interface{}) { // addReplicaSet enqueues the deployment that manages a ReplicaSet when the ReplicaSet is created. func (dc *DeploymentController) addReplicaSet(obj interface{}) { rs := obj.(*extensions.ReplicaSet) - glog.V(4).Infof("ReplicaSet %s added.", rs.Name) - if d := dc.getDeploymentForReplicaSet(rs); d != nil { + + if rs.DeletionTimestamp != nil { + // On a restart of the controller manager, it's possible for an object to + // show up in a state that is already pending deletion. + dc.deleteReplicaSet(rs) + return + } + + // If it has a ControllerRef, that's all that matters. + if controllerRef := controller.GetControllerOf(rs); controllerRef != nil { + if controllerRef.Kind != controllerKind.Kind { + // It's controller by a different type of controller. + return + } + glog.V(4).Infof("ReplicaSet %s added.", rs.Name) + d, err := dc.dLister.Deployments(rs.Namespace).Get(controllerRef.Name) + if err != nil { + return + } + dc.enqueueDeployment(d) + return + } + + // Otherwise, it's an orphan. Get a list of all matching Deployments and sync + // them to see if anyone wants to adopt it. + ds := dc.getDeploymentsForReplicaSet(rs) + if len(ds) == 0 { + return + } + glog.V(4).Infof("Orphan ReplicaSet %s added.", rs.Name) + for _, d := range ds { dc.enqueueDeployment(d) } } -// getDeploymentForReplicaSet returns the deployment managing the given ReplicaSet. -func (dc *DeploymentController) getDeploymentForReplicaSet(rs *extensions.ReplicaSet) *extensions.Deployment { +// getDeploymentsForReplicaSet returns a list of Deployments that potentially +// match a ReplicaSet. +func (dc *DeploymentController) getDeploymentsForReplicaSet(rs *extensions.ReplicaSet) []*extensions.Deployment { deployments, err := dc.dLister.GetDeploymentsForReplicaSet(rs) if err != nil || len(deployments) == 0 { - glog.V(4).Infof("Error: %v. No deployment found for ReplicaSet %v, deployment controller will avoid syncing.", err, rs.Name) return nil } // Because all ReplicaSet's belonging to a deployment should have a unique label key, @@ -213,11 +240,12 @@ func (dc *DeploymentController) getDeploymentForReplicaSet(rs *extensions.Replic // If that happens we should probably dynamically repair the situation by ultimately // trying to clean up one of the controllers, for now we just return the older one if len(deployments) > 1 { - sort.Sort(util.BySelectorLastUpdateTime(deployments)) + // ControllerRef will ensure we don't do anything crazy, but more than one + // item in this list nevertheless constitutes user error. glog.V(4).Infof("user error! more than one deployment is selecting replica set %s/%s with labels: %#v, returning %s/%s", rs.Namespace, rs.Name, rs.Labels, deployments[0].Namespace, deployments[0].Name) } - return deployments[0] + return deployments } // updateReplicaSet figures out what deployment(s) manage a ReplicaSet when the ReplicaSet @@ -232,16 +260,45 @@ func (dc *DeploymentController) updateReplicaSet(old, cur interface{}) { // Two different versions of the same replica set will always have different RVs. return } - // TODO: Write a unittest for this case - glog.V(4).Infof("ReplicaSet %s updated.", curRS.Name) - if d := dc.getDeploymentForReplicaSet(curRS); d != nil { - dc.enqueueDeployment(d) + + curControllerRef := controller.GetControllerOf(curRS) + oldControllerRef := controller.GetControllerOf(oldRS) + controllerRefChanged := !reflect.DeepEqual(curControllerRef, oldControllerRef) + if controllerRefChanged && + oldControllerRef != nil && oldControllerRef.Kind == controllerKind.Kind { + // The ControllerRef was changed. Sync the old controller, if any. + d, err := dc.dLister.Deployments(oldRS.Namespace).Get(oldControllerRef.Name) + if err == nil { + dc.enqueueDeployment(d) + } } - // A number of things could affect the old deployment: labels changing, - // pod template changing, etc. - if !apiequality.Semantic.DeepEqual(oldRS, curRS) { - if oldD := dc.getDeploymentForReplicaSet(oldRS); oldD != nil { - dc.enqueueDeployment(oldD) + + // If it has a ControllerRef, that's all that matters. + if curControllerRef != nil { + if curControllerRef.Kind != controllerKind.Kind { + // It's controlled by a different type of controller. + return + } + glog.V(4).Infof("ReplicaSet %s updated.", curRS.Name) + d, err := dc.dLister.Deployments(curRS.Namespace).Get(curControllerRef.Name) + if err != nil { + return + } + dc.enqueueDeployment(d) + return + } + + // Otherwise, it's an orphan. If anything changed, sync matching controllers + // to see if anyone wants to adopt it now. + labelChanged := !reflect.DeepEqual(curRS.Labels, oldRS.Labels) + if labelChanged || controllerRefChanged { + ds := dc.getDeploymentsForReplicaSet(curRS) + if len(ds) == 0 { + return + } + glog.V(4).Infof("Orphan ReplicaSet %s updated.", curRS.Name) + for _, d := range ds { + dc.enqueueDeployment(d) } } } @@ -268,10 +325,23 @@ func (dc *DeploymentController) deleteReplicaSet(obj interface{}) { return } } - glog.V(4).Infof("ReplicaSet %s deleted.", rs.Name) - if d := dc.getDeploymentForReplicaSet(rs); d != nil { - dc.enqueueDeployment(d) + + controllerRef := controller.GetControllerOf(rs) + if controllerRef == nil { + // No controller should care about orphans being deleted. + return } + if controllerRef.Kind != controllerKind.Kind { + // It's controlled by a different type of controller. + return + } + glog.V(4).Infof("ReplicaSet %s deleted.", rs.Name) + + d, err := dc.dLister.Deployments(rs.Namespace).Get(controllerRef.Name) + if err != nil { + return + } + dc.enqueueDeployment(d) } // deletePod will enqueue a Recreate Deployment once all of its pods have stopped running. @@ -351,35 +421,34 @@ func (dc *DeploymentController) getDeploymentForPod(pod *v1.Pod) *extensions.Dep // Find the owning replica set var rs *extensions.ReplicaSet var err error - // Look at the owner reference - controllerRef := controller.GetControllerOf(&pod.ObjectMeta) - if controllerRef != nil { + controllerRef := controller.GetControllerOf(pod) + if controllerRef == nil { + // No controller owns this Pod. + return nil + } + if controllerRef.Kind != extensions.SchemeGroupVersion.WithKind("ReplicaSet").Kind { // Not a pod owned by a replica set. - if controllerRef.Kind != extensions.SchemeGroupVersion.WithKind("ReplicaSet").Kind { - return nil - } - rs, err = dc.rsLister.ReplicaSets(pod.Namespace).Get(controllerRef.Name) - if err != nil { - glog.V(4).Infof("Cannot get replicaset %q for pod %q: %v", controllerRef.Name, pod.Name, err) - return nil - } - } else { - // Fallback to listing replica sets. - rss, err := dc.rsLister.GetPodReplicaSets(pod) - if err != nil { - glog.V(4).Infof("Cannot list replica sets for pod %q: %v", pod.Name, err) - return nil - } - // TODO: Handle multiple replica sets gracefully - // For now we return the oldest replica set. - if len(rss) > 1 { - utilruntime.HandleError(fmt.Errorf("more than one ReplicaSet is selecting pod %q with labels: %+v", pod.Name, pod.Labels)) - sort.Sort(controller.ReplicaSetsByCreationTimestamp(rss)) - } - rs = rss[0] + return nil + } + rs, err = dc.rsLister.ReplicaSets(pod.Namespace).Get(controllerRef.Name) + if err != nil { + glog.V(4).Infof("Cannot get replicaset %q for pod %q: %v", controllerRef.Name, pod.Name, err) + return nil } - return dc.getDeploymentForReplicaSet(rs) + // Now find the Deployment that owns that ReplicaSet. + controllerRef = controller.GetControllerOf(rs) + if controllerRef == nil { + return nil + } + if controllerRef.Kind != controllerKind.Kind { + return nil + } + d, err := dc.dLister.Deployments(rs.Namespace).Get(controllerRef.Name) + if err != nil { + return nil + } + return d } // worker runs a worker thread that just dequeues items, processes them, and marks them done. diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index dff3d54b8af..28bf649e4c7 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -17,6 +17,7 @@ limitations under the License. package deployment import ( + "strconv" "testing" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -488,8 +489,6 @@ func TestGetPodMapForReplicaSets(t *testing.T) { d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - // Two ReplicaSets that match labels for both Deployments, - // but have ControllerRefs to make ownership explicit. rs1 := newReplicaSet(d, "rs1", 1) rs2 := newReplicaSet(d, "rs2", 1) @@ -545,6 +544,282 @@ func TestGetPodMapForReplicaSets(t *testing.T) { } } +func TestAddReplicaSet(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // Two ReplicaSets that match labels for both Deployments, + // but have ControllerRefs to make ownership explicit. + rs1 := newReplicaSet(d1, "rs1", 1) + rs2 := newReplicaSet(d2, "rs2", 1) + + f.dLister = append(f.dLister, d1, d2) + f.objects = append(f.objects, d1, d2, rs1, rs2) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + dc.addReplicaSet(rs1) + if got, want := dc.queue.Len(), 1; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } + key, done := dc.queue.Get() + if key == nil || done { + t.Fatalf("failed to enqueue controller for rs %v", rs1.Name) + } + expectedKey, _ := controller.KeyFunc(d1) + if got, want := key.(string), expectedKey; got != want { + t.Errorf("queue.Get() = %v, want %v", got, want) + } + + dc.addReplicaSet(rs2) + if got, want := dc.queue.Len(), 1; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } + key, done = dc.queue.Get() + if key == nil || done { + t.Fatalf("failed to enqueue controller for rs %v", rs2.Name) + } + expectedKey, _ = controller.KeyFunc(d2) + if got, want := key.(string), expectedKey; got != want { + t.Errorf("queue.Get() = %v, want %v", got, want) + } +} + +func TestAddReplicaSetOrphan(t *testing.T) { + f := newFixture(t) + + // 2 will match the RS, 1 won't. + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d3 := newDeployment("d3", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d3.Spec.Selector.MatchLabels = map[string]string{"foo": "notbar"} + + // Make the RS an orphan. Expect matching Deployments to be queued. + rs := newReplicaSet(d1, "rs1", 1) + rs.OwnerReferences = nil + + f.dLister = append(f.dLister, d1, d2, d3) + f.objects = append(f.objects, d1, d2, d3) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + dc.addReplicaSet(rs) + if got, want := dc.queue.Len(), 2; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } +} + +func TestUpdateReplicaSet(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // Two ReplicaSets that match labels for both Deployments, + // but have ControllerRefs to make ownership explicit. + rs1 := newReplicaSet(d1, "rs1", 1) + rs2 := newReplicaSet(d2, "rs2", 1) + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs1, rs2) + f.objects = append(f.objects, d1, d2, rs1, rs2) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + prev := *rs1 + bumpResourceVersion(rs1) + dc.updateReplicaSet(&prev, rs1) + if got, want := dc.queue.Len(), 1; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } + key, done := dc.queue.Get() + if key == nil || done { + t.Fatalf("failed to enqueue controller for rs %v", rs1.Name) + } + expectedKey, _ := controller.KeyFunc(d1) + if got, want := key.(string), expectedKey; got != want { + t.Errorf("queue.Get() = %v, want %v", got, want) + } + + prev = *rs2 + bumpResourceVersion(rs2) + dc.updateReplicaSet(&prev, rs2) + if got, want := dc.queue.Len(), 1; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } + key, done = dc.queue.Get() + if key == nil || done { + t.Fatalf("failed to enqueue controller for rs %v", rs2.Name) + } + expectedKey, _ = controller.KeyFunc(d2) + if got, want := key.(string), expectedKey; got != want { + t.Errorf("queue.Get() = %v, want %v", got, want) + } +} + +func TestUpdateReplicaSetOrphanWithNewLabels(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // RS matches both, but is an orphan. + rs := newReplicaSet(d1, "rs1", 1) + rs.OwnerReferences = nil + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, d1, d2, rs) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + // Change labels and expect all matching controllers to queue. + prev := *rs + prev.Labels = map[string]string{"foo": "notbar"} + bumpResourceVersion(rs) + dc.updateReplicaSet(&prev, rs) + if got, want := dc.queue.Len(), 2; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } +} + +func TestUpdateReplicaSetChangeControllerRef(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + rs := newReplicaSet(d1, "rs1", 1) + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, d1, d2, rs) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + // Change ControllerRef and expect both old and new to queue. + prev := *rs + prev.OwnerReferences = []metav1.OwnerReference{*newControllerRef(d2)} + bumpResourceVersion(rs) + dc.updateReplicaSet(&prev, rs) + if got, want := dc.queue.Len(), 2; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } +} + +func TestUpdateReplicaSetRelease(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + rs := newReplicaSet(d1, "rs1", 1) + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, d1, d2, rs) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + // Remove ControllerRef and expect all matching controller to sync orphan. + prev := *rs + rs.OwnerReferences = nil + bumpResourceVersion(rs) + dc.updateReplicaSet(&prev, rs) + if got, want := dc.queue.Len(), 2; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } +} + +func TestDeleteReplicaSet(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // Two ReplicaSets that match labels for both Deployments, + // but have ControllerRefs to make ownership explicit. + rs1 := newReplicaSet(d1, "rs1", 1) + rs2 := newReplicaSet(d2, "rs2", 1) + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs1, rs2) + f.objects = append(f.objects, d1, d2, rs1, rs2) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + dc.deleteReplicaSet(rs1) + if got, want := dc.queue.Len(), 1; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } + key, done := dc.queue.Get() + if key == nil || done { + t.Fatalf("failed to enqueue controller for rs %v", rs1.Name) + } + expectedKey, _ := controller.KeyFunc(d1) + if got, want := key.(string), expectedKey; got != want { + t.Errorf("queue.Get() = %v, want %v", got, want) + } + + dc.deleteReplicaSet(rs2) + if got, want := dc.queue.Len(), 1; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } + key, done = dc.queue.Get() + if key == nil || done { + t.Fatalf("failed to enqueue controller for rs %v", rs2.Name) + } + expectedKey, _ = controller.KeyFunc(d2) + if got, want := key.(string), expectedKey; got != want { + t.Errorf("queue.Get() = %v, want %v", got, want) + } +} + +func TestDeleteReplicaSetOrphan(t *testing.T) { + f := newFixture(t) + + d1 := newDeployment("d1", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d2 := newDeployment("d2", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + + // Make the RS an orphan. Expect matching Deployments to be queued. + rs := newReplicaSet(d1, "rs1", 1) + rs.OwnerReferences = nil + + f.dLister = append(f.dLister, d1, d2) + f.rsLister = append(f.rsLister, rs) + f.objects = append(f.objects, d1, d2, rs) + + // Create the fixture but don't start it, + // so nothing happens in the background. + dc, _ := f.newController() + + dc.deleteReplicaSet(rs) + if got, want := dc.queue.Len(), 0; got != want { + t.Fatalf("queue.Len() = %v, want %v", got, want) + } +} + +func bumpResourceVersion(obj metav1.Object) { + ver, _ := strconv.ParseInt(obj.GetResourceVersion(), 10, 32) + obj.SetResourceVersion(strconv.FormatInt(ver+1, 10)) +} + // generatePodFromRS creates a pod, with the input ReplicaSet's selector and its template func generatePodFromRS(rs *extensions.ReplicaSet) *v1.Pod { trueVar := true From 57fb5e2bba0b55053973398fd8abb3371379c64f Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Sun, 26 Feb 2017 16:28:40 -0800 Subject: [PATCH 04/12] Deployment: Update Lister documentation for ControllerRef. The Deployment Listers still use selectors, because this is the behavior expected by callers. This clarifies the meaning of the returned list. Some callers may need to switch to using GetControllerOf() instead, but that is a separate, case-by-case issue. --- .../extensions/internalversion/deployment_expansion.go | 5 ++++- .../listers/extensions/v1beta1/deployment_expansion.go | 5 ++++- .../listers/extensions/v1beta1/deployment_expansion.go | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/client/listers/extensions/internalversion/deployment_expansion.go b/pkg/client/listers/extensions/internalversion/deployment_expansion.go index c42010fd1e2..e60e0bc62f6 100644 --- a/pkg/client/listers/extensions/internalversion/deployment_expansion.go +++ b/pkg/client/listers/extensions/internalversion/deployment_expansion.go @@ -34,7 +34,10 @@ type DeploymentListerExpansion interface { // DeploymentNamespaeLister. type DeploymentNamespaceListerExpansion interface{} -// GetDeploymentsForReplicaSet returns a list of deployments managing a replica set. Returns an error only if no matching deployments are found. +// GetDeploymentsForReplicaSet returns a list of Deployments that potentially +// match a ReplicaSet. Only the one specified in the ReplicaSet's ControllerRef +// will actually manage it. +// Returns an error only if no matching Deployments are found. func (s *deploymentLister) GetDeploymentsForReplicaSet(rs *extensions.ReplicaSet) ([]*extensions.Deployment, error) { if len(rs.Labels) == 0 { return nil, fmt.Errorf("no deployments found for ReplicaSet %v because it has no labels", rs.Name) diff --git a/pkg/client/listers/extensions/v1beta1/deployment_expansion.go b/pkg/client/listers/extensions/v1beta1/deployment_expansion.go index e08799ebacc..a26a9b3db14 100644 --- a/pkg/client/listers/extensions/v1beta1/deployment_expansion.go +++ b/pkg/client/listers/extensions/v1beta1/deployment_expansion.go @@ -34,7 +34,10 @@ type DeploymentListerExpansion interface { // DeploymentNamespaeLister. type DeploymentNamespaceListerExpansion interface{} -// GetDeploymentsForReplicaSet returns a list of deployments managing a replica set. Returns an error only if no matching deployments are found. +// GetDeploymentsForReplicaSet returns a list of Deployments that potentially +// match a ReplicaSet. Only the one specified in the ReplicaSet's ControllerRef +// will actually manage it. +// Returns an error only if no matching Deployments are found. func (s *deploymentLister) GetDeploymentsForReplicaSet(rs *extensions.ReplicaSet) ([]*extensions.Deployment, error) { if len(rs.Labels) == 0 { return nil, fmt.Errorf("no deployments found for ReplicaSet %v because it has no labels", rs.Name) diff --git a/staging/src/k8s.io/client-go/listers/extensions/v1beta1/deployment_expansion.go b/staging/src/k8s.io/client-go/listers/extensions/v1beta1/deployment_expansion.go index 276aed7af6c..c3ba99904b2 100644 --- a/staging/src/k8s.io/client-go/listers/extensions/v1beta1/deployment_expansion.go +++ b/staging/src/k8s.io/client-go/listers/extensions/v1beta1/deployment_expansion.go @@ -34,7 +34,10 @@ type DeploymentListerExpansion interface { // DeploymentNamespaeLister. type DeploymentNamespaceListerExpansion interface{} -// GetDeploymentsForReplicaSet returns a list of deployments managing a replica set. Returns an error only if no matching deployments are found. +// GetDeploymentsForReplicaSet returns a list of Deployments that potentially +// match a ReplicaSet. Only the one specified in the ReplicaSet's ControllerRef +// will actually manage it. +// Returns an error only if no matching Deployments are found. func (s *deploymentLister) GetDeploymentsForReplicaSet(rs *extensions.ReplicaSet) ([]*extensions.Deployment, error) { if len(rs.Labels) == 0 { return nil, fmt.Errorf("no deployments found for ReplicaSet %v because it has no labels", rs.Name) From 37534b66df9874cf34990a8d5e2470b1f74f6c80 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Thu, 2 Mar 2017 10:49:38 -0800 Subject: [PATCH 05/12] Deployment: Always set BlockOwnerDeletion in ControllerRef. --- pkg/controller/controller_ref_manager.go | 2 +- pkg/controller/deployment/sync.go | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/controller/controller_ref_manager.go b/pkg/controller/controller_ref_manager.go index d3bc26473a0..17b8c8b14ad 100644 --- a/pkg/controller/controller_ref_manager.go +++ b/pkg/controller/controller_ref_manager.go @@ -296,7 +296,7 @@ func (m *ReplicaSetControllerRefManager) AdoptReplicaSet(replicaSet *extensions. // Note that ValidateOwnerReferences() will reject this patch if another // OwnerReference exists with controller=true. addControllerPatch := fmt.Sprintf( - `{"metadata":{"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true}],"uid":"%s"}}`, + `{"metadata":{"ownerReferences":[{"apiVersion":"%s","kind":"%s","name":"%s","uid":"%s","controller":true,"blockOwnerDeletion":true}],"uid":"%s"}}`, m.controllerKind.GroupVersion(), m.controllerKind.Kind, m.controller.GetName(), m.controller.GetUID(), replicaSet.UID) return m.rsControl.PatchReplicaSet(replicaSet.Namespace, replicaSet.Name, []byte(addControllerPatch)) diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 4571e4a4345..b0083999b0f 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -616,12 +616,14 @@ func (dc *DeploymentController) isScalingEvent(d *extensions.Deployment, rsList // newControllerRef returns a ControllerRef pointing to the deployment. func newControllerRef(d *extensions.Deployment) *metav1.OwnerReference { + blockOwnerDeletion := true isController := true return &metav1.OwnerReference{ - APIVersion: controllerKind.GroupVersion().String(), - Kind: controllerKind.Kind, - Name: d.Name, - UID: d.UID, - Controller: &isController, + APIVersion: controllerKind.GroupVersion().String(), + Kind: controllerKind.Kind, + Name: d.Name, + UID: d.UID, + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &isController, } } From d96c4847b65b9f1859a603c9da3a661a975d9741 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Fri, 3 Mar 2017 16:10:27 -0800 Subject: [PATCH 06/12] Deployment: Filter Pods by Deployment selector in addition to ControllerRef. Deployment should ignore Pods that don't match the selector, even if they have a ControllerRef pointing to one of the ReplicaSets it owns. The ReplicaSet itself will orphan the Pod as soon as it syncs. --- .../deployment/deployment_controller.go | 21 ++++++++++++------- .../deployment/deployment_controller_test.go | 4 ++-- pkg/controller/deployment/sync.go | 9 +++----- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index be51c9aabb1..f1238839bc5 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -371,7 +371,7 @@ func (dc *DeploymentController) deletePod(obj interface{}) { if err != nil { return } - podMap, err := dc.getPodMapForReplicaSets(d.Namespace, rsList) + podMap, err := dc.getPodMapForDeployment(d, rsList) if err != nil { return } @@ -506,11 +506,17 @@ func (dc *DeploymentController) getReplicaSetsForDeployment(d *extensions.Deploy return cm.ClaimReplicaSets(rsList) } -// getPodMapForReplicaSets scans the list of all Pods and returns a map from -// RS UID to Pods controlled by that RS, based on the Pod's ControllerRef. -func (dc *DeploymentController) getPodMapForReplicaSets(namespace string, rsList []*extensions.ReplicaSet) (map[types.UID]*v1.PodList, error) { - // List all Pods. - pods, err := dc.podLister.Pods(namespace).List(labels.Everything()) +// getPodMapForDeployment returns the Pods managed by a Deployment. +// +// It returns a map from ReplicaSet UID to a list of Pods controlled by that RS, +// according to the Pod's ControllerRef. +func (dc *DeploymentController) getPodMapForDeployment(d *extensions.Deployment, rsList []*extensions.ReplicaSet) (map[types.UID]*v1.PodList, error) { + // Get all Pods that potentially belong to this Deployment. + selector, err := metav1.LabelSelectorAsSelector(d.Spec.Selector) + if err != nil { + return nil, err + } + pods, err := dc.podLister.Pods(d.Namespace).List(selector) if err != nil { return nil, err } @@ -583,8 +589,7 @@ func (dc *DeploymentController) syncDeployment(key string) error { return err } // List all Pods owned by this Deployment, grouped by their ReplicaSet. - // This is expensive, so do it once and pass it along to subroutines. - podMap, err := dc.getPodMapForReplicaSets(d.Namespace, rsList) + podMap, err := dc.getPodMapForDeployment(d, rsList) if err != nil { return err } diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index 28bf649e4c7..fd8375d0fcd 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -515,9 +515,9 @@ func TestGetPodMapForReplicaSets(t *testing.T) { defer close(stopCh) informers.Start(stopCh) - podMap, err := c.getPodMapForReplicaSets(d.Namespace, f.rsLister) + podMap, err := c.getPodMapForDeployment(d, f.rsLister) if err != nil { - t.Fatalf("getPodMapForReplicaSets() error: %v", err) + t.Fatalf("getPodMapForDeployment() error: %v", err) } podCount := 0 for _, podList := range podMap { diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index b0083999b0f..4f725d84e4a 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -100,8 +100,7 @@ func (dc *DeploymentController) checkPausedConditions(d *extensions.Deployment) // getAllReplicaSetsAndSyncRevision returns all the replica sets for the provided deployment (new and all old), with new RS's and deployment's revision updated. // // rsList should come from getReplicaSetsForDeployment(d). -// podMap should come from getPodMapForReplicaSets(rsList). -// These are passed around to avoid repeating expensive API calls. +// podMap should come from getPodMapForDeployment(d, rsList). // // 1. Get all old RSes this deployment targets, and calculate the max revision number among them (maxOldV). // 2. Get new RS this deployment targets (whose pod template matches deployment's), and update new RS's revision number to (maxOldV + 1), @@ -134,8 +133,7 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *extensions.D // targets, with pod-template-hash information synced. // // rsList should come from getReplicaSetsForDeployment(d). -// podMap should come from getPodMapForReplicaSets(rsList). -// These are passed around to avoid repeating expensive API calls. +// podMap should come from getPodMapForDeployment(d, rsList). func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) ([]*extensions.ReplicaSet, *v1.PodList, error) { syncedRSList := []*extensions.ReplicaSet{} for _, rs := range rsList { @@ -594,8 +592,7 @@ func calculateStatus(allRSs []*extensions.ReplicaSet, newRS *extensions.ReplicaS // by looking at the desired-replicas annotation in the active replica sets of the deployment. // // rsList should come from getReplicaSetsForDeployment(d). -// podMap should come from getPodMapForReplicaSets(rsList). -// These are passed around to avoid repeating expensive API calls. +// podMap should come from getPodMapForDeployment(d, rsList). func (dc *DeploymentController) isScalingEvent(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) (bool, error) { newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(d, rsList, podMap, false) if err != nil { From f020c9ae6c9a0ed21540a1dcb78d821a1d564b60 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Fri, 3 Mar 2017 17:00:30 -0800 Subject: [PATCH 07/12] Deployment: Update overlapping e2e test for ControllerRef. --- test/e2e/deployment.go | 117 +++---------------------------------- test/e2e/framework/util.go | 13 ----- 2 files changed, 9 insertions(+), 121 deletions(-) diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go index 9c0e9b225cf..ea642b6bc7c 100644 --- a/test/e2e/deployment.go +++ b/test/e2e/deployment.go @@ -19,7 +19,6 @@ package e2e import ( "fmt" "math/rand" - "strings" "time" . "github.com/onsi/ginkgo" @@ -136,15 +135,7 @@ func checkDeploymentRevision(c clientset.Interface, ns, deploymentName, revision return deployment, newRS } -func stopDeploymentOverlap(c clientset.Interface, internalClient internalclientset.Interface, ns, deploymentName, overlapWith string) { - stopDeploymentMaybeOverlap(c, internalClient, ns, deploymentName, overlapWith) -} - func stopDeployment(c clientset.Interface, internalClient internalclientset.Interface, ns, deploymentName string) { - stopDeploymentMaybeOverlap(c, internalClient, ns, deploymentName, "") -} - -func stopDeploymentMaybeOverlap(c clientset.Interface, internalClient internalclientset.Interface, ns, deploymentName, overlapWith string) { deployment, err := c.Extensions().Deployments(ns).Get(deploymentName, metav1.GetOptions{}) Expect(err).NotTo(HaveOccurred()) @@ -166,18 +157,7 @@ func stopDeploymentMaybeOverlap(c clientset.Interface, internalClient internalcl options := metav1.ListOptions{LabelSelector: selector.String()} rss, err := c.Extensions().ReplicaSets(ns).List(options) Expect(err).NotTo(HaveOccurred()) - // RSes may be created by overlapping deployments right after this deployment is deleted, ignore them - if len(overlapWith) == 0 { - Expect(rss.Items).Should(HaveLen(0)) - } else { - noOverlapRSes := []extensions.ReplicaSet{} - for _, rs := range rss.Items { - if !strings.HasPrefix(rs.Name, overlapWith) { - noOverlapRSes = append(noOverlapRSes, rs) - } - } - Expect(noOverlapRSes).Should(HaveLen(0)) - } + Expect(rss.Items).Should(HaveLen(0)) framework.Logf("Ensuring deployment %s's Pods were deleted", deploymentName) var pods *v1.PodList if err := wait.PollImmediate(time.Second, timeout, func() (bool, error) { @@ -186,18 +166,8 @@ func stopDeploymentMaybeOverlap(c clientset.Interface, internalClient internalcl return false, err } // Pods may be created by overlapping deployments right after this deployment is deleted, ignore them - if len(overlapWith) == 0 && len(pods.Items) == 0 { + if len(pods.Items) == 0 { return true, nil - } else if len(overlapWith) != 0 { - noOverlapPods := []v1.Pod{} - for _, pod := range pods.Items { - if !strings.HasPrefix(pod.Name, overlapWith) { - noOverlapPods = append(noOverlapPods, pod) - } - } - if len(noOverlapPods) == 0 { - return true, nil - } } return false, nil }); err != nil { @@ -1105,8 +1075,8 @@ func testScaledRolloutDeployment(f *framework.Framework) { func testOverlappingDeployment(f *framework.Framework) { ns := f.Namespace.Name c := f.ClientSet - internalClient := f.InternalClientset + // Create first deployment. deploymentName := "first-deployment" podLabels := map[string]string{"name": redisImageName} replicas := int32(1) @@ -1119,7 +1089,7 @@ func testOverlappingDeployment(f *framework.Framework) { err = framework.WaitForDeploymentRevisionAndImage(c, ns, deploy.Name, "1", redisImage) Expect(err).NotTo(HaveOccurred(), "The first deployment failed to update to revision 1") - Expect(err).NotTo(HaveOccurred()) + // Create second deployment with overlapping selector. deploymentName = "second-deployment" By(fmt.Sprintf("Creating deployment %q with overlapping selector", deploymentName)) podLabels["other-label"] = "random-label" @@ -1127,85 +1097,16 @@ func testOverlappingDeployment(f *framework.Framework) { deployOverlapping, err := c.Extensions().Deployments(ns).Create(d) Expect(err).NotTo(HaveOccurred(), "Failed creating the second deployment") - // Wait for overlapping annotation updated to both deployments - 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") - options := metav1.ListOptions{} - rsList, err := c.Extensions().ReplicaSets(ns).List(options) - Expect(err).NotTo(HaveOccurred(), "Failed listing all replica sets in namespace %s", ns) - Expect(rsList.Items).To(HaveLen(int(replicas))) - Expect(rsList.Items[0].Spec.Template.Spec.Containers).To(HaveLen(1)) - Expect(rsList.Items[0].Spec.Template.Spec.Containers[0].Image).To(Equal(deploy.Spec.Template.Spec.Containers[0].Image)) - - By("Deleting the first deployment") - stopDeploymentOverlap(c, internalClient, ns, deploy.Name, deployOverlapping.Name) - - // Wait for overlapping annotation cleared - By("Waiting for the second deployment to clear overlapping annotation") - err = framework.WaitForOverlappingAnnotationMatch(c, ns, deployOverlapping.Name, "") - Expect(err).NotTo(HaveOccurred(), "Failed to clear the second deployment's overlapping annotation") - // Wait for it to be updated to revision 1 err = framework.WaitForDeploymentRevisionAndImage(c, ns, deployOverlapping.Name, "1", nginxImage) Expect(err).NotTo(HaveOccurred(), "The second deployment failed to update to revision 1") - // Now the second deployment is synced - By("Checking the second overlapping deployment is synced") - rsList, err = c.Extensions().ReplicaSets(ns).List(options) + // Both deployments should proceed independently. + By("Checking each deployment creates its own replica set") + options := metav1.ListOptions{} + rsList, err := c.Extensions().ReplicaSets(ns).List(options) Expect(err).NotTo(HaveOccurred(), "Failed listing all replica sets in namespace %s", ns) - Expect(rsList.Items).To(HaveLen(int(replicas))) - Expect(rsList.Items[0].Spec.Template.Spec.Containers).To(HaveLen(1)) - Expect(rsList.Items[0].Spec.Template.Spec.Containers[0].Image).To(Equal(deployOverlapping.Spec.Template.Spec.Containers[0].Image)) - - deploymentName = "third-deployment" - podLabels = map[string]string{"name": nginxImageName} - By(fmt.Sprintf("Creating deployment %q", deploymentName)) - d = framework.NewDeployment(deploymentName, replicas, podLabels, nginxImageName, nginxImage, extensions.RollingUpdateDeploymentStrategyType) - 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, 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 = 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 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 - By("Checking the second deployment is not synced") - Expect(deployOverlapping.Annotations[deploymentutil.RevisionAnnotation]).To(Equal("1")) - - // Update the second deployment's selector to make it not overlap with the third deployment - By(fmt.Sprintf("Updating deployment %q selector to make it not overlap with existing one", deployOverlapping.Name)) - deployOverlapping, err = framework.UpdateDeploymentWithRetries(c, ns, deployOverlapping.Name, func(update *extensions.Deployment) { - update.Spec.Selector = deploy.Spec.Selector - update.Spec.Template.Labels = deploy.Spec.Template.Labels - }) - Expect(err).NotTo(HaveOccurred()) - - // Wait for the second deployment to be synced - By("Checking the second deployment is now synced") - err = framework.WaitForDeploymentRevisionAndImage(c, ns, deployOverlapping.Name, "2", redisImage) - Expect(err).NotTo(HaveOccurred(), "The second deployment failed to update to revision 2") + Expect(rsList.Items).To(HaveLen(2)) } func testFailedDeployment(f *framework.Framework) { diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index bcde917991d..be66bf8270a 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -3397,19 +3397,6 @@ func WaitForDeploymentRevisionAndImage(c clientset.Interface, ns, deploymentName return nil } -func WaitForOverlappingAnnotationMatch(c clientset.Interface, ns, deploymentName, expected string) error { - return wait.Poll(Poll, 1*time.Minute, func() (bool, error) { - deployment, err := c.Extensions().Deployments(ns).Get(deploymentName, metav1.GetOptions{}) - if err != nil { - return false, err - } - if deployment.Annotations[deploymentutil.OverlapAnnotation] == expected { - return true, nil - } - return false, nil - }) -} - // CheckNewRSAnnotations check if the new RS's annotation is as expected func CheckNewRSAnnotations(c clientset.Interface, ns, deploymentName string, expectedAnnotations map[string]string) error { deployment, err := c.Extensions().Deployments(ns).Get(deploymentName, metav1.GetOptions{}) From 111b9ce9b5a13203ddfe7bd14f1fd4bb7444cc30 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Fri, 3 Mar 2017 17:34:58 -0800 Subject: [PATCH 08/12] Deployment: Fix data race in unit tests. --- .../deployment/deployment_controller_test.go | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index fd8375d0fcd..4f2b3ce9431 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -635,8 +635,9 @@ func TestUpdateReplicaSet(t *testing.T) { dc, _ := f.newController() prev := *rs1 - bumpResourceVersion(rs1) - dc.updateReplicaSet(&prev, rs1) + next := *rs1 + bumpResourceVersion(&next) + dc.updateReplicaSet(&prev, &next) if got, want := dc.queue.Len(), 1; got != want { t.Fatalf("queue.Len() = %v, want %v", got, want) } @@ -650,8 +651,9 @@ func TestUpdateReplicaSet(t *testing.T) { } prev = *rs2 - bumpResourceVersion(rs2) - dc.updateReplicaSet(&prev, rs2) + next = *rs2 + bumpResourceVersion(&next) + dc.updateReplicaSet(&prev, &next) if got, want := dc.queue.Len(), 1; got != want { t.Fatalf("queue.Len() = %v, want %v", got, want) } @@ -686,8 +688,9 @@ func TestUpdateReplicaSetOrphanWithNewLabels(t *testing.T) { // Change labels and expect all matching controllers to queue. prev := *rs prev.Labels = map[string]string{"foo": "notbar"} - bumpResourceVersion(rs) - dc.updateReplicaSet(&prev, rs) + next := *rs + bumpResourceVersion(&next) + dc.updateReplicaSet(&prev, &next) if got, want := dc.queue.Len(), 2; got != want { t.Fatalf("queue.Len() = %v, want %v", got, want) } @@ -712,8 +715,9 @@ func TestUpdateReplicaSetChangeControllerRef(t *testing.T) { // Change ControllerRef and expect both old and new to queue. prev := *rs prev.OwnerReferences = []metav1.OwnerReference{*newControllerRef(d2)} - bumpResourceVersion(rs) - dc.updateReplicaSet(&prev, rs) + next := *rs + bumpResourceVersion(&next) + dc.updateReplicaSet(&prev, &next) if got, want := dc.queue.Len(), 2; got != want { t.Fatalf("queue.Len() = %v, want %v", got, want) } @@ -737,9 +741,10 @@ func TestUpdateReplicaSetRelease(t *testing.T) { // Remove ControllerRef and expect all matching controller to sync orphan. prev := *rs - rs.OwnerReferences = nil - bumpResourceVersion(rs) - dc.updateReplicaSet(&prev, rs) + next := *rs + next.OwnerReferences = nil + bumpResourceVersion(&next) + dc.updateReplicaSet(&prev, &next) if got, want := dc.queue.Len(), 2; got != want { t.Fatalf("queue.Len() = %v, want %v", got, want) } From f2a2895a78df4650db14ce8bcaeaa4bac43695bc Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Mon, 6 Mar 2017 11:01:12 -0800 Subject: [PATCH 09/12] Deployment: Check that ControllerRef UID matches. --- .../deployment/deployment_controller.go | 49 +++++++++---------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index f1238839bc5..304164ad94b 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -203,15 +203,11 @@ func (dc *DeploymentController) addReplicaSet(obj interface{}) { // If it has a ControllerRef, that's all that matters. if controllerRef := controller.GetControllerOf(rs); controllerRef != nil { - if controllerRef.Kind != controllerKind.Kind { - // It's controller by a different type of controller. + d := dc.resolveControllerRef(rs.Namespace, controllerRef) + if d == nil { return } glog.V(4).Infof("ReplicaSet %s added.", rs.Name) - d, err := dc.dLister.Deployments(rs.Namespace).Get(controllerRef.Name) - if err != nil { - return - } dc.enqueueDeployment(d) return } @@ -264,26 +260,20 @@ func (dc *DeploymentController) updateReplicaSet(old, cur interface{}) { curControllerRef := controller.GetControllerOf(curRS) oldControllerRef := controller.GetControllerOf(oldRS) controllerRefChanged := !reflect.DeepEqual(curControllerRef, oldControllerRef) - if controllerRefChanged && - oldControllerRef != nil && oldControllerRef.Kind == controllerKind.Kind { + if controllerRefChanged && oldControllerRef != nil { // The ControllerRef was changed. Sync the old controller, if any. - d, err := dc.dLister.Deployments(oldRS.Namespace).Get(oldControllerRef.Name) - if err == nil { + if d := dc.resolveControllerRef(oldRS.Namespace, oldControllerRef); d != nil { dc.enqueueDeployment(d) } } // If it has a ControllerRef, that's all that matters. if curControllerRef != nil { - if curControllerRef.Kind != controllerKind.Kind { - // It's controlled by a different type of controller. + d := dc.resolveControllerRef(curRS.Namespace, curControllerRef) + if d == nil { return } glog.V(4).Infof("ReplicaSet %s updated.", curRS.Name) - d, err := dc.dLister.Deployments(curRS.Namespace).Get(curControllerRef.Name) - if err != nil { - return - } dc.enqueueDeployment(d) return } @@ -331,16 +321,11 @@ func (dc *DeploymentController) deleteReplicaSet(obj interface{}) { // No controller should care about orphans being deleted. return } - if controllerRef.Kind != controllerKind.Kind { - // It's controlled by a different type of controller. + d := dc.resolveControllerRef(rs.Namespace, controllerRef) + if d == nil { return } glog.V(4).Infof("ReplicaSet %s deleted.", rs.Name) - - d, err := dc.dLister.Deployments(rs.Namespace).Get(controllerRef.Name) - if err != nil { - return - } dc.enqueueDeployment(d) } @@ -431,7 +416,7 @@ func (dc *DeploymentController) getDeploymentForPod(pod *v1.Pod) *extensions.Dep return nil } rs, err = dc.rsLister.ReplicaSets(pod.Namespace).Get(controllerRef.Name) - if err != nil { + if err != nil || rs.UID != controllerRef.UID { glog.V(4).Infof("Cannot get replicaset %q for pod %q: %v", controllerRef.Name, pod.Name, err) return nil } @@ -441,13 +426,27 @@ func (dc *DeploymentController) getDeploymentForPod(pod *v1.Pod) *extensions.Dep if controllerRef == nil { return nil } + return dc.resolveControllerRef(rs.Namespace, controllerRef) +} + +// resolveControllerRef returns the controller referenced by a ControllerRef, +// or nil if the ControllerRef could not be resolved to a matching controller +// of the corrrect Kind. +func (dc *DeploymentController) resolveControllerRef(namespace string, controllerRef *metav1.OwnerReference) *extensions.Deployment { + // We can't look up by UID, so look up by Name and then verify UID. + // Don't even try to look up by Name if it's the wrong Kind. if controllerRef.Kind != controllerKind.Kind { return nil } - d, err := dc.dLister.Deployments(rs.Namespace).Get(controllerRef.Name) + d, err := dc.dLister.Deployments(namespace).Get(controllerRef.Name) if err != nil { return nil } + if d.UID != controllerRef.UID { + // The controller we found with this Name is not the same one that the + // ControllerRef points to. + return nil + } return d } From 94b3c216a17ec40ddcfff8c695cc2a3a8e6ac5d6 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Mon, 6 Mar 2017 11:32:42 -0800 Subject: [PATCH 10/12] Deployment: Consolidate Adopt/Release unit tests. --- .../deployment/deployment_controller_test.go | 47 ++++--------------- 1 file changed, 9 insertions(+), 38 deletions(-) diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index 4f2b3ce9431..7e2638b8563 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -420,50 +420,21 @@ func TestGetReplicaSetsForDeployment(t *testing.T) { } } -func TestGetReplicaSetsForDeploymentAdopt(t *testing.T) { +func TestGetReplicaSetsForDeploymentAdoptRelease(t *testing.T) { f := newFixture(t) d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) // RS with matching labels, but orphaned. Should be adopted and returned. - rs := newReplicaSet(d, "rs", 1) - rs.OwnerReferences = nil - - f.dLister = append(f.dLister, d) - f.rsLister = append(f.rsLister, rs) - f.objects = append(f.objects, d, rs) - - // Start the fixture. - c, informers := f.newController() - stopCh := make(chan struct{}) - defer close(stopCh) - informers.Start(stopCh) - - rsList, err := c.getReplicaSetsForDeployment(d) - if err != nil { - t.Fatalf("getReplicaSetsForDeployment() error: %v", err) - } - rsNames := []string{} - for _, rs := range rsList { - rsNames = append(rsNames, rs.Name) - } - if len(rsNames) != 1 || rsNames[0] != rs.Name { - t.Errorf("getReplicaSetsForDeployment() = %v, want [%v]", rsNames, rs.Name) - } -} - -func TestGetReplicaSetsForDeploymentRelease(t *testing.T) { - f := newFixture(t) - - d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) - + rsAdopt := newReplicaSet(d, "rsAdopt", 1) + rsAdopt.OwnerReferences = nil // RS with matching ControllerRef, but wrong labels. Should be released. - rs := newReplicaSet(d, "rs", 1) - rs.Labels = map[string]string{"foo": "notbar"} + rsRelease := newReplicaSet(d, "rsRelease", 1) + rsRelease.Labels = map[string]string{"foo": "notbar"} f.dLister = append(f.dLister, d) - f.rsLister = append(f.rsLister, rs) - f.objects = append(f.objects, d, rs) + f.rsLister = append(f.rsLister, rsAdopt, rsRelease) + f.objects = append(f.objects, d, rsAdopt, rsRelease) // Start the fixture. c, informers := f.newController() @@ -479,8 +450,8 @@ func TestGetReplicaSetsForDeploymentRelease(t *testing.T) { for _, rs := range rsList { rsNames = append(rsNames, rs.Name) } - if len(rsNames) != 0 { - t.Errorf("getReplicaSetsForDeployment() = %v, want []", rsNames) + if len(rsNames) != 1 || rsNames[0] != rsAdopt.Name { + t.Errorf("getReplicaSetsForDeployment() = %v, want [%v]", rsNames, rsAdopt.Name) } } From cec3899b96044af5804ad83694a1565df82a798f Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Mon, 6 Mar 2017 11:54:43 -0800 Subject: [PATCH 11/12] Deployment: Remove Overlap and SelectorUpdate annotations. These are not used anymore since ControllerRef now protects against fighting between controllers with overlapping selectors. --- hack/make-rules/test-cmd-util.sh | 4 - .../deployment/util/deployment_util.go | 66 ----------- .../deployment/util/deployment_util_test.go | 108 ------------------ pkg/kubectl/stop.go | 5 - pkg/printers/internalversion/describe.go | 4 - pkg/registry/extensions/deployment/BUILD | 1 - .../extensions/deployment/strategy.go | 12 -- 7 files changed, 200 deletions(-) diff --git a/hack/make-rules/test-cmd-util.sh b/hack/make-rules/test-cmd-util.sh index 882c2976a0c..59b86209d65 100644 --- a/hack/make-rules/test-cmd-util.sh +++ b/hack/make-rules/test-cmd-util.sh @@ -2382,10 +2382,6 @@ run_deployment_tests() { # 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 diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index b0fcfcb0d37..296dba29acf 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -67,14 +67,6 @@ const ( RollbackTemplateUnchanged = "DeploymentRollbackTemplateUnchanged" // RollbackDone is the done rollback event reason RollbackDone = "DeploymentRollback" - // OverlapAnnotation marks deployments with overlapping selector with other deployments - // TODO: Delete this annotation when we gracefully handle overlapping selectors. - // See https://github.com/kubernetes/kubernetes/issues/2210 - OverlapAnnotation = "deployment.kubernetes.io/error-selector-overlapping-with" - // SelectorUpdateAnnotation marks the last time deployment selector update - // TODO: Delete this annotation when we gracefully handle overlapping selectors. - // See https://github.com/kubernetes/kubernetes/issues/2210 - SelectorUpdateAnnotation = "deployment.kubernetes.io/selector-updated-at" // Reasons for deployment conditions // @@ -295,8 +287,6 @@ var annotationsToSkip = map[string]bool{ RevisionHistoryAnnotation: true, DesiredReplicasAnnotation: true, MaxReplicasAnnotation: true, - OverlapAnnotation: true, - SelectorUpdateAnnotation: true, } // skipCopyAnnotation returns true if we should skip copying the annotation with the given annotation key @@ -1020,59 +1010,3 @@ func DeploymentDeepCopy(deployment *extensions.Deployment) (*extensions.Deployme } return copied, nil } - -// SelectorUpdatedBefore returns true if the former deployment's selector -// is updated before the latter, false otherwise. -func SelectorUpdatedBefore(d1, d2 *extensions.Deployment) bool { - t1, t2 := LastSelectorUpdate(d1), LastSelectorUpdate(d2) - return t1.Before(t2) -} - -// LastSelectorUpdate returns the last time given deployment's selector is updated -func LastSelectorUpdate(d *extensions.Deployment) metav1.Time { - t := d.Annotations[SelectorUpdateAnnotation] - if len(t) > 0 { - parsedTime, err := time.Parse(time.RFC3339, t) - // If failed to parse the time, use creation timestamp instead - if err != nil { - return d.CreationTimestamp - } - return metav1.Time{Time: parsedTime} - } - // If it's never updated, use creation timestamp instead - return d.CreationTimestamp -} - -// BySelectorLastUpdateTime sorts a list of deployments by the last update time of their selector, -// first using their creation timestamp and then their names as a tie breaker. -type BySelectorLastUpdateTime []*extensions.Deployment - -func (o BySelectorLastUpdateTime) Len() int { return len(o) } -func (o BySelectorLastUpdateTime) Swap(i, j int) { o[i], o[j] = o[j], o[i] } -func (o BySelectorLastUpdateTime) Less(i, j int) bool { - ti, tj := LastSelectorUpdate(o[i]), LastSelectorUpdate(o[j]) - if ti.Equal(tj) { - if o[i].CreationTimestamp.Equal(o[j].CreationTimestamp) { - return o[i].Name < o[j].Name - } - return o[i].CreationTimestamp.Before(o[j].CreationTimestamp) - } - 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 := metav1.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 := metav1.LabelSelectorAsSelector(other.Spec.Selector) - if err != 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 -} diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index add606ce043..18abfeb04d3 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -1189,111 +1189,3 @@ 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) - } - } -} diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 5a70a3958e5..c46d1039f50 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -441,11 +441,6 @@ 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 := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) if err != nil { diff --git a/pkg/printers/internalversion/describe.go b/pkg/printers/internalversion/describe.go index ad19bc4d55f..0a6d6606c3f 100644 --- a/pkg/printers/internalversion/describe.go +++ b/pkg/printers/internalversion/describe.go @@ -2439,10 +2439,6 @@ func (dd *DeploymentDescriber) Describe(namespace, name string, describerSetting } w.Write(LEVEL_0, "NewReplicaSet:\t%s\n", printReplicaSetsByLabels(newRSs)) } - overlapWith := d.Annotations[deploymentutil.OverlapAnnotation] - if len(overlapWith) > 0 { - w.Write(LEVEL_0, "!!!WARNING!!! This deployment has overlapping label selector with deployment %q and won't behave as expected. Please fix it before continue.\n", overlapWith) - } if describerSettings.ShowEvents { events, err := dd.Core().Events(namespace).Search(api.Scheme, d) if err == nil && events != nil { diff --git a/pkg/registry/extensions/deployment/BUILD b/pkg/registry/extensions/deployment/BUILD index 752a9b1e0c7..a7c8a9a66be 100644 --- a/pkg/registry/extensions/deployment/BUILD +++ b/pkg/registry/extensions/deployment/BUILD @@ -20,7 +20,6 @@ go_library( "//pkg/api:go_default_library", "//pkg/apis/extensions:go_default_library", "//pkg/apis/extensions/validation:go_default_library", - "//pkg/controller/deployment/util:go_default_library", "//vendor:k8s.io/apimachinery/pkg/apis/meta/internalversion", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", "//vendor:k8s.io/apimachinery/pkg/fields", diff --git a/pkg/registry/extensions/deployment/strategy.go b/pkg/registry/extensions/deployment/strategy.go index 831082b24ec..059e1b20998 100644 --- a/pkg/registry/extensions/deployment/strategy.go +++ b/pkg/registry/extensions/deployment/strategy.go @@ -19,9 +19,7 @@ package deployment import ( "fmt" "reflect" - "time" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -34,7 +32,6 @@ import ( "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/apis/extensions" "k8s.io/kubernetes/pkg/apis/extensions/validation" - "k8s.io/kubernetes/pkg/controller/deployment/util" ) // deploymentStrategy implements behavior for Deployments. @@ -93,15 +90,6 @@ func (deploymentStrategy) PrepareForUpdate(ctx genericapirequest.Context, obj, o !reflect.DeepEqual(newDeployment.Annotations, oldDeployment.Annotations) { newDeployment.Generation = oldDeployment.Generation + 1 } - - // Records timestamp on selector updates in annotation - if !reflect.DeepEqual(newDeployment.Spec.Selector, oldDeployment.Spec.Selector) { - if newDeployment.Annotations == nil { - newDeployment.Annotations = make(map[string]string) - } - now := metav1.Now() - newDeployment.Annotations[util.SelectorUpdateAnnotation] = now.Format(time.RFC3339) - } } // ValidateUpdate is the default update validation for an end user. From 8c4bcb38fbfd0bfe6d8694d07dc144ce795c2985 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Mon, 6 Mar 2017 14:51:18 -0800 Subject: [PATCH 12/12] Deployment: Filter by ControllerRef in Reaper. We don't want to delete ReplicaSets we don't own. --- .../deployment/util/deployment_util.go | 29 +++++++++++++++++++ pkg/kubectl/BUILD | 1 + pkg/kubectl/stop.go | 25 +++++++++------- pkg/kubectl/stop_test.go | 12 ++++++++ 4 files changed, 57 insertions(+), 10 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 296dba29acf..2320de1f9ea 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -593,6 +593,35 @@ func ListReplicaSets(deployment *extensions.Deployment, getRSList rsListFunc) ([ return owned, nil } +// ListReplicaSets returns a slice of RSes the given deployment targets. +// Note that this does NOT attempt to reconcile ControllerRef (adopt/orphan), +// because only the controller itself should do that. +// However, it does filter out anything whose ControllerRef doesn't match. +// TODO: Remove the duplicate. +func ListReplicaSetsInternal(deployment *internalextensions.Deployment, getRSList func(string, metav1.ListOptions) ([]*internalextensions.ReplicaSet, error)) ([]*internalextensions.ReplicaSet, error) { + // TODO: Right now we list replica sets by their labels. We should list them by selector, i.e. the replica set's selector + // should be a superset of the deployment's selector, see https://github.com/kubernetes/kubernetes/issues/19830. + namespace := deployment.Namespace + selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) + if err != nil { + return nil, err + } + options := metav1.ListOptions{LabelSelector: selector.String()} + all, err := getRSList(namespace, options) + if err != nil { + return all, err + } + // Only include those whose ControllerRef matches the Deployment. + owned := make([]*internalextensions.ReplicaSet, 0, len(all)) + for _, rs := range all { + controllerRef := controller.GetControllerOf(rs) + if controllerRef != nil && controllerRef.UID == deployment.UID { + owned = append(owned, rs) + } + } + return owned, nil +} + // ListPods returns a list of pods the given deployment targets. // This needs a list of ReplicaSets for the Deployment, // which can be found with ListReplicaSets(). diff --git a/pkg/kubectl/BUILD b/pkg/kubectl/BUILD index 386dd98aada..98923897872 100644 --- a/pkg/kubectl/BUILD +++ b/pkg/kubectl/BUILD @@ -152,6 +152,7 @@ go_test( "//vendor:k8s.io/apimachinery/pkg/runtime/schema", "//vendor:k8s.io/apimachinery/pkg/util/intstr", "//vendor:k8s.io/apimachinery/pkg/util/sets", + "//vendor:k8s.io/apimachinery/pkg/util/uuid", "//vendor:k8s.io/apimachinery/pkg/watch", "//vendor:k8s.io/client-go/rest", "//vendor:k8s.io/client-go/rest/fake", diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index c46d1039f50..70cea6deba0 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -420,7 +420,6 @@ func (reaper *JobReaper) Stop(namespace, name string, timeout time.Duration, gra func (reaper *DeploymentReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *metav1.DeleteOptions) error { deployments := reaper.dClient.Deployments(namespace) - replicaSets := reaper.rsClient.ReplicaSets(namespace) rsReaper := &ReplicaSetReaper{reaper.rsClient, reaper.pollInterval, reaper.timeout} deployment, err := reaper.updateDeploymentWithRetries(namespace, name, func(d *extensions.Deployment) { @@ -441,20 +440,26 @@ func (reaper *DeploymentReaper) Stop(namespace, name string, timeout time.Durati return err } - // Stop all replica sets. - selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) + // Stop all replica sets belonging to this Deployment. + rss, err := deploymentutil.ListReplicaSetsInternal(deployment, + func(namespace string, options metav1.ListOptions) ([]*extensions.ReplicaSet, error) { + rsList, err := reaper.rsClient.ReplicaSets(namespace).List(options) + if err != nil { + return nil, err + } + rss := make([]*extensions.ReplicaSet, 0, len(rsList.Items)) + for i := range rsList.Items { + rss = append(rss, &rsList.Items[i]) + } + return rss, nil + }) if err != nil { return err } - options := metav1.ListOptions{LabelSelector: selector.String()} - rsList, err := replicaSets.List(options) - if err != nil { - return err - } errList := []error{} - for _, rc := range rsList.Items { - if err := rsReaper.Stop(rc.Namespace, rc.Name, timeout, gracePeriod); err != nil { + for _, rs := range rss { + if err := rsReaper.Stop(rs.Namespace, rs.Name, timeout, gracePeriod); err != nil { scaleGetErr, ok := err.(ScaleError) if errors.IsNotFound(err) || (ok && errors.IsNotFound(scaleGetErr.ActualError)) { continue diff --git a/pkg/kubectl/stop_test.go b/pkg/kubectl/stop_test.go index 3f45ad8b507..95a8b243472 100644 --- a/pkg/kubectl/stop_test.go +++ b/pkg/kubectl/stop_test.go @@ -27,6 +27,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/watch" testcore "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/api" @@ -429,6 +430,7 @@ func TestDeploymentStop(t *testing.T) { deployment := extensions.Deployment{ ObjectMeta: metav1.ObjectMeta{ Name: name, + UID: uuid.NewUUID(), Namespace: ns, }, Spec: extensions.DeploymentSpec{ @@ -440,6 +442,7 @@ func TestDeploymentStop(t *testing.T) { }, } template := deploymentutil.GetNewReplicaSetTemplateInternal(&deployment) + trueVar := true tests := []struct { Name string Objs []runtime.Object @@ -478,6 +481,15 @@ func TestDeploymentStop(t *testing.T) { Name: name, Namespace: ns, Labels: map[string]string{"k1": "v1"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: extensions.SchemeGroupVersion.String(), + Kind: "ReplicaSet", + Name: deployment.Name, + UID: deployment.UID, + Controller: &trueVar, + }, + }, }, Spec: extensions.ReplicaSetSpec{ Template: template,