fix(dash): give instances a real Ready status instead of a Running catch-all#106
Open
michaelroy-amd wants to merge 1 commit into
Open
fix(dash): give instances a real Ready status instead of a Running catch-all#106michaelroy-amd wants to merge 1 commit into
michaelroy-amd wants to merge 1 commit into
Conversation
…tch-all InstanceStatus had no Ready state, so merge_instance hardcoded every discovered instance to Running regardless of whether it had actually passed a readiness probe. Managed services with a "ready" record status, Lemonade endpoints (built only after a health probe), and Docker containers all collapsed into the same Running label, hiding real lifecycle differences from the TUI and any future readiness-aware consumer. Add InstanceStatus::Ready, thread a status field through DiscoveredService so each discovery source reports what it actually knows, and drop merge_instance's hardcoded override in favor of svc.status. Managed-service records now map status strings (including "recovering", previously dropped from the scrapeable set entirely) through to Ready/Running/Starting. Lemonade endpoints are built as Ready since every call site only constructs them post-health-probe. Docker-discovered instances have no status source of their own, so they stay Starting until their first successful vLLM Prometheus (or Lemonade stats) scrape promotes them to Ready. Add an is_serving() helper (Ready | Running) for TUI call sites that only care about "is this actively serving" rather than the precise state, and add Ready arms to the exhaustive status_meta/status_role matches in the instances tab. Relates to EAI-7350 Signed-off-by: Michael Roy <michael.roy@amd.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InstanceStatushad noReadystate, somerge_instancehardcoded every discovered instance toRunningregardless of whether it had actually passed a readiness probe. Managed services with a"ready"record status, Lemonade endpoints (only ever built after a health probe), and Docker-discovered containers all collapsed into the sameRunninglabel, hiding real lifecycle differences from the TUI and any future readiness-aware consumer (e.g. chat endpoint selection).Root Cause
rocm_dash_core::traits::merge_instanceunconditionally setstatus: InstanceStatus::Runningon everyInstanceit built, andInstanceStatusitself had no variant to represent "passed its readiness probe" as distinct from "process is up." Every discovery source (managed-service registry, Lemonade collector, Docker collector) therefore reported the same status label no matter what it actually knew about the endpoint's readiness.Changes
InstanceStatus::Readyand anis_serving()helper (Ready | Running) for call sites that only care about "is this actively serving."statusfield toDiscoveredServiceso each discovery source reports what it actually knows;merge_instancenow usessvc.statusinstead of a hardcodedRunning.rocm-dash-daemon::registry::discovered_from_recordmaps a managed-service record's status string (ready/running/starting/recovering) to the matchingInstanceStatus."recovering"was previously dropped from the scrapeable set entirely (a bug relative toapps/rocm/src/main.rs's own "is this service live" checks, which treatrecoveringas live) — it's now scraped and mapped toStarting.rocm-dash-collectors::lemonade::lemonade_servicebuilds asReadysince every call site only constructs it after a successful/api/v1/healthprobe.rocm-dash-collectors::dockerhas no readiness signal of its own, so Docker-discovered instances startStarting;rocm-dash-daemon::runner::run_looppromotes them toReadyon their first successful vLLM Prometheus scrape or Lemonade stats fetch (documented scope: this is the only readiness signal available for Docker-discovered instances).Readyarms to the exhaustivestatus_meta/status_rolematches incrates/rocm-dash-tui/src/ui/tabs/instances.rs; switch the== InstanceStatus::Runningequality checks inhome.rs/dock.rs/launcher.rsto.is_serving().Test Plan
cargo build -p rocm-dash-core -p rocm-dash-collectors -p rocm-dash-daemon -p rocm-dash-tuicargo clippy -p rocm-dash-core -p rocm-dash-collectors -p rocm-dash-daemon -p rocm-dash-tui --all-targets --all-features -- -D warningscargo test -p rocm-dash-core -p rocm-dash-collectors -p rocm-dash-daemon(all green, including 2 new tests:instance_status_ready_round_trips_as_lowercase_json,instance_status_is_serving_covers_ready_and_running_only, anddiscovered_from_record_maps_status_string_to_instance_status)cargo test -p rocm-dash-tui -- --test-threads=1(543 + 16 + 5 tests green)Relates to EAI-7350