Merge pull request #29971 from caesarxuchao/fix-kubectl-rolling-update-with-gc

Automatic merge from submit-queue

[GarbageCollector] Fix kubectl rolling-update to work with GC

This changes the order of the [Rename()](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/rolling_updater.go#L532) function. After the change, Rename() first deletes the old RC and orphans its pods, then creates the new RC, which will then have a chance to adopt the orphaned pods.

This also fixes the "should support rolling-update to same image" [test](https://github.com/kubernetes/kubernetes/blob/master/test/e2e/kubectl.go#L915) when the garbage collector is on.

Here is the detailed explanation on why the test would have failed:
`kubectl rolling-update` will [rename](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/rolling_updater.go#L532-L546) the RC. It first creates the an identical RC (including spec.selectors) with the new name, then it deletes the existing RC. When GC is turned on, the newly created RC cannot adopt the existing pod, because it has a controllerRef pointing to the exising RC, so the new RC will create new pods and expect to see the creation. However, the new RC and the old RC have the same selector, so sometimes the old RC, instead of the new RC, has its [expectation lowered](https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/replication/replication_controller.go#L346-L362), the new RC's expectation will stuck forever. The e2e test then times out when executing `kubectl delete newRC`, because there is the new RC will not scale down as its expectation is not fulfilled.

A side-note, we should fix [rm.getPodController()](https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/replication/replication_controller.go#L346) to respect pod's controllerref, that will prevent similar bugs.

Also note that an old version `kubectl rolling-update` will not work with the GC. We cannot fix that.

pkg/client/unversioned/replication_controllers.go

@@ -33,7 +33,7 @@ type ReplicationControllerInterface interface {
 	Create(ctrl *api.ReplicationController) (*api.ReplicationController, error)
 	Update(ctrl *api.ReplicationController) (*api.ReplicationController, error)
 	UpdateStatus(ctrl *api.ReplicationController) (*api.ReplicationController, error)
-	Delete(name string) error
+	Delete(name string, options *api.DeleteOptions) error
 	Watch(opts api.ListOptions) (watch.Interface, error)
 }
 
@@ -84,8 +84,8 @@ func (c *replicationControllers) UpdateStatus(controller *api.ReplicationControl
 }
 
 // Delete deletes an existing replication controller.
-func (c *replicationControllers) Delete(name string) error {
-	return c.r.Delete().Namespace(c.ns).Resource("replicationControllers").Name(name).Do().Error()
+func (c *replicationControllers) Delete(name string, options *api.DeleteOptions) error {
+	return c.r.Delete().Namespace(c.ns).Resource("replicationControllers").Name(name).Body(options).Do().Error()
 }
 
 // Watch returns a watch.Interface that watches the requested controllers.

pkg/client/unversioned/replication_controllers_test.go

@@ -165,7 +165,7 @@ func TestDeleteController(t *testing.T) {
 		Request:  simple.Request{Method: "DELETE", Path: testapi.Default.ResourcePath(getRCResourceName(), ns, "foo"), Query: simple.BuildQueryValues(nil)},
 		Response: simple.Response{StatusCode: 200},
 	}
-	err := c.Setup(t).ReplicationControllers(ns).Delete("foo")
+	err := c.Setup(t).ReplicationControllers(ns).Delete("foo", nil)
 	defer c.Close()
 	c.Validate(t, nil, err)
 }

pkg/client/unversioned/testclient/fake_replication_controllers.go

@@ -72,7 +72,7 @@ func (c *FakeReplicationControllers) UpdateStatus(controller *api.ReplicationCon
 	return obj.(*api.ReplicationController), err
 }
 
-func (c *FakeReplicationControllers) Delete(name string) error {
+func (c *FakeReplicationControllers) Delete(name string, options *api.DeleteOptions) error {
 	_, err := c.Fake.Invokes(NewDeleteAction("replicationcontrollers", c.Namespace, name), &api.ReplicationController{})
 	return err
 }

pkg/kubectl/rolling_updater.go

@@ -513,11 +513,11 @@ func (r *RollingUpdater) cleanupWithClients(oldRc, newRc *api.ReplicationControl
 	case DeleteRollingUpdateCleanupPolicy:
 		// delete old rc
 		fmt.Fprintf(config.Out, "Update succeeded. Deleting %s\n", oldRc.Name)
-		return r.c.ReplicationControllers(r.ns).Delete(oldRc.Name)
+		return r.c.ReplicationControllers(r.ns).Delete(oldRc.Name, nil)
 	case RenameRollingUpdateCleanupPolicy:
 		// delete old rc
 		fmt.Fprintf(config.Out, "Update succeeded. Deleting old controller: %s\n", oldRc.Name)
-		if err := r.c.ReplicationControllers(r.ns).Delete(oldRc.Name); err != nil {
+		if err := r.c.ReplicationControllers(r.ns).Delete(oldRc.Name, nil); err != nil {
 			return err
 		}
 		fmt.Fprintf(config.Out, "Renaming %s to %s\n", newRc.Name, oldRc.Name)
@@ -533,13 +533,28 @@ func Rename(c client.ReplicationControllersNamespacer, rc *api.ReplicationContro
 	oldName := rc.Name
 	rc.Name = newName
 	rc.ResourceVersion = ""
-
-	_, err := c.ReplicationControllers(rc.Namespace).Create(rc)
+	// First delete the oldName RC and orphan its pods.
+	trueVar := true
+	err := c.ReplicationControllers(rc.Namespace).Delete(oldName, &api.DeleteOptions{OrphanDependents: &trueVar})
+	if err != nil && !errors.IsNotFound(err) {
+		return err
+	}
+	err = wait.Poll(5*time.Second, 60*time.Second, func() (bool, error) {
+		_, err := c.ReplicationControllers(rc.Namespace).Get(oldName)
+		if err == nil {
+			return false, nil
+		} else if errors.IsNotFound(err) {
+			return true, nil
+		} else {
+			return false, err
+		}
+	})
 	if err != nil {
 		return err
 	}
-	err = c.ReplicationControllers(rc.Namespace).Delete(oldName)
-	if err != nil && !errors.IsNotFound(err) {
+	// Then create the same RC with the new name.
+	_, err = c.ReplicationControllers(rc.Namespace).Create(rc)
+	if err != nil {
 		return err
 	}
 	return nil

pkg/kubectl/rolling_updater_test.go

@@ -27,6 +27,7 @@ import (
 	"time"
 
 	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/api/testapi"
 	apitesting "k8s.io/kubernetes/pkg/api/testing"
 	"k8s.io/kubernetes/pkg/api/unversioned"
@@ -1143,20 +1144,27 @@ func TestRollingUpdater_cleanupWithClients(t *testing.T) {
 				"delete",
 			},
 		},
-		{
-			name:      "rename",
-			policy:    RenameRollingUpdateCleanupPolicy,
-			responses: []runtime.Object{rcExisting},
-			expected: []string{
-				"get",
-				"update",
-				"get",
-				"get",
-				"delete",
-				"create",
-				"delete",
-			},
-		},
+		//{
+		// This cases is separated to a standalone
+		// TestRollingUpdater_cleanupWithClients_Rename. We have to do this
+		// because the unversioned fake client is unable to delete objects.
+		// TODO: uncomment this case when the unversioned fake client uses
+		// pkg/client/testing/core.
+		//	{
+		//		name:      "rename",
+		//		policy:    RenameRollingUpdateCleanupPolicy,
+		//		responses: []runtime.Object{rcExisting},
+		//		expected: []string{
+		//			"get",
+		//			"update",
+		//			"get",
+		//			"get",
+		//			"delete",
+		//			"create",
+		//			"delete",
+		//		},
+		//	},
+		//},
 	}
 
 	for _, test := range tests {
@@ -1189,6 +1197,39 @@ func TestRollingUpdater_cleanupWithClients(t *testing.T) {
 	}
 }
 
+// TestRollingUpdater_cleanupWithClients_Rename tests the rename cleanup policy. It's separated to
+// a standalone test because the unversioned fake client is unable to delete
+// objects.
+// TODO: move this test back to TestRollingUpdater_cleanupWithClients
+// when the fake client uses pkg/client/testing/core in the future.
+func TestRollingUpdater_cleanupWithClients_Rename(t *testing.T) {
+	rc := oldRc(2, 2)
+	rcExisting := newRc(1, 3)
+	expectedActions := []string{"delete", "get", "create"}
+	fake := &testclient.Fake{}
+	fake.AddReactor("*", "*", func(action testclient.Action) (handled bool, ret runtime.Object, err error) {
+		switch action.(type) {
+		case testclient.CreateAction:
+			return true, nil, nil
+		case testclient.GetAction:
+			return true, nil, errors.NewNotFound(unversioned.GroupResource{}, "")
+		case testclient.DeleteAction:
+			return true, nil, nil
+		}
+		return false, nil, nil
+	})
+
+	err := Rename(fake, rcExisting, rc.Name)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for j, action := range fake.Actions() {
+		if e, a := expectedActions[j], action.GetVerb(); e != a {
+			t.Errorf("unexpected action: expected %s, got %s", e, a)
+		}
+	}
+}
+
 func TestFindSourceController(t *testing.T) {
 	ctrl1 := api.ReplicationController{
 		ObjectMeta: api.ObjectMeta{

pkg/kubectl/stop.go

@@ -202,7 +202,7 @@ func (reaper *ReplicationControllerReaper) Stop(namespace, name string, timeout
 			return err
 		}
 	}
-	return rc.Delete(name)
+	return rc.Delete(name, nil)
 }
 
 // TODO(madhusudancs): Implement it when controllerRef is implemented - https://github.com/kubernetes/kubernetes/issues/2210

test/e2e/density.go

@@ -659,7 +659,7 @@ var _ = framework.KubeDescribe("Density", func() {
 			By("Removing additional replication controllers if any")
 			for i := 1; i <= nodeCount; i++ {
 				name := additionalPodsPrefix + "-" + strconv.Itoa(i)
-				c.ReplicationControllers(ns).Delete(name)
+				c.ReplicationControllers(ns).Delete(name, nil)
 			}
 		})
 	}

test/e2e/resource_quota.go

@@ -258,7 +258,7 @@ var _ = framework.KubeDescribe("ResourceQuota", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Deleting a ReplicationController")
-		err = f.Client.ReplicationControllers(f.Namespace.Name).Delete(replicationController.Name)
+		err = f.Client.ReplicationControllers(f.Namespace.Name).Delete(replicationController.Name, nil)
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Ensuring resource quota status released usage")

test/e2e/service.go

@@ -1976,7 +1976,7 @@ func (t *ServiceTestFixture) Cleanup() []error {
 		// TODO(mikedanese): Wait.
 
 		// Then, delete the RC altogether.
-		if err := t.Client.ReplicationControllers(t.Namespace).Delete(rcName); err != nil {
+		if err := t.Client.ReplicationControllers(t.Namespace).Delete(rcName, nil); err != nil {
 			errs = append(errs, err)
 		}
 	}

test/e2e/serviceloadbalancers.go

@@ -136,7 +136,7 @@ func (h *haproxyControllerTester) start(namespace string) (err error) {
 }
 
 func (h *haproxyControllerTester) stop() error {
-	return h.client.ReplicationControllers(h.rcNamespace).Delete(h.rcName)
+	return h.client.ReplicationControllers(h.rcNamespace).Delete(h.rcName, nil)
 }
 
 func (h *haproxyControllerTester) lookup(ingressKey string) string {

test/integration/framework/master_utils.go

@@ -351,7 +351,7 @@ func StartPods(namespace string, numPods int, host string, restClient *client.Cl
 	} else {
 		// Delete the rc, otherwise when we restart master components for the next benchmark
 		// the rc controller will race with the pods controller in the rc manager.
-		return restClient.ReplicationControllers(namespace).Delete(rc.Name)
+		return restClient.ReplicationControllers(namespace).Delete(rc.Name, nil)
 	}
 }