Skip to content

Await manager shutdown before stopping envtest - #6179

Merged
aponcedeleonch merged 1 commit into
mainfrom
fix-telemetry-teardown-fl
Aug 3, 2026
Merged

Await manager shutdown before stopping envtest#6179
aponcedeleonch merged 1 commit into
mainfrom
fix-telemetry-teardown-fl

Conversation

@aponcedeleonch

Copy link
Copy Markdown
Member

Summary

  • Operator CI / Operator Tests Integration has a flaky AfterSuite failure: timeout waiting for process kube-apiserver to stop. It accounts for 32 of the last 51 failures of that job, all in mcp-telemetry-config (27) and mcp-group (5).
  • Root cause: SuiteEnv.Stop cancelled the suite context, slept 100ms, then tore down the envtest control plane. 100ms is not enough for the controller manager to finish, so its informers still held long-running watch requests when kube-apiserver got SIGTERM. kube-apiserver's graceful shutdown blocks draining those requests, overruns envtest's 20s ControlPlaneStopTimeout, and fails the suite. The CI logs show it directly: reflectors log Failed to watch ... connection refused during teardown, so the manager was demonstrably still running after the API server closed its listener.
  • Why only those two suites: they are the only ones with fewer specs than Ginkgo procs (5 specs, 7 procs in CI). BeforeSuite runs on every proc, so the idle procs boot a control plane and cancel the context moments after the informers established their watches — the worst case for the race.
  • Fix: wait for Manager.Start to return before stopping the control plane, instead of guessing at a delay. controller-runtime's Informers.Start blocks on waitGroup.Wait() until every informer has stopped, so that return is a real guarantee the watch connections are closed.
  • Also bound the manager's own GracefulShutdownTimeout (10s) and the wait (15s) safely under envtest's 20s stop timeout, so a genuinely wedged runnable is reported as a manager shutdown problem rather than mis-attributed to apiserver teardown.

All 11 operator integration suites share this helper, so no per-suite changes are needed.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Reproduced the flake locally first, then verified the fix:

Result
Baseline, ginkgo -p --repeat=6 mcp-telemetry-config Flaked on attempt 2 — identical 20.121s / same error as CI
Baseline, second --repeat run Flaked again
Fix, 10 consecutive mcp-telemetry-config runs 10/10 pass
Fix, soak: 15 rounds × (mcp-telemetry-config + mcp-group) 30/30 suite runs pass, 0 timeouts, 11m34s
Fix, full ginkgo -p ./cmd/thv-operator/test-integration/... (11 suites) Pass, 5m19s

Roughly 40 clean runs of the previously-flaky suites, against a baseline that failed twice in ~17.

task lint-fix reports one pre-existing failure unrelated to this change (cmd/thv/app/upgrade.go:204, gosec G115); confirmed present on the base commit by re-running with the change stashed.

Does this introduce a user-facing change?

No. Test infrastructure only.

Special notes for reviewers

Two deliberate omissions, both open to a different call:

  • The idle-proc waste is untouched. mcp-telemetry-config and mcp-group each boot 7 control planes for 5 specs and spend ~38s almost entirely on control-plane startup, because the suites use BeforeSuite (per-proc) rather than SynchronizedBeforeSuite. That inefficiency is what makes this race so easy to hit, but fixing it means sharing one API server across procs and namespace-isolating the specs — worth a follow-up PR, and not needed to stop the flake.
  • No tracking issue exists for this flake; I searched and found nothing matching, hence no Fixes #.

The AddReportEntry in the timeout branch is intentional: if the manager ever does hang, teardown still proceeds and the report names the real cause instead of surfacing a second, misleading apiserver timeout.

Generated with Claude Code

SuiteEnv.Stop cancelled the suite context, slept 100ms, then tore down
the envtest control plane. That sleep was too short for the controller
manager to finish, so its informers still held long-running watch
requests when kube-apiserver got SIGTERM. kube-apiserver's graceful
shutdown blocks draining those requests, overran envtest's 20s
ControlPlaneStopTimeout, and failed AfterSuite with "timeout waiting for
process kube-apiserver to stop".

This accounted for 32 of the last 51 Operator Tests Integration
failures. It concentrated in mcp-telemetry-config and mcp-group because
they are the only suites with fewer specs than Ginkgo procs (5 specs, 7
procs), so their idle procs cancel the context moments after the
informers establish their watches.

Wait for Manager.Start to return instead of guessing at a delay.
controller-runtime's Informers.Start blocks until every informer has
stopped, so that return is a real guarantee the watches are closed. Bound
both the manager's own graceful shutdown and the wait well under
envtest's stop timeout, so a wedged runnable is reported as such rather
than as an apiserver teardown failure.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Aug 3, 2026
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.57%. Comparing base (eedf566) to head (ffd827b).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6179      +/-   ##
==========================================
+ Coverage   72.56%   72.57%   +0.01%     
==========================================
  Files         736      737       +1     
  Lines       76391    76430      +39     
==========================================
+ Hits        55430    55470      +40     
+ Misses      17032    17025       -7     
- Partials     3929     3935       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aponcedeleonch
aponcedeleonch merged commit ebbdcce into main Aug 3, 2026
48 checks passed
@aponcedeleonch
aponcedeleonch deleted the fix-telemetry-teardown-fl branch August 3, 2026 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants