diff --git a/pkg/controller/deployment/sync.go b/pkg/controller/deployment/sync.go index 072629ec63f..bee9cebceff 100644 --- a/pkg/controller/deployment/sync.go +++ b/pkg/controller/deployment/sync.go @@ -111,11 +111,11 @@ func (dc *DeploymentController) checkPausedConditions(d *extensions.Deployment) // This may lead to stale reads of replica sets, thus incorrect deployment status. func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList, createIfNotExisted bool) (*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { // List the deployment's RSes & Pods and apply pod-template-hash info to deployment's adopted RSes/Pods - rsList, podList, err := dc.rsAndPodsWithHashKeySynced(d, rsList, podMap) + rsList, err := dc.rsAndPodsWithHashKeySynced(d, rsList, podMap) if err != nil { return nil, nil, fmt.Errorf("error labeling replica sets and pods with pod-template-hash: %v", err) } - _, allOldRSs, err := deploymentutil.FindOldReplicaSets(d, rsList, podList) + _, allOldRSs, err := deploymentutil.FindOldReplicaSets(d, rsList) if err != nil { return nil, nil, err } @@ -134,7 +134,7 @@ func (dc *DeploymentController) getAllReplicaSetsAndSyncRevision(d *extensions.D // // rsList should come from getReplicaSetsForDeployment(d). // podMap should come from getPodMapForDeployment(d, rsList). -func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) ([]*extensions.ReplicaSet, *v1.PodList, error) { +func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deployment, rsList []*extensions.ReplicaSet, podMap map[types.UID]*v1.PodList) ([]*extensions.ReplicaSet, error) { syncedRSList := []*extensions.ReplicaSet{} for _, rs := range rsList { // Add pod-template-hash information if it's not in the RS. @@ -142,16 +142,11 @@ func (dc *DeploymentController) rsAndPodsWithHashKeySynced(d *extensions.Deploym // that aren't constrained by the pod-template-hash. syncedRS, err := dc.addHashKeyToRSAndPods(rs, podMap[rs.UID]) if err != nil { - return nil, nil, err + return nil, err } syncedRSList = append(syncedRSList, syncedRS) } - // Put all Pods from podMap into one list. - syncedPodList := &v1.PodList{} - for _, podList := range podMap { - syncedPodList.Items = append(syncedPodList.Items, podList.Items...) - } - return syncedRSList, syncedPodList, nil + return syncedRSList, nil } // addHashKeyToRSAndPods adds pod-template-hash information to the given rs, if it's not already there, with the following steps: diff --git a/pkg/controller/deployment/util/BUILD b/pkg/controller/deployment/util/BUILD index 7b4e638136b..551c297f601 100644 --- a/pkg/controller/deployment/util/BUILD +++ b/pkg/controller/deployment/util/BUILD @@ -35,7 +35,6 @@ go_library( "//vendor:k8s.io/apimachinery/pkg/api/equality", "//vendor:k8s.io/apimachinery/pkg/api/meta", "//vendor:k8s.io/apimachinery/pkg/apis/meta/v1", - "//vendor:k8s.io/apimachinery/pkg/labels", "//vendor:k8s.io/apimachinery/pkg/runtime", "//vendor:k8s.io/apimachinery/pkg/types", "//vendor:k8s.io/apimachinery/pkg/util/errors", diff --git a/pkg/controller/deployment/util/deployment_util.go b/pkg/controller/deployment/util/deployment_util.go index ee7b3995c13..490ea7a3398 100644 --- a/pkg/controller/deployment/util/deployment_util.go +++ b/pkg/controller/deployment/util/deployment_util.go @@ -28,7 +28,6 @@ import ( apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/errors" @@ -503,11 +502,7 @@ func GetAllReplicaSets(deployment *extensions.Deployment, c clientset.Interface) if err != nil { return nil, nil, nil, err } - podList, err := ListPods(deployment, rsList, podListFromClient(c)) - if err != nil { - return nil, nil, nil, err - } - oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList, podList) + oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList) if err != nil { return nil, nil, nil, err } @@ -526,11 +521,7 @@ func GetAllReplicaSetsV15(deployment *extensions.Deployment, c clientset.Interfa if err != nil { return nil, nil, nil, err } - podList, err := ListPodsV15(deployment, rsList, podListFromClient(c)) - if err != nil { - return nil, nil, nil, err - } - oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList, podList) + oldRSes, allOldRSes, err := FindOldReplicaSets(deployment, rsList) if err != nil { return nil, nil, nil, err } @@ -548,11 +539,7 @@ func GetOldReplicaSets(deployment *extensions.Deployment, c clientset.Interface) if err != nil { return nil, nil, err } - podList, err := ListPods(deployment, rsList, podListFromClient(c)) - if err != nil { - return nil, nil, err - } - return FindOldReplicaSets(deployment, rsList, podList) + return FindOldReplicaSets(deployment, rsList) } // GetNewReplicaSet returns a replica set that matches the intent of the given deployment; get ReplicaSetList from client interface. @@ -794,44 +781,24 @@ func FindNewReplicaSet(deployment *extensions.Deployment, rsList []*extensions.R return nil, nil } -// FindOldReplicaSets returns the old replica sets targeted by the given Deployment, with the given PodList and slice of RSes. +// FindOldReplicaSets returns the old replica sets targeted by the given Deployment, with the given slice of RSes. // 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 FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet, podList *v1.PodList) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { - // Find all pods whose labels match deployment.Spec.Selector, and corresponding replica sets for pods in podList. - // All pods and replica sets are labeled with pod-template-hash to prevent overlapping - oldRSs := map[string]*extensions.ReplicaSet{} - allOldRSs := map[string]*extensions.ReplicaSet{} - requiredRSs := []*extensions.ReplicaSet{} - allRSs := []*extensions.ReplicaSet{} +func FindOldReplicaSets(deployment *extensions.Deployment, rsList []*extensions.ReplicaSet) ([]*extensions.ReplicaSet, []*extensions.ReplicaSet, error) { + var requiredRSs []*extensions.ReplicaSet + var allRSs []*extensions.ReplicaSet newRS, err := FindNewReplicaSet(deployment, rsList) if err != nil { - return requiredRSs, allRSs, err + return nil, nil, err } - for _, pod := range podList.Items { - podLabelsSelector := labels.Set(pod.ObjectMeta.Labels) - for _, rs := range rsList { - rsLabelsSelector, err := metav1.LabelSelectorAsSelector(rs.Spec.Selector) - if err != nil { - return nil, nil, fmt.Errorf("invalid label selector: %v", err) - } - // Filter out new replica set - if newRS != nil && rs.UID == newRS.UID { - continue - } - // TODO: If there are no pods for a deployment, we will never return old replica sets....! - allOldRSs[rs.ObjectMeta.Name] = rs - if rsLabelsSelector.Matches(podLabelsSelector) { - oldRSs[rs.ObjectMeta.Name] = rs - } + for _, rs := range rsList { + // Filter out new replica set + if newRS != nil && rs.UID == newRS.UID { + continue + } + allRSs = append(allRSs, rs) + if *(rs.Spec.Replicas) != 0 { + requiredRSs = append(requiredRSs, rs) } - } - for key := range oldRSs { - value := oldRSs[key] - requiredRSs = append(requiredRSs, value) - } - for key := range allOldRSs { - value := allOldRSs[key] - allRSs = append(allRSs, value) } return requiredRSs, allRSs, nil } diff --git a/pkg/controller/deployment/util/deployment_util_test.go b/pkg/controller/deployment/util/deployment_util_test.go index 18abfeb04d3..18869302880 100644 --- a/pkg/controller/deployment/util/deployment_util_test.go +++ b/pkg/controller/deployment/util/deployment_util_test.go @@ -241,7 +241,7 @@ func generateDeployment(image string) extensions.Deployment { } } -func TestGetNewRC(t *testing.T) { +func TestGetNewRS(t *testing.T) { newDeployment := generateDeployment("nginx") newRC := generateRS(newDeployment) @@ -297,27 +297,23 @@ func TestGetNewRC(t *testing.T) { } } -func TestGetOldRCs(t *testing.T) { +func TestGetOldRSs(t *testing.T) { newDeployment := generateDeployment("nginx") newRS := generateRS(newDeployment) newRS.Status.FullyLabeledReplicas = *(newRS.Spec.Replicas) - newPod := generatePodFromRS(newRS) // create 2 old deployments and related replica sets/pods, with the same labels but different template oldDeployment := generateDeployment("nginx") oldDeployment.Spec.Template.Spec.Containers[0].Name = "nginx-old-1" oldRS := generateRS(oldDeployment) oldRS.Status.FullyLabeledReplicas = *(oldRS.Spec.Replicas) - oldPod := generatePodFromRS(oldRS) oldDeployment2 := generateDeployment("nginx") oldDeployment2.Spec.Template.Spec.Containers[0].Name = "nginx-old-2" oldRS2 := generateRS(oldDeployment2) oldRS2.Status.FullyLabeledReplicas = *(oldRS2.Spec.Replicas) - oldPod2 := generatePodFromRS(oldRS2) // create 1 ReplicaSet that existed before the deployment, // with the same labels as the deployment, but no ControllerRef. - existedPod := generatePod(newDeployment.Spec.Template.Labels, "foo") existedRS := generateRSWithLabel(newDeployment.Spec.Template.Labels, "foo") existedRS.Status.FullyLabeledReplicas = *(existedRS.Spec.Replicas) @@ -329,13 +325,6 @@ func TestGetOldRCs(t *testing.T) { { "No old ReplicaSets", []runtime.Object{ - &v1.PodList{ - Items: []v1.Pod{ - generatePod(newDeployment.Spec.Template.Labels, "foo"), - generatePod(newDeployment.Spec.Template.Labels, "bar"), - newPod, - }, - }, &extensions.ReplicaSetList{ Items: []extensions.ReplicaSet{ generateRS(generateDeployment("foo")), @@ -344,21 +333,11 @@ func TestGetOldRCs(t *testing.T) { }, }, }, - []*extensions.ReplicaSet{}, + nil, }, { "Has old ReplicaSet", []runtime.Object{ - &v1.PodList{ - Items: []v1.Pod{ - oldPod, - oldPod2, - generatePod(map[string]string{"name": "bar"}, "bar"), - generatePod(map[string]string{"name": "xyz"}, "xyz"), - existedPod, - generatePod(newDeployment.Spec.Template.Labels, "abc"), - }, - }, &extensions.ReplicaSetList{ Items: []extensions.ReplicaSet{ oldRS2, @@ -376,12 +355,10 @@ func TestGetOldRCs(t *testing.T) { for _, test := range tests { fakeClient := &fake.Clientset{} - fakeClient = addListPodsReactor(fakeClient, test.objs[0]) - fakeClient = addListRSReactor(fakeClient, test.objs[1]) - fakeClient = addGetRSReactor(fakeClient, test.objs[1]) - fakeClient = addUpdatePodsReactor(fakeClient) + fakeClient = addListRSReactor(fakeClient, test.objs[0]) + fakeClient = addGetRSReactor(fakeClient, test.objs[0]) fakeClient = addUpdateRSReactor(fakeClient) - rss, _, err := GetOldReplicaSets(&newDeployment, fakeClient) + _, rss, err := GetOldReplicaSets(&newDeployment, fakeClient) if err != nil { t.Errorf("In test case %s, got unexpected error %v", test.test, err) } @@ -558,6 +535,7 @@ func TestFindOldReplicaSets(t *testing.T) { deployment := generateDeployment("nginx") newRS := generateRS(deployment) + *(newRS.Spec.Replicas) = 1 newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey] = "hash" newRS.CreationTimestamp = later @@ -571,70 +549,54 @@ func TestFindOldReplicaSets(t *testing.T) { oldRS.Status.FullyLabeledReplicas = *(oldRS.Spec.Replicas) oldRS.CreationTimestamp = before - newPod := generatePodFromRS(newRS) - oldPod := generatePodFromRS(oldRS) - tests := []struct { - test string - deployment extensions.Deployment - rsList []*extensions.ReplicaSet - podList *v1.PodList - expected []*extensions.ReplicaSet + test string + deployment extensions.Deployment + rsList []*extensions.ReplicaSet + podList *v1.PodList + expected []*extensions.ReplicaSet + expectedRequire []*extensions.ReplicaSet }{ { - test: "Get old ReplicaSets", - deployment: deployment, - rsList: []*extensions.ReplicaSet{&newRS, &oldRS}, - podList: &v1.PodList{ - Items: []v1.Pod{ - newPod, - oldPod, - }, - }, - expected: []*extensions.ReplicaSet{&oldRS}, + test: "Get old ReplicaSets", + deployment: deployment, + rsList: []*extensions.ReplicaSet{&newRS, &oldRS}, + expected: []*extensions.ReplicaSet{&oldRS}, + expectedRequire: nil, }, { - test: "Get old ReplicaSets with no new ReplicaSet", - deployment: deployment, - rsList: []*extensions.ReplicaSet{&oldRS}, - podList: &v1.PodList{ - Items: []v1.Pod{ - oldPod, - }, - }, - expected: []*extensions.ReplicaSet{&oldRS}, + test: "Get old ReplicaSets with no new ReplicaSet", + deployment: deployment, + rsList: []*extensions.ReplicaSet{&oldRS}, + expected: []*extensions.ReplicaSet{&oldRS}, + expectedRequire: nil, }, { - test: "Get old ReplicaSets with two new ReplicaSets, only the oldest new ReplicaSet is seen as new ReplicaSet", - deployment: deployment, - rsList: []*extensions.ReplicaSet{&oldRS, &newRS, &newRSDup}, - podList: &v1.PodList{ - Items: []v1.Pod{ - newPod, - oldPod, - }, - }, - expected: []*extensions.ReplicaSet{&oldRS, &newRS}, + test: "Get old ReplicaSets with two new ReplicaSets, only the oldest new ReplicaSet is seen as new ReplicaSet", + deployment: deployment, + rsList: []*extensions.ReplicaSet{&oldRS, &newRS, &newRSDup}, + expected: []*extensions.ReplicaSet{&oldRS, &newRS}, + expectedRequire: []*extensions.ReplicaSet{&newRS}, }, { - test: "Get empty old ReplicaSets", - deployment: deployment, - rsList: []*extensions.ReplicaSet{&newRS}, - podList: &v1.PodList{ - Items: []v1.Pod{ - newPod, - }, - }, - expected: []*extensions.ReplicaSet{}, + test: "Get empty old ReplicaSets", + deployment: deployment, + rsList: []*extensions.ReplicaSet{&newRS}, + expected: nil, + expectedRequire: nil, }, } for _, test := range tests { - old, _, err := FindOldReplicaSets(&test.deployment, test.rsList, test.podList) - sort.Sort(controller.ReplicaSetsByCreationTimestamp(old)) + requireRS, allRS, err := FindOldReplicaSets(&test.deployment, test.rsList) + sort.Sort(controller.ReplicaSetsByCreationTimestamp(allRS)) sort.Sort(controller.ReplicaSetsByCreationTimestamp(test.expected)) - if !reflect.DeepEqual(old, test.expected) || err != nil { - t.Errorf("In test case %q, expected %#v, got %#v: %v", test.test, test.expected, old, err) + if !reflect.DeepEqual(allRS, test.expected) || err != nil { + t.Errorf("In test case %q, expected %#v, got %#v: %v", test.test, test.expected, allRS, err) + } + // RSs are getting filtered correctly by rs.spec.replicas + if !reflect.DeepEqual(requireRS, test.expectedRequire) || err != nil { + t.Errorf("In test case %q, expected %#v, got %#v: %v", test.test, test.expectedRequire, requireRS, err) } } }