Ignore succeeded pods in HPA replica calculations

Project: kubernetes/kubernetes | Language: Go | Concepts: error-handling, testing-strategy, refactoring | Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/141302

The problem

The Horizontal Pod Autoscaler (HPA) was including pods in the Succeeded phase when calculating the desired number of replicas. This led to incorrect scaling decisions, as these pods are already terminated and shouldn't affect scaling.

Why it's non-trivial

A naive fix might only exclude Failed pods, missing that Succeeded pods are also terminal and should not be counted. Additionally, the fix must ensure that tests cover these scenarios and that the code remains consistent with how other terminal pods are handled.

The approach

1. Update pod filtering logic to ignore all terminal pods, not just failed ones:

Before:

-		if pod.DeletionTimestamp != nil || pod.Status.Phase == v1.PodFailed {
+		if pod.DeletionTimestamp != nil || podutil.IsPodTerminal(pod) {
 			ignoredPods.Insert(pod.Name)
 			continue
 		}

Annotation:
Previously, only pods being deleted or in the Failed phase were ignored. The change uses podutil.IsPodTerminal(pod) to ignore all terminal pods, including both Failed and Succeeded pods. This ensures that any pods which will not run again are excluded from scaling calculations.

2. Add and update test cases to verify correct behavior:

• Add new scenarios to TestReplicaCalcResourceScale to check that succeeded pods are ignored for both scaling up and scaling down, for both pod-level and container-level metrics.

Example added test case:

+		{
+			name: "scale up: succeeded pods ignored",
+			fixture: calcScenario{
+				currentReplicas: 2,
+				podReadiness:    []v1.ConditionStatus{v1.ConditionTrue, v1.ConditionTrue, v1.ConditionFalse, v1.ConditionFalse},
+				podPhase:        []v1.PodPhase{v1.PodRunning, v1.PodRunning, v1.PodSucceeded, v1.PodSucceeded},
+				resource: &cpuResource{
+					requests: cpuRequests(4, "1.0"),
+					levels:   makePodMetricLevels(500, 700),
+				},
+			},
+			targetUtilization:   30,
+			expectedReplicas:    4,
+			expectedUtilization: 60,
+			expectedRawValue:    numContainersPerPod * 600,
+		},

Annotation:
This test ensures that when there are two running and two succeeded pods, only the running pods are considered in scaling calculations.

• Similar cases are added for scaling down and for container-specific metrics.

• Add a direct unit test in TestGroupPods to confirm that succeeded pods are placed in the ignoredPods set:

+		}, {
+			name: "ignore pods in a succeeded state",
+			pods: []*v1.Pod{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "succeeded",
+					},
+					Status: v1.PodStatus{
+						Phase: v1.PodSucceeded,
+					},
+				},
+			},
+			metrics: metricsclient.PodMetricsInfo{
+				"succeeded": metricsclient.PodMetric{Value: 1},
+			},
+			resource:            v1.ResourceCPU,
+			expectReadyPodCount: 0,
+			expectUnreadyPods:   sets.New[string](),
+			expectMissingPods:   sets.New[string](),
+			expectIgnoredPods:   sets.New[string]("succeeded"),
+		},

Annotation:
This test directly checks that a pod in the Succeeded phase is ignored.

3. Refactor comments and test setup to use "terminal pods" instead of just "failed pods":

-		// Failed pods aren't included in currentReplicas.
+		// Terminal pods aren't included in currentReplicas.

Annotation:
This clarifies the intent and aligns the code and comments with the updated logic.

From the review thread

I think I’d prefer not to introduce a new unit test. Instead, we could add "scale down: succeeded pods ignored" (and its container-metric variant) to TestReplicaCalcResourceScale, next to the existing failed-pods cases.
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/podautoscaler/replica_calculator_test.go#L467

Fair - latest commit does this.

Takeaways

• Always handle all terminal states (not just "failed") when filtering resources that should not impact system calculations.
• When fixing a bug, add or update tests that directly verify the new behavior, especially for edge cases.
• Prefer updating and extending existing comprehensive tests over adding redundant new ones, to keep the test suite maintainable and focused.