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 d075ad86b84..ff9c9561f77 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) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 01b5f33ebf8..ee7b3995c13 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 @@ -513,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 } @@ -559,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 { @@ -623,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 { @@ -651,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. @@ -698,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 70cea6deba0..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) @@ -440,8 +450,15 @@ 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, + 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/pkg/printers/internalversion/describe.go b/pkg/printers/internalversion/describe.go index 91f8da8cfa7..57fbf4498b0 100644 --- a/pkg/printers/internalversion/describe.go +++ b/pkg/printers/internalversion/describe.go @@ -2445,6 +2445,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 { 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) }