chore: Update pod metrics when pod is completed (kubernetes-sigs#2259)

Author: jigisha620

pkg/controllers/metrics/pod/controller.go

@@ -39,6 +39,7 @@ import (
 	"sigs.k8s.io/karpenter/pkg/controllers/state"
 	"sigs.k8s.io/karpenter/pkg/metrics"
 	"sigs.k8s.io/karpenter/pkg/operator/injection"
+	podutils "sigs.k8s.io/karpenter/pkg/utils/pod"
 )
 
 const (
@@ -304,28 +305,11 @@ func (c *Controller) recordPodStartupMetric(pod *corev1.Pod, schedulableTime tim
 		c.pendingPods.Insert(key)
 		return
 	}
-	cond, ok := lo.Find(pod.Status.Conditions, func(c corev1.PodCondition) bool {
-		return c.Type == corev1.PodReady
+	cond, ready := lo.Find(pod.Status.Conditions, func(c corev1.PodCondition) bool {
+		return c.Type == corev1.PodReady && c.Status == corev1.ConditionTrue
 	})
 	if c.pendingPods.Has(key) {
-		if !ok || cond.Status != corev1.ConditionTrue {
-			PodUnstartedTimeSeconds.Set(time.Since(pod.CreationTimestamp.Time).Seconds(), map[string]string{
-				podName:      pod.Name,
-				podNamespace: pod.Namespace,
-			})
-			if !schedulableTime.IsZero() {
-				PodProvisioningUnstartedTimeSeconds.Set(time.Since(schedulableTime).Seconds(), map[string]string{
-					podName:      pod.Name,
-					podNamespace: pod.Namespace,
-				})
-			} else {
-				// Idempotently delete the unstarted_time_seconds metric if the schedulable time is zero
-				PodProvisioningUnstartedTimeSeconds.Delete(map[string]string{
-					podName:      pod.Name,
-					podNamespace: pod.Namespace,
-				})
-			}
-		} else {
+		if ready || podutils.IsTerminal(pod) {
 			// Delete the unstarted metric since the pod is now started
 			PodUnstartedTimeSeconds.Delete(map[string]string{
 				podName:      pod.Name,
@@ -342,6 +326,23 @@ func (c *Controller) recordPodStartupMetric(pod *corev1.Pod, schedulableTime tim
 			c.pendingPods.Delete(key)
 			// Clear cluster state's representation of these pods as we don't need to keep track of them anymore
 			c.cluster.ClearPodSchedulingMappings(client.ObjectKeyFromObject(pod))
+		} else {
+			PodUnstartedTimeSeconds.Set(time.Since(pod.CreationTimestamp.Time).Seconds(), map[string]string{
+				podName:      pod.Name,
+				podNamespace: pod.Namespace,
+			})
+			if !schedulableTime.IsZero() {
+				PodProvisioningUnstartedTimeSeconds.Set(time.Since(schedulableTime).Seconds(), map[string]string{
+					podName:      pod.Name,
+					podNamespace: pod.Namespace,
+				})
+			} else {
+				// Idempotently delete the unstarted_time_seconds metric if the schedulable time is zero
+				PodProvisioningUnstartedTimeSeconds.Delete(map[string]string{
+					podName:      pod.Name,
+					podNamespace: pod.Namespace,
+				})
+			}
 		}
 	}
 }

pkg/controllers/metrics/pod/suite_test.go

@@ -248,6 +248,92 @@ var _ = Describe("Pod Metrics", func() {
 		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_startup_duration_seconds", nil)
 		Expect(found).To(BeTrue())
 	})
+	It("should update the pod startup and unstarted time metrics when the pod has succeeded", func() {
+		p := test.Pod()
+		p.Status.Phase = corev1.PodPending
+
+		fakeClock.Step(1 * time.Hour)
+		cluster.MarkPodSchedulingDecisions(ctx, map[*corev1.Pod]error{}, map[string][]*corev1.Pod{"n1": {p}}, map[string][]*corev1.Pod{"nc1": {p}})
+		ExpectApplied(ctx, env.Client, p)
+		ExpectReconcileSucceeded(ctx, podController, client.ObjectKeyFromObject(p)) //This will add pod to pending pods and unscheduled pods set
+		_, found := FindMetricWithLabelValues("karpenter_pods_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeTrue())
+		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeTrue())
+
+		// Pod has now succeeded but readiness condition is set to false because the pod is now completed
+		p.Status.Phase = corev1.PodSucceeded
+		p.Status.Conditions = []corev1.PodCondition{
+			{Type: corev1.PodReady, Status: corev1.ConditionFalse, LastTransitionTime: metav1.Now()},
+			{Type: corev1.PodScheduled, Status: corev1.ConditionTrue, LastTransitionTime: metav1.Now()},
+		}
+		ExpectApplied(ctx, env.Client, p)
+		ExpectReconcileSucceeded(ctx, podController, client.ObjectKeyFromObject(p)) //This will check if the pod was scheduled and completed
+		_, found = FindMetricWithLabelValues("karpenter_pods_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeFalse())
+		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeFalse())
+
+		_, found = FindMetricWithLabelValues("karpenter_pods_startup_duration_seconds", nil)
+		Expect(found).To(BeTrue())
+		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_startup_duration_seconds", nil)
+		Expect(found).To(BeTrue())
+	})
+	It("should update the pod startup and unstarted time metrics when the pod has failed", func() {
+		p := test.Pod()
+		p.Status.Phase = corev1.PodPending
+
+		fakeClock.Step(1 * time.Hour)
+		cluster.MarkPodSchedulingDecisions(ctx, map[*corev1.Pod]error{}, map[string][]*corev1.Pod{"n1": {p}}, map[string][]*corev1.Pod{"nc1": {p}})
+		ExpectApplied(ctx, env.Client, p)
+		ExpectReconcileSucceeded(ctx, podController, client.ObjectKeyFromObject(p)) //This will add pod to pending pods and unscheduled pods set
+		_, found := FindMetricWithLabelValues("karpenter_pods_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeTrue())
+		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeTrue())
+
+		// Pod has now failed and readiness condition is set to false
+		p.Status.Phase = corev1.PodFailed
+		p.Status.Conditions = []corev1.PodCondition{
+			{Type: corev1.PodReady, Status: corev1.ConditionFalse, LastTransitionTime: metav1.Now()},
+			{Type: corev1.PodScheduled, Status: corev1.ConditionTrue, LastTransitionTime: metav1.Now()},
+		}
+		ExpectApplied(ctx, env.Client, p)
+		ExpectReconcileSucceeded(ctx, podController, client.ObjectKeyFromObject(p)) //This will check if the pod was scheduled and completed
+		_, found = FindMetricWithLabelValues("karpenter_pods_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeFalse())
+		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_unstarted_time_seconds", map[string]string{
+			"name":      p.GetName(),
+			"namespace": p.GetNamespace(),
+		})
+		Expect(found).To(BeFalse())
+
+		_, found = FindMetricWithLabelValues("karpenter_pods_startup_duration_seconds", nil)
+		Expect(found).To(BeTrue())
+		_, found = FindMetricWithLabelValues("karpenter_pods_provisioning_startup_duration_seconds", nil)
+		Expect(found).To(BeTrue())
+	})
 	It("should create and delete provisioning undecided metrics based on scheduling simulatinos", func() {
 		p := test.Pod()
 		p.Status.Phase = corev1.PodPending

pkg/controllers/node/termination/controller.go

@@ -132,17 +132,18 @@ func (c *Controller) finalize(ctx context.Context, node *corev1.Node) (reconcile
 		}
 		return reconcile.Result{}, serrors.Wrap(fmt.Errorf("tainting node, %w", err), "taint", pretty.Taint(v1.DisruptedNoScheduleTaint))
 	}
-
 	for _, f := range []terminationFunc{
 		c.awaitDrain,
 		c.awaitVolumeDetachment,
 		c.awaitInstanceTermination,
 	} {
 		result, err := f(ctx, nodeClaim, node, nodeTerminationTime)
+
 		if result != nil || err != nil {
 			return *result, err
 		}
 	}
+
 	if err := c.removeFinalizer(ctx, node); err != nil {
 		return reconcile.Result{}, err
 	}

pkg/controllers/node/termination/suite_test.go

@@ -18,6 +18,7 @@ package termination_test
 
 import (
 	"context"
+	"fmt"
 	"sync"
 	"testing"
 	"time"
@@ -89,6 +90,7 @@ var _ = Describe("Termination", func() {
 
 		nodePool = test.NodePool()
 		nodeClaim, node = test.NodeClaimAndNode(v1.NodeClaim{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{v1.TerminationFinalizer}}})
+
 		node.Labels[v1.NodePoolLabelKey] = test.NodePool().Name
 		cloudProvider.CreatedNodeClaims[node.Spec.ProviderID] = nodeClaim
 	})
@@ -189,6 +191,8 @@ var _ = Describe("Termination", func() {
 			Expect(env.Client.Delete(ctx, node)).To(Succeed())
 			// We only reconcile once since this label should be applied before draining the node
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))
+			ExpectObjectReconcileFailed(ctx, env.Client, queue, pod)
+
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
 			Expect(node.Labels[corev1.LabelNodeExcludeBalancers]).Should(Equal("karpenter"))
 		})
@@ -200,27 +204,29 @@ var _ = Describe("Termination", func() {
 				ObjectMeta:  metav1.ObjectMeta{OwnerReferences: defaultOwnerRefs},
 			})
 			ExpectApplied(ctx, env.Client, node, nodeClaim, podEvict, podSkip)
+			ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
 
 			// Trigger Termination Controller
 			Expect(env.Client.Delete(ctx, node)).To(Succeed())
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectObjectReconciled(ctx, env.Client, terminationController, node)
 			Expect(queue.Has(podSkip)).To(BeFalse())
 			ExpectObjectReconciled(ctx, env.Client, queue, podEvict)
 
 			// Expect node to exist and be draining
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // DrainValidation
 			ExpectNodeWithNodeClaimDraining(env.Client, node.Name)
 
 			// Expect podEvict to be evicting, and delete it
 			EventuallyExpectTerminating(ctx, env.Client, podEvict)
 			ExpectDeleted(ctx, env.Client, podEvict)
+			ExpectObjectReconciled(ctx, env.Client, queue, podSkip)
 
 			// Reconcile to delete node
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // DrainValidation
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // VolumeDetachment
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationInitiation
-			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationValidation
+			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // FinalizerRemoval
 			ExpectNotFound(ctx, env.Client, node)
 		})
 		It("should not evict pods that tolerate karpenter disruption taint with exists operator", func() {
@@ -231,15 +237,16 @@ var _ = Describe("Termination", func() {
 				ObjectMeta:  metav1.ObjectMeta{OwnerReferences: defaultOwnerRefs},
 			})
 			ExpectApplied(ctx, env.Client, node, nodeClaim, podEvict, podSkip)
+			ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
 
 			// Trigger Termination Controller
 			Expect(env.Client.Delete(ctx, node)).To(Succeed())
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectObjectReconciled(ctx, env.Client, terminationController, node)
 			Expect(queue.Has(podSkip)).To(BeFalse())
 			ExpectObjectReconciled(ctx, env.Client, queue, podEvict)
 
 			// Expect node to exist and be draining
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // DrainValidation
 			ExpectNodeWithNodeClaimDraining(env.Client, node.Name)
 
 			// Expect podEvict to be evicting, and delete it
@@ -250,10 +257,10 @@ var _ = Describe("Termination", func() {
 
 			// Reconcile to delete node
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // DrainValidation
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // VolumeDetachment
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationInitiation
-			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationValidation
+			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // FinalizerRemoval
 			ExpectNotFound(ctx, env.Client, node)
 		})
 		It("should evict pods that tolerate the node.kubernetes.io/unschedulable taint", func() {
@@ -271,6 +278,7 @@ var _ = Describe("Termination", func() {
 			ExpectObjectReconciled(ctx, env.Client, queue, podEvict)
 
 			// Expect node to exist and be draining
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // DrainValidation
 			ExpectNodeWithNodeClaimDraining(env.Client, node.Name)
 
 			// Expect podEvict to be evicting, and delete it
@@ -279,10 +287,10 @@ var _ = Describe("Termination", func() {
 
 			// Reconcile to delete node
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // DrainValidation
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // VolumeDetachment
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationInitiation
-			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationValidation
+			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // FinalizerRemoval
 			ExpectNotFound(ctx, env.Client, node)
 		})
 		It("should delete nodes that have pods without an ownerRef", func() {
@@ -303,17 +311,18 @@ var _ = Describe("Termination", func() {
 			EventuallyExpectTerminating(ctx, env.Client, pod)
 
 			// Expect node to exist and be draining
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // DrainValidation
 			ExpectNodeWithNodeClaimDraining(env.Client, node.Name)
 
 			// Delete no owner refs pod to simulate successful eviction
 			ExpectDeleted(ctx, env.Client, pod)
 
 			// Reconcile node to evict pod and delete node
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // DrainValidation
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // VolumeDetachment
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationInitiation
-			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationValidation
+			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // FinalizerRemoval
 			ExpectNotFound(ctx, env.Client, node)
 		})
 		It("should delete nodes with terminal pods", func() {
@@ -367,7 +376,7 @@ var _ = Describe("Termination", func() {
 			Expect(queue.Has(podNoEvict)).To(BeTrue())
 
 			// Attempt to evict the pod, but fail to do so
-			ExpectObjectReconciled(ctx, env.Client, queue, podNoEvict)
+			ExpectObjectReconcileFailed(ctx, env.Client, queue, podNoEvict)
 
 			// Expect podNoEvict to fail eviction due to PDB, and be retried
 			Expect(queue.Has(podNoEvict)).To(BeTrue())
@@ -378,10 +387,10 @@ var _ = Describe("Termination", func() {
 
 			// Reconcile to delete node
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // DrainValidation
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // VolumeDetachment
 			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationInitiation
-			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // InstanceTerminationValidation
+			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))    // InstanceTerminationValidation
+			ExpectNotRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // Finalizer Removal
 			ExpectNotFound(ctx, env.Client, node)
 		})
 		It("should evict pods in order and wait until pods are fully deleted", func() {
@@ -432,6 +441,7 @@ var _ = Describe("Termination", func() {
 				ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node))
 				ExpectNodeWithNodeClaimDraining(env.Client, node.Name)
 				for _, pod := range podGroup {
+					Expect(queue.Has(pod)).To(BeTrue())
 					ExpectObjectReconciled(ctx, env.Client, queue, pod)
 				}
 				// Start draining the pod group, but don't complete it yet
@@ -440,7 +450,7 @@ var _ = Describe("Termination", func() {
 				// Look at the next pod group and ensure that none of the pods have started terminating on it
 				if i != len(podGroups)-1 {
 					for _, pod := range podGroups[i+1] {
-						ExpectObjectReconciled(ctx, env.Client, queue, pod)
+						Expect(queue.Has(pod)).To(BeFalse())
 					}
 					ConsistentlyExpectNotTerminating(ctx, env.Client, lo.Map(podGroups[i+1], func(p *corev1.Pod, _ int) client.Object { return p })...)
 				}
@@ -518,6 +528,7 @@ var _ = Describe("Termination", func() {
 
 			// Expect mirror pod to not be queued for eviction
 			Expect(queue.Has(podNoEvict)).To(BeFalse())
+			ExpectObjectReconciled(ctx, env.Client, queue, podEvict)
 
 			// Expect podEvict to be enqueued for eviction then be successful
 			EventuallyExpectTerminating(ctx, env.Client, podEvict)
@@ -712,8 +723,8 @@ var _ = Describe("Termination", func() {
 			})
 			ExpectApplied(ctx, env.Client, node, pod)
 
-			// Trigger eviction queue with the pod key still in it
-			ExpectObjectReconciled(ctx, env.Client, queue, pod)
+			// Trigger eviction queue with the old pod key still in it
+			queue.Add(pod)
 
 			Consistently(func(g Gomega) {
 				g.Expect(env.Client.Get(ctx, client.ObjectKeyFromObject(pod), pod)).To(Succeed())
@@ -920,29 +931,6 @@ var _ = Describe("Termination", func() {
 			Expect(ok).To(BeTrue())
 			Expect(lo.FromPtr(m.GetHistogram().SampleCount)).To(BeNumerically("==", 1))
 		})
-		It("should update the eviction queueDepth metric when reconciling pods", func() {
-			minAvailable := intstr.FromInt32(0)
-			labelSelector := map[string]string{test.RandomName(): test.RandomName()}
-			pdb := test.PodDisruptionBudget(test.PDBOptions{
-				Labels: labelSelector,
-				// Don't let any pod evict
-				MinAvailable: &minAvailable,
-			})
-			ExpectApplied(ctx, env.Client, pdb, node, nodeClaim)
-			pods := test.Pods(5, test.PodOptions{NodeName: node.Name, ObjectMeta: metav1.ObjectMeta{
-				OwnerReferences: defaultOwnerRefs,
-				Labels:          labelSelector,
-			}})
-			ExpectApplied(ctx, env.Client, lo.Map(pods, func(p *corev1.Pod, _ int) client.Object { return p })...)
-
-			wqDepthBefore, _ := FindMetricWithLabelValues("workqueue_adds_total", map[string]string{"name": "eviction.workqueue"})
-			Expect(env.Client.Delete(ctx, node)).To(Succeed())
-			node = ExpectNodeExists(ctx, env.Client, node.Name)
-			ExpectRequeued(ExpectObjectReconciled(ctx, env.Client, terminationController, node)) // Drain
-			wqDepthAfter, ok := FindMetricWithLabelValues("workqueue_adds_total", map[string]string{"name": "eviction.workqueue"})
-			Expect(ok).To(BeTrue())
-			Expect(lo.FromPtr(wqDepthAfter.GetCounter().Value) - lo.FromPtr(wqDepthBefore.GetCounter().Value)).To(BeNumerically("==", 5))
-		})
 	})
 })