Fix DaemonSet pods circling in Failed/Pending during disruption

Author: moko-poi

pkg/controllers/disruption/suite_test.go

@@ -1386,7 +1386,7 @@ var _ = Describe("Candidate Filtering", func() {
 		Expect(err.Error()).To(Equal(fmt.Sprintf(`pdb prevents pod evictions (PodDisruptionBudget=[%s])`, client.ObjectKeyFromObject(budget))))
 		Expect(recorder.DetectedEvent(fmt.Sprintf(`Pdb prevents pod evictions (PodDisruptionBudget=[%s])`, client.ObjectKeyFromObject(budget)))).To(BeTrue())
 	})
-	It("should not consider candidates that have fully blocking PDBs on daemonset pods", func() {
+	It("should consider candidates that have fully blocking PDBs on daemonset pods since daemonset pods are not evicted", func() {
 		daemonSet := test.DaemonSet()
 		nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{
 			ObjectMeta: metav1.ObjectMeta{
@@ -1428,10 +1428,55 @@ var _ = Describe("Candidate Filtering", func() {
 		Expect(err).ToNot(HaveOccurred())
 
 		Expect(cluster.DeepCopyNodes()).To(HaveLen(1))
-		_, err = disruption.NewCandidate(ctx, env.Client, recorder, fakeClock, cluster.DeepCopyNodes()[0], pdbLimits, nodePoolMap, nodePoolInstanceTypeMap, queue, disruption.GracefulDisruptionClass)
-		Expect(err).To(HaveOccurred())
-		Expect(err.Error()).To(Equal(fmt.Sprintf(`pdb prevents pod evictions (PodDisruptionBudget=[%s])`, client.ObjectKeyFromObject(budget))))
-		Expect(recorder.DetectedEvent(fmt.Sprintf(`Pdb prevents pod evictions (PodDisruptionBudget=[%s])`, client.ObjectKeyFromObject(budget)))).To(BeTrue())
+		// DaemonSet pods are not evicted during disruption, so PDB does not block the node from being a candidate
+		c, err := disruption.NewCandidate(ctx, env.Client, recorder, fakeClock, cluster.DeepCopyNodes()[0], pdbLimits, nodePoolMap, nodePoolInstanceTypeMap, queue, disruption.GracefulDisruptionClass)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(c).ToNot(BeNil())
+		Expect(c.NodeClaim.Name).To(Equal(nodeClaim.Name))
+	})
+	It("should consider candidates with only daemonset pods as these pods should not be evicted during disruption", func() {
+		daemonSet := test.DaemonSet()
+		nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					v1.NodePoolLabelKey:            nodePool.Name,
+					corev1.LabelInstanceTypeStable: mostExpensiveInstance.Name,
+					v1.CapacityTypeLabelKey:        mostExpensiveOffering.Requirements.Get(v1.CapacityTypeLabelKey).Any(),
+					corev1.LabelTopologyZone:       mostExpensiveOffering.Requirements.Get(corev1.LabelTopologyZone).Any(),
+				},
+			},
+		})
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node, daemonSet)
+		pod := test.Pod(test.PodOptions{
+			ObjectMeta: metav1.ObjectMeta{
+				OwnerReferences: []metav1.OwnerReference{
+					{
+						APIVersion: "apps/v1",
+						Kind:       "DaemonSet",
+						Name:       daemonSet.Name,
+						UID:        daemonSet.UID,
+						Controller: lo.ToPtr(true),
+					},
+				},
+			},
+		})
+		ExpectApplied(ctx, env.Client, pod)
+		ExpectManualBinding(ctx, env.Client, pod, node)
+
+		ExpectMakeNodesAndNodeClaimsInitializedAndStateUpdated(ctx, env.Client, nodeStateController, nodeClaimStateController, []*corev1.Node{node}, []*v1.NodeClaim{nodeClaim})
+
+		var err error
+		pdbLimits, err = pdb.NewLimits(ctx, env.Client)
+		Expect(err).ToNot(HaveOccurred())
+
+		Expect(cluster.DeepCopyNodes()).To(HaveLen(1))
+		c, err := disruption.NewCandidate(ctx, env.Client, recorder, fakeClock, cluster.DeepCopyNodes()[0], pdbLimits, nodePoolMap, nodePoolInstanceTypeMap, queue, disruption.GracefulDisruptionClass)
+		// A candidate with only DaemonSet pods should be created successfully
+		// DaemonSet pods are not considered reschedulable, so the node should be treated similarly to an empty node
+		Expect(err).ToNot(HaveOccurred())
+		Expect(c).ToNot(BeNil())
+		Expect(c.NodeClaim.Name).To(Equal(nodeClaim.Name))
+		Expect(c.Node.Name).To(Equal(node.Name))
 	})
 	It("should consider candidates that have fully blocking PDBs on mirror pods", func() {
 		nodeClaim, node := test.NodeClaimAndNode(v1.NodeClaim{

pkg/controllers/node/termination/suite_test.go

@@ -469,6 +469,54 @@ var _ = Describe("Termination", func() {
 			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
 			ExpectNotFound(ctx, env.Client, node)
 		})
+		It("should not evict daemonset pods during node disruption", func() {
+			daemonSet := test.DaemonSet()
+			ExpectApplied(ctx, env.Client, daemonSet)
+
+			regularPod := test.Pod(test.PodOptions{NodeName: node.Name, ObjectMeta: metav1.ObjectMeta{OwnerReferences: defaultOwnerRefs}})
+			daemonSetPod := test.Pod(test.PodOptions{NodeName: node.Name, ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{
+				APIVersion:         "apps/v1",
+				Kind:               "DaemonSet",
+				Name:               daemonSet.Name,
+				UID:                daemonSet.UID,
+				Controller:         lo.ToPtr(true),
+				BlockOwnerDeletion: lo.ToPtr(true),
+			}}}})
+
+			ExpectApplied(ctx, env.Client, node, nodeClaim, regularPod, daemonSetPod)
+
+			// Add disrupted taint to node to simulate disruption
+			node.Spec.Taints = append(node.Spec.Taints, v1.DisruptedNoScheduleTaint)
+			ExpectApplied(ctx, env.Client, node)
+
+			// Trigger Termination Controller
+			Expect(env.Client.Delete(ctx, node)).To(Succeed())
+			node = ExpectNodeExists(ctx, env.Client, node.Name)
+
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // DrainInitiation
+			// Only the regular pod should be added to the eviction queue
+			ExpectObjectReconciled(ctx, env.Client, queue, regularPod)
+
+			// DaemonSet pod should NOT be in the eviction queue
+			Expect(queue.Has(daemonSetPod)).To(BeFalse())
+
+			// Expect node to exist and be draining
+			ExpectNodeWithNodeClaimDraining(env.Client, node.Name)
+
+			// Only regular pod should be terminating
+			EventuallyExpectTerminating(ctx, env.Client, regularPod)
+			ExpectDeleted(ctx, env.Client, regularPod)
+
+			// DaemonSet pod should still be running (not terminating)
+			pod := ExpectPodExists(ctx, env.Client, daemonSetPod.Name, daemonSetPod.Namespace)
+			Expect(pod.DeletionTimestamp).To(BeNil())
+
+			// Reconcile to delete node - node should be deleted even with DaemonSet pod still running
+			node = ExpectNodeExists(ctx, env.Client, node.Name)
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // DrainValidation, VolumeDetachment, InstanceTerminationInitiation
+			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
+			ExpectNotFound(ctx, env.Client, node)
+		})
 		It("should evict non-critical pods first", func() {
 			podEvict := test.Pod(test.PodOptions{NodeName: node.Name, ObjectMeta: metav1.ObjectMeta{OwnerReferences: defaultOwnerRefs}})
 			podNodeCritical := test.Pod(test.PodOptions{NodeName: node.Name, PriorityClassName: "system-node-critical", ObjectMeta: metav1.ObjectMeta{OwnerReferences: defaultOwnerRefs}})

pkg/utils/pod/scheduling.go

@@ -52,11 +52,13 @@ func IsReschedulable(pod *corev1.Pod) bool {
 // - Is an active pod (isn't terminal or actively terminating)
 // - Doesn't tolerate the "karpenter.sh/disruption=disrupting" taint
 // - Isn't a mirror pod (https://kubernetes.io/docs/tasks/configure-pod-container/static-pod/)
+// - Isn't owned by a DaemonSet (DaemonSet pods will be handled by DaemonSet controller)
 // - Does not have the "karpenter.sh/do-not-disrupt=true" annotation (https://karpenter.sh/docs/concepts/disruption/#pod-level-controls)
 func IsEvictable(pod *corev1.Pod) bool {
 	return IsActive(pod) &&
 		!ToleratesDisruptedNoScheduleTaint(pod) &&
 		!IsOwnedByNode(pod) &&
+		!IsOwnedByDaemonSet(pod) &&
 		!HasDoNotDisrupt(pod)
 }
 
@@ -72,14 +74,18 @@ func IsWaitingEviction(pod *corev1.Pod, clk clock.Clock) bool {
 // - Doesn't tolerate the "karpenter.sh/disruption=disrupting" taint
 // - Isn't a pod that has been terminating past its terminationGracePeriodSeconds
 // - Isn't a mirror pod (https://kubernetes.io/docs/tasks/configure-pod-container/static-pod/)
+// - Isn't owned by a DaemonSet (DaemonSet pods will be handled by DaemonSet controller)
 // Note: pods with the `karpenter.sh/do-not-disrupt` annotation are included since node drain should stall until these pods are evicted or become terminal, even though Karpenter won't orchestrate the eviction.
 func IsDrainable(pod *corev1.Pod, clk clock.Clock) bool {
 	return !ToleratesDisruptedNoScheduleTaint(pod) &&
 		!IsStuckTerminating(pod, clk) &&
 		// Mirror pods cannot be deleted through the API server since they are created and managed by kubelet
 		// This means they are effectively read-only and can't be controlled by API server calls
 		// https://kubernetes.io/docs/reference/generated/kubectl/kubectl-commands#drain
-		!IsOwnedByNode(pod)
+		!IsOwnedByNode(pod) &&
+		// DaemonSet pods should not be drained during disruption as they will be automatically
+		// managed by the DaemonSet controller and recreated on other nodes when needed
+		!IsOwnedByDaemonSet(pod)
 }
 
 // IsPodEligibleForForcedEviction checks if a pod needs to be deleted with a reduced grace period ensuring that the pod: