Merge pull request #21030 from janetkuo/deployment-label-adopted

pkg/controller/deployment/deployment_controller.go

@@ -665,6 +665,10 @@ func (dc *DeploymentController) getNewReplicaSet(deployment extensions.Deploymen
 	newRevision := strconv.FormatInt(maxOldRevision+1, 10)
 
 	existingNewRS, err := deploymentutil.GetNewReplicaSetFromList(deployment, dc.client,
+		func(namespace string, options api.ListOptions) (*api.PodList, error) {
+			podList, err := dc.podStore.Pods(namespace).List(options.LabelSelector)
+			return &podList, err
+		},
 		func(namespace string, options api.ListOptions) ([]extensions.ReplicaSet, error) {
 			return dc.rsStore.ReplicaSets(namespace).List(options.LabelSelector)
 		})

pkg/util/deployment/deployment_test.go

@@ -22,14 +22,44 @@ import (
 	"time"
 
 	"k8s.io/kubernetes/pkg/api"
-	"k8s.io/kubernetes/pkg/api/testapi"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
-	"k8s.io/kubernetes/pkg/client/unversioned/testclient/simple"
+	"k8s.io/kubernetes/pkg/client/testing/core"
+	"k8s.io/kubernetes/pkg/client/unversioned/testclient"
 	"k8s.io/kubernetes/pkg/runtime"
 )
 
+func addListRSReactor(fakeClient *fake.Clientset, obj runtime.Object) *fake.Clientset {
+	fakeClient.AddReactor("list", "replicasets", func(action core.Action) (handled bool, ret runtime.Object, err error) {
+		return true, obj, nil
+	})
+	return fakeClient
+}
+
+func addListPodsReactor(fakeClient *fake.Clientset, obj runtime.Object) *fake.Clientset {
+	fakeClient.AddReactor("list", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) {
+		return true, obj, nil
+	})
+	return fakeClient
+}
+
+func addUpdateRSReactor(fakeClient *fake.Clientset) *fake.Clientset {
+	fakeClient.AddReactor("update", "replicasets", func(action core.Action) (handled bool, ret runtime.Object, err error) {
+		obj := action.(testclient.UpdateAction).GetObject().(*extensions.ReplicaSet)
+		return true, obj, nil
+	})
+	return fakeClient
+}
+
+func addUpdatePodsReactor(fakeClient *fake.Clientset) *fake.Clientset {
+	fakeClient.AddReactor("update", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) {
+		obj := action.(testclient.UpdateAction).GetObject().(*api.Pod)
+		return true, obj, nil
+	})
+	return fakeClient
+}
+
 func newPod(now time.Time, ready bool, beforeSec int) api.Pod {
 	conditionStatus := api.ConditionFalse
 	if ready {
@@ -190,47 +220,47 @@ func TestGetNewRC(t *testing.T) {
 
 	tests := []struct {
 		test     string
-		rsList   extensions.ReplicaSetList
+		objs     []runtime.Object
 		expected *extensions.ReplicaSet
 	}{
 		{
 			"No new ReplicaSet",
-			extensions.ReplicaSetList{
-				Items: []extensions.ReplicaSet{
-					generateRS(generateDeployment("foo")),
-					generateRS(generateDeployment("bar")),
+			[]runtime.Object{
+				&api.PodList{},
+				&extensions.ReplicaSetList{
+					Items: []extensions.ReplicaSet{
+						generateRS(generateDeployment("foo")),
+						generateRS(generateDeployment("bar")),
+					},
 				},
 			},
 			nil,
 		},
 		{
 			"Has new ReplicaSet",
-			extensions.ReplicaSetList{
-				Items: []extensions.ReplicaSet{
-					generateRS(generateDeployment("foo")),
-					generateRS(generateDeployment("bar")),
-					generateRS(generateDeployment("abc")),
-					newRC,
-					generateRS(generateDeployment("xyz")),
+			[]runtime.Object{
+				&api.PodList{},
+				&extensions.ReplicaSetList{
+					Items: []extensions.ReplicaSet{
+						generateRS(generateDeployment("foo")),
+						generateRS(generateDeployment("bar")),
+						generateRS(generateDeployment("abc")),
+						newRC,
+						generateRS(generateDeployment("xyz")),
+					},
 				},
 			},
 			&newRC,
 		},
 	}
 
-	ns := api.NamespaceDefault
 	for _, test := range tests {
-		c := &simple.Client{
-			Request: simple.Request{
-				Method: "GET",
-				Path:   testapi.Default.ResourcePath("replicaSets", ns, ""),
-			},
-			Response: simple.Response{
-				StatusCode: 200,
-				Body:       &test.rsList,
-			},
-		}
-		rs, err := GetNewReplicaSet(newDeployment, c.Setup(t).Clientset)
+		fakeClient := &fake.Clientset{}
+		fakeClient = addListPodsReactor(fakeClient, test.objs[0])
+		fakeClient = addListRSReactor(fakeClient, test.objs[1])
+		fakeClient = addUpdatePodsReactor(fakeClient)
+		fakeClient = addUpdateRSReactor(fakeClient)
+		rs, err := GetNewReplicaSet(newDeployment, fakeClient)
 		if err != nil {
 			t.Errorf("In test case %s, got unexpected error %v", test.test, err)
 		}
@@ -313,12 +343,24 @@ func TestGetOldRCs(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		rss, _, err := GetOldReplicaSets(newDeployment, fake.NewSimpleClientset(test.objs...))
+		fakeClient := &fake.Clientset{}
+		fakeClient = addListPodsReactor(fakeClient, test.objs[0])
+		fakeClient = addListRSReactor(fakeClient, test.objs[1])
+		fakeClient = addUpdatePodsReactor(fakeClient)
+		fakeClient = addUpdateRSReactor(fakeClient)
+		rss, _, err := GetOldReplicaSets(newDeployment, fakeClient)
 		if err != nil {
 			t.Errorf("In test case %s, got unexpected error %v", test.test, err)
 		}
 		if !equal(rss, test.expected) {
-			t.Errorf("In test case %q, expected %v, got %v", test.test, test.expected, rss)
+			t.Errorf("In test case %q, expected:", test.test)
+			for _, rs := range test.expected {
+				t.Errorf("rs = %+v", rs)
+			}
+			t.Errorf("In test case %q, got:", test.test)
+			for _, rs := range rss {
+				t.Errorf("rs = %+v", rs)
+			}
 		}
 	}
 }

pkg/util/labels/labels.go

@@ -54,6 +54,19 @@ func CloneAndRemoveLabel(labels map[string]string, labelKey string) map[string]s
 	return newLabels
 }
 
+// AddLabel returns a map with the given key and value added to the given map.
+func AddLabel(labels map[string]string, labelKey string, labelValue string) map[string]string {
+	if labelKey == "" {
+		// Dont need to add a label.
+		return labels
+	}
+	if labels == nil {
+		labels = make(map[string]string)
+	}
+	labels[labelKey] = labelValue
+	return labels
+}
+
 // Clones the given selector and returns a new selector with the given key and value added.
 // Returns the given selector, if labelKey is empty.
 func CloneSelectorAndAddLabel(selector *unversioned.LabelSelector, labelKey string, labelValue uint32) *unversioned.LabelSelector {
@@ -93,3 +106,21 @@ func CloneSelectorAndAddLabel(selector *unversioned.LabelSelector, labelKey stri
 
 	return newSelector
 }
+
+// AddLabelToSelector returns a selector with the given key and value added to the given selector's MatchLabels.
+func AddLabelToSelector(selector *unversioned.LabelSelector, labelKey string, labelValue string) *unversioned.LabelSelector {
+	if labelKey == "" {
+		// Dont need to add a label.
+		return selector
+	}
+	if selector.MatchLabels == nil {
+		selector.MatchLabels = make(map[string]string)
+	}
+	selector.MatchLabels[labelKey] = labelValue
+	return selector
+}
+
+// SelectorHasLabel checks if the given selector contains the given label key in its MatchLabels
+func SelectorHasLabel(selector *unversioned.LabelSelector, labelKey string) bool {
+	return len(selector.MatchLabels[labelKey]) > 0
+}

test/e2e/deployment.go

@@ -66,6 +66,9 @@ var _ = Describe("Deployment", func() {
 	It("[Flaky] deployment should support rollback when there's replica set with no revision", func() {
 		testRollbackDeploymentRSNoRevision(f)
 	})
+	It("deployment should label adopted RSs and pods", func() {
+		testDeploymentLabelAdopted(f)
+	})
 })
 
 func newRS(rsName string, replicas int, rsPodLabels map[string]string, imageName string, image string) *extensions.ReplicaSet {
@@ -272,6 +275,17 @@ func testRollingUpdateDeployment(f *Framework) {
 
 	// Check if it's updated to revision 1 correctly
 	checkDeploymentRevision(c, ns, deploymentName, "1", "redis", "redis")
+
+	// There should be 1 old RS (nginx-controller, which is adopted)
+	deployment, err := c.Extensions().Deployments(ns).Get(deploymentName)
+	Expect(err).NotTo(HaveOccurred())
+	_, allOldRSs, err := deploymentutil.GetOldReplicaSets(*deployment, c)
+	Expect(err).NotTo(HaveOccurred())
+	Expect(len(allOldRSs)).Should(Equal(1))
+	// The old RS should contain pod-template-hash in its selector, label, and template label
+	Expect(len(allOldRSs[0].Labels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
+	Expect(len(allOldRSs[0].Spec.Selector.MatchLabels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
+	Expect(len(allOldRSs[0].Spec.Template.Labels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
 }
 
 func testRollingUpdateDeploymentEvents(f *Framework) {
@@ -796,3 +810,72 @@ func testRollbackDeploymentRSNoRevision(f *Framework) {
 	// Check if it's still revision 3 and still has the old pod template
 	checkDeploymentRevision(c, ns, deploymentName, "3", deploymentImageName, deploymentImage)
 }
+
+func testDeploymentLabelAdopted(f *Framework) {
+	ns := f.Namespace.Name
+	// TODO: remove unversionedClient when the refactoring is done. Currently some
+	// functions like verifyPod still expects a unversioned#Client.
+	unversionedClient := f.Client
+	c := clientset.FromUnversionedClient(unversionedClient)
+	// Create nginx pods.
+	podName := "nginx"
+	podLabels := map[string]string{"name": podName}
+
+	rsName := "nginx-controller"
+	replicas := 3
+	_, err := c.Extensions().ReplicaSets(ns).Create(newRS(rsName, replicas, podLabels, podName, podName))
+	Expect(err).NotTo(HaveOccurred())
+	// Verify that the required pods have come up.
+	err = verifyPods(unversionedClient, ns, podName, false, 3)
+	if err != nil {
+		Logf("error in waiting for pods to come up: %s", err)
+		Expect(err).NotTo(HaveOccurred())
+	}
+
+	// Create a nginx deployment to adopt the old rs.
+	deploymentName := "nginx-deployment"
+	Logf("Creating deployment %s", deploymentName)
+	_, err = c.Extensions().Deployments(ns).Create(newDeployment(deploymentName, replicas, podLabels, podName, podName, extensions.RollingUpdateDeploymentStrategyType, nil))
+	Expect(err).NotTo(HaveOccurred())
+	defer func() {
+		deployment, err := c.Extensions().Deployments(ns).Get(deploymentName)
+		Expect(err).NotTo(HaveOccurred())
+		Logf("deleting deployment %s", deploymentName)
+		Expect(c.Extensions().Deployments(ns).Delete(deploymentName, nil)).NotTo(HaveOccurred())
+		// TODO: remove this once we can delete replica sets with deployment
+		newRS, err := deploymentutil.GetNewReplicaSet(*deployment, c)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(c.Extensions().ReplicaSets(ns).Delete(newRS.Name, nil)).NotTo(HaveOccurred())
+	}()
+
+	// The RS and pods should be relabeled before the status is updated by syncRollingUpdateDeployment
+	err = waitForDeploymentStatus(c, ns, deploymentName, replicas, replicas-1, replicas+1, 0)
+	Expect(err).NotTo(HaveOccurred())
+
+	// Check if it's updated to revision 1 correctly
+	checkDeploymentRevision(c, ns, deploymentName, "1", "nginx", "nginx")
+
+	// 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)
+	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())
+	Expect(len(newRS.Labels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
+	Expect(len(newRS.Spec.Selector.MatchLabels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
+	Expect(len(newRS.Spec.Template.Labels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
+	// All pods targeted by the deployment should contain pod-template-hash in their labels, and there should be only 3 pods
+	selector, err := unversioned.LabelSelectorAsSelector(deployment.Spec.Selector)
+	Expect(err).NotTo(HaveOccurred())
+	options := api.ListOptions{LabelSelector: selector}
+	pods, err := c.Core().Pods(ns).List(options)
+	Expect(err).NotTo(HaveOccurred())
+	for _, pod := range pods.Items {
+		Expect(len(pod.Labels[extensions.DefaultDeploymentUniqueLabelKey])).Should(BeNumerically(">", 0))
+	}
+	Expect(len(pods.Items)).Should(Equal(replicas))
+}