Fix workload broadcast ordering and record dedup keys only after successful emit - #80
Conversation
Replace the per-frame goroutine fan-out with a single ordered broadcast worker draining a bounded queue, so a remove can never overtake the lifecycle upsert it follows and resurrect a ghost workload on peers. Signed-off-by: Mallikh Kaula <mallikh@users.noreply.github.com>
Split the dedup check from the record: a failed broker emit answers 500 without recording the key, so the peer retry is emitted instead of being swallowed as a duplicate. Applies to lifecycle upserts and removals. Signed-off-by: Mallikh Kaula <mallikh@users.noreply.github.com>
Ordering diagrams for the serialized broadcast worker and the dedup-after-emit sequence, plus a reading-order entry in the README. Signed-off-by: Mallikh Kaula <mallikh@users.noreply.github.com>
d9e390f to
53b844c
Compare
|
Rebased onto current develop — the old branch was sitting on a pre-restructure base and couldn't merge anymore. The only rebase fallout was the hand-rolled versions.json bump, which I dropped: versions are automation-managed now, so the bump is declared in the release-intent block at the top of the description instead. Code and tests are unchanged; |
|
I'm currently reviewing this change. Overall, this looks like a change we want to address those two bugs (I added a mermaid sequence diagram to the description to help explain them better). I'll upload a new commit with my feedback and then we'll get this merged ASAP |
Signed-off-by: Kaylee Lubick <klubick@nvidia.com>
Signed-off-by: Kaylee Lubick <klubick@nvidia.com>
Signed-off-by: Kaylee Lubick <klubick@nvidia.com>
kjlubick
left a comment
There was a problem hiding this comment.
LGTM. I folded your doc changes into spec.md and applied some general style fixes to the tests you provided. I also noted an existing race condition related to the snapshots and fixed that.
Signed-off-by: Kaylee Lubick <klubick@nvidia.com>
Signed-off-by: Kaylee Lubick <klubick@nvidia.com>
Noah-Tervalon-Nvidia
left a comment
There was a problem hiding this comment.
I really like these fixes. Thanks for updating this! LGTM.
Changelog title
Workload broadcast reliability: ordered broadcasts, dedup after emit
Changelog body
Bumps
Description
Fixes two reliability bugs in
services/nvpair-workload-managerthat could corrupt the scheduler's view of cluster load:removecould overtake its ownupserton a peer and resurrect a ghost workload -- phantom load in the scheduler. A single ordered worker (broadcastCh+broadcastLoop) now emits frames in order; a full queue drops with a warning and the heartbeat/backfill re-syncs.dedupIndex.seen()/add()are now split and the key is added only on success.sequenceDiagram participant Origin as Origin broker participant Sender as Origin workload manager participant Receiver as Peer workload manager participant Peer as Peer broker and workload store Note over Origin: Example 1 - messages arrive out of order Origin->>Sender: workload:started for workload 7 Sender->>Sender: Start goroutine A to broadcast started Origin->>Sender: workloads:remove for workload 7 Sender->>Sender: Start goroutine B to broadcast remove Note over Sender: Goroutine B finishes first Sender->>Receiver: workloads:remove for workload 7 Receiver->>Peer: workloads:remove for workload 7 Note over Peer: Workload 7 is absent Sender->>Receiver: workload:started for workload 7 Receiver->>Peer: workloads:upsert for workload 7 Note over Peer: Workload 7 now appears again Note over Origin: Example 2 - a failed emit loses a retry Origin->>Sender: workload:started for workload 8 Sender->>Receiver: broadcast started for workload 8 Receiver->>Receiver: Record workload 8 dedup key Receiver->>Peer: Try to emit workloads:upsert Note over Receiver: Write to broker fails Receiver-->>Sender: HTTP 500 Sender->>Receiver: Retry started for workload 8 Receiver->>Receiver: Key exists, so skip the emit Receiver-->>Sender: HTTP 200 Note over Peer: Workload 8 was never addedScope
Changes are confined to
services/nvpair-workload-manager: broadcast ordering, per-key deduplication, focused tests, and the component spec. Frozen-node handling remains outside this PR (issue #6). Component versions are declared above;services/versions.jsonis updated by release automation.Validation
go test -count=1 ./...andgo vet ./...passed inservices/nvpair-workload-manageron Windows with Go 1.27.0.Risk
Checklist
git commit -s), certifying the Developer Certificate of Origin.pair-release-intent:v1block above (services/versions.jsonis automation-managed; CI rejects hand edits), and described user-visible changes so they reach the release notes.