Skip to content

Fix flaky integration TestSingleBinaryWithMemberlistScaling - #7802

Open
CharlieTLe wants to merge 1 commit into
cortexproject:masterfrom
CharlieTLe:flaky-memberlist-scaling
Open

Fix flaky integration TestSingleBinaryWithMemberlistScaling#7802
CharlieTLe wants to merge 1 commit into
cortexproject:masterfrom
CharlieTLe:flaky-memberlist-scaling

Conversation

@CharlieTLe

Copy link
Copy Markdown
Member

Fixes #7801.

What

TestSingleBinaryWithMemberlistScaling scales a single-binary memberlist cluster to 30 instances and tears it down to 3, one instance at a time. After each s.Stop() it waits for every survivor to report the reduced memberlist_client_cluster_members_count:

// TODO(#4360): Remove this when issue is resolved.
//   Wait until memberlist for all nodes has recognised the instance left.
//   This means that we will not gossip tombstones to leaving nodes.
for _, c := range instances {
    require.NoError(t, c.WaitSumMetrics(e2e.Equals(float64(len(instances))), "memberlist_client_cluster_members_count"))
}

That wait had no timeout of its own. WaitSumMetrics polls with the service's generic retry backoff, which newSingleBinary sets to MaxRetries: 100 of up to 500ms — a container-readiness budget worth ~50 s.

A departing instance's memberlist leave message is gossiped best effort. When a survivor misses it, the fast repair is memberlist's push/pull full-state sync, which runs every 30 s, so ~50 s buys one — at best two — repair opportunities, and one of those can be spent trying to sync with the instance that just went away.

That is exactly what happened in the run this fixes (arm64 job): after cortex-22 was stopped, cortex-3 logged Push/Pull with cortex-22-e8b03be2 failed 28 s later. pushPull() only selects peers in StateAlive (vendor/github.com/hashicorp/memberlist/state.go:633-639), so cortex-3 still had the departed instance alive at that point — it had neither received the leave nor yet suspected the node. The wait gave up 48.2 s after the stop, reporting 22 members instead of 21, and cortex-3's next push/pull was due roughly 10 s later. A remote StateLeft is applied directly by mergeStatedeadNode (state.go:1313-1315), so that sync would have repaired it.

The other paths are slower still:

  • Gossip retransmits a leave RetransmitMult * ceil(log10(N+1)) = 8 times at 22 nodes, and during a fast scale down much of that budget goes to nodes dead for less than GossipToTheDeadTime (30 s), which memberlist keeps gossiping to (state.go:575-595). Each of those sends blocks memberlist's single gossip goroutine for -memberlist.packet-dial-timeout (5 s), because Docker blackholes a removed container's address so the dial ends in i/o timeout rather than connection refused (pkg/ring/kv/memberlist/tcp_transport.go:521-542). The regular ~5 s cadence of the WriteTo failed lines in the failing log is exactly this.
  • Failure detection is slowest: with the ProbeInterval Cortex configures (5 s) a peer is probed about once every len(members) * 5s ≈ 110 s at 22 nodes, and the suspicion timer starts at SuspicionMaxTimeoutMult (6×) SuspicionMult * log10(N+1) * ProbeInterval160 s, shrinking only on confirmations that never arrive — every other node already recorded the leave and suspectNode ignores suspect messages for non-alive nodes (state.go:1171-1174).

The change

The assertion and the metric are both correct, so they are unchanged. What was wrong is the budget: it was inherited from a readiness check rather than chosen for gossip convergence (added in 03911b6 / #4361, itself a workaround for #4360). This PR gives the wait an explicit budget derived from PushPullInterval — 2 minutes, i.e. four sync opportunities instead of one marginal window — documents the three convergence paths and their costs so the number isn't a mystery, logs which instances are still behind (mirroring the final tombstone assertion, which already does this and has "proven extremely useful"), and treats a transient scrape failure as "not converged yet" rather than aborting the wait outright the way WaitSumMetrics does.

This does not slow the test in the normal case: the wait returns on the first successful poll, and every earlier scale-down step in the referenced run converged in about 3.5 s.

Alternatives considered

Worth noting that the workaround this PR repairs only partially achieves its stated goal: memberlist keeps gossiping to a node for GossipToTheDeadTime (30 s) after marking it dead, so the departing node stays in the gossip pool past the point where the member count drops. Fully avoiding that would mean sleeping 30 s per scale-down step. #4360 remains the real fix.

No CHANGELOG entry: test-only change, matching a7e4c78 ("Fix flaky pkg/compactor tests", #7796).

Verification

  • go vet -tags "integration,requires_docker,integration_memberlist" ./integration/... — clean, no output.
  • gofmt -l / goimports -local github.com/cortexproject/cortex -l on the changed file — no output.

I was not able to run this integration test. It needs Docker and ~31 containers, and the memberlist integration tests do not run in my local environment at all: the unmodified TestSingleBinaryWithMemberlist/default fails there on the very first instance (the service cortex-1 is not ready; ... connect: connection refused, container exits after ~2 s), and TestSingleBinaryWithMemberlistScaling died during scale-up at cortex-10 for the same reason — both in StartAndWaitReady, code this PR does not touch. My environment is also darwin/arm64, which is not the arm64 Linux CI runner, so a local pass would not have proven the flake fixed either way. CI on this PR is the real check.

Since the flake is probabilistic, a single green CI run does not prove much; the argument for the fix rests on the timing analysis above — the wait budget was ~1.6 push/pull intervals and is now 4.

Fixes cortexproject#7801.

The scale-down loop stops one instance at a time and, after each stop, waits for
every surviving instance to report the reduced
memberlist_client_cluster_members_count. That wait had no timeout of its own: it
used WaitSumMetrics, which polls with the service's generic retry backoff, and
newSingleBinary sets that to 100 retries of up to 500ms - a container-readiness
budget worth about 50s.

A departing instance's memberlist leave message is gossiped best effort. When a
survivor misses it, the fast repair is memberlist's push/pull full state sync,
which runs every 30s, so a ~50s budget buys one - at best two - repair
opportunities, and one of those can be spent trying to sync with the instance
that just went away. That is exactly what happened in the run this fixes:
cortex-3 logged "Push/Pull with cortex-22 failed" 28s after cortex-22 was
stopped - pushPull only selects peers in StateAlive, so cortex-3 still had the
departed instance alive at that point - the wait gave up 48.2s after the stop
reporting 22 members instead of 21, and cortex-3's next sync was due about 10s
later. A remote StateLeft is applied directly by mergeState, so that sync would
have repaired it.

The other paths are slower still. Gossip retransmits a leave message
RetransmitMult * ceil(log10(N+1)) = 8 times at 22 nodes, and during a fast scale
down much of that budget goes to nodes that died less than GossipToTheDeadTime
(30s) ago; each of those sends blocks memberlist's single gossip goroutine for
-memberlist.packet-dial-timeout (5s), because Docker blackholes a removed
container's address so the dial ends in "i/o timeout" rather than "connection
refused". Failure detection is slowest of all: with the ProbeInterval Cortex
configures (5s) a peer is probed about once every len(members) * 5s, and the
suspicion timer starts at SuspicionMaxTimeoutMult (6) * SuspicionMult *
log10(N+1) * ProbeInterval, roughly 160s at 22 nodes, shrinking only on
confirmations that never arrive because every other node already recorded the
leave and ignores suspect messages for non-alive nodes.

Keep the assertion and the metric - both are correct - and give the wait an
explicit budget derived from PushPullInterval instead of the readiness backoff,
so it covers several sync opportunities rather than one marginal window. Log the
instances that are still behind, mirroring the final tombstone assertion which
already does this, and treat a transient scrape failure as "not converged yet"
instead of aborting the wait the way WaitSumMetrics does.

This does not slow the test down in the normal case: the wait returns on the
first successful poll, and in the referenced run every earlier scale-down step
converged in about 3.5s.

Signed-off-by: Charlie Le <charlie_le@apple.com>
@dosubot dosubot Bot added component/memberlist go Pull requests that update Go code type/flaky-test labels Aug 21, 2026
@CharlieTLe
CharlieTLe requested review from a team and yeya24 and removed request for a team August 21, 2026 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant