Merge pull request #51279 from kow3ns/daemonset-respects-termination

Automatic merge from submit-queue (batch tested with PRs 51628, 51637, 51490, 51279, 51302)

Ensure that DaemonSet respects termination

**What this PR does / why we need it**:
#43077 correctly prevents the DaemonSet controller from adopting deleted Pods, but, as pointed out in #50477, the controller now has no sensitivity to the termination lifecycle (i.e TerminationGracePeriodSeconds) of the Pods it creates. This PR attempts to balance the two. DaemonSet controller will now consider deleted Pods owned by a DaemonSet during creation, but it will not consider deleted Pods as targets for adoption.

fixes #50477

```release-note
#43077 introduced a condition where DaemonSet controller did not respect the TerminationGracePeriodSeconds of the Pods it created. This is now corrected.
```

pkg/controller/daemon/daemon_controller.go

@@ -743,7 +743,7 @@ func (dsc *DaemonSetsController) getDaemonPods(ds *extensions.DaemonSet) ([]*v1.
 	}
 	// If any adoptions are attempted, we should first recheck for deletion with
 	// an uncached quorum read sometime after listing Pods (see #42639).
-	canAdoptFunc := controller.RecheckDeletionTimestamp(func() (metav1.Object, error) {
+	dsNotDeleted := controller.RecheckDeletionTimestamp(func() (metav1.Object, error) {
 		fresh, err := dsc.kubeClient.ExtensionsV1beta1().DaemonSets(ds.Namespace).Get(ds.Name, metav1.GetOptions{})
 		if err != nil {
 			return nil, err
@@ -753,8 +753,9 @@ func (dsc *DaemonSetsController) getDaemonPods(ds *extensions.DaemonSet) ([]*v1.
 		}
 		return fresh, nil
 	})
+
 	// Use ControllerRefManager to adopt/orphan as needed.
-	cm := controller.NewPodControllerRefManager(dsc.podControl, ds, selector, controllerKind, canAdoptFunc)
+	cm := controller.NewPodControllerRefManager(dsc.podControl, ds, selector, controllerKind, dsNotDeleted)
 	return cm.ClaimPods(pods)
 }
 
@@ -770,10 +771,6 @@ func (dsc *DaemonSetsController) getNodesToDaemonPods(ds *extensions.DaemonSet)
 	// Group Pods by Node name.
 	nodeToDaemonPods := make(map[string][]*v1.Pod)
 	for _, pod := range claimedPods {
-		// Skip terminating pods
-		if pod.DeletionTimestamp != nil {
-			continue
-		}
 		nodeName := pod.Spec.NodeName
 		nodeToDaemonPods[nodeName] = append(nodeToDaemonPods[nodeName], pod)
 	}
@@ -838,6 +835,9 @@ func (dsc *DaemonSetsController) manage(ds *extensions.DaemonSet, hash string) e
 			// If there's non-daemon pods left on this node, we will create it in the next sync loop
 			var daemonPodsRunning []*v1.Pod
 			for _, pod := range daemonPods {
+				if pod.DeletionTimestamp != nil {
+					continue
+				}
 				if pod.Status.Phase == v1.PodFailed {
 					msg := fmt.Sprintf("Found failed daemon pod %s/%s on node %s, will try to kill it", pod.Namespace, node.Name, pod.Name)
 					glog.V(2).Infof(msg)

pkg/controller/daemon/daemon_controller_test.go

@@ -1213,15 +1213,30 @@ func TestNodeDaemonLaunchesToleratePod(t *testing.T) {
 		ds.Spec.UpdateStrategy = *strategy
 		setDaemonSetToleration(ds, noScheduleTolerations)
 		manager, podControl, _ := newTestController(ds)
-
+		addNodes(manager.nodeStore, 0, 1, nil)
-		node := newNode("untainted", nil)
-		manager.nodeStore.Add(node)
 		manager.dsStore.Add(ds)
 
 		syncAndValidateDaemonSets(t, manager, ds, podControl, 1, 0, 0)
 	}
 }
 
+// DaemonSet should launch a pod on a not ready node with taint notReady:NoExecute.
+func TestDaemonSetRespectsTermination(t *testing.T) {
+	for _, strategy := range updateStrategies() {
+		ds := newDaemonSet("foo")
+		ds.Spec.UpdateStrategy = *strategy
+		manager, podControl, _ := newTestController(ds)
+
+		addNodes(manager.nodeStore, 0, 1, simpleNodeLabel)
+		pod := newPod(fmt.Sprintf("%s-", "node-0"), "node-0", simpleDaemonSetLabel, ds)
+		dt := metav1.Now()
+		pod.DeletionTimestamp = &dt
+		manager.podStore.Add(pod)
+		manager.dsStore.Add(ds)
+		syncAndValidateDaemonSets(t, manager, ds, podControl, 0, 0, 0)
+	}
+}
+
 func setNodeTaint(node *v1.Node, taints []v1.Taint) {
 	node.Spec.Taints = taints
 }
@@ -1837,12 +1852,6 @@ func TestGetNodesToDaemonPods(t *testing.T) {
 			newPod("non-matching-owned-0-", "node-0", simpleDaemonSetLabel2, ds),
 			newPod("non-matching-orphan-1-", "node-1", simpleDaemonSetLabel2, nil),
 			newPod("matching-owned-by-other-0-", "node-0", simpleDaemonSetLabel, ds2),
-			func() *v1.Pod {
-				pod := newPod("matching-owned-2-but-set-for-deletion", "node-2", simpleDaemonSetLabel, ds)
-				now := metav1.Now()
-				pod.DeletionTimestamp = &now
-				return pod
-			}(),
 		}
 		for _, pod := range ignoredPods {
 			manager.podStore.Add(pod)

pkg/controller/daemon/update.go

@@ -408,7 +408,8 @@ func (dsc *DaemonSetsController) getUnavailableNumbers(ds *extensions.DaemonSet,
 		}
 		available := false
 		for _, pod := range daemonPods {
-			if podutil.IsPodAvailable(pod, ds.Spec.MinReadySeconds, metav1.Now()) {
+			//for the purposes of update we ensure that the Pod is both available and not terminating
+			if podutil.IsPodAvailable(pod, ds.Spec.MinReadySeconds, metav1.Now()) && pod.DeletionTimestamp == nil {
 				available = true
 				break
 			}

pkg/controller/daemon/update_test.go

@@ -21,6 +21,7 @@ import (
 
 	"k8s.io/api/core/v1"
 	extensions "k8s.io/api/extensions/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
@@ -236,6 +237,34 @@ func TestGetUnavailableNumbers(t *testing.T) {
 			maxUnavailable: 1,
 			numUnavailable: 0,
 		},
+		{
+			name: "Two nodes with pods, MaxUnavailable in percents, pod terminating",
+			Manager: func() *daemonSetsController {
+				manager, _, _ := newTestController()
+				addNodes(manager.nodeStore, 0, 2, nil)
+				return manager
+			}(),
+			ds: func() *extensions.DaemonSet {
+				ds := newDaemonSet("x")
+				intStr := intstr.FromString("50%")
+				ds.Spec.UpdateStrategy.RollingUpdate = &extensions.RollingUpdateDaemonSet{MaxUnavailable: &intStr}
+				return ds
+			}(),
+			nodeToPods: func() map[string][]*v1.Pod {
+				mapping := make(map[string][]*v1.Pod)
+				pod0 := newPod("pod-0", "node-0", simpleDaemonSetLabel, nil)
+				pod1 := newPod("pod-1", "node-1", simpleDaemonSetLabel, nil)
+				now := metav1.Now()
+				markPodReady(pod0)
+				markPodReady(pod1)
+				pod1.DeletionTimestamp = &now
+				mapping["node-0"] = []*v1.Pod{pod0}
+				mapping["node-1"] = []*v1.Pod{pod1}
+				return mapping
+			}(),
+			maxUnavailable: 1,
+			numUnavailable: 1,
+		},
 	}
 
 	for _, c := range cases {