From 725ec0cc5ec5f783eec349ad3d70bcc0b678f6c0 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Thu, 16 Mar 2017 10:08:22 -0700 Subject: [PATCH 1/3] kubectl: Check for Deployment overlap annotation in reaper. This effectively reverts the client-side changes in cec3899b96044af5804ad83694a1565df82a798f. We have to maintain the old behavior on the client side to support version skew when talking to old servers that set the annotation. However, the new server-side behavior is still to NOT set the annotation. --- pkg/controller/deployment/util/deployment_util.go | 5 +++++ pkg/kubectl/stop.go | 7 +++++++ pkg/printers/internalversion/describe.go | 4 ++++ 3 files changed, 16 insertions(+) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 01b5f33ebf8..ff10001c683 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -67,6 +67,10 @@ 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 no longer need to support a client + // talking to a server older than v1.6. + OverlapAnnotation = "deployment.kubernetes.io/error-selector-overlapping-with" // Reasons for deployment conditions // @@ -287,6 +291,7 @@ var annotationsToSkip = map[string]bool{ RevisionHistoryAnnotation: true, DesiredReplicasAnnotation: true, MaxReplicasAnnotation: true, + OverlapAnnotation: true, } // skipCopyAnnotation returns true if we should skip copying the annotation with the given annotation key diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 70cea6deba0..9ccd9e40792 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -440,6 +440,13 @@ func (reaper *DeploymentReaper) Stop(namespace, name string, timeout time.Durati return err } + // Do not cascade deletion for overlapping deployments. + // A Deployment with this annotation will not create or manage anything, + // so we can assume any matching ReplicaSets belong to another Deployment. + if len(deployment.Annotations[deploymentutil.OverlapAnnotation]) > 0 { + return deployments.Delete(name, nil) + } + // Stop all replica sets belonging to this Deployment. rss, err := deploymentutil.ListReplicaSetsInternal(deployment, func(namespace string, options metav1.ListOptions) ([]*extensions.ReplicaSet, error) { diff --git a/pkg/printers/internalversion/describe.go b/pkg/printers/internalversion/describe.go index 19f50a65c3d..f8cfabf4376 100644 --- a/pkg/printers/internalversion/describe.go +++ b/pkg/printers/internalversion/describe.go @@ -2441,6 +2441,10 @@ 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 continuing.\n", overlapWith) + } if describerSettings.ShowEvents { events, err := dd.Core().Events(namespace).Search(api.Scheme, d) if err == nil && events != nil { From fa23729a6d668d812fab228e566cb0ac23f2f2de Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Thu, 16 Mar 2017 11:43:09 -0700 Subject: [PATCH 2/3] kubectl: Use v1.5-compatible ownership logic when listing dependents. In particular, we should not assume ControllerRefs are necessarily set. However, we can still use ControllerRefs that do exist to avoid interfering with controllers that do use it. --- .../deployment/util/deployment_util.go | 109 ++++++++++++------ pkg/kubectl/BUILD | 1 + pkg/kubectl/history.go | 2 +- pkg/kubectl/rollback.go | 2 +- pkg/kubectl/stop.go | 14 ++- pkg/kubectl/stop_test.go | 69 ++++++++++- test/e2e/framework/util.go | 2 +- 7 files changed, 159 insertions(+), 40 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index ff10001c683..ee7b3995c13 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -518,14 +518,15 @@ func GetAllReplicaSets(deployment *extensions.Deployment, c clientset.Interface) return oldRSes, allOldRSes, newRS, nil } -// GetAllReplicaSetsV15 is a compatibility function that behaves the way -// GetAllReplicaSets() used to in v1.5.x. +// GetAllReplicaSetsV15 is a compatibility function that emulates the behavior +// from v1.5.x (list matching objects by selector) except that it leaves out +// objects that are explicitly marked as being controlled by something else. func GetAllReplicaSetsV15(deployment *extensions.Deployment, c clientset.Interface) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, *extensions.ReplicaSet, error) { rsList, err := ListReplicaSetsV15(deployment, rsListFromClient(c)) if err != nil { return nil, nil, nil, err } - podList, err := ListPodsV15(deployment, podListFromClient(c)) + podList, err := ListPodsV15(deployment, rsList, podListFromClient(c)) if err != nil { return nil, nil, nil, err } @@ -564,8 +565,9 @@ func GetNewReplicaSet(deployment *extensions.Deployment, c clientset.Interface) return FindNewReplicaSet(deployment, rsList) } -// GetNewReplicaSetV15 is a compatibility function that behaves the way -// GetNewReplicaSet() used to in v1.5.x. +// GetNewReplicaSetV15 is a compatibility function that emulates the behavior +// from v1.5.x (list matching objects by selector) except that it leaves out +// objects that are explicitly marked as being controlled by something else. func GetNewReplicaSetV15(deployment *extensions.Deployment, c clientset.Interface) (*extensions.ReplicaSet, error) { rsList, err := ListReplicaSetsV15(deployment, rsListFromClient(c)) if err != nil { @@ -628,26 +630,10 @@ func ListReplicaSets(deployment *extensions.Deployment, getRSList rsListFunc) ([ return owned, nil } -// ListReplicaSetsV15 is a compatibility function that behaves the way -// ListReplicaSets() used to in v1.5.x. +// ListReplicaSetsV15 is a compatibility function that emulates the behavior +// from v1.5.x (list matching objects by selector) except that it leaves out +// objects that are explicitly marked as being controlled by something else. func ListReplicaSetsV15(deployment *extensions.Deployment, getRSList rsListFunc) ([]*extensions.ReplicaSet, error) { - 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) -} - -// 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 { @@ -656,17 +642,49 @@ func ListReplicaSetsInternal(deployment *internalextensions.Deployment, getRSLis options := metav1.ListOptions{LabelSelector: selector.String()} all, err := getRSList(namespace, options) if err != nil { - return all, err + return nil, err } - // Only include those whose ControllerRef matches the Deployment. - owned := make([]*internalextensions.ReplicaSet, 0, len(all)) + // Since this function maintains compatibility with v1.5, the objects we want + // do not necessarily have ControllerRefs pointing to us. + // However, we can at least avoid interfering with other controllers that do + // use ControllerRef. + filtered := 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) + if controllerRef != nil && controllerRef.UID != deployment.UID { + continue } + filtered = append(filtered, rs) } - return owned, nil + return filtered, nil +} + +// ListReplicaSetsInternalV15 is ListReplicaSetsV15 for internalextensions. +// TODO: Remove the duplicate when call sites are updated to ListReplicaSetsV15. +func ListReplicaSetsInternalV15(deployment *internalextensions.Deployment, getRSList func(string, metav1.ListOptions) ([]*internalextensions.ReplicaSet, error)) ([]*internalextensions.ReplicaSet, error) { + 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 nil, err + } + // Since this function maintains compatibility with v1.5, the objects we want + // do not necessarily have ControllerRefs pointing to us. + // However, we can at least avoid interfering with other controllers that do + // use ControllerRef. + filtered := make([]*internalextensions.ReplicaSet, 0, len(all)) + for _, rs := range all { + controllerRef := controller.GetControllerOf(rs) + if controllerRef != nil && controllerRef.UID != deployment.UID { + continue + } + filtered = append(filtered, rs) + } + return filtered, nil } // ListPods returns a list of pods the given deployment targets. @@ -703,16 +721,39 @@ func ListPods(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet return owned, nil } -// ListPodsV15 is a compatibility function that behaves the way -// ListPods() used to in v1.5.x. -func ListPodsV15(deployment *extensions.Deployment, getPodList podListFunc) (*v1.PodList, error) { +// ListPodsV15 is a compatibility function that emulates the behavior +// from v1.5.x (list matching objects by selector) except that it leaves out +// objects that are explicitly marked as being controlled by something else. +func ListPodsV15(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) + podList, err := getPodList(namespace, options) + if err != nil { + return nil, err + } + // Since this function maintains compatibility with v1.5, the objects we want + // do not necessarily have ControllerRefs pointing to one of our ReplicaSets. + // However, we can at least avoid interfering with other controllers that do + // use ControllerRef. + rsMap := make(map[types.UID]bool, len(rsList)) + for _, rs := range rsList { + rsMap[rs.UID] = true + } + filtered := make([]v1.Pod, 0, len(podList.Items)) + for i := range podList.Items { + pod := &podList.Items[i] + controllerRef := controller.GetControllerOf(pod) + if controllerRef != nil && !rsMap[controllerRef.UID] { + continue + } + filtered = append(filtered, *pod) + } + podList.Items = filtered + return podList, nil } // EqualIgnoreHash returns true if two given podTemplateSpec are equal, ignoring the diff in value of Labels[pod-template-hash] diff --git a/pkg/kubectl/BUILD b/pkg/kubectl/BUILD index 9138c980c93..2df52fc48ea 100644 --- a/pkg/kubectl/BUILD +++ b/pkg/kubectl/BUILD @@ -72,6 +72,7 @@ go_library( "//pkg/client/clientset_generated/internalclientset/typed/extensions/internalversion:go_default_library", "//pkg/client/retry:go_default_library", "//pkg/client/unversioned:go_default_library", + "//pkg/controller:go_default_library", "//pkg/controller/deployment/util:go_default_library", "//pkg/credentialprovider:go_default_library", "//pkg/kubectl/resource:go_default_library", diff --git a/pkg/kubectl/history.go b/pkg/kubectl/history.go index 0c9398bbb84..f8077d0027a 100644 --- a/pkg/kubectl/history.go +++ b/pkg/kubectl/history.go @@ -65,7 +65,7 @@ func (h *DeploymentHistoryViewer) ViewHistory(namespace, name string, revision i if err != nil { return "", fmt.Errorf("failed to retrieve deployment %s: %v", name, err) } - _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(deployment, versionedClient) + _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSetsV15(deployment, versionedClient) if err != nil { return "", fmt.Errorf("failed to retrieve replica sets from deployment %s: %v", name, err) } diff --git a/pkg/kubectl/rollback.go b/pkg/kubectl/rollback.go index f57143227f5..398a0079f39 100644 --- a/pkg/kubectl/rollback.go +++ b/pkg/kubectl/rollback.go @@ -140,7 +140,7 @@ func simpleDryRun(deployment *extensions.Deployment, c clientset.Interface, toRe return "", fmt.Errorf("failed to convert deployment, %v", err) } versionedClient := versionedClientsetForDeployment(c) - _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(externalDeployment, versionedClient) + _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSetsV15(externalDeployment, versionedClient) if err != nil { return "", fmt.Errorf("failed to retrieve replica sets from deployment %s: %v", deployment.Name, err) } diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 9ccd9e40792..04df667c628 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -37,6 +37,7 @@ import ( batchclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/batch/internalversion" coreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion" extensionsclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/extensions/internalversion" + "k8s.io/kubernetes/pkg/controller" deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util" "k8s.io/kubernetes/pkg/util" ) @@ -355,7 +356,16 @@ func (reaper *StatefulSetReaper) Stop(namespace, name string, timeout time.Durat } errList := []error{} - for _, pod := range podList.Items { + for i := range podList.Items { + pod := &podList.Items[i] + // Since the client must maintain compatibility with a v1.5 server, + // we can't assume the Pods will have ControllerRefs pointing to 'ss'. + // However, we can at least avoid interfering with other controllers + // that do use ControllerRef. + controllerRef := controller.GetControllerOf(pod) + if controllerRef != nil && controllerRef.UID != ss.UID { + continue + } if err := pods.Delete(pod.Name, gracePeriod); err != nil { if !errors.IsNotFound(err) { errList = append(errList, err) @@ -448,7 +458,7 @@ func (reaper *DeploymentReaper) Stop(namespace, name string, timeout time.Durati } // Stop all replica sets belonging to this Deployment. - rss, err := deploymentutil.ListReplicaSetsInternal(deployment, + rss, err := deploymentutil.ListReplicaSetsInternalV15(deployment, func(namespace string, options metav1.ListOptions) ([]*extensions.ReplicaSet, error) { rsList, err := reaper.rsClient.ReplicaSets(namespace).List(options) if err != nil { diff --git a/pkg/kubectl/stop_test.go b/pkg/kubectl/stop_test.go index 95a8b243472..ea3109ffa26 100644 --- a/pkg/kubectl/stop_test.go +++ b/pkg/kubectl/stop_test.go @@ -476,6 +476,7 @@ func TestDeploymentStop(t *testing.T) { &deployment, // GET &extensions.ReplicaSetList{ // LIST Items: []extensions.ReplicaSet{ + // ReplicaSet owned by this Deployment. { ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -484,7 +485,7 @@ func TestDeploymentStop(t *testing.T) { OwnerReferences: []metav1.OwnerReference{ { APIVersion: extensions.SchemeGroupVersion.String(), - Kind: "ReplicaSet", + Kind: "Deployment", Name: deployment.Name, UID: deployment.UID, Controller: &trueVar, @@ -495,6 +496,72 @@ func TestDeploymentStop(t *testing.T) { Template: template, }, }, + // ReplicaSet owned by something else (should be ignored). + { + ObjectMeta: metav1.ObjectMeta{ + Name: "rs2", + Namespace: ns, + Labels: map[string]string{"k1": "v1"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: extensions.SchemeGroupVersion.String(), + Kind: "Deployment", + Name: "somethingelse", + UID: uuid.NewUUID(), + Controller: &trueVar, + }, + }, + }, + Spec: extensions.ReplicaSetSpec{ + Template: template, + }, + }, + }, + }, + }, + StopError: nil, + ExpectedActions: []string{"get:deployments", "update:deployments", + "get:deployments", "list:replicasets", "get:replicasets", + "get:replicasets", "update:replicasets", "get:replicasets", + "get:replicasets", "delete:replicasets", "delete:deployments"}, + }, + { + Name: "Deployment with single replicaset, no ControllerRef (old cluster)", + Objs: []runtime.Object{ + &deployment, // GET + &extensions.ReplicaSetList{ // LIST + Items: []extensions.ReplicaSet{ + // ReplicaSet that matches but with no ControllerRef. + { + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + Labels: map[string]string{"k1": "v1"}, + }, + Spec: extensions.ReplicaSetSpec{ + Template: template, + }, + }, + // ReplicaSet owned by something else (should be ignored). + { + ObjectMeta: metav1.ObjectMeta{ + Name: "rs2", + Namespace: ns, + Labels: map[string]string{"k1": "v1"}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: extensions.SchemeGroupVersion.String(), + Kind: "Deployment", + Name: "somethingelse", + UID: uuid.NewUUID(), + Controller: &trueVar, + }, + }, + }, + Spec: extensions.ReplicaSetSpec{ + Template: template, + }, + }, }, }, }, diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index d71ea81f2cd..658d6b16b8e 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -3555,7 +3555,7 @@ func logPodsOfDeployment(c clientset.Interface, deployment *extensions.Deploymen var podList *v1.PodList var err error if v15Compatible { - podList, err = deploymentutil.ListPodsV15(deployment, podListFunc) + podList, err = deploymentutil.ListPodsV15(deployment, rsList, podListFunc) } else { podList, err = deploymentutil.ListPods(deployment, rsList, podListFunc) } From de92f90f12dbc17c8ecdc1b54121d976e17dcd15 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Thu, 16 Mar 2017 14:52:01 -0700 Subject: [PATCH 3/3] Deployment: Clear obsolete OverlapAnnotaiton. This ensures old clients will not assume the Deployment is blocked. --- .../deployment/deployment_controller.go | 14 ++++++++++ .../deployment/deployment_controller_test.go | 26 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/pkg/controller/deployment/deployment_controller.go b/pkg/controller/deployment/deployment_controller.go index 304164ad94b..ea3d9e12cbd 100644 --- a/pkg/controller/deployment/deployment_controller.go +++ b/pkg/controller/deployment/deployment_controller.go @@ -581,6 +581,20 @@ func (dc *DeploymentController) syncDeployment(key string) error { return nil } + // This is the point at which we used to add/remove the overlap annotation. + // Now we always remove it if it exists, because it is obsolete as of 1.6. + // Although the server no longer adds or looks at the annotation, + // it's important to remove it from controllers created before the upgrade, + // so that old clients (e.g. kubectl reaper) know they can no longer assume + // the controller is blocked due to selector overlap and has no dependents. + if _, ok := d.Annotations[util.OverlapAnnotation]; ok { + delete(d.Annotations, util.OverlapAnnotation) + d, err = dc.client.ExtensionsV1beta1().Deployments(d.Namespace).UpdateStatus(d) + if err != nil { + return fmt.Errorf("couldn't remove obsolete overlap annotation from deployment %v: %v", key, err) + } + } + // List ReplicaSets owned by this Deployment, while reconciling ControllerRef // through adoption/orphaning. rsList, err := dc.getReplicaSetsForDeployment(d) diff --git a/pkg/controller/deployment/deployment_controller_test.go b/pkg/controller/deployment/deployment_controller_test.go index 7e2638b8563..802b33df2fb 100644 --- a/pkg/controller/deployment/deployment_controller_test.go +++ b/pkg/controller/deployment/deployment_controller_test.go @@ -254,6 +254,32 @@ func TestSyncDeploymentCreatesReplicaSet(t *testing.T) { f.run(getKey(d, t)) } +func TestSyncDeploymentClearsOverlapAnnotation(t *testing.T) { + f := newFixture(t) + + d := newDeployment("foo", 1, nil, nil, nil, map[string]string{"foo": "bar"}) + d.Annotations[util.OverlapAnnotation] = "overlap" + f.dLister = append(f.dLister, d) + f.objects = append(f.objects, d) + + rs := newReplicaSet(d, "deploymentrs-4186632231", 1) + + f.expectUpdateDeploymentStatusAction(d) + f.expectCreateRSAction(rs) + f.expectUpdateDeploymentStatusAction(d) + f.expectUpdateDeploymentStatusAction(d) + + f.run(getKey(d, t)) + + d, err := f.client.ExtensionsV1beta1().Deployments(d.Namespace).Get(d.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("can't get deployment: %v", err) + } + if _, ok := d.Annotations[util.OverlapAnnotation]; ok { + t.Errorf("OverlapAnnotation = %q, wanted absent", d.Annotations[util.OverlapAnnotation]) + } +} + func TestSyncDeploymentDontDoAnythingDuringDeletion(t *testing.T) { f := newFixture(t)