Skip to content

fix(dash-daemon): retire the old Lemonade instance when its host or port changes - #937

Closed
jussielo-amd wants to merge 2 commits into
mainfrom
fix-dash-daemon-retire-old-lemonade-instance
Closed

jussielo-amd wants to merge 2 commits into
mainfrom
fix-dash-daemon-retire-old-lemonade-instance

Conversation

@jussielo-amd

Copy link
Copy Markdown
Collaborator

Summary

Fixes ROCMAI-866. When the Lemonade endpoint the dashboard daemon is watching changes host or port, the dashboard kept showing the endpoint it had left. The Lemonade instance id encodes host and port, so a moved endpoint is a new id. The daemon purged the old id's telemetry and announced the new id, but never retired the old instance: it stayed in the daemon state and in every snapshot afterwards, no InstanceGone event was sent for it, and the vLLM Prometheus scrape kept polling its port as if it were a vLLM server, because the scrape filter only skips the current Lemonade id. The result was two Lemonade rows (one dead) and a scrape warning on every instance cadence for as long as the daemon ran. If the endpoint later went quiet, only the current id was retired, so the dead one was never cleaned up.

Now, when the probe reports a different id, the daemon retires the old one first (InstanceGone, removed from the state, telemetry purged, as it already was) and then upserts and announces the new one. A snapshot carries exactly one Lemonade instance, and the old endpoint is no longer in the set the vLLM scrape polls, because that set is derived from the instances in state.

What changed

The fix is in the Lemonade reconcile step in crates/rocm-dash-daemon/src/runner.rs: on an id change the previous id goes through the same retire step as an endpoint that goes quiet, before the new id is upserted and announced. An in-place refresh of the same id is unchanged and still carries live fields over.

The vLLM scrape target selection moves out of run_loop into a small function, vllm_scrape_targets, with the filter unchanged. It exists so the tests can ask which endpoints would be polled. This is in the first commit, which changes no behaviour.

User-visible behaviour

  • After the Lemonade host or port changes, the dashboard (Instances and Serving views) shows one Lemonade instance, the one at the new endpoint, instead of that one plus a stale row for the old endpoint. The TUI already removes an instance on InstanceGone and clamps its selection, so nothing in those views depended on the stale row.
  • A subscriber to the daemon event stream (including a recorded session) now sees InstanceGone for the old id followed by InstanceDiscovered for the new id when the endpoint moves; before, it saw only InstanceDiscovered for the new id.
  • The spurious vLLM scrape warning for the dead Lemonade endpoint, shown in the header warning badge, no longer appears after the endpoint moves.
  • If the endpoint goes quiet after a move, nothing is left behind: before, the old row stayed forever.
  • If the endpoint moves away and back (A to B to A), A is announced gone and B announced when it moves, and the reverse when it returns, so there is always one instance. Before, A was announced as discovered a second time while never having been announced gone.

Behaviour that did not change: a probe that answers with the same id is not announced again and keeps its live fields; an endpoint that goes quiet is announced gone once; nothing is announced when Lemonade never answers.

Verification

Ran: cargo fmt --all --check; cargo clippy -p rocm-dash-daemon -p e2e-cucumber --all-targets -- -D warnings; cargo test -p rocm-dash-daemon --no-fail-fast (56 unit, 4 + 9 + 11 integration tests before; 65 unit and the same integration counts after, nine new tests, none lost or renamed apart from the lifecycle test keeping its name); cargo xtask manifest --check; cargo xtask check-crate-edges; the leak scan against origin/main. No documentation changed, so the link check was not needed.

Not run: clippy for x86_64-pc-windows-msvc (the target is installed but a dependency's build script needs the MSVC lib.exe, which this host does not have); the full workspace test gate, the smoke script and the end-to-end suite, which CI carries. The code touched is not platform-gated.

New tests, each shown to fail when the code it covers is broken, by editing the branch under test and reverting the edit:

  • Skip the retire on an id change (purge only, the old behaviour): the host-change, port-change, move-away-and-back and lifecycle tests fail.
  • Retire without purging the telemetry: the host-change, port-change, endpoint-going-quiet, come-back-elsewhere and lifecycle tests fail.
  • Retire the new id instead of the old one: the host-change, port-change, move-away-and-back and lifecycle tests fail.
  • Drop the Lemonade exclusion from the scrape target filter: seven of the new tests fail, including the first-answer and same-endpoint rows.

State matrix for the Lemonade source, one test per row (announcements, the instances a snapshot would carry, per-id telemetry, and the pairs the vLLM scrape would poll are all asserted together):

State Expected
Never answers nothing announced, no instances, no telemetry, no scrape targets
First answer discovered once, one instance, not a vLLM target
Same endpoint again nothing announced, live fields and telemetry kept
Host changes gone old, discovered new, one instance, old telemetry purged, no scrape target
Port changes same as host change
Goes quiet gone once, no instances, telemetry purged
Comes back elsewhere after quiet discovered new only, one instance
Same answer twice in one tick announced once, one instance
Moves away and back gone and discovered on each move, one instance at the end

The existing lifecycle test had pinned the stale instance (a reviewer on the earlier restructuring asked that it be a conscious pin); that assertion is now inverted.

No Gherkin scenario is added. The dash scenarios drive the interactive dashboard through a pseudo-terminal against managed-service records and demo data, and no scenario starts a Lemonade endpoint, let alone moves one mid-session (the daemon's Lemonade host and port are fixed for the life of a run, so the integration test in crates/rocm-dash-daemon/tests/discovery_reconcile.rs cannot move it either). The change is covered at the unit level through the reconcile step and the scrape-target selection, and the existing integration test for the Lemonade endpoint lifecycle still passes.

Notes for reviewers

Read the two commits in order. The first only adds tests (plus the behaviour-neutral extraction above), and its host-change, port-change and move-away-and-back rows pin the defect so you can see they pass on the unmodified logic; the second changes the reconcile step and flips exactly those assertions, so the diff of the test file between the two commits is the behaviour change.

Overlap with open work: #927 (managed registry read failure) also edits runner.rs, in the managed-service block and the discovery state, and appends tests to the same module. I placed the new tests before the existing lifecycle test rather than at the end of the module so the two do not collide, and a trial merge of this branch with the head of #927 completes without conflicts.

Add one test per lifecycle row of the Lemonade source: never answered, first answer, the same endpoint again, a host change, a port change, the endpoint going quiet, coming back elsewhere, the same answer twice in one tick, and moving away and back. Each row asserts the announcements, the instances a snapshot would carry, the per-id telemetry and the (id, port) pairs the vLLM scrape would poll.

The vLLM scrape target selection moves out of run_loop into vllm_scrape_targets, unchanged, so the tests can ask which endpoints would be polled.

The host-change, port-change and move-away-and-back rows pin what the code does today: the superseded instance is neither announced gone nor removed and is still a scrape target. A following change flips them.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…ort changes

The Lemonade id encodes host and port, so moving the endpoint yields a new id. The old id's telemetry was purged and the new id announced, but the old instance was never retired: it stayed in the state and in every snapshot, no InstanceGone was sent for it, and the vLLM scrape kept polling its port, because the scrape filter only skips the current Lemonade id.

On an id change the old id is now retired first (InstanceGone, removed from the state, telemetry purged) and then the new id is upserted and announced, so a snapshot carries exactly one Lemonade instance and nothing polls the endpoint it left.

The rows of the Lemonade state matrix that pinned the stale instance, and the pin in the lifecycle test, now assert the retired behaviour.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>
@jussielo-amd
jussielo-amd marked this pull request as ready for review November 5, 2026 21:46
@jussielo-amd
jussielo-amd requested a review from a team as a code owner November 5, 2026 21:46
@jussielo-amd
jussielo-amd requested review from rominf and a balanced review from Copilot November 5, 2026 21:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Only synthetic tests exercise the identity-transition branch because production fixes host/port per run, so a maintainer must confirm that ROCMAI-866 reaches this path and the dashboard symptom is resolved.

No open issues 🎉

What changed in this PR

Fixes stale Lemonade instances in the dashboard daemon by retiring the previous instance when reconciliation receives a different endpoint ID. This removes the old dashboard row and prevents its endpoint from being scraped as a vLLM server.

Changes:

  • Retire the old instance and purge its telemetry before announcing the replacement.
  • Extract vLLM scrape-target selection into a helper without changing its filtering behavior.
  • Add nine lifecycle tests and update the existing lifecycle test to assert retirement of superseded instances.
File Description
crates/​rocm-dash-daemon/​src/​runner.rs Fixes Lemonade identity-change reconciliation, extracts scrape-target selection, and expands lifecycle regression coverage.

Review effort: Balanced

@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

Superseded by #927: this pull request's commits are included in that branch (head fc67f51), and #927's description covers this change. Closing so the review happens in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants