From 9dd046bfdc5925e37dab167a145d5a0838132bf8 Mon Sep 17 00:00:00 2001 From: Charlie Le Date: Fri, 21 Aug 2026 11:41:53 -0700 Subject: [PATCH] Fix flaky integration TestSingleBinaryWithMemberlistScaling Fixes #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 --- ...tegration_memberlist_single_binary_test.go | 64 ++++++++++++++++++- 1 file changed, 61 insertions(+), 3 deletions(-) diff --git a/integration/integration_memberlist_single_binary_test.go b/integration/integration_memberlist_single_binary_test.go index acd886db441..83c23124e09 100644 --- a/integration/integration_memberlist_single_binary_test.go +++ b/integration/integration_memberlist_single_binary_test.go @@ -323,9 +323,7 @@ func TestSingleBinaryWithMemberlistScaling(t *testing.T) { // 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")) - } + requireMemberlistMembersCount(t, instances, len(instances)) } require.NoError(t, stop.Wait()) @@ -361,6 +359,66 @@ func TestSingleBinaryWithMemberlistScaling(t *testing.T) { expectedRingMembers, expectedTombstones) } +// memberlistConvergenceTimeout bounds how long we wait for every surviving instance to +// notice that a stopped instance left the memberlist cluster. +// +// An instance learns about a departure in one of three ways, in increasing order of cost: +// +// 1. The "leave" message the departing instance gossips on shutdown. This usually arrives +// within a second, but it is best effort: it has a finite retransmit budget +// (RetransmitMult * ceil(log10(N+1)), 8 transmissions at 22 nodes) and during an +// aggressive scale down much of that budget is spent on peers that are already gone. +// Memberlist keeps gossiping to nodes that died less than GossipToTheDeadTime (30s) +// ago, and each such send blocks the single gossip goroutine for +// -memberlist.packet-dial-timeout (5s), because a removed container's address is +// blackholed and the dial ends in "i/o timeout" rather than "connection refused". +// 2. The next push/pull full state sync (memberlist.PushPullInterval, 30s). This reliably +// repairs a missed leave, since a remote StateLeft is applied directly, but a sync can +// pick the departed instance itself and fail, costing another interval. +// 3. Failure detection, which is far slower than either: with the ProbeInterval Cortex +// configures (5s) a given peer is probed roughly once every len(members) * 5s, and the +// suspicion timer then runs for SuspicionMult * log10(N+1) * ProbeInterval, starting at +// SuspicionMaxTimeoutMult (6x) that value and only shrinking as other nodes confirm the +// suspicion. No confirmation ever arrives here, because every other node already +// recorded the leave and ignores suspect messages for non-alive nodes. At 22 nodes that +// is ~160s of suspicion on top of the probe delay. +// +// So the budget has to cover several push/pull intervals. It used to be whatever the +// per-service retry backoff gave (100 retries of up to 500ms, see newSingleBinary), which is +// ~50s: one marginal sync window, and the test failed whenever a single leave message was +// lost. See #7801. +const memberlistConvergenceTimeout = 2 * time.Minute + +// requireMemberlistMembersCount waits until every instance reports exactly expectedMembers +// in memberlist_client_cluster_members_count. +func requireMemberlistMembersCount(t *testing.T, instances []*e2ecortex.CortexService, expectedMembers int) { + t.Helper() + + require.Eventually(t, func() bool { + converged := true + + for _, c := range instances { + metrics, err := c.SumMetrics([]string{"memberlist_client_cluster_members_count"}) + if err != nil { + // Scraping can fail transiently while the cluster is churning, so retry + // instead of failing the test. + converged = false + t.Logf("%s: failed to fetch memberlist_client_cluster_members_count: %s\n", c.Name(), err) + continue + } + + // Don't short circuit the check, so we log every instance which is behind. + if metrics[0] != float64(expectedMembers) { + converged = false + t.Logf("%s: memberlist_client_cluster_members_count=%f, expected %d\n", c.Name(), metrics[0], expectedMembers) + } + } + + return converged + }, memberlistConvergenceTimeout, time.Second, + "expected all instances to see %d memberlist cluster members", expectedMembers) +} + func TestHATrackerWithMemberlistClusterSync(t *testing.T) { s, err := e2e.NewScenario(networkName) require.NoError(t, err)