fix: emit eviction metrics for background evictions that never get deleted

Pods evicted in background may stay in Succeeded/Failed phase forever without being garbage-collected, so the metric was silently dropped. Similarly, entries whose background eviction timed out in cleanCache were expired without recording an outcome. - UpdateFunc: emit "success"/"error" metric when a pod transitions to PodSucceeded/PodFailed respectively, matching the label convention used elsewhere in the file - cleanCache: add onAssumedTimeout callback on evictionRequestsCache, wired in NewPodEvictor to emit "error" for entries that exceed the assumed-eviction timeout - Add TestEvictionInBackgroundMetrics_PodCompleted and TestEvictionInBackgroundMetrics_AssumedTimeout to cover both paths Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
2026-08-19 20:46:26 +00:00 · 2026-06-03 19:08:15 +02:00
parent 3588dacd57
commit 81fb28c18c
2 changed files with 175 additions and 2 deletions
@@ -69,6 +69,7 @@ type evictionRequestsCache struct {
 	requestsPerNamespace         map[string]uint
 	requestsTotal                uint
 	assumedRequestTimeoutSeconds uint
+	onAssumedTimeout             func(item evictionRequestItem)
 }

 func newEvictionRequestsCache(assumedRequestTimeoutSeconds uint) *evictionRequestsCache {
@@ -95,6 +96,9 @@ func (erc *evictionRequestsCache) cleanCache(ctx context.Context) {
 			requestAgeSeconds := uint(metav1.Now().Sub(item.assumedTimestamp.Local()).Seconds())
 			if requestAgeSeconds > erc.assumedRequestTimeoutSeconds {
 				klog.V(4).InfoS("Assumed eviction request in background timed out, deleting", "timeout", erc.assumedRequestTimeoutSeconds, "podNamespace", item.podNamespace, "podName", item.podName)
+				if erc.onAssumedTimeout != nil {
+					erc.onAssumedTimeout(item)
+				}
 				erc.deleteItem(uid)
 			}
 		}
@@ -290,6 +294,12 @@ func NewPodEvictor(

 	if featureGates.Enabled(features.EvictionsInBackground) {
 		erCache := newEvictionRequestsCache(assumedEvictionRequestTimeoutSeconds)
+		if podEvictor.metricsEnabled {
+			erCache.onAssumedTimeout = func(item evictionRequestItem) {
+				metrics.PodsEvicted.With(map[string]string{"result": "error", "strategy": item.strategyName, "namespace": item.podNamespace, "node": item.podNodeName, "profile": item.profileName}).Inc()
+				metrics.PodsEvictedTotal.With(map[string]string{"result": "error", "strategy": item.strategyName, "namespace": item.podNamespace, "node": item.podNodeName, "profile": item.profileName}).Inc()
+			}
+		}

 		handlerRegistration, err := podInformer.AddEventHandler(
 			cache.ResourceEventHandlerFuncs{
@@ -330,8 +340,18 @@ func NewPodEvictor(
 					}
 					// Remove completed/suceeeded or failed pods from the cache
 					if newPod.Status.Phase == v1.PodSucceeded || newPod.Status.Phase == v1.PodFailed {
-						klog.V(3).InfoS("Pod with eviction in background completed. Removing pod from the cache.", "pod", klog.KObj(newPod))
-						erCache.deletePod(newPod)
+						if item, exists := erCache.getPod(newPod); exists {
+							klog.V(3).InfoS("Pod with eviction in background completed. Removing pod from the cache.", "pod", klog.KObj(newPod))
+							if item.evictionAssumed && podEvictor.metricsEnabled {
+								result := "success"
+								if newPod.Status.Phase == v1.PodFailed {
+									result = "error"
+								}
+								metrics.PodsEvicted.With(map[string]string{"result": result, "strategy": item.strategyName, "namespace": item.podNamespace, "node": item.podNodeName, "profile": item.profileName}).Inc()
+								metrics.PodsEvictedTotal.With(map[string]string{"result": result, "strategy": item.strategyName, "namespace": item.podNamespace, "node": item.podNodeName, "profile": item.profileName}).Inc()
+							}
+							erCache.deletePod(newPod)
+						}
 						return
 					}
 					// Ignore any pod that does not have eviction in progress
@@ -683,6 +683,159 @@ func TestEvictionInBackgroundMetrics_InformerRace(t *testing.T) {
 	metricstest.AssertVectorCount(t, "descheduler_pods_evicted_total", map[string]string{"result": "success"}, 1)
 }

+// TestEvictionInBackgroundMetrics_PodCompleted verifies that when a pod
+// transitions to a terminal phase via UpdateFunc (without ever being deleted),
+// the correct metric is emitted: "success" for PodSucceeded, "error" for PodFailed.
+func TestEvictionInBackgroundMetrics_PodCompleted(t *testing.T) {
+	testCases := []struct {
+		name           string
+		phase          v1.PodPhase
+		expectedResult string
+	}{
+		{name: "PodSucceeded emits success", phase: v1.PodSucceeded, expectedResult: "success"},
+		{name: "PodFailed emits error", phase: v1.PodFailed, expectedResult: "error"},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx, cancel := context.WithTimeout(context.Background(), wait.ForeverTestTimeout)
+			defer cancel()
+
+			deschedulermetrics.Register()
+			deschedulermetrics.PodsEvicted.Reset()
+			deschedulermetrics.PodsEvictedTotal.Reset()
+
+			node1 := test.BuildTestNode("n1", 2000, 3000, 10, nil)
+			ownerRef1 := test.GetReplicaSetOwnerRefList()
+			p1 := test.BuildTestPod("p1", 100, 0, node1.Name, func(pod *v1.Pod) {
+				pod.Namespace = "dev"
+				pod.ObjectMeta.OwnerReferences = ownerRef1
+				pod.Annotations = map[string]string{
+					EvictionRequestAnnotationKey: "",
+				}
+			})
+
+			client := fakeclientset.NewSimpleClientset(node1, p1)
+			sharedInformerFactory := informers.NewSharedInformerFactory(client, 0)
+			_, eventRecorder := utils.GetRecorderAndBroadcaster(ctx, client)
+
+			podEvictor, err := NewPodEvictor(
+				ctx,
+				client,
+				eventRecorder,
+				sharedInformerFactory.Core().V1().Pods().Informer(),
+				initFeatureGates(),
+				NewOptions().WithMetricsEnabled(true),
+			)
+			if err != nil {
+				t.Fatalf("Unexpected error when creating a pod evictor: %v", err)
+			}
+
+			client.PrependReactor("create", "pods", func(action core.Action) (bool, runtime.Object, error) {
+				if action.GetSubresource() != "eviction" {
+					return false, nil, nil
+				}
+				return true, nil, &apierrors.StatusError{
+					ErrStatus: metav1.Status{
+						Reason:  metav1.StatusReasonTooManyRequests,
+						Message: "Eviction triggered evacuation",
+					},
+				}
+			})
+
+			sharedInformerFactory.Start(ctx.Done())
+			sharedInformerFactory.WaitForCacheSync(ctx.Done())
+
+			evictOpts := EvictOptions{StrategyName: "TestStrategy", ProfileName: "TestProfile"}
+			podEvictor.EvictPod(ctx, p1, evictOpts)
+
+			metricstest.AssertVectorCount(t, "descheduler_pods_evicted_total", map[string]string{"result": "background"}, 1)
+
+			// Transition the pod to a terminal phase without deleting it.
+			p1Updated := p1.DeepCopy()
+			p1Updated.Status.Phase = tc.phase
+			client.CoreV1().Pods(p1.Namespace).UpdateStatus(ctx, p1Updated, metav1.UpdateOptions{})
+
+			// Wait for UpdateFunc to fire and remove the pod from the cache.
+			if err := wait.PollUntilContextTimeout(ctx, 100*time.Millisecond, wait.ForeverTestTimeout, true, func(ctx context.Context) (bool, error) {
+				return !podEvictor.erCache.hasPod(p1), nil
+			}); err != nil {
+				t.Fatalf("Timed out waiting for background eviction to complete: %v", err)
+			}
+
+			metricstest.AssertVectorCount(t, "descheduler_pods_evicted_total", map[string]string{"result": tc.expectedResult}, 1)
+		})
+	}
+}
+
+// TestEvictionInBackgroundMetrics_AssumedTimeout verifies that when an assumed
+// eviction entry expires in cleanCache, the "error" metric is emitted.
+func TestEvictionInBackgroundMetrics_AssumedTimeout(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), wait.ForeverTestTimeout)
+	defer cancel()
+
+	deschedulermetrics.Register()
+	deschedulermetrics.PodsEvicted.Reset()
+	deschedulermetrics.PodsEvictedTotal.Reset()
+
+	node1 := test.BuildTestNode("n1", 2000, 3000, 10, nil)
+	ownerRef1 := test.GetReplicaSetOwnerRefList()
+	p1 := test.BuildTestPod("p1", 100, 0, node1.Name, func(pod *v1.Pod) {
+		pod.Namespace = "dev"
+		pod.ObjectMeta.OwnerReferences = ownerRef1
+		pod.Annotations = map[string]string{
+			EvictionRequestAnnotationKey: "",
+		}
+	})
+
+	client := fakeclientset.NewSimpleClientset(node1, p1)
+	sharedInformerFactory := informers.NewSharedInformerFactory(client, 0)
+	_, eventRecorder := utils.GetRecorderAndBroadcaster(ctx, client)
+
+	podEvictor, err := NewPodEvictor(
+		ctx,
+		client,
+		eventRecorder,
+		sharedInformerFactory.Core().V1().Pods().Informer(),
+		initFeatureGates(),
+		NewOptions().WithMetricsEnabled(true),
+	)
+	if err != nil {
+		t.Fatalf("Unexpected error when creating a pod evictor: %v", err)
+	}
+
+	client.PrependReactor("create", "pods", func(action core.Action) (bool, runtime.Object, error) {
+		if action.GetSubresource() != "eviction" {
+			return false, nil, nil
+		}
+		return true, nil, &apierrors.StatusError{
+			ErrStatus: metav1.Status{
+				Reason:  metav1.StatusReasonTooManyRequests,
+				Message: "Eviction triggered evacuation",
+			},
+		}
+	})
+
+	sharedInformerFactory.Start(ctx.Done())
+	sharedInformerFactory.WaitForCacheSync(ctx.Done())
+
+	evictOpts := EvictOptions{StrategyName: "TestStrategy", ProfileName: "TestProfile"}
+	podEvictor.EvictPod(ctx, p1, evictOpts)
+
+	metricstest.AssertVectorCount(t, "descheduler_pods_evicted_total", map[string]string{"result": "background"}, 1)
+
+	// Back-date all assumed entries so they appear expired, then run cleanCache.
+	podEvictor.erCache.mu.Lock()
+	for uid, item := range podEvictor.erCache.requests {
+		item.assumedTimestamp = metav1.NewTime(time.Now().Add(-time.Hour))
+		podEvictor.erCache.requests[uid] = item
+	}
+	podEvictor.erCache.mu.Unlock()
+	podEvictor.erCache.cleanCache(ctx)
+
+	metricstest.AssertVectorCount(t, "descheduler_pods_evicted_total", map[string]string{"result": "error"}, 1)
+}
+
 func assertEqualEvents(t *testing.T, expected []string, actual <-chan string) {
 	t.Logf("Assert for events: %v", expected)
 	c := time.After(wait.ForeverTestTimeout)