Repository navigation
internal/rest/resources: Record heartbeat send times after all sends complete - #838
Conversation
…complete clusterClients.Query executes its callback concurrently across all cluster members. The logic goes something like this: ``` read_lock() target_member := hbInfo.ClusterMembers[addr] read_unlock() send_to_target_member(hbInfo) target_member.LastHeartbeat = time.Now() lock() hbInfo.ClusterMembers[addr] = target_member unlock() ``` The problem with this logic is that send_to_target_member will read all of hbInfo (ClusterMembers included) when it sends it over the wire, and that part isn't protected by the concurrency control mechanism. It should also be noted that there is no guarantee or need that all members receive last-round's LastHeartbeat info (just that they get that for themselves). In fact, all the other members of ClusterMembers besides the one that corresponds to the recipient are ignored. A follow-on change could make it so that send_to_target_member (really intState.InternalDatabase.SendHeartbeat()) only sends what the recipient cares about, but it would not remove the need for a concurrency control mechanism to let the sending goroutines concurrently update the LastHeartbeat value. Signed-off-by: Joseph Robert Torsella <joey.torsella@canonical.com>
|
I spent a good deal of time trying to get this issue reproduced with -race and with the existing e2e tests but it just doesn't seem to reproduce lower than the ~100 member threshold where I started seeing it in the sim, which makes some sense as the chance for the issue to happen scales with the number of members (I don't think quadratically but don't feel sure of that right now). I do have the logs from the sim that show this issue when it happened if you are curious, but after some effort I've given up on a reproducible test case whose costs are less than its benefits. |
There was a problem hiding this comment.
🟢 Approval recommended
The change safely eliminates the reported map race while preserving heartbeat behavior.
0 open findings
What changed in this PR
Prevents concurrent heartbeat map reads and writes during parallel sends.
Changes:
- Collects successful send timestamps in a synchronized map.
- Applies timestamps after all sends complete.
| File | Description |
|---|---|
internal/rest/resources/heartbeat.go |
Defers heartbeat map mutations until concurrent encoding finishes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
roosterfish
left a comment
roosterfish
left a comment
There was a problem hiding this comment.
Nice one, thanks! Combined with the trace in #827 I think its fine to leave as is.
Maybe worth adding a comment that the write to hbInfo.ClusterMembers[addr] inside one routine is the culprit here which is then causing any of the other routines to panic inside SendHeartbeat? Just so the same mistake isn't repeated again.
|
@jrtorsella Could you please also open up a PR to backport this into v3? |
Yes of course. Meant to add the comment you mentioned today, sorry - was caught up in the perf testing work all yesterday. I'll take out a backport PR to v3 (and also finish implementing your feedback on the other PR), thanks! |
clusterClients.Query executes its callback concurrently across all cluster members. The logic goes something like this:
The problem with this logic is that send_to_target_member will read all of hbInfo (ClusterMembers included) when it sends it over the wire, and that part isn't protected by the concurrency control mechanism.
It should also be noted that there is no guarantee or need that all members receive last-round's LastHeartbeat info (just that they get that for themselves). In fact, all the other members of ClusterMembers besides the one that corresponds to the recipient are ignored. A follow-on change could make it so that send_to_target_member (really intState.InternalDatabase.SendHeartbeat()) only sends what the recipient cares about, but it would not remove the need for a concurrency control mechanism to let the sending goroutines concurrently update the LastHeartbeat value.
Closes #827
https://warthogs.atlassian.net/browse/LXD-5258