diff --git a/docs/ci-hardware-testing.md b/docs/ci-hardware-testing.md index c5886620f..9369f6c42 100644 --- a/docs/ci-hardware-testing.md +++ b/docs/ci-hardware-testing.md @@ -142,10 +142,19 @@ gh workflow run e2e-selfhosted.yml --ref -f platform=app-dev-gpu ## The shared pre-warmed runtime Nearly every GPU serve scenario points its `data/runtimes` at one shared, -pre-warmed managed runtime (`E2E_SHARED_RUNTIMES_DIR`), so a multi-GiB +pre-warmed managed runtime tree (`E2E_SHARED_RUNTIMES_DIR`), so a multi-GiB `rocm install sdk` happens once per runner instead of once per scenario. The tree lives on the runner's persistent workspace and survives `git clean`. +The tree may hold **more than one** runtime — the pre-warm installs a newer one +side by side when the channel index publishes it (below) — so scenarios must not +rely on the CLI auto-selecting a runtime, which it deliberately declines to do +once two are installed. Each scenario keeps its own config dir, so the pre-warm's +`--activate` is invisible to it; the precondition steps re-activate from the +tree's own `active.json`, which lives inside the shared tree and is therefore +visible through the symlink. Without that, a serve fails with `no active ROCm +runtime is configured` while the precondition still passes. + It is a **cache with invalidation**, not a one-shot install. Each self-hosted lane calls ```bash diff --git a/tests/e2e-cucumber/src/capability.rs b/tests/e2e-cucumber/src/capability.rs index 9ca0d6a0a..de2197cff 100644 --- a/tests/e2e-cucumber/src/capability.rs +++ b/tests/e2e-cucumber/src/capability.rs @@ -196,55 +196,50 @@ pub fn collect_versions(runtimes_dir: Option<&std::path::Path>) -> PlatformVersi } /// Read the active managed runtime's `(version, install_root)` from the runtimes -/// registry: prefer the runtime named by `active.json`, else the sole installed -/// manifest. Returns `None` when nothing is installed. +/// registry. Returns `None` when the tree names no single runtime. +/// +/// Which runtime that is comes from [`crate::shared_runtime::runtime_key_to_activate`], +/// the same answer the scenarios activate — so the version this report attributes +/// a run to is the version the run actually served on. This used to fall back to +/// the first `read_dir` entry when `active.json` named nothing, which was a +/// coin flip as soon as the pre-warm started keeping a newer runtime alongside +/// the old one: the report could name one ROCm version while the serve used +/// another. Reporting no version is the better failure — an absent field reads +/// as unknown, a wrong one reads as fact. /// /// The install_root is resolved from `runtimes_dir` (the shared tree we were /// handed) as `/wheel/`, NOT from the manifest's own -/// `install_root` field. That field records the absolute path where the runtime -/// was first installed — on Strix a per-scenario temp dir that no longer exists -/// by report time — so trusting it made `vllm`/`lemonade` probe a dead path and -/// come back `None`. On MI300X the two coincide (prewarm installs in place), -/// which is why it worked there but not on Strix. Falls back to the manifest -/// path if the derived one is absent, for any tree that predates the wheel layout. +/// `install_root` field, which records where the runtime was first installed and +/// need not be where it lives now. On MI300X the two coincide (the pre-warm +/// installs in place); on Strix they did not, and trusting the field made +/// `vllm`/`lemonade` probe a dead path and report no versions at all. +/// +/// The manifest fallback is load-bearing, not legacy — do not read it as dead +/// code. `MANAGED_RUNTIME_FORMATS` is `["wheel", "tarball"]`, and a tarball +/// runtime lives at `/tarball/`, which the derived `wheel` path never +/// matches. For those the fallback is the only correct answer. It also still +/// covers a tree predating the `wheel/` layout. +/// +/// One shape can no longer reach here: a runtime whose recorded root left the +/// tree entirely. `runtime_key_to_activate` now filters those out, so the +/// fallback yields an in-tree path or nothing. Where every entry is such a +/// corpse this returns `None` and the report simply omits the version — absent +/// reads as unknown, which is the honest outcome; a wrong version reads as fact. fn active_runtime_install_root( runtimes_dir: &std::path::Path, ) -> Option<(String, std::path::PathBuf)> { - let registry = runtimes_dir.join("registry"); - let entries: Vec = std::fs::read_dir(®istry) - .ok()? - .filter_map(|e| e.ok().map(|e| e.path())) - .filter(|p| p.extension().is_some_and(|x| x == "json")) - .collect(); - // Prefer the active runtime's key if active.json names one. - let active_key = std::fs::read_to_string(runtimes_dir.join("active.json")) - .ok() - .and_then(|t| { - serde_json::from_str::(&t) - .ok()? - .get("runtime_key")? - .as_str() - .map(str::to_owned) - }); - let pick = entries - .iter() - .find(|p| { - active_key - .as_deref() - .is_some_and(|k| p.file_stem().and_then(|s| s.to_str()) == Some(k)) - }) - .or_else(|| entries.first())?; + let key = crate::shared_runtime::runtime_key_to_activate(runtimes_dir)?; + let manifest = runtimes_dir.join("registry").join(format!("{key}.json")); let json: serde_json::Value = - serde_json::from_str(&std::fs::read_to_string(pick).ok()?).ok()?; + serde_json::from_str(&std::fs::read_to_string(manifest).ok()?).ok()?; let version = json.get("version")?.as_str()?.to_owned(); - // Runtime key = the manifest file stem (e.g. release-wheel-gfx1151-7-13-0). - let key = pick.file_stem().and_then(|s| s.to_str()); // Resolve the root inside the shared tree first; fall back to the manifest's // recorded install_root only if that derived path doesn't exist. - let derived = key.map(|k| runtimes_dir.join("wheel").join(k)); - let root = match derived { - Some(d) if d.is_dir() => d, - _ => std::path::PathBuf::from(json.get("install_root")?.as_str()?), + let derived = runtimes_dir.join("wheel").join(&key); + let root = if derived.is_dir() { + derived + } else { + std::path::PathBuf::from(json.get("install_root")?.as_str()?) }; Some((version, root)) } @@ -789,4 +784,57 @@ Local model engines "strix-halo-wsl" ); } + + fn write_manifest(runtimes_dir: &std::path::Path, key: &str, version: &str) { + let registry = runtimes_dir.join("registry"); + std::fs::create_dir_all(®istry).expect("create registry"); + let install_root = runtimes_dir.join("wheel").join(key); + std::fs::create_dir_all(&install_root).expect("create install root"); + std::fs::write( + registry.join(format!("{key}.json")), + serde_json::json!({ + "runtime_key": key, + "version": version, + "install_root": install_root, + }) + .to_string(), + ) + .expect("write manifest"); + } + + /// The report must name the ROCm version the run actually served on. The + /// pre-warm keeps a newer runtime beside the old one, so picking whichever + /// manifest `read_dir` yielded first could attribute a run to the version it + /// did NOT use — and read as fact. + #[test] + fn reports_the_active_runtimes_version_when_several_are_installed() { + let tmp = tempfile::TempDir::with_prefix("capability-").expect("temp dir"); + let dir = tmp.path(); + write_manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0", "7.13.0"); + write_manifest(dir, "release-wheel-multi-arch-7-14-0", "7.14.0"); + std::fs::write( + dir.join("active.json"), + r#"{"runtime_key": "release-wheel-multi-arch-7-14-0"}"#, + ) + .expect("write marker"); + + let (version, root) = active_runtime_install_root(dir).expect("a runtime is named"); + assert_eq!(version, "7.14.0"); + assert_eq!( + root, + dir.join("wheel").join("release-wheel-multi-arch-7-14-0") + ); + } + + /// Several runtimes and no marker: report nothing rather than guess. An + /// absent version reads as unknown; a wrong one reads as fact. + #[test] + fn reports_no_version_when_the_tree_names_no_runtime() { + let tmp = tempfile::TempDir::with_prefix("capability-").expect("temp dir"); + let dir = tmp.path(); + write_manifest(dir, "release-wheel-gfx94x-dcgpu-7-13-0", "7.13.0"); + write_manifest(dir, "release-wheel-multi-arch-7-14-0", "7.14.0"); + + assert!(active_runtime_install_root(dir).is_none()); + } } diff --git a/tests/e2e-cucumber/src/lib.rs b/tests/e2e-cucumber/src/lib.rs index 7fccc5695..0d01ba4de 100644 --- a/tests/e2e-cucumber/src/lib.rs +++ b/tests/e2e-cucumber/src/lib.rs @@ -13,6 +13,7 @@ pub mod panic_capture; pub mod reader_failure; pub mod send_until; pub mod serve_log; +pub mod shared_runtime; use std::path::{Path, PathBuf}; diff --git a/tests/e2e-cucumber/src/shared_runtime.rs b/tests/e2e-cucumber/src/shared_runtime.rs new file mode 100644 index 000000000..df9ec0a0a --- /dev/null +++ b/tests/e2e-cucumber/src/shared_runtime.rs @@ -0,0 +1,440 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Pick which runtime a scenario should activate out of a shared pre-warm tree. +//! +//! Scenarios that only need *a* runtime present point their `data/runtimes` at a +//! shared, install-once tree (see `E2eWorld::use_shared_runtimes`). Serving out of +//! that tree used to need no further wiring: the CLI auto-selects a runtime when +//! exactly one is installed, and the tree held exactly one. +//! +//! That stopped being true once the pre-warm started adopting a newer runtime +//! side by side with the old one. The CLI's auto-selection is deliberately +//! all-or-nothing — with two installed it refuses to guess and serve fails with +//! "no active ROCm runtime is configured" — so a tree holding more than one +//! silently broke every GPU serve scenario. The count is not something the suite +//! controls: it follows whatever the upstream channel index has published. +//! +//! So the scenario names its runtime explicitly instead of relying on the count. +//! The pre-warm activates the runtime it installed and the CLI records that in +//! `/active.json`, which lives *inside* the shared tree and is therefore +//! visible through the symlink even though each scenario keeps its own config dir. +//! Reading it back is what makes selection deterministic for any tree contents. +//! +//! Naming a runtime is not the same as naming a *usable* one, which is the second +//! half of this. A registry entry can outlive the folder it points at, and a name +//! the CLI rejects fails the scenario just as surely as no name at all — so what +//! gets named is filtered down to the entries `activate` can accept. + +use std::path::Path; + +/// The runtime key a scenario should activate for a shared tree at `runtimes_dir`. +/// +/// `Some(key)` means "run `rocm runtimes activate `"; `None` means there is +/// nothing to name and the caller should leave selection to the CLI — either the +/// tree is empty (the scenario installs its own) or it holds exactly one runtime, +/// which the CLI already auto-selects. +/// +/// Chooses only among [`activatable_runtime_keys`] — entries the CLI would +/// actually accept — so neither branch below can name a runtime that fails to +/// activate. +/// +/// Prefers `active.json` because that records the runtime the pre-warm actually +/// installed and verified — but only when the registry still holds it, so a +/// marker left behind by a pruned runtime doesn't strand the scenario. Falls back +/// to the sole registry manifest so a tree written before the pre-warm learned to +/// activate still resolves. When neither names one runtime, returns `None` rather +/// than picking arbitrarily: choosing the wrong one of several would serve against +/// an unintended ROCm version, and a clear "no active runtime" failure beats a +/// silently mismatched pass. +#[must_use] +pub fn runtime_key_to_activate(runtimes_dir: &Path) -> Option { + let installed = activatable_runtime_keys(runtimes_dir); + active_runtime_key(runtimes_dir) + .filter(|key| installed.iter().any(|installed| installed == key)) + .or_else(|| match installed.as_slice() { + [only] => Some(only.clone()), + _ => None, + }) +} + +/// The `runtime_key` recorded in `/active.json`, if it names one. +fn active_runtime_key(runtimes_dir: &Path) -> Option { + let text = std::fs::read_to_string(runtimes_dir.join("active.json")).ok()?; + let key = serde_json::from_str::(&text) + .ok()? + .get("runtime_key")? + .as_str()? + .trim() + .to_owned(); + (!key.is_empty()).then_some(key) +} + +/// The registry keys `rocm runtimes activate` would actually accept. +/// +/// A registry entry is not enough: the CLI refuses to activate a runtime whose +/// recorded `install_root` is gone ("install root is missing"). That is not +/// hypothetical here — a scenario that installs through its own `data/runtimes` +/// symlink records its per-scenario temp dir as the install root, and the folder +/// dies with the scenario, leaving a registry entry pointing at nothing (see +/// rocm-cli#315/#316). The pre-warm normally evicts those, but its repair is +/// deliberately non-fatal and skips the whole tree when `runtimes list` itself +/// fails — which a poisoned entry makes it do. So the suite must expect to meet +/// one and step over it rather than name it and fail. +/// +/// Judged by the same rule the pre-warm's own repair uses: an install root +/// inside this tree is sound, one outside it is a corpse. Checking existence +/// alone would be wrong — on a runner where a foreign path happens to exist the +/// scenario would serve against a runtime outside the shared tree. +fn activatable_runtime_keys(runtimes_dir: &Path) -> Vec { + let roots = comparable_roots(runtimes_dir); + registry_runtime_keys(runtimes_dir) + .into_iter() + .filter(|key| { + let Ok(text) = + std::fs::read_to_string(runtimes_dir.join("registry").join(format!("{key}.json"))) + else { + return false; + }; + let Ok(json) = serde_json::from_str::(&text) else { + return false; + }; + // No recorded root: nothing to disqualify it on, so keep it and let + // the CLI have the final say. + let Some(root) = json.get("install_root").and_then(|v| v.as_str()) else { + return true; + }; + let root = Path::new(root); + roots.iter().any(|base| root.starts_with(base)) + }) + .collect() +} + +/// Every spelling of `runtimes_dir` a recorded `install_root` might match. +/// +/// The two sides are resolved-vs-as-given by construction, so one comparison is +/// not enough. The CLI canonicalizes an install root before recording it, while +/// `E2E_SHARED_RUNTIMES_DIR` reaches us verbatim — `validated_shared_dir` checks +/// that it is absolute and free of `..`, and deliberately does not resolve it. +/// Reach the tree through a symlinked component of the workspace and the two +/// spellings differ, so *every healthy* entry looks out-of-tree: selection comes +/// back empty and the serve fails with the very error this module exists to +/// prevent — silently, since declining to activate is best-effort by design. +/// +/// This mirrors `xtask::e2e_prewarm::assess`, which compares against both +/// spellings for the same reason. Windows needs the third: `canonicalize` yields +/// a `\\?\` verbatim path there and the CLI records a plain one, so the prefix +/// has to come off or the comparison fails on it rather than on the folder. That +/// is the same trap `runtime_steps::linked_runtimes_target` documents, and 8.3 +/// short paths (which `canonicalize` expands) are why resolving matters even +/// when no symlink is involved. +fn comparable_roots(runtimes_dir: &Path) -> Vec { + let mut roots = vec![runtimes_dir.to_path_buf()]; + let Ok(resolved) = runtimes_dir.canonicalize() else { + return roots; + }; + for candidate in [strip_verbatim_prefix(&resolved), resolved] { + if !roots.contains(&candidate) { + roots.push(candidate); + } + } + roots +} + +/// Drop Windows' `\\?\` verbatim prefix, which never `starts_with`-matches the +/// plain path the CLI records. A no-op on every other platform. +fn strip_verbatim_prefix(path: &Path) -> std::path::PathBuf { + let text = path.to_string_lossy(); + if let Some(rest) = text.strip_prefix(r"\\?\UNC\") { + return std::path::PathBuf::from(format!(r"\\{rest}")); + } + match text.strip_prefix(r"\\?\") { + Some(rest) => std::path::PathBuf::from(rest), + None => path.to_path_buf(), + } +} + +/// Every runtime key present in the registry, empty when it is absent. +/// +/// Public so a caller that declines to activate can name what it found: "no +/// runtime to activate" is only actionable alongside the list it chose from. +/// Deliberately unfiltered — a diagnostic should report the tree as it is, so a +/// key skipped by [`activatable_runtime_keys`] still shows up in the message. +#[must_use] +pub fn registry_runtime_keys(runtimes_dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(runtimes_dir.join("registry")) else { + return Vec::new(); + }; + entries + .filter_map(|entry| { + let path = entry.ok()?.path(); + if path.extension().is_some_and(|ext| ext == "json") { + // Runtime key = the manifest file stem, the same convention + // `capability::active_runtime_install_root` relies on. + path.file_stem()?.to_str().map(str::to_owned) + } else { + None + } + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn write(path: &Path, body: &str) { + std::fs::create_dir_all(path.parent().expect("path has a parent")).expect("create dir"); + std::fs::write(path, body).expect("write file"); + } + + /// A runtime installed in place: its recorded root lives inside the tree. + /// + /// This is the shape the CLI really writes, so it is what the tests use. + /// A manifest with no `install_root` at all takes the "nothing to judge on" + /// branch and would pass whether or not the root is checked; since + /// `InstalledRuntimeManifest::install_root` is non-optional and + /// `load_runtime_manifests` drops anything that fails to deserialize, such a + /// manifest cannot arise in CI either. Testing against it proved nothing. + fn installed(dir: &Path, key: &str) { + let root = dir.join("wheel").join(key); + std::fs::create_dir_all(&root).expect("create install root"); + // Record the CANONICAL root, because that is what the CLI writes. It + // matters on Windows, where a temp dir is handed to us in 8.3 short form + // (`RUNNER~1`) and canonicalizing expands it: writing the short spelling + // here would test a manifest the CLI never produces, and the two spellings + // do not `starts_with`-match. + let root = root.canonicalize().expect("canonicalize install root"); + write( + &dir.join("registry").join(format!("{key}.json")), + &serde_json::json!({ "runtime_key": key, "install_root": root }).to_string(), + ); + } + + /// A runtime whose recorded root is a dead per-scenario temp dir — the + /// rocm-cli#315 poisoning the pre-warm failed to evict. + fn poisoned(dir: &Path, key: &str) { + write( + &dir.join("registry").join(format!("{key}.json")), + &serde_json::json!({ + "runtime_key": key, + "install_root": format!("/tmp/rocm-e2e-gone/data/runtimes/wheel/{key}"), + }) + .to_string(), + ); + } + + fn active(dir: &Path, key: &str) { + write( + &dir.join("active.json"), + &format!(r#"{{"runtime_key": "{key}"}}"#), + ); + } + + /// The regression this module exists for: a tree the pre-warm grew a second + /// runtime in must still name one, or every GPU serve scenario fails with + /// "no active ROCm runtime is configured". + #[test] + fn names_the_active_runtime_when_several_are_installed() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + active(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-multi-arch-7-14-0") + ); + } + + /// A tree from before the pre-warm activated: one runtime, no marker. The + /// CLI auto-selects here, so naming it is optional — but resolving it keeps + /// the step's behaviour identical whether or not a marker was written. + #[test] + fn falls_back_to_the_sole_manifest_without_a_marker() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-gfx94x-dcgpu-7-13-0") + ); + } + + /// Several runtimes and no marker: refuse to guess. Activating an arbitrary + /// one would serve against an unintended ROCm version and pass, which is + /// worse than the CLI's own explicit failure. + #[test] + fn refuses_to_guess_between_several_without_a_marker() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!(runtime_key_to_activate(dir), None); + } + + /// An empty tree is the first scenario on a cold runner; it installs its own + /// runtime, so there is nothing to activate beforehand. + #[test] + fn names_nothing_for_an_empty_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + assert_eq!(runtime_key_to_activate(tmp.path()), None); + } + + /// A marker that survives the runtime it names (a prune, a hand-cleaned tree) + /// must not strand the scenario: fall through to the registry. + #[test] + fn ignores_a_marker_naming_no_installed_runtime() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + active(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-gfx94x-dcgpu-7-13-0") + ); + } + + /// Corrupt or half-written markers are treated as absent, not fatal: the + /// registry still answers the question. + #[test] + fn treats_an_unreadable_marker_as_absent() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + write(&dir.join("active.json"), "{ not json"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-gfx94x-dcgpu-7-13-0") + ); + } + + /// The regression from the first attempt at this fix: the sole *registry* + /// entry was a corpse pointing at a dead per-scenario temp dir, so naming it + /// made every activate fail with "install root is missing" — the suite + /// traded one silent breakage for a louder one. Skip it and name the runtime + /// that is really there. + #[test] + fn skips_a_runtime_whose_install_root_left_the_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-multi-arch-7-14-0") + ); + } + + /// Reaching the tree through a symlinked parent must not disqualify every + /// healthy runtime in it. + /// + /// The CLI records a canonicalized `install_root`; `E2E_SHARED_RUNTIMES_DIR` + /// arrives unresolved. Compare only the spelling we were handed and the two + /// never match, so selection comes back empty and the serve fails with the + /// error this module exists to prevent — the original bug through a different + /// door, and silent. Approximates a symlinked component of the runner's + /// workspace, which is the shape that would trigger it in CI. + #[test] + fn resolves_a_tree_reached_through_a_symlinked_parent() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let real = tmp.path().join("real"); + std::fs::create_dir_all(&real).expect("create real tree"); + // The manifest records the resolved root, as the CLI writes it. + installed(&real, "release-wheel-multi-arch-7-14-0"); + + let link = tmp.path().join("link"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + #[cfg(windows)] + std::os::windows::fs::symlink_dir(&real, &link).expect("symlink"); + + // Selection is handed the unresolved spelling, exactly as CI would. + assert_eq!( + runtime_key_to_activate(&link).as_deref(), + Some("release-wheel-multi-arch-7-14-0"), + "a tree reached through a symlink must still resolve its runtimes" + ); + } + + /// The symlink tolerance must not become "any path anywhere": a corpse + /// pointing outside the tree stays disqualified when the tree is reached + /// through a link, or the fix above would have re-admitted every poisoned + /// entry it was meant to skip. + #[test] + fn still_skips_a_corpse_when_the_tree_is_reached_through_a_symlink() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let real = tmp.path().join("real"); + std::fs::create_dir_all(&real).expect("create real tree"); + poisoned(&real, "release-wheel-gfx94x-dcgpu-7-13-0"); + + let link = tmp.path().join("link"); + #[cfg(unix)] + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + #[cfg(windows)] + std::os::windows::fs::symlink_dir(&real, &link).expect("symlink"); + + assert_eq!(runtime_key_to_activate(&link), None); + } + + /// A marker naming a poisoned runtime must not override a sound one either: + /// the pre-warm activates before a scenario poisons the tree, so the stale + /// marker outlives the runtime it names. + #[test] + fn ignores_a_marker_naming_a_runtime_that_left_the_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + active(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!( + runtime_key_to_activate(dir).as_deref(), + Some("release-wheel-multi-arch-7-14-0") + ); + } + + /// Every runtime is a corpse: name none. Letting the CLI report "no active + /// ROCm runtime is configured" beats an activate that cannot succeed. + #[test] + fn names_nothing_when_every_runtime_left_the_tree() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!(runtime_key_to_activate(dir), None); + } + + /// The diagnostic reports the tree as it is: a skipped runtime still has to + /// appear, or "no runtime to activate" names an empty tree that isn't empty. + #[test] + fn reports_a_skipped_runtime_in_the_registry_listing() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + poisoned(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + + assert_eq!( + registry_runtime_keys(dir), + vec!["release-wheel-gfx94x-dcgpu-7-13-0"] + ); + } + + /// A marker with a blank key names nothing. + #[test] + fn treats_a_blank_marker_key_as_absent() { + let tmp = tempfile::TempDir::with_prefix("shared-runtime-").expect("temp dir"); + let dir = tmp.path(); + installed(dir, "release-wheel-gfx94x-dcgpu-7-13-0"); + installed(dir, "release-wheel-multi-arch-7-14-0"); + active(dir, " "); + + assert_eq!(runtime_key_to_activate(dir), None); + } +} diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 9a28e44d3..57c1d7643 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -141,8 +141,8 @@ fn shared_uv_cache_dir() -> Option { validated_shared_dir("E2E_SHARED_UV_CACHE_DIR") } -/// A persistent directory holding ONE installed managed-runtime tree -/// (`runtimes/registry/*` + the TheRock venv) shared across scenarios that only +/// A persistent directory holding the installed managed-runtime trees +/// (`runtimes/registry/*` + the TheRock venvs) shared across scenarios that only /// need *a* runtime active. Set by CI (`E2E_SHARED_RUNTIMES_DIR`) on the runner's /// persistent disk; unset for local runs, where every scenario installs its own. /// @@ -154,10 +154,14 @@ fn shared_uv_cache_dir() -> Option { /// (see [`E2eWorld::use_shared_runtimes`]) so the install happens once per runner. /// Scenarios that ASSERT a clean slate ("a machine with no CLI-managed runtimes", /// "Installing the SDK") deliberately do NOT opt in — they keep their empty -/// isolated runtimes dir. A serve resolves the shared runtime via -/// `single_ready_runtime` (no active-key wiring needed) and `runtimes list` -/// reports it `status=ready`, so both the precondition and serve are satisfied -/// (verified by hand on MI300X: serve + chat completion through a symlinked tree). +/// isolated runtimes dir. +/// +/// The tree may hold MORE THAN ONE runtime: `xtask e2e-prewarm` installs a newer +/// one side by side when the channel index publishes it, so the count tracks +/// upstream releases rather than anything the suite controls. A serve therefore +/// cannot lean on the CLI's `single_ready_runtime` fallback, which deliberately +/// refuses to guess once two are installed — the scenario names its runtime +/// explicitly instead (see [`E2eWorld::activate_shared_runtime`]). fn shared_runtimes_dir() -> Option { validated_shared_dir("E2E_SHARED_RUNTIMES_DIR") } @@ -362,6 +366,57 @@ impl E2eWorld { Ok(real) } + /// Point this scenario's config at a specific runtime in the shared tree. + /// + /// The shared tree is reached through a symlinked `data/runtimes`, but the + /// config dir stays per-scenario — so the activation `xtask e2e-prewarm` + /// performed is invisible here and every scenario starts with no active + /// runtime. That was harmless only while the CLI could auto-select, which it + /// stops doing as soon as the tree holds a second runtime (serve then fails + /// with "no active ROCm runtime is configured"). Re-activating from the + /// tree's own `active.json` makes the choice explicit and independent of how + /// many runtimes the pre-warm has accumulated. + /// + /// Also writes the shared tree's `active.json`, because for a symlinked + /// scenario that marker IS the shared one — the activation is not confined + /// to this scenario's config. + /// + /// Best-effort: a no-op for local runs (no shared tree), and it does not + /// fail the scenario when the tree names no runtime or the activation is + /// refused. Both of those leave the serve that follows failing with "no + /// active ROCm runtime is configured", and NEITHER precondition assertion + /// can see it — `installed: none` reports an empty registry, not an + /// unset active key, and `engines list` scans every manifest regardless of + /// which is active. So say what happened on stderr instead of failing here: + /// the serve's own failure is the one worth reading, and this is the line + /// that explains it. + pub fn activate_shared_runtime(&self) { + let Some(shared) = shared_runtimes_dir() else { + return; + }; + let Some(key) = e2e_cucumber::shared_runtime::runtime_key_to_activate(&shared) else { + eprintln!( + "shared runtime: no runtime to activate in {}; a serve will fail unless exactly \ + one is installed. Installed: {:?}", + shared.display(), + e2e_cucumber::shared_runtime::registry_runtime_keys(&shared) + ); + return; + }; + let (stdout, stderr, rc) = run_rocm(self, &["runtimes", "activate", &key]); + if rc != 0 { + eprintln!( + "shared runtime: {}", + e2e_cucumber::cli_failure_report( + &["runtimes", "activate", &key], + rc, + &stdout, + &stderr + ) + ); + } + } + /// Plant a fake pre-existing (non-CLI) ROCm install in the scenario's isolated /// tree and record its path so `isolate_cmd` exports it as `ROCM_PATH`. The /// CLI's `detect_legacy_rocm_summary` treats any directory containing a known diff --git a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs index bec98c56c..caae41986 100644 --- a/tests/e2e-cucumber/tests/e2e/runtime_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/runtime_steps.rs @@ -129,6 +129,12 @@ async fn setup_active_runtime(world: &mut E2eWorld) { if stdout.contains("installed: none") { crate::run_rocm_ok(world, &["install", "sdk"]); } + // Name the runtime rather than leaving the CLI to infer it: the shared tree + // grows a second runtime whenever the channel index publishes one, and the + // CLI refuses to auto-select from more than one (see + // `E2eWorld::activate_shared_runtime`). Without this the step still passes — + // a runtime IS present — and the serve that follows fails instead. + world.activate_shared_runtime(); let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); assert!( !stdout.contains("installed: none"), @@ -147,6 +153,11 @@ async fn setup_runtime_with_engine(world: &mut E2eWorld) { if stdout.contains("installed: none") { crate::run_rocm_ok(world, &["install", "sdk"]); } + // Same reason as `a managed runtime is active`: pin the runtime explicitly, + // or the serve that follows refuses to pick one. Not for `assert_engine_ready` + // below — `engines list` scans every registered manifest and never consults + // the active key, which is exactly why it cannot stand in for this call. + world.activate_shared_runtime(); assert_engine_ready(world); } @@ -275,6 +286,16 @@ async fn user_checks_for_updates(world: &mut E2eWorld) { world.cli_rc = Some(rc); } +/// The runtime key `runtimes list` reports as active, if any. +fn active_runtime_key(world: &E2eWorld) -> Option { + let (stdout, _, _) = crate::run_rocm(world, &["runtimes", "list"]); + let key = stdout + .lines() + .find_map(|line| line.trim().strip_prefix("active_runtime_key:"))? + .trim(); + (!key.is_empty() && key != "").then(|| key.to_owned()) +} + /// Freshness verdicts `runtime_update_plan` can emit, plus the degraded `error` /// form used when the index cannot be reached. `xtask e2e-prewarm` routes on /// exactly these, so a rename here must break this scenario rather than silently @@ -288,11 +309,36 @@ async fn assert_update_reports_freshness(world: &mut E2eWorld) { assert_eq!(rc, 0, "`rocm update` failed:\n{stdout}"); // The line `xtask e2e-prewarm` parses: `runtime ... status=`. - let Some(line) = stdout - .lines() - .map(str::trim) - .find(|line| line.starts_with("runtime ")) - else { + // The report carries one such line per installed runtime, newest first, and + // the shared tree holds more than one — so select the ACTIVE runtime's line + // rather than whichever came first, or this scenario reports on a runtime the + // run never used. + let active = active_runtime_key(world); + let runtime_lines = || { + stdout + .lines() + .map(str::trim) + .filter(|line| line.starts_with("runtime ")) + }; + let line = match active.as_deref() { + // Something is active: assert on ITS line or not at all. Falling back to + // the first line here would report on a runtime the run did not use — + // the misattribution this selection exists to remove — and it would pass + // while doing it. + Some(key) => { + let found = runtime_lines().find(|line| line.split_whitespace().nth(1) == Some(key)); + assert!( + found.is_some(), + "runtime `{key}` is active but the update report has no `runtime {key} …` \ + line:\n{stdout}" + ); + found + } + // Nothing active: the single-runtime case this scenario was written + // against, where the sole line is unambiguously the right one. + None => runtime_lines().next(), + }; + let Some(line) = line else { panic!("no `runtime …` line in the update report:\n{stdout}"); }; let status = line