Fix statefulset parallel scale down

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

The problem

When scaling down a Kubernetes StatefulSet with podManagementPolicy: Parallel, the controller could get stuck if a previous scale-up failed and resource quota prevents creation of missing pods. This deadlock happens because the controller tries to create missing pods before deleting "condemned" pods (those outside the desired range), but can't create them due to quota, and doesn't free up quota because it hasn't deleted the condemned pods yet.

Why it's non-trivial

A naive fix might just try to delete pods or create missing ones without considering the order of operations. However, the order is crucial: if the controller doesn't delete out-of-range pods first, it can't free up resources required to create the missing in-range pods, leading to a deadlock. The solution must also preserve the existing behavior for the default OrderedReady policy.

The approach

1. Extracting and reordering condemned pod processing

A new helper method, processCondemnedPods, is introduced to encapsulate the logic for handling condemned pods and their associated persistent volume claims:

+// processCondemnedPods fixes retention-policy claims and processes Pods
+// whose ordinals are outside the desired replica range.
+func (ssc *defaultStatefulSetControl) processCondemnedPods(
+	ctx context.Context,
+	set *apps.StatefulSet,
+	updateSet *apps.StatefulSet,
+	firstUnavailablePod *v1.Pod,
+	monotonic bool,
+	condemned []*v1.Pod,
+	now time.Time,
+) (bool, error) {
+	// ... inner functions omitted for brevity ...
+	return runForAll(condemned, processCondemnedFn, monotonic)
+}

This helper first ensures any persistent volume claims are updated for retention policy, then deletes the condemned pods. It returns early if any operation fails.

2. Changing the order of operations for Parallel policy

In the main reconciliation loop, the controller now processes condemned pods before attempting to create missing replicas, but only for the Parallel policy (!monotonic):

+	processCondemnedPodsFn := func() (bool, error) {
+		return ssc.processCondemnedPods(
+			ctx,
+			set,
+			updateSet,
+			firstUnavailablePod,
+			monotonic,
+			condemned,
+			now,
+		)
+	}
+
+	if !monotonic {
+		if shouldExit, err := processCondemnedPodsFn(); shouldExit || err != nil {
+			updateStatus(&status, set.Spec.MinReadySeconds, currentRevision, updateRevision, now, replicas, condemned)
+			return &status, err
+		}
+	}

This ensures that for Parallel StatefulSets, condemned pods are deleted before any attempt to create missing pods, allowing quota to be freed up.

For OrderedReady (monotonic) policy, the previous order is preserved:

+	// Preserve the existing processing order for OrderedReady StatefulSets.
+	if monotonic {
+		// ... explanatory comment ...
+		if shouldExit, err := processCondemnedPodsFn(); shouldExit || err != nil {
+			updateStatus(&status, set.Spec.MinReadySeconds, currentRevision, updateRevision, now, replicas, condemned)
+			return &status, err
+		}
+	}

This block ensures that the logic change only affects the Parallel policy.

3. Regression test for the new behavior

A new test verifies that, when a missing pod can't be created due to quota, the condemned pod is deleted first:

+func TestParallelScaleDownProcessesCondemnedPodsBeforeMissingReplicas(t *testing.T) {
+	set := burst(newStatefulSet(2))
+	client := fake.NewSimpleClientset(set)
+	om, _, ssc := setupController(client)
+	// ... set up pods 0 (in-range) and 2 (condemned), pod 1 missing ...
+	// Simulate ResourceQuota preventing creation of missing Pod 1.
+	createErr := errors.New("exceeded quota")
+	om.SetCreateStatefulPodError(createErr, 1)
+	// ... run update and assert that condemned pod is deleted before error is returned ...
+}

The test sets up a scale-down scenario and asserts that the condemned pod is deleted even if pod creation fails due to quota.

From the review thread

[janetkuo]: The use of nested anonymous functions makes the code harder to read and follow. We can significantly improve readability by extracting the processCondemnedPods logic into a named helper method on defaultStatefulSetControl. This reduces the nesting level in updateStatefulSet and makes the main control flow much cleaner.

[suii2210]: Thanks - updated as suggested. I extracted processCondemnedPods into a named helper, simplifying updateStatefulSet. The targeted and full StatefulSet tests pass.

Takeaways

• Order of operations can be critical in controllers: When managing resources with dependencies (like quotas), always consider whether the order of create/delete actions can cause deadlocks or resource starvation.
• Refactor for clarity when control flow gets complex: Extracting logic into named helper functions reduces nesting and improves readability, making maintenance easier.
• Always add regression tests for deadlock or blocking conditions: If a bug depends on a specific sequence of events, write a test that reproduces the scenario to prevent future regressions.