From 866c599198b5329ae7071b37fba48e50449aa74f Mon Sep 17 00:00:00 2001 From: Haoran Wang Date: Sat, 1 Apr 2017 17:16:07 +0800 Subject: [PATCH] Clean up pre-ControllerRef compatibility logic --- .../deployment/util/deployment_util.go | 110 +----------------- pkg/kubectl/history.go | 2 +- pkg/kubectl/rollback.go | 2 +- pkg/kubectl/stop.go | 9 +- pkg/kubectl/stop_test.go | 46 -------- test/e2e/framework/util.go | 74 +++--------- test/e2e/upgrades/deployments.go | 13 ++- 7 files changed, 33 insertions(+), 223 deletions(-) diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index 3b5bcbb6d3a..cf038b80466 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -513,25 +513,6 @@ func GetAllReplicaSets(deployment *extensions.Deployment, c clientset.Interface) return oldRSes, allOldRSes, newRS, nil } -// 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 - } - oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList) - if err != nil { - return nil, nil, nil, err - } - newRS, err := FindNewReplicaSet(deployment, rsList) - if err != nil { - return nil, nil, nil, err - } - return oldRSes, allOldRSes, newRS, nil -} - // GetOldReplicaSets returns the old replica sets targeted by the given Deployment; get PodList and ReplicaSetList from client interface. // Note that the first set of old replica sets doesn't include the ones with no pods, and the second set of old replica sets include all old replica sets. func GetOldReplicaSets(deployment *extensions.Deployment, c clientset.Interface) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { @@ -552,17 +533,6 @@ func GetNewReplicaSet(deployment *extensions.Deployment, c clientset.Interface) return FindNewReplicaSet(deployment, rsList) } -// 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 { - return nil, err - } - return FindNewReplicaSet(deployment, rsList) -} - // rsListFromClient returns an rsListFunc that wraps the given client. func rsListFromClient(c clientset.Interface) rsListFunc { return func(namespace string, options metav1.ListOptions) ([]*extensions.ReplicaSet, error) { @@ -617,10 +587,9 @@ func ListReplicaSets(deployment *extensions.Deployment, getRSList rsListFunc) ([ return owned, nil } -// 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) { +// ListReplicaSetsInternal is ListReplicaSets for internalextensions. +// TODO: Remove the duplicate when call sites are updated to ListReplicaSets. +func ListReplicaSetsInternal(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 { @@ -631,45 +600,13 @@ func ListReplicaSetsV15(deployment *extensions.Deployment, getRSList rsListFunc) 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([]*extensions.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 -} - -// 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. + // Only include those whose ControllerRef matches the Deployment. filtered := make([]*internalextensions.ReplicaSet, 0, len(all)) for _, rs := range all { controllerRef := controller.GetControllerOf(rs) - if controllerRef != nil && controllerRef.UID != deployment.UID { - continue + if controllerRef != nil && controllerRef.UID == deployment.UID { + filtered = append(filtered, rs) } - filtered = append(filtered, rs) } return filtered, nil } @@ -708,41 +645,6 @@ func ListPods(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet return owned, nil } -// 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()} - 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] // We ignore pod-template-hash because the hash result would be different upon podTemplateSpec API changes // (e.g. the addition of a new field will cause the hash code to change) diff --git a/pkg/kubectl/history.go b/pkg/kubectl/history.go index 29c86b66ab9..54ea1e1b273 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.GetAllReplicaSetsV15(deployment, versionedClient) + _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(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 2a3dbaabb0b..95b6fc0e27b 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.GetAllReplicaSetsV15(externalDeployment, versionedClient) + _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(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 04df667c628..ccae1ce6684 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -358,12 +358,9 @@ func (reaper *StatefulSetReaper) Stop(namespace, name string, timeout time.Durat errList := []error{} 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 { + // Ignore Pod if it's an orphan or owned by someone else. + if controllerRef == nil || controllerRef.UID != ss.UID { continue } if err := pods.Delete(pod.Name, gracePeriod); err != nil { @@ -458,7 +455,7 @@ func (reaper *DeploymentReaper) Stop(namespace, name string, timeout time.Durati } // Stop all replica sets belonging to this Deployment. - rss, err := deploymentutil.ListReplicaSetsInternalV15(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 { diff --git a/pkg/kubectl/stop_test.go b/pkg/kubectl/stop_test.go index ea3109ffa26..18db58da2f0 100644 --- a/pkg/kubectl/stop_test.go +++ b/pkg/kubectl/stop_test.go @@ -525,52 +525,6 @@ func TestDeploymentStop(t *testing.T) { "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, - }, - }, - }, - }, - }, - 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"}, - }, } for _, test := range tests { diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 0779b9dbb5a..41e47c2b67e 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -3206,16 +3206,6 @@ func NewDeployment(deploymentName string, replicas int32, podLabels map[string]s // Note that the status should stay valid at all times unless shortly after a scaling event or the deployment is just created. // To verify that the deployment status is valid and wait for the rollout to finish, use WaitForDeploymentStatus instead. func WaitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deployment) error { - return waitForDeploymentStatusValid(c, d, false) -} - -// WaitForDeploymentStatusValidV15 is a compatibility function that behaves the -// way WaitForDeploymentStatusValid() did in v1.5.x. -func WaitForDeploymentStatusValidV15(c clientset.Interface, d *extensions.Deployment) error { - return waitForDeploymentStatusValid(c, d, true) -} - -func waitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deployment, v15Compatible bool) error { var ( oldRSs, allOldRSs, allRSs []*extensions.ReplicaSet newRS *extensions.ReplicaSet @@ -3229,11 +3219,7 @@ func waitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deploymen if err != nil { return false, err } - if v15Compatible { - oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSetsV15(deployment, c) - } else { - oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSets(deployment, c) - } + oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSets(deployment, c) if err != nil { return false, err } @@ -3279,7 +3265,7 @@ func waitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deploymen if err == wait.ErrWaitTimeout { logReplicaSetsOfDeployment(deployment, allOldRSs, newRS) - logPodsOfDeployment(c, deployment, allRSs, v15Compatible) + logPodsOfDeployment(c, deployment, allRSs) err = fmt.Errorf("%s", reason) } if err != nil { @@ -3291,16 +3277,6 @@ func waitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deploymen // Waits for the deployment to reach desired state. // Returns an error if the deployment's rolling update strategy (max unavailable or max surge) is broken at any times. func WaitForDeploymentStatus(c clientset.Interface, d *extensions.Deployment) error { - return waitForDeploymentStatus(c, d, false) -} - -// WaitForDeploymentStatusV15 is a compatibility function that behaves the way -// WaitForDeploymentStatus() did in v1.5.x. -func WaitForDeploymentStatusV15(c clientset.Interface, d *extensions.Deployment) error { - return waitForDeploymentStatus(c, d, true) -} - -func waitForDeploymentStatus(c clientset.Interface, d *extensions.Deployment, v15Compatible bool) error { var ( oldRSs, allOldRSs, allRSs []*extensions.ReplicaSet newRS *extensions.ReplicaSet @@ -3313,11 +3289,7 @@ func waitForDeploymentStatus(c clientset.Interface, d *extensions.Deployment, v1 if err != nil { return false, err } - if v15Compatible { - oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSetsV15(deployment, c) - } else { - oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSets(deployment, c) - } + oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSets(deployment, c) if err != nil { return false, err } @@ -3336,13 +3308,13 @@ func waitForDeploymentStatus(c clientset.Interface, d *extensions.Deployment, v1 maxCreated := *(deployment.Spec.Replicas) + deploymentutil.MaxSurge(*deployment) if totalCreated > maxCreated { logReplicaSetsOfDeployment(deployment, allOldRSs, newRS) - logPodsOfDeployment(c, deployment, allRSs, v15Compatible) + logPodsOfDeployment(c, deployment, allRSs) return false, fmt.Errorf("total pods created: %d, more than the max allowed: %d", totalCreated, maxCreated) } minAvailable := deploymentutil.MinAvailable(deployment) if deployment.Status.AvailableReplicas < minAvailable { logReplicaSetsOfDeployment(deployment, allOldRSs, newRS) - logPodsOfDeployment(c, deployment, allRSs, v15Compatible) + logPodsOfDeployment(c, deployment, allRSs) return false, fmt.Errorf("total pods available: %d, less than the min required: %d", deployment.Status.AvailableReplicas, minAvailable) } @@ -3352,7 +3324,7 @@ func waitForDeploymentStatus(c clientset.Interface, d *extensions.Deployment, v1 if err == wait.ErrWaitTimeout { logReplicaSetsOfDeployment(deployment, allOldRSs, newRS) - logPodsOfDeployment(c, deployment, allRSs, v15Compatible) + logPodsOfDeployment(c, deployment, allRSs) } if err != nil { return fmt.Errorf("error waiting for deployment %q status to match expectation: %v", d.Name, err) @@ -3422,7 +3394,7 @@ func WatchRecreateDeployment(c clientset.Interface, d *extensions.Deployment) er if err == nil && nerr == nil { Logf("%+v", d) logReplicaSetsOfDeployment(d, allOldRSs, newRS) - logPodsOfDeployment(c, d, append(allOldRSs, newRS), false) + logPodsOfDeployment(c, d, append(allOldRSs, newRS)) } return false, fmt.Errorf("deployment %q is running new pods alongside old pods: %#v", d.Name, status) } @@ -3442,16 +3414,6 @@ func WatchRecreateDeployment(c clientset.Interface, d *extensions.Deployment) er // WaitForDeploymentRevisionAndImage waits for the deployment's and its new RS's revision and container image to match the given revision and image. // Note that deployment revision and its new RS revision should be updated shortly, so we only wait for 1 minute here to fail early. func WaitForDeploymentRevisionAndImage(c clientset.Interface, ns, deploymentName string, revision, image string) error { - return waitForDeploymentRevisionAndImage(c, ns, deploymentName, revision, image, false) -} - -// WaitForDeploymentRevisionAndImageV15 is a compatibility function that behaves -// the way WaitForDeploymentRevisionAndImage() did in v1.5.x. -func WaitForDeploymentRevisionAndImageV15(c clientset.Interface, ns, deploymentName string, revision, image string) error { - return waitForDeploymentRevisionAndImage(c, ns, deploymentName, revision, image, true) -} - -func waitForDeploymentRevisionAndImage(c clientset.Interface, ns, deploymentName string, revision, image string, v15Compatible bool) error { var deployment *extensions.Deployment var newRS *extensions.ReplicaSet var reason string @@ -3462,11 +3424,9 @@ func waitForDeploymentRevisionAndImage(c clientset.Interface, ns, deploymentName return false, err } // The new ReplicaSet needs to be non-nil and contain the pod-template-hash label - if v15Compatible { - newRS, err = deploymentutil.GetNewReplicaSetV15(deployment, c) - } else { - newRS, err = deploymentutil.GetNewReplicaSet(deployment, c) - } + + newRS, err = deploymentutil.GetNewReplicaSet(deployment, c) + if err != nil { return false, err } @@ -3613,24 +3573,20 @@ func WaitForDeploymentWithCondition(c clientset.Interface, ns, deploymentName, r _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(deployment, c) if err == nil { logReplicaSetsOfDeployment(deployment, allOldRSs, newRS) - logPodsOfDeployment(c, deployment, append(allOldRSs, newRS), false) + logPodsOfDeployment(c, deployment, append(allOldRSs, newRS)) } } return pollErr } -func logPodsOfDeployment(c clientset.Interface, deployment *extensions.Deployment, rsList []*extensions.ReplicaSet, v15Compatible bool) { +func logPodsOfDeployment(c clientset.Interface, deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) { minReadySeconds := deployment.Spec.MinReadySeconds podListFunc := func(namespace string, options metav1.ListOptions) (*v1.PodList, error) { return c.Core().Pods(namespace).List(options) } - var podList *v1.PodList - var err error - if v15Compatible { - podList, err = deploymentutil.ListPodsV15(deployment, rsList, podListFunc) - } else { - podList, err = deploymentutil.ListPods(deployment, rsList, podListFunc) - } + + podList, err := deploymentutil.ListPods(deployment, rsList, podListFunc) + if err != nil { Logf("Failed to list Pods of Deployment %s: %v", deployment.Name, err) return diff --git a/test/e2e/upgrades/deployments.go b/test/e2e/upgrades/deployments.go index 20ceb4b8e60..55f3759417f 100644 --- a/test/e2e/upgrades/deployments.go +++ b/test/e2e/upgrades/deployments.go @@ -42,6 +42,7 @@ type DeploymentUpgradeTest struct { func (DeploymentUpgradeTest) Name() string { return "deployment-upgrade" } // Setup creates a deployment and makes sure it has a new and an old replica set running. +// This calls in to client code and should not be expected to work against a cluster more than one minor version away from the current version. func (t *DeploymentUpgradeTest) Setup(f *framework.Framework) { deploymentName := "deployment-hash-test" c := f.ClientSet @@ -56,13 +57,13 @@ func (t *DeploymentUpgradeTest) Setup(f *framework.Framework) { // Wait for it to be updated to revision 1 By(fmt.Sprintf("Waiting deployment %q to be updated to revision 1", deploymentName)) - err = framework.WaitForDeploymentRevisionAndImageV15(c, ns, deploymentName, "1", nginxImage) + err = framework.WaitForDeploymentRevisionAndImage(c, ns, deploymentName, "1", nginxImage) framework.ExpectNoError(err) By(fmt.Sprintf("Waiting deployment %q to complete", deploymentName)) - framework.ExpectNoError(framework.WaitForDeploymentStatusValidV15(c, deployment)) + framework.ExpectNoError(framework.WaitForDeploymentStatusValid(c, deployment)) - rs, err := deploymentutil.GetNewReplicaSetV15(deployment, c) + rs, err := deploymentutil.GetNewReplicaSet(deployment, c) framework.ExpectNoError(err) if rs == nil { framework.ExpectNoError(fmt.Errorf("expected a new replica set for deployment %q, found none", deployment.Name)) @@ -84,12 +85,12 @@ func (t *DeploymentUpgradeTest) Setup(f *framework.Framework) { // Wait for it to be updated to revision 2 By(fmt.Sprintf("Waiting deployment %q to be updated to revision 2", deploymentName)) - framework.ExpectNoError(framework.WaitForDeploymentRevisionAndImageV15(c, ns, deploymentName, "2", nginxImage)) + framework.ExpectNoError(framework.WaitForDeploymentRevisionAndImage(c, ns, deploymentName, "2", nginxImage)) By(fmt.Sprintf("Waiting deployment %q to complete", deploymentName)) - framework.ExpectNoError(framework.WaitForDeploymentStatusV15(c, deployment)) + framework.ExpectNoError(framework.WaitForDeploymentStatus(c, deployment)) - rs, err = deploymentutil.GetNewReplicaSetV15(deployment, c) + rs, err = deploymentutil.GetNewReplicaSet(deployment, c) framework.ExpectNoError(err) if rs == nil { framework.ExpectNoError(fmt.Errorf("expected a new replica set for deployment %q", deployment.Name))