Fix scaled down deployments cannot identify old replica sets

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,24 +134,19 @@ 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.
 		// Otherwise, new RS produced by Deployment will overlap with pre-existing ones
 		// 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:

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

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
 }

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