From 2ced966cd52226d90c97660e8a24019fc0e0f668 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Wed, 1 Jun 2016 14:14:40 -0700 Subject: [PATCH 1/2] List RSes once when getting old/new RSes in deployment e2e tests --- pkg/kubectl/describe.go | 5 +-- pkg/kubectl/history.go | 8 +--- pkg/util/deployment/deployment.go | 65 ++++++++++++++++++++++--------- test/e2e/deployment.go | 4 +- test/e2e/framework/util.go | 6 +-- 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/pkg/kubectl/describe.go b/pkg/kubectl/describe.go index c73e405347b..6d4e4667934 100644 --- a/pkg/kubectl/describe.go +++ b/pkg/kubectl/describe.go @@ -1922,12 +1922,9 @@ func (dd *DeploymentDescriber) Describe(namespace, name string, describerSetting ru := d.Spec.Strategy.RollingUpdate fmt.Fprintf(out, "RollingUpdateStrategy:\t%s max unavailable, %s max surge\n", ru.MaxUnavailable.String(), ru.MaxSurge.String()) } - oldRSs, _, err := deploymentutil.GetOldReplicaSets(d, dd) + oldRSs, _, newRS, err := deploymentutil.GetAllReplicaSets(d, dd) if err == nil { fmt.Fprintf(out, "OldReplicaSets:\t%s\n", printReplicaSetsByLabels(oldRSs)) - } - newRS, err := deploymentutil.GetNewReplicaSet(d, dd) - if err == nil { var newRSs []*extensions.ReplicaSet if newRS != nil { newRSs = append(newRSs, newRS) diff --git a/pkg/kubectl/history.go b/pkg/kubectl/history.go index 938f0329508..0e81ebf2819 100644 --- a/pkg/kubectl/history.go +++ b/pkg/kubectl/history.go @@ -66,13 +66,9 @@ func (h *DeploymentHistoryViewer) History(namespace, name string) (HistoryInfo, if err != nil { return historyInfo, fmt.Errorf("failed to retrieve deployment %s: %v", name, err) } - _, allOldRSs, err := deploymentutil.GetOldReplicaSets(deployment, h.c) + _, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(deployment, h.c) if err != nil { - return historyInfo, fmt.Errorf("failed to retrieve old replica sets from deployment %s: %v", name, err) - } - newRS, err := deploymentutil.GetNewReplicaSet(deployment, h.c) - if err != nil { - return historyInfo, fmt.Errorf("failed to retrieve new replica set from deployment %s: %v", name, err) + return historyInfo, fmt.Errorf("failed to retrieve replica sets from deployment %s: %v", name, err) } allRSs := allOldRSs if newRS != nil { diff --git a/pkg/util/deployment/deployment.go b/pkg/util/deployment/deployment.go index 9df49a2d8bc..9355ed4b456 100644 --- a/pkg/util/deployment/deployment.go +++ b/pkg/util/deployment/deployment.go @@ -45,41 +45,70 @@ const ( RollbackRevisionNotFound = "DeploymentRollbackRevisionNotFound" RollbackTemplateUnchanged = "DeploymentRollbackTemplateUnchanged" RollbackDone = "DeploymentRollback" + + all getReplicaSetType = "All" + old getReplicaSetType = "Old" + new getReplicaSetType = "New" ) +type getReplicaSetType string + +// GetAllReplicaSets returns the old and new replica sets targeted by the given Deployment. It gets 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. +// The third returned value is the new replica set, and it may be nil if it doesn't exist yet. +func GetAllReplicaSets(deployment *extensions.Deployment, c clientset.Interface) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, *extensions.ReplicaSet, error) { + return getReplicaSets(deployment, c, all) +} + // 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) { - rsList, err := ListReplicaSets(deployment, - func(namespace string, options api.ListOptions) ([]extensions.ReplicaSet, error) { - rsList, err := c.Extensions().ReplicaSets(namespace).List(options) - return rsList.Items, err - }) - if err != nil { - return nil, nil, fmt.Errorf("error listing ReplicaSets: %v", err) - } - podList, err := ListPods(deployment, - func(namespace string, options api.ListOptions) (*api.PodList, error) { - return c.Core().Pods(namespace).List(options) - }) - if err != nil { - return nil, nil, fmt.Errorf("error listing Pods: %v", err) - } - return FindOldReplicaSets(deployment, rsList, podList) + oldRSes, allOldRSes, _, err := getReplicaSets(deployment, c, old) + return oldRSes, allOldRSes, err } // GetNewReplicaSet returns a replica set that matches the intent of the given deployment; get ReplicaSetList from client interface. // Returns nil if the new replica set doesn't exist yet. func GetNewReplicaSet(deployment *extensions.Deployment, c clientset.Interface) (*extensions.ReplicaSet, error) { + _, _, newRS, err := getReplicaSets(deployment, c, new) + return newRS, err +} + +func getReplicaSets(deployment *extensions.Deployment, c clientset.Interface, getType getReplicaSetType) (oldRSes []*extensions.ReplicaSet, allOldRSes []*extensions.ReplicaSet, newRS *extensions.ReplicaSet, err error) { + // List all RSes rsList, err := ListReplicaSets(deployment, func(namespace string, options api.ListOptions) ([]extensions.ReplicaSet, error) { rsList, err := c.Extensions().ReplicaSets(namespace).List(options) return rsList.Items, err }) if err != nil { - return nil, fmt.Errorf("error listing ReplicaSets: %v", err) + return nil, nil, nil, fmt.Errorf("error listing ReplicaSets: %v", err) } - return FindNewReplicaSet(deployment, rsList) + + // Skip getting old replica sets when we only need new replica set + if getType != new { + // List all pods + podList, err := ListPods(deployment, + func(namespace string, options api.ListOptions) (*api.PodList, error) { + return c.Core().Pods(namespace).List(options) + }) + if err != nil { + return nil, nil, nil, fmt.Errorf("error listing Pods: %v", err) + } + oldRSes, allOldRSes, err = FindOldReplicaSets(deployment, rsList, podList) + if err != nil { + return nil, nil, nil, fmt.Errorf("error getting old ReplicaSets: %v", err) + } + } + + // Skip getting new replica set when we only need old replica sets + if getType != old { + newRS, err = FindNewReplicaSet(deployment, rsList) + if err != nil { + return nil, nil, nil, fmt.Errorf("error getting new ReplicaSet: %v", err) + } + } + return oldRSes, allOldRSes, newRS, nil } // TODO: switch this to full namespacers diff --git a/test/e2e/deployment.go b/test/e2e/deployment.go index 81b7c7c9287..e3ce387e68a 100644 --- a/test/e2e/deployment.go +++ b/test/e2e/deployment.go @@ -927,13 +927,11 @@ func testDeploymentLabelAdopted(f *framework.Framework) { // There should be no old RSs (overlapping RS) deployment, err := c.Extensions().Deployments(ns).Get(deploymentName) Expect(err).NotTo(HaveOccurred()) - oldRSs, allOldRSs, err := deploymentutil.GetOldReplicaSets(deployment, c) + oldRSs, allOldRSs, newRS, err := deploymentutil.GetAllReplicaSets(deployment, c) Expect(err).NotTo(HaveOccurred()) Expect(len(oldRSs)).Should(Equal(0)) Expect(len(allOldRSs)).Should(Equal(0)) // New RS should contain pod-template-hash in its selector, label, and template label - newRS, err := deploymentutil.GetNewReplicaSet(deployment, c) - Expect(err).NotTo(HaveOccurred()) err = framework.CheckRSHashLabel(newRS) Expect(err).NotTo(HaveOccurred()) // All pods targeted by the deployment should contain pod-template-hash in their labels, and there should be only 3 pods diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 00b5a4b118e..6d88b4fcd88 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -2834,11 +2834,7 @@ func WaitForDeploymentStatus(c clientset.Interface, ns, deploymentName string, d if err != nil { return false, err } - oldRSs, allOldRSs, err = deploymentutil.GetOldReplicaSets(deployment, c) - if err != nil { - return false, err - } - newRS, err = deploymentutil.GetNewReplicaSet(deployment, c) + oldRSs, allOldRSs, newRS, err = deploymentutil.GetAllReplicaSets(deployment, c) if err != nil { return false, err } From 57ec715c613ed60aa22e6a85394b2ae1186a7cc6 Mon Sep 17 00:00:00 2001 From: Janet Kuo Date: Wed, 1 Jun 2016 15:00:29 -0700 Subject: [PATCH 2/2] Address comments --- pkg/util/deployment/deployment.go | 82 +++++++++++++++---------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/pkg/util/deployment/deployment.go b/pkg/util/deployment/deployment.go index 9355ed4b456..0c348657401 100644 --- a/pkg/util/deployment/deployment.go +++ b/pkg/util/deployment/deployment.go @@ -45,70 +45,70 @@ const ( RollbackRevisionNotFound = "DeploymentRollbackRevisionNotFound" RollbackTemplateUnchanged = "DeploymentRollbackTemplateUnchanged" RollbackDone = "DeploymentRollback" - - all getReplicaSetType = "All" - old getReplicaSetType = "Old" - new getReplicaSetType = "New" ) -type getReplicaSetType string - // GetAllReplicaSets returns the old and new replica sets targeted by the given Deployment. It gets 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. // The third returned value is the new replica set, and it may be nil if it doesn't exist yet. func GetAllReplicaSets(deployment *extensions.Deployment, c clientset.Interface) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, *extensions.ReplicaSet, error) { - return getReplicaSets(deployment, c, all) + rsList, err := listReplicaSets(deployment, c) + if err != nil { + return nil, nil, nil, err + } + podList, err := listPods(deployment, c) + if err != nil { + return nil, nil, nil, err + } + oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList, podList) + 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) { - oldRSes, allOldRSes, _, err := getReplicaSets(deployment, c, old) - return oldRSes, allOldRSes, err + rsList, err := listReplicaSets(deployment, c) + if err != nil { + return nil, nil, err + } + podList, err := listPods(deployment, c) + if err != nil { + return nil, nil, err + } + return FindOldReplicaSets(deployment, rsList, podList) } // GetNewReplicaSet returns a replica set that matches the intent of the given deployment; get ReplicaSetList from client interface. // Returns nil if the new replica set doesn't exist yet. func GetNewReplicaSet(deployment *extensions.Deployment, c clientset.Interface) (*extensions.ReplicaSet, error) { - _, _, newRS, err := getReplicaSets(deployment, c, new) - return newRS, err + rsList, err := listReplicaSets(deployment, c) + if err != nil { + return nil, err + } + return FindNewReplicaSet(deployment, rsList) } -func getReplicaSets(deployment *extensions.Deployment, c clientset.Interface, getType getReplicaSetType) (oldRSes []*extensions.ReplicaSet, allOldRSes []*extensions.ReplicaSet, newRS *extensions.ReplicaSet, err error) { - // List all RSes - rsList, err := ListReplicaSets(deployment, +// listReplicaSets lists all RSes the given deployment targets with the given client interface. +func listReplicaSets(deployment *extensions.Deployment, c clientset.Interface) ([]extensions.ReplicaSet, error) { + return ListReplicaSets(deployment, func(namespace string, options api.ListOptions) ([]extensions.ReplicaSet, error) { rsList, err := c.Extensions().ReplicaSets(namespace).List(options) return rsList.Items, err }) - if err != nil { - return nil, nil, nil, fmt.Errorf("error listing ReplicaSets: %v", err) - } +} - // Skip getting old replica sets when we only need new replica set - if getType != new { - // List all pods - podList, err := ListPods(deployment, - func(namespace string, options api.ListOptions) (*api.PodList, error) { - return c.Core().Pods(namespace).List(options) - }) - if err != nil { - return nil, nil, nil, fmt.Errorf("error listing Pods: %v", err) - } - oldRSes, allOldRSes, err = FindOldReplicaSets(deployment, rsList, podList) - if err != nil { - return nil, nil, nil, fmt.Errorf("error getting old ReplicaSets: %v", err) - } - } - - // Skip getting new replica set when we only need old replica sets - if getType != old { - newRS, err = FindNewReplicaSet(deployment, rsList) - if err != nil { - return nil, nil, nil, fmt.Errorf("error getting new ReplicaSet: %v", err) - } - } - return oldRSes, allOldRSes, newRS, nil +// listReplicaSets lists all Pods the given deployment targets with the given client interface. +func listPods(deployment *extensions.Deployment, c clientset.Interface) (*api.PodList, error) { + return ListPods(deployment, + func(namespace string, options api.ListOptions) (*api.PodList, error) { + return c.Core().Pods(namespace).List(options) + }) } // TODO: switch this to full namespacers