Merge pull request #64797 from janetkuo/ds-deletion

Automatic merge from submit-queue (batch tested with PRs 64749, 64797). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Handle deleted DaemonSet properly

**What this PR does / why we need it**:
After kubectl reapers are removed (#63979) and foreground deletion are used, DaemonSet controller may race with garbage collector when it tries to update DaemonSet status of the DaemonSet being deleted. 

Here's what happened:
1. Someone/something performs a foreground deletion on a DaemonSet
1. DaemonSet finalizer and DeletionTimestamp are both set
1. DaemonSet history objects (ControllerRevisions) and pods are being deleted by garbage collector; meanwhile, DaemonSet controller tries to update DaemonSet status. 
    * Updating DaemonSet status requires constructing DaemonSet history objects, to figure out current revision and which pods do/don't belong to current revision
1. When updating DaemonSet status, DaemonSet controller tries to create a DaemonSet history object that matches current DaemonSet spec
1. Garbage collector then tries to delete that DaemonSet history object. And repeat. 

Because we can't make DaemonSet pods be deleted before DaemonSet history objects (DaemonSet history objects don't own DaemonSet pods!), we cannot reliably calculate DaemonSet status without history objects anyways. Therefore, we don't update DaemonSet status for DaemonSet being deleted. 

Note that the reason why the kubectl delete hack works is because it forces DaemonSet pods to be removed before history objects. 

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #64313

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```

pkg/controller/daemon/daemon_controller.go

@@ -1176,6 +1176,18 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error {
 		return fmt.Errorf("couldn't get key for object %#v: %v", ds, err)
 	}
 
+	// If the DaemonSet is being deleted (either by foreground deletion or
+	// orphan deletion), we cannot be sure if the DaemonSet history objects
+	// it owned still exist -- those history objects can either be deleted
+	// or orphaned. Garbage collector doesn't guarantee that it will delete
+	// DaemonSet pods before deleting DaemonSet history objects, because
+	// DaemonSet history doesn't own DaemonSet pods. We cannot reliably
+	// calculate the status of a DaemonSet being deleted. Therefore, return
+	// here without updating status for the DaemonSet being deleted.
+	if ds.DeletionTimestamp != nil {
+		return nil
+	}
+
 	// Construct histories of the DaemonSet, and get the hash of current history
 	cur, old, err := dsc.constructHistory(ds)
 	if err != nil {
@@ -1183,7 +1195,7 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error {
 	}
 	hash := cur.Labels[apps.DefaultDaemonSetUniqueLabelKey]
 
-	if ds.DeletionTimestamp != nil || !dsc.expectations.SatisfiedExpectations(dsKey) {
+	if !dsc.expectations.SatisfiedExpectations(dsKey) {
 		// Only update status.
 		return dsc.updateDaemonSetStatus(ds, hash)
 	}

pkg/kubectl/cmd/BUILD

@@ -105,7 +105,6 @@ go_library(
         "//vendor/github.com/renstrom/dedent:go_default_library",
         "//vendor/github.com/spf13/cobra:go_default_library",
         "//vendor/github.com/spf13/pflag:go_default_library",
-        "//vendor/k8s.io/api/apps/v1:go_default_library",
         "//vendor/k8s.io/api/autoscaling/v1:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/api/policy/v1beta1:go_default_library",
@@ -127,7 +126,6 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/util/net:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/strategicpatch:go_default_library",
-        "//vendor/k8s.io/apimachinery/pkg/util/uuid:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/yaml:go_default_library",

pkg/kubectl/cmd/delete.go

@@ -24,15 +24,9 @@ import (
 	"github.com/golang/glog"
 	"github.com/spf13/cobra"
 
-	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/util/uuid"
-	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
@@ -291,16 +285,6 @@ func (o *DeleteOptions) DeleteResult(r *resource.Result) error {
 }
 
 func (o *DeleteOptions) deleteResource(info *resource.Info, deleteOptions *metav1.DeleteOptions) error {
-	// TODO: this should be removed as soon as DaemonSet controller properly handles object deletion
-	// see https://github.com/kubernetes/kubernetes/issues/64313 for details
-	mapping := info.ResourceMapping()
-	if mapping.Resource.GroupResource() == (schema.GroupResource{Group: "extensions", Resource: "daemonsets"}) ||
-		mapping.Resource.GroupResource() == (schema.GroupResource{Group: "apps", Resource: "daemonsets"}) {
-		if err := updateDaemonSet(info.Namespace, info.Name, o.DynamicClient); err != nil {
-			return err
-		}
-	}
-
 	if err := resource.NewHelper(info.Client, info.Mapping).DeleteWithOptions(info.Namespace, info.Name, deleteOptions); err != nil {
 		return cmdutil.AddSourceToErr("deleting", info.Source, err)
 	}
@@ -309,53 +293,6 @@ func (o *DeleteOptions) deleteResource(info *resource.Info, deleteOptions *metav
 	return nil
 }
 
-func updateDaemonSet(namespace, name string, dynamicClient dynamic.Interface) error {
-	dsClient := dynamicClient.Resource(schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "daemonsets"}).Namespace(namespace)
-	obj, err := dsClient.Get(name, metav1.GetOptions{})
-	if err != nil {
-		return err
-	}
-	ds := &appsv1.DaemonSet{}
-	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, ds); err != nil {
-		return err
-	}
-
-	// We set the nodeSelector to a random label. This label is nearly guaranteed
-	// to not be set on any node so the DameonSetController will start deleting
-	// daemon pods. Once it's done deleting the daemon pods, it's safe to delete
-	// the DaemonSet.
-	ds.Spec.Template.Spec.NodeSelector = map[string]string{
-		string(uuid.NewUUID()): string(uuid.NewUUID()),
-	}
-	// force update to avoid version conflict
-	ds.ResourceVersion = ""
-
-	out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(ds)
-	if err != nil {
-		return err
-	}
-	if _, err = dsClient.Update(&unstructured.Unstructured{Object: out}); err != nil {
-		return err
-	}
-
-	// Wait for the daemon set controller to kill all the daemon pods.
-	if err := wait.Poll(1*time.Second, 5*time.Minute, func() (bool, error) {
-		updatedObj, err := dsClient.Get(name, metav1.GetOptions{})
-		if err != nil {
-			return false, nil
-		}
-		updatedDS := &appsv1.DaemonSet{}
-		if err := runtime.DefaultUnstructuredConverter.FromUnstructured(updatedObj.Object, ds); err != nil {
-			return false, nil
-		}
-
-		return updatedDS.Status.CurrentNumberScheduled+updatedDS.Status.NumberMisscheduled == 0, nil
-	}); err != nil {
-		return err
-	}
-	return nil
-}
-
 // deletion printing is special because we do not have an object to print.
 // This mirrors name printer behavior
 func (o *DeleteOptions) PrintObj(info *resource.Info) {