fix: re-gossip dead/suspect accusations on stale alive and unknown node#345
fix: re-gossip dead/suspect accusations on stale alive and unknown node#345emam07 wants to merge 2 commits into
Conversation
During mass cluster restarts a node can get permanently stuck as dead
in peers' views because the refutation mechanism never triggers. Two
bugs prevent the accusation from reaching the restarted node:
1. aliveNode() silently dropped stale alive messages (inc <= current)
even when the local state was dead/suspect. It now re-queues the
dead/suspect message so the restarted node can receive and refute it.
2. deadNode() and suspectNode() silently dropped messages about nodes
not yet in the local map. Freshly-joined nodes during a mass restart
act as a gossip black hole. They now forward the message so it can
propagate through nodes with incomplete cluster views.
Adds [INFO] logs when nodes are marked suspect/dead for observability.
Four new tests cover both bug scenarios directly.
Fixes hashicorp#311
|
|
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
github-project-automation
Bot
moved this to Needs Triage
in Nomad - Community Issues Triage
|
CLA check is passing over here, you can proceed with reviewing the PR and further processes. |
|
The two failing tests (TestShuffleNodes and TestMemberList_ProbeNode_Awareness_MissedNack) are pre-existing flaky tests unrelated to this PR. This branch only modifies state.go — neither failing test exercises that code path:
Could you re-run the failed job? Happy to investigate further if it reproduces consistently. |
|
@emam07 just a heads up that this is on my review queue. I'll try to get to it soon. |
tgross
left a comment
tgross
left a comment
There was a problem hiding this comment.
Hi @emam07! Overall I'm having trouble with this PR. As I've left in my comments, I don't think the characterization of the bug is quite accurate around the freshly-joined node issue. And I'm concerned about the added traffic this is going to generate in the case where a node is legitimately dead. Have you run with this patch in your own environment such that you can characterize that? It's pretty clear this was LLM-generated so it's unclear to me whether this is a drive-by patch for fun or whether you're tackling something you've seen yourself.
Also:
The node stays dead in affected peers' views permanently — until an accidental TCP push/pull sync fixes it.
This is part of the purpose of the TCP push/pull sync! So the cluster shouldn't get itself into a permanently wedged state where nodes never come back, but nodes may take a long time to recover if huge chunks of the cluster have dropped state all at once.
| // Update the state | ||
| state.Incarnation = s.Incarnation | ||
| state.State = StateSuspect | ||
| m.logger.Printf("[INFO] memberlist: Marking %s as suspect (incarnation: %d, from: %s)", s.Node, s.Incarnation, s.From) |
There was a problem hiding this comment.
These logs are going to be extremely noisy in real-world large clusters (ex 1000s of Consul nodes) and balloon operator costs. Let's remove all these new logs.
| // If we've never heard about this node before, forward the suspect message | ||
| // anyway so it can propagate through nodes that may know the target. During | ||
| // mass restarts, freshly joined nodes have incomplete cluster views and | ||
| // silently dropping the message here creates a gossip black hole. | ||
| if !ok { | ||
| m.logger.Printf("[WARN] memberlist: Forwarding suspect msg for unknown node %s (inc: %d, from: %s)", | ||
| s.Node, s.Incarnation, s.From) | ||
| m.encodeAndBroadcast(s.Node, suspectMsg, s) | ||
| return | ||
| } |
There was a problem hiding this comment.
It's not clear to me why this part is needed, and despite the retransmit limit this seems like it's going to increase cluster traffic unnecessarily when a node has actually left permanently.
In the SWIM protocol the Confirm message ("Refute" in this library) overrides all Alive or Suspect messages regardless of incarnation. That's why you made the fix in aliveNode(): the restarted node is going to broadcast its initial alive message with incarnation=1, get told its incarnation is stale, and then increment is incarnation appropriately for the next message. So the nodes in the "black hole" will converge on getting an alive message for that node, with incarnation higher than any dead/suspect message they've dropped anyways.
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tests for the mass-restart incarnation race condition fix |
| // Scenario: A node is declared dead at a high incarnation (e.g. 100). The | ||
| // node then restarts and re-announces itself at incarnation 1. Two separate | ||
| // bugs prevented the dead accusation from ever reaching the restarted node: | ||
| // | ||
| // Bug 1 (aliveNode): a stale alive(inc=1) arriving at a node that already | ||
| // holds dead(inc=100) was silently dropped — the dead message was never | ||
| // re-gossiped, so the restarted node never learned it needed to refute. | ||
| // | ||
| // Bug 2 (deadNode / suspectNode): a dead/suspect message arriving at a node | ||
| // that has never heard of the target was silently dropped instead of being | ||
| // forwarded, creating a gossip black hole in freshly-joined nodes. | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
Having entirely separate tests as you've done here undermines the reasoning for why you need both fixes. The tests exercise the low-level behavior you've explained in the PR description but not proven that you've fixed the actual user-facing beahvior we care about.
3 participants
Problem
During mass cluster restarts, nodes can get permanently stuck as dead
in peers' views. The refutation mechanism never triggers because the
accusation (dead/suspect message) never reaches the restarted node.
Root cause — two silent drops in state.go:
Bug 1 (
aliveNode, line ~1076): When a node receivesalive(node2, inc=1)but already holdsdead(node2, inc=100), itreturns silently. It does not re-gossip the dead accusation back toward
the restarted node, so the node never calls
refute().Bug 2 (
deadNode/suspectNode): When a dead or suspect messagearrives at a node that has never heard of the target (common in
freshly-joined nodes during a mass restart), it is silently dropped
instead of forwarded. This creates a gossip black hole that prevents
the accusation from propagating through nodes with incomplete views.
Both bugs together mean the restarted node broadcasts
alive(inc=1)indefinitely but no node ever sends back the
dead(inc=100)accusationit needs to refute. The node stays dead in affected peers' views
permanently — until an accidental TCP push/pull sync fixes it.
Reported in #311.
Fix
aliveNode(): when a stale alive message is received for a dead orsuspect node, re-queue the accusation for gossip so the restarted
node can receive it and refute.
deadNode()/suspectNode(): forward dead/suspect messages forunknown nodes rather than dropping them. The
TransmitLimitedQueuealready bounds retransmissions to
RetransmitMult × log(N+1).[INFO]log lines when nodes are marked suspect or dead foroperator visibility.
Tests
Four new tests in
state_test.go:TestMemberList_AliveNode_ReGossipsDeadAccusationTestMemberList_AliveNode_ReGossipsSuspectAccusationTestMemberList_DeadNode_UnknownNode_ForwardsMessageTestMemberList_SuspectNode_UnknownNode_ForwardsMessageAll existing tests pass.