diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 6c92c9a49..656aabb58 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -727,7 +727,14 @@ fn check_5_amdgpu_blacklisted(e: &Examination, symptom: &str) -> Diagnosis { summary: "Remove amdgpu from any modprobe blacklist and load it.".to_owned(), commands, needs_sudo: true, - needs_reboot: !blacklisted.is_empty(), + // Catalog-aligned, not state-derived: fix.rs's FixRecipe for this fix-id sets + // needs_reboot unconditionally. On this no-blacklist path the plan is just + // `modprobe amdgpu` (or that plus a Secure Boot signing note, whose own + // remedy can require a reboot) -- diagnose conforms to the catalog rather + // than the reverse (see fix.rs's assert_needs_reboot_matches_the_catalog) so + // `rocm diagnose` and `rocm fix` never disagree; over-warning is limited to + // the plain no-Secure-Boot sub-case, judged cheaper than the drift it replaces. + needs_reboot: true, fix_id: "fix-5-amdgpu-load".to_owned(), auto_applicable: false, verify: "lsmod | grep amdgpu && rocminfo | head -n 5".to_owned(), @@ -2976,6 +2983,56 @@ mod tests { ); } + fn assert_fix_5_needs_reboot(e: &Examination) { + let report = diagnose(e, ""); + let hit = report + .matched + .iter() + .find(|d| d.id == "fix-5-amdgpu-load") + .expect("amdgpu-not-loaded should be diagnosed"); + let fix = hit.fix.as_ref().unwrap(); + crate::fix::assert_needs_reboot_matches_the_catalog("fix-5-amdgpu-load", fix.needs_reboot); + + let text = render_report_text(&report, report.matched.len()); + let lines: Vec<&str> = text.lines().collect(); + let id_line = lines + .iter() + .position(|l| l.trim_start() == "id: fix-5-amdgpu-load") + .expect("fix-5-amdgpu-load should appear in the rendered report"); + let flags_line = lines[id_line..] + .iter() + .find(|l| l.trim_start().starts_with("flags:")) + .expect("fix-5-amdgpu-load should have a flags: line"); + assert_eq!( + flags_line.trim_start(), + "flags: requires sudo, requires reboot, manual only (`rocm fix` will NOT run it automatically)", + "rendered flags: line for fix-5-amdgpu-load: {flags_line}" + ); + } + + #[test] + fn fix_5_amdgpu_load_needs_reboot_matches_catalog_when_blacklisted() { + let mut e = linux_base(); + e.amdgpu_loaded = Some(false); + e.amdgpu_blacklisted_in = vec!["/etc/modprobe.d/blacklist.conf".to_owned()]; + assert_fix_5_needs_reboot(&e); + } + + #[test] + fn fix_5_amdgpu_load_needs_reboot_matches_catalog_when_not_blacklisted() { + // `check_5_amdgpu_blacklisted` used to compute `needs_reboot` as + // `!blacklisted.is_empty()`, so this case (module simply not loaded, + // no blacklist entry involved) used to report `false` here while the + // `fix-5-amdgpu-load` FixRecipe catalog said `true` unconditionally -- + // the drift issue #418 fixed. Pin the now-unconditional `true` so a + // regression back to the conditional fails here rather than only + // being visible by eyeballing `rocm diagnose` vs `rocm fix` output. + let mut e = linux_base(); + e.amdgpu_loaded = Some(false); + assert!(e.amdgpu_blacklisted_in.is_empty()); + assert_fix_5_needs_reboot(&e); + } + #[test] fn arch_covered_is_negative_and_filtered() { let mut e = linux_base(); diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index ce46bd2b8..acf22e88b 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -704,6 +704,25 @@ pub(crate) fn assert_plan_matches_the_catalog_copy(fix_id: &str, commands: &[&st ); } +/// Assert that the `needs_reboot` a [`crate::diagnose::Fix`] carries matches +/// the catalog recipe's. +/// +/// Same hand-maintained-copies problem as [`assert_plan_matches_the_catalog_copy`], +/// for a single flag instead of the command block: `FixRecipe` and `Fix` set +/// `needs_reboot` independently, so a silent divergence means `rocm diagnose` +/// and `rocm fix ` tell a user different things about the same fix-id +/// (this is what happened with `fix-5-amdgpu-load` before it was closed here). +#[cfg(test)] +pub(crate) fn assert_needs_reboot_matches_the_catalog(fix_id: &str, needs_reboot: bool) { + let recipe = find_recipe(fix_id) + .unwrap_or_else(|| panic!("{fix_id}: no catalog recipe to compare against")); + assert_eq!( + recipe.needs_reboot, needs_reboot, + "{fix_id}: diagnose's needs_reboot has drifted from the catalog recipe \ + `rocm fix` reports; a user may see either surface for the same fix-id" + ); +} + fn find_recipe(fix_id: &str) -> Option<&'static FixRecipe> { RECIPES.iter().find(|r| r.fix_id == fix_id) } @@ -768,11 +787,12 @@ pub fn list_recipes() -> String { /// values render as the same text from either command. This only /// standardizes wording, not the underlying values: `FixRecipe` (fix.rs) and /// diagnose's `Fix` still supply those independently, so a fix-id's rendered -/// flags can still differ if the two disagree on a value (known example: -/// `fix-5-amdgpu-load`'s `needs_reboot`). Also out of scope: the bare -/// `rocm fix` catalog listing (`list_recipes`) describes the same -/// `auto_applicable` property with a separate, untouched AUTO/PRINT-ONLY -/// vocabulary. +/// flags can still differ if the two disagree on a value; `assert_needs_reboot_matches_the_catalog` +/// and `assert_plan_matches_the_catalog_copy` are targeted regression tests +/// that pin specific fix-ids against that drift, not a blanket guarantee for +/// every fix-id. Also out of scope: the bare `rocm fix` catalog listing +/// (`list_recipes`) describes the same `auto_applicable` property with a +/// separate, untouched AUTO/PRINT-ONLY vocabulary. // These mirror the `FixRecipe`/`Fix` struct fields, where // `clippy::struct_excessive_bools` is already allowed workspace-wide; that // allow doesn't reach this free function's parameters, so @@ -1712,4 +1732,34 @@ mod tests { ); } } + + #[test] + fn needs_reboot_true_fix_ids_match_the_known_set() { + // assert_needs_reboot_matches_the_catalog only ever runs for + // fix-5-amdgpu-load, so the other 23 fix-ids' hardcoded needs_reboot + // literals in diagnose.rs have no guard against drifting from the + // catalog. This doesn't reach into diagnose.rs, but it does catch an + // accidental catalog edit and pins the expected set by name so a + // deliberate catalog change forces a look at diagnose.rs's matching + // literals too. + let expected: std::collections::BTreeSet<&str> = [ + "fix-3-rocm-kernel", + "fix-5-amdgpu-load", + "fix-11-iommu", + "fix-12-installer", + "fix-14-adrenalin-too-old", + ] + .into_iter() + .collect(); + let actual: std::collections::BTreeSet<&str> = RECIPES + .iter() + .filter(|r| r.needs_reboot) + .map(|r| r.fix_id) + .collect(); + assert_eq!( + actual, expected, + "the catalog's needs_reboot:true set has changed -- update diagnose.rs's \ + matching literals and this test's expected set together" + ); + } } diff --git a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs index fb6d2da6a..2c59e6de1 100644 --- a/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs +++ b/tests/e2e-cucumber/tests/e2e/diagnose_steps.rs @@ -438,8 +438,9 @@ async fn assert_every_cause_has_flags(world: &mut E2eWorld) { // the always-on auto/manual marker) rather than a specific fix-id's exact // flags: the top match is environment-dependent, and a shared vocabulary // doesn't guarantee diagnose and the fix.rs catalog agree on the - // underlying values for a given fix-id (known drift: fix-5-amdgpu-load's - // needs_reboot). + // underlying values for a given fix-id -- unit tests in diagnose.rs call + // fix.rs's `assert_needs_reboot_matches_the_catalog` to pin per-fix-id + // values against the catalog for that. let causes = output.lines().filter(|l| l.contains("score=")).count(); assert!(causes > 0, "no scored causes to check:\n{output}"); let flag_lines: Vec<&str> = output