Fix flaky integration TestSingleBinaryWithMemberlistScaling - #7802
Open
CharlieTLe wants to merge 1 commit into
Open
Fix flaky integration TestSingleBinaryWithMemberlistScaling#7802CharlieTLe wants to merge 1 commit into
CharlieTLe wants to merge 1 commit into
Conversation
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>
CharlieTLe
requested review from
a team and
yeya24
and removed request for
a team
August 21, 2026 19:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7801.
What
TestSingleBinaryWithMemberlistScalingscales a single-binary memberlist cluster to 30 instances and tears it down to 3, one instance at a time. After eachs.Stop()it waits for every survivor to report the reducedmemberlist_client_cluster_members_count:That wait had no timeout of its own.
WaitSumMetricspolls with the service's generic retry backoff, whichnewSingleBinarysets toMaxRetries: 100of up to500ms— a container-readiness budget worth ~50 s.A departing instance's memberlist
leavemessage 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-22was stopped,cortex-3loggedPush/Pull with cortex-22-e8b03be2 failed28 s later.pushPull()only selects peers inStateAlive(vendor/github.com/hashicorp/memberlist/state.go:633-639), socortex-3still 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, andcortex-3's next push/pull was due roughly 10 s later. A remoteStateLeftis applied directly bymergeState→deadNode(state.go:1313-1315), so that sync would have repaired it.The other paths are slower still:
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 thanGossipToTheDeadTime(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 ini/o timeoutrather thanconnection refused(pkg/ring/kv/memberlist/tcp_transport.go:521-542). The regular ~5 s cadence of theWriteTo failedlines in the failing log is exactly this.ProbeIntervalCortex configures (5 s) a peer is probed about once everylen(members) * 5s≈ 110 s at 22 nodes, and the suspicion timer starts atSuspicionMaxTimeoutMult(6×)SuspicionMult * log10(N+1) * ProbeInterval≈ 160 s, shrinking only on confirmations that never arrive — every other node already recorded the leave andsuspectNodeignores 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 wayWaitSumMetricsdoes.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
maxCortex— weakens the coverage the test exists for ("these numbers seem enough to reliably reproduce some unwanted consequences of slow propagation"), and the suspicion timeout scales withlog10(N)so it barely helps: 12 nodes still gives a ~130 s worst case.memberlist_client_cluster_members_countis the right signal for the stated intent.TestSingleBinaryWithMemberlistScaling#4289 and Flaky test TestSingleBinaryWithMemberlistScaling #4351 are from 2021, long before arm64 CI (Add ARM64 architecture support to integration tests #7068); arm64 only raises the probability (containers start ~6.3 s apart, 30 Cortex processes atGOMAXPROCS=4).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 -lon 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/defaultfails there on the very first instance (the service cortex-1 is not ready; ... connect: connection refused, container exits after ~2 s), andTestSingleBinaryWithMemberlistScalingdied during scale-up atcortex-10for the same reason — both inStartAndWaitReady, 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.