volumemanager: stop leaked reconcilers in tests

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

The problem

Many tests in reconciler_test.go started a background reconciler goroutine that was never stopped. This caused goroutines to leak after each test, which in turn led to resource starvation and test flakes—especially when tests were run in parallel. Some tests would fail with timeouts because previous reconcilers were still running and interfering.

Why it's non-trivial

A naive fix might just try to stop the goroutine by closing a channel or using a global variable, but this doesn't scale well with parallel tests or proper context management. The reconciler needs to be stopped in a way that is safe, idiomatic, and compatible with Go's context-based cancellation, especially in a test suite where many tests run concurrently.

The approach

1. Change the reconciler interface and implementation to use context cancellation instead of a stop channel:

-func (rc *reconciler) Run(ctx context.Context, stopCh <-chan struct{}) {
+func (rc *reconciler) Run(ctx context.Context) {
     logger := klog.FromContext(ctx)
     rc.reconstructVolumes(logger)
     logger.Info("Reconciler: start to sync state")
-    wait.Until(func() { rc.reconcile(ctx) }, rc.loopSleepDuration, stopCh)
+    wait.Until(func() { rc.reconcile(ctx) }, rc.loopSleepDuration, ctx.Done())
 }

This change makes the reconciler loop stop when the provided context is canceled, rather than relying on a separate stop channel. This is more idiomatic in modern Go and simplifies resource cleanup.

2. Update the interface to match:

-type Reconciler interface {
-    Run(ctx context.Context, stopCh <-chan struct{})
+type Reconciler interface {
+    Run(ctx context.Context)

The interface now only requires a context, not a stop channel.

3. Update all test and production call sites to use context cancellation:

Example from a test:

-   stopChan, stoppedChan := make(chan struct{}), make(chan struct{})
-   go func() {
-       reconciler.Run(ctx, stopChan)
-       close(stoppedChan)
-   }()
+   runReconciler(ctx, reconciler)

Tests now use a helper that launches the reconciler with the test's context, so cancellation is tied to the test's lifecycle.

Example from production code:

-   go vm.reconciler.Run(ctx, ctx.Done())
+   go vm.reconciler.Run(ctx)

Production code already had a context, so the change is trivial.

4. In parallel subtests, move context creation into each subtest:

-logger, ctx := ktesting.NewTestContext(t)
...
-t.Run(testName, func(t *testing.T) {
-    t.Parallel()
+    logger, ctx := ktesting.NewTestContext(t)

This ensures each parallel subtest gets its own context and logger, so cancelling one doesn't affect others, and leaked reconcilers from one subtest don't interfere with siblings.

From the review thread

carlory:
Could you update the Run(ctx context.Context, stopCh <-chan struct{}) to Run(ctx context.Context)?

stopCh can be replaced by ctx.Done()

krishhna24:
Done — Reconciler.Run now takes only ctx and the loop stops on ctx.Done(). The prod caller in volume_manager.go already passed ctx.Done() so it just drops the arg; the test sites cancel a per-run child context instead of closing a stop channel. Matches the attach/detach and pluginmanager reconcilers, which are already ctx-only.

Takeaways

• Prefer context-based cancellation over custom stop channels for goroutine lifecycle management, especially in concurrent or test-heavy code.
• Always tie the lifetime of background goroutines in tests to the test's context to avoid leaks and cross-test interference.
• When running parallel subtests, ensure each subtest has its own context and resources to prevent resource contention or accidental sharing.