Skip to content

Gate dual-era e2e on the full replica count - #6057

Merged
ChrisJBurns merged 2 commits into
mainfrom
fix-dual-era-readyreplicas-flake
Jul 28, 2026
Merged

Gate dual-era e2e on the full replica count#6057
ChrisJBurns merged 2 commits into
mainfrom
fix-dual-era-readyreplicas-flake

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Dual-Era Multi-Replica Backend failed intermittently on the v1.33.7 lifecycle leg, asserting status.readyReplicas == 2 and reading 1:

[FAILED] Expected
    <int32>: 1
to be equivalent to
    <int>: 2
dual_era_k8s_test.go:217

The value was accurate, not stale. testutil.CheckPodsReady returns as soon as one matching pod is ready — its own doc comment says "at least one pod", and it bails only on runningPods == 0. The spec used it as a two-replica gate:

Eventually(... CheckPodsReady(proxy labels) ...)     // clears at 1 of 2
Eventually(... CheckPodsReady(backendPodLabels) ...) // clears at 1 of 2
Expect(server.Status.ReadyReplicas).To(BeEquivalentTo(2)) // runs anyway

So the assertion ran while the second proxy replica genuinely had not come up. The test never waited for two replicas at all.

The same file already documented this trap — its pod-eviction spec says "CheckPodsReady only requires ONE ready pod ... it never proves the StatefulSet actually replaced the evicted one" — and the gates above it were tripping over exactly that.

A consequence worth calling out: the backend replica count was asserted nowhere, so the following spec ("serves many Modern requests across the multi-replica deployment") could run against a single-replica backend and silently void the premise of the suite.

What changed

  • Add testutil.CheckPodsReadyCount for workloads with a known replica count, and use it for both gates so the spec waits for what its name claims. Terminating pods are excluded from the ready count, matching the operator's own accounting in categorizePodStatus ([BUG] Inflated ReadyReplicas During Rolling Updates in k8s #4498), so a pod on its way out cannot satisfy a gate.
  • CheckPodsReady keeps its one-pod contract — single-replica callers in the vMCP suite rely on it deliberately, so it was left alone rather than tightened.
  • Poll the status block instead of reading it once. The reconciler writes Status, so it trails pod readiness by up to one reconcile even with correct gates. This part is necessary but was not the cause.
  • Replace the hand-rolled counting loop in the pod-eviction spec with the new helper.

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)

Test-only change. Verified with go build ./test/e2e/... and go vet on both touched packages.

The count logic was checked against a fake client rather than assumed, in a throwaway harness (not committed), confirming: 1-of-2 ready fails with got 1 ready of 2; the same state passes the old CheckPodsReady, which is the flake mechanism reproduced directly; 2-of-2 passes; a terminating-but-Ready pod does not count; an over-count fails rather than silently passing.

The suite itself needs a kind cluster with NodePort mappings and runs in CI only (test-e2e-lifecycle.yml), so CI here is the real signal.

On intermittency: a rerun of the original failing job on #6055 — which contains no fix — passed all three legs including v1.33.7. So a green run does not by itself prove this fixed; what the change does is make the gate assert the condition it was always supposed to.

Does this introduce a user-facing change?

No.

Special notes for reviewers

Deliberately does not raise timeout (3m) or add a sleep.

Two things found while diagnosing this and left out to keep the PR test-only:

  1. cmd/thv-operator/api/v1beta1/mcpserver_types.go:398 says BackendReplicas "controls the backend Deployment", but the backend is a StatefulSet (pkg/runner/config.go:316, pkg/container/runtime/types.go:300, created in pkg/container/kubernetes/client.go:567). Doc comment only, no behavior impact.
  2. .claude/rules/testing.md says nothing about polling or replica counts, so "never gate a multi-replica workload with a one-pod helper" is not written down anywhere a future spec author would see it.

An earlier revision of this PR attributed the flake to reconciler status lag and fixed only the polling. That diagnosis was wrong; the branch has been rewritten to tell the correct story in one commit.

Generated with Claude Code

JAORMX
JAORMX previously approved these changes Jul 27, 2026
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 27, 2026
The "brings up both proxy and backend replicas ready" spec failed
intermittently on the v1.33.7 lifecycle leg, asserting
status.readyReplicas == 2 and reading 1.

The value was accurate, not stale. testutil.CheckPodsReady returns as
soon as ONE matching pod is ready -- its own doc comment says "at least
one pod" -- and the spec used it as a two-replica gate. Both gates
cleared with 1 of 2 replicas ready, so the assertion ran while the
second proxy replica genuinely had not come up. The same file already
documented this trap in its pod-eviction spec ("CheckPodsReady only
requires ONE ready pod ... it never proves the StatefulSet actually
replaced the evicted one"); the gates above it were tripping over it.

Add CheckPodsReadyCount for workloads with a known replica count and use
it for both gates, so the spec waits for what its name claims. The
backend count was previously asserted nowhere, which let the following
spec spray Modern requests at a possibly single-replica backend and
silently void the premise of the suite. Also poll the status block
rather than reading it once: the reconciler writes it, so it trails pod
readiness by up to one reconcile even with correct gates.

Terminating pods are excluded from the ready count, matching the
operator's own accounting in categorizePodStatus (issue #4498), so a pod
on its way out cannot satisfy a gate. CheckPodsReady keeps its one-pod
contract -- single-replica callers in the vMCP suite rely on it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jhrozek
jhrozek force-pushed the fix-dual-era-readyreplicas-flake branch from 06dfda4 to 553eb98 Compare July 27, 2026 20:26
@jhrozek jhrozek changed the title Poll MCPServer status in the multi-replica dual-era test Gate dual-era e2e on the full replica count Jul 27, 2026
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/XS Extra small PR: < 100 lines changed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6057      +/-   ##
==========================================
+ Coverage   72.20%   72.23%   +0.02%     
==========================================
  Files         721      721              
  Lines       75062    75085      +23     
==========================================
+ Hits        54198    54234      +36     
+ Misses      17003    16973      -30     
- Partials     3861     3878      +17     

☔ 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.

@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 28, 2026
@ChrisJBurns
ChrisJBurns merged commit e3de20e into main Jul 28, 2026
91 of 93 checks passed
@ChrisJBurns
ChrisJBurns deleted the fix-dual-era-readyreplicas-flake branch July 28, 2026 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants