Merge pull request #43239 from enisoc/kubectl-controller-ref

Automatic merge from submit-queue

kubectl: Use v1.5-compatible ownership logic when listing dependents.

**What this PR does / why we need it**:

This restores compatibility between kubectl 1.6 and clusters running Kubernetes 1.5.x. It introduces transitional ownership logic in which the client considers ControllerRef when it exists, but does not require it to exist.

If we were to ignore ControllerRef altogether (pre-1.6 client behavior), we would introduce a new failure mode in v1.6 because controllers that used to get stuck due to selector overlap will now make progress. For example, that means when reaping ReplicaSets of an overlapping Deployment, we would risk deleting ReplicaSets belonging to a different Deployment that we aren't about to delete.

This transitional logic avoids such surprises in 1.6 clusters, and does no worse than kubectl 1.5 did in 1.5 clusters. To prevent this when kubectl 1.5 is used against 1.6 clusters, we can cherrypick this change.

**Which issue this PR fixes**:

Fixes #43159

**Special notes for your reviewer**:

**Release note**:
```release-note
```

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)

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)
 

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]

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",

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)
 	}

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)
 	}

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 {

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,
+							},
+						},
 					},
 				},
 			},

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 {

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)
 	}