feat(breakfix): BFX break-fix suite (BFX01-BFX06) with NICo coverage - #562
Open
abegnoche wants to merge 8 commits into
Open
feat(breakfix): BFX break-fix suite (BFX01-BFX06) with NICo coverage#562abegnoche wants to merge 8 commits into
abegnoche wants to merge 8 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded BFX01–BFX06 validations for bare-metal and Kubernetes suites. Added NICo API-backed queries with structured gaps and my-isv demo templates. Added provider configuration, suite wiring, documentation, labels, and tests. ChangesBreak-fix validation contracts
NICo provider workflows
my-isv provider workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BareMetalConfig
participant MaintenanceEvents
participant CommonHelpers
participant NICoREST
participant MaintenanceEventsCheck
BareMetalConfig->>MaintenanceEvents: invoke query command
MaintenanceEvents->>CommonHelpers: list_site_machines
CommonHelpers->>NICoREST: retrieve paginated site machines
MaintenanceEvents->>MaintenanceEventsCheck: emit normalized event result
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
abegnoche
force-pushed
the
cursor/bfx-remediation-suite-4f18
branch
from
July 30, 2026 15:23
85d9bae to
52bc942
Compare
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Introduce suites/remediation.yaml covering all open M8 break-fix issues (BFX01-BFX06): validation classes, my-isv demo stubs, and NICo provider wiring. Move BFX03-01 (HardwareSerialCheck) out of bare_metal into the new plain suite with capability gating per PR #561. NICo implements read-only observability where the tenant REST API exposes signals (maintenance events, repair history, serial inventory). Mutating lifecycle actions, GPUd/Sentinel/Maestro agents, NV switch firmware, retirement notices, and tenant notification channels emit structured skips with documented gap IDs. Stacks on PR #561 (capability/requires model). Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
abegnoche
force-pushed
the
cursor/bfx-remediation-suite-4f18
branch
from
August 4, 2026 20:10
52bc942 to
4a7e1b9
Compare
Break-fix is mostly bare-metal plus k8s-only cordon/GPU reset, so a plain remediation suite is not justified the way storage is. Wire BM BFX into bare_metal, k8s actions into k8s, and rename scripts/modules to breakfix. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Both nico configs imported suites/bare_metal.yaml, so `--provider nico --suite bare_metal` could not resolve and demanded an explicit -f. This was the only such collision in the repo. Fold key_access.yaml's setup/teardown steps into bare_metal.yaml and delete the file. The test step was already identical in both; only the throwaway-key provisioning was unique. Those two steps carry `skip: true` so a routine run stays read-only -- setup_key_access mutates the site (POST sshkey + sshkeygroup, best-effort PATCH of the SSH-key SOL flag), which should not be a side effect of ordinary bare-metal validation. Unskip both to make AUTH-XX-03 pass without a pre-synced key. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The test-plan coverage guardrail requires each plan entry's labels to equal the union of labels across the suite wirings declaring that test_id. Folding the BFX checks into bare_metal.yaml and k8s.yaml added platform labels to the wiring but not to docs/test-plan.yaml, leaving `make test` red on all 13 BFX entries. Add bare_metal to the eleven bare-metal entries, and kubernetes to BFX01-01 and BFX01-04, which live in the k8s suite. Also apply the pre-commit import ordering and whitespace fixes the BFX commits missed. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
… checks
Against a live NICo site both checks passed while proving nothing.
events_queryable and history_queryable are set by the provider script the
moment the machine-list call succeeds, so MaintenanceEventsCheck passed with
zero events -- it would pass identically against a site with no maintenance
capability at all.
Skip instead of passing when the evidence list is empty. An empty list cannot
distinguish a working query API from one that returns nothing, and a provider
should not clear BFX02 without demonstrating the capability. A failed or
absent API still fails, so the stricter reading only affects the vacuous case.
Also stop counting InUse as repair history. _REPAIR_STATUSES is
{Maintenance, Reset, Error, Repairing}, but the filter admitted InUse too --
a normal lifecycle state -- so machines that had never been repaired produced
repair records.
Verified on a NICo dev site: BFX02-01 now skips (site has no maintenance
events), BFX02-03 still passes on genuine Error history across all 6 machines.
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
changed the title
RetirementNoticesCheck kept the vacuous-pass pattern that BFX02-01 and BFX02-03 shed in 3c2df67: notices_queryable is set by the provider script whenever the API call succeeds, so the check passed on an empty notice list. The my-isv demo made this visible by passing with "0 notice(s) at site". Skip on an empty list, matching the other two BFX02 checks. Give the my-isv stub a sample notice so the demo still exercises the pass path. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
marked this pull request as ready for review
August 5, 2026 20:03
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-05 20:04:41 UTC | Commit: b537792 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
isvctl/configs/providers/nico/config/bare_metal.yaml (1)
565-571: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
machine_idandrack_idare declared but never passed to a step.No step in this file consumes
{{machine_id}}or{{rack_id}}. The mutating break-fix scripts accept only--org,--site-id, and--api-base. If these settings exist for the node and rack maintenance workflows, wire them into the matching stepargs. If they are placeholders for a later change, add a short comment stating that.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/config/bare_metal.yaml` around lines 565 - 571, Update the settings and related maintenance steps in the provider configuration so machine_id and rack_id are either passed through their matching step args using the existing template values, or marked with a brief comment as intentional placeholders if no current step supports them. Ensure no declared setting remains unused without that clarification.isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py (2)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour break-fix stub scripts duplicate the same inline gap-reporting block. Each script repeats the same argparse setup,
parse_known_argscall, skip payload, andsys.exit(emit(...))inside the module guard, with the namesp,a, andr. Only the skip reason, gap ID, andoperationkeys differ. Add one shared helper inbreakfix/_common.pythat takes the reason, gap ID, and operation payload, then have each script call it from a typedmain().
isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py#L15-L27: call the shared helper with gapBFX01-05and{"requested": False, "node_removed_from_pool": False}from amain() -> intfunction.isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py#L15-L27: call the shared helper with gapBFX01-01and{"requested": False, "completed": False}from amain() -> intfunction.isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py#L15-L27: call the shared helper with gapBFX01-02and{"requested": False, "accepted": False}from amain() -> intfunction.isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py#L15-L27: call the shared helper with gapBFX01-03and{"requested": False, "accepted": False}from amain() -> intfunction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py` around lines 15 - 27, Replace the duplicated module-guard logic with a shared typed helper in isvctl/configs/providers/nico/scripts/breakfix/_common.py that accepts the skip reason, gap ID, and operation payload, parses the existing --org, --site-id, and --api-base arguments, emits the skip result, and returns an exit code. In isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py#L15-L27, call it from main() -> int with gap BFX01-05 and operation {"requested": False, "node_removed_from_pool": False}; apply the same main() structure in reset_gpus.py#L15-L27 with BFX01-01 and {"requested": False, "completed": False}, return_node_maintenance.py#L15-L27 with BFX01-02 and {"requested": False, "accepted": False}, and return_rack_maintenance.py#L15-L27 with BFX01-03 and {"requested": False, "accepted": False}.
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the stub logic into a typed
main()function.
query_bmc_kernel_logs.pyin the same directory definesdef main() -> int:and callssys.exit(main()). This script puts all logic in the module guard and uses the namesp,a, andr. The same shape repeats in the other break-fix stub scripts. Align the stubs with themain()convention and use descriptive names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py` around lines 15 - 27, Move the argument parsing and stub-result construction from the __main__ guard into a typed main() -> int function, then invoke it with sys.exit(main()). Within main(), replace the abbreviated p, a, and r variables with descriptive names while preserving the existing required arguments, skip_result call, operation payload, and emitted exit status.isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py (1)
23-23: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe generic
"log"keyword makes the signal test broad.
_LOG_KEYWORDScontains"log", so any BMC probe whose text mentions logging counts as a kernel-log entry.entry_countthen reports matched probes, not log entries. Consider dropping"log"or renaming the reported field tomatched_probe_countsoBmcKernelLogCheckevidence stays accurate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py` at line 23, Remove the generic "log" token from _LOG_KEYWORDS so BmcKernelLogCheck only matches specific kernel-log indicators and entry_count continues to represent log entries rather than broad probe matches.isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py (1)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stub shape as the other break-fix stubs.
Move the logic into a typed
main()function and use descriptive variable names, to matchquery_bmc_kernel_logs.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py` around lines 15 - 27, Refactor the __main__ block into a typed main() function, matching the structure used by query_bmc_kernel_logs.py. Replace abbreviated variables such as p, a, and r with descriptive names, preserve the existing argument parsing, skip_result call, operation payload, and emitted exit status, and invoke main() from the __main__ guard.isvctl/configs/providers/nico/scripts/breakfix/_common.py (1)
59-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to
emit,machine_labels, andhistory_entries.The coding guidelines require a docstring on every function.
list_site_machinesandskip_resulthave one; these three do not.♻️ Proposed docstrings
def emit(result: dict[str, Any]) -> int: + """Print the result as JSON and return the process exit code.""" print(json.dumps(result, indent=2)) return 0 if result.get("success") else 1 def machine_labels(machine: dict[str, Any]) -> dict[str, str]: + """Return the machine labels as a string-to-string mapping, dropping null values.""" labels = machine.get("labels") or {} if not isinstance(labels, dict): return {} return {str(k): str(v) for k, v in labels.items() if v is not None} def history_entries(machine: dict[str, Any]) -> list[dict[str, Any]]: + """Return the machine status-history entries that are dict objects.""" history = machine.get("statusHistory") or [] return [entry for entry in history if isinstance(entry, dict)]As per coding guidelines: "Every function and class must have docstrings following PEP 257".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py` around lines 59 - 73, Add concise PEP 257-compliant docstrings to the emit, machine_labels, and history_entries functions, documenting each function’s purpose and return value while preserving their existing behavior and logic.Source: Coding guidelines
isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py (1)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stub shape as the other break-fix stubs.
Move the logic into a typed
main()function and use descriptive variable names, to matchquery_bmc_kernel_logs.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py` around lines 15 - 27, Refactor the module-level CLI logic in the __main__ block into a typed main() function, using descriptive names instead of p, a, and r while preserving the existing arguments, skip_result call, operation payload, and emitted exit status. Update the entry point to invoke main(), matching the structure used by query_bmc_kernel_logs.py.isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py (1)
15-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame stub shape as the other break-fix stubs.
Move the logic into a typed
main()function and use descriptive variable names, to matchquery_bmc_kernel_logs.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py` around lines 15 - 27, Refactor the __main__ block into a typed main() function, moving argument parsing and result construction into it. Replace abbreviated variables such as p, a, and r with descriptive names, matching the structure and naming style used by query_bmc_kernel_logs.py, while preserving the existing arguments, skip_result call, operation values, and exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@isvctl/configs/providers/my-isv/scripts/breakfix/_stub.py`:
- Around line 21-30: Add concise PEP 257 docstrings to the new functions:
document the return contract of base_result and demo_or_not_implemented in
isvctl/configs/providers/my-isv/scripts/breakfix/_stub.py (lines 21-30), and
document the behavior of each main function in cordon_node.py (17-33),
query_bmc_kernel_logs.py (17-29), query_failure_notifications.py (17-30),
query_maintenance_events.py (17-39), query_planned_notifications.py (17-30),
query_repair_history.py (17-42), query_retirement_notices.py (17-38), and
query_serial_numbers.py (17-42). Ensure every function has a concise PEP
257-compliant docstring without changing its behavior.
In
`@isvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.py`:
- Around line 17-30: Add concise PEP 257-compliant docstrings immediately inside
each main function, describing its template operation:
query_node_health_agents.py (lines 17-30) for the node health-agent query,
query_switch_firmware.py (lines 17-29) for the switch-firmware query,
request_host_replacement.py (lines 17-35) for host replacement, reset_gpus.py
(lines 17-31) for GPU reset, return_node_maintenance.py (lines 17-36) for node
maintenance, and return_rack_maintenance.py (lines 17-31) for rack maintenance.
Preserve the existing argument parsing and result flow.
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 17-29: Update list_site_machines around the forge_get_all call to
catch the expected HTTP, URL, response-decoding, and JSON exceptions in addition
to NicoAuthError. For each handled failure, set result["error"] to the exception
text and return [], result so emit() always receives structured JSON.
In `@isvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.py`:
- Around line 24-32: Add concise PEP 257-compliant docstrings to every specified
new function: `_opened_at` and `main` in
`isvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.py`,
`_repair_entries` and `main` in
`isvctl/configs/providers/nico/scripts/breakfix/query_repair_history.py`, `main`
in `isvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.py`,
and `main` in
`isvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.py`;
preserve each function’s existing behavior.
In `@isvtest/src/isvtest/validations/breakfix.py`:
- Around line 33-38: Add concise PEP 257-compliant docstrings to every function
and class in the file, including _record_label, _maybe_skip, and each run
method. Ensure each docstring briefly describes the symbol’s purpose, while
preserving all existing behavior.
- Around line 330-336: Update the health-agent validation around the agents list
and not_running check so an empty agents result is treated as failure when
agents_observable is true. Report that no health-agent records were found, while
preserving the existing failure for non-running records and success behavior
when at least one agent is running.
- Around line 285-288: Update the existing-workload validation in the
surrounding BFX01-04 flow to require an exact True value for
operation.get("existing_workloads_running"). Treat missing, false, or any
non-boolean value as failure via set_failed, and only call set_passed when the
value is exactly True.
In `@isvtest/tests/test_breakfix.py`:
- Around line 10-17: Update the validation imports and tests in test_breakfix.py
to include PlannedMaintenanceNotificationCheck. Change
test_planned_notification_passes to instantiate the planned-maintenance check,
and retain a separate test that instantiates FailureNotificationCheck so both
validations are covered.
---
Nitpick comments:
In `@isvctl/configs/providers/nico/config/bare_metal.yaml`:
- Around line 565-571: Update the settings and related maintenance steps in the
provider configuration so machine_id and rack_id are either passed through their
matching step args using the existing template values, or marked with a brief
comment as intentional placeholders if no current step supports them. Ensure no
declared setting remains unused without that clarification.
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 59-73: Add concise PEP 257-compliant docstrings to the emit,
machine_labels, and history_entries functions, documenting each function’s
purpose and return value while preserving their existing behavior and logic.
In `@isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py`:
- Line 23: Remove the generic "log" token from _LOG_KEYWORDS so
BmcKernelLogCheck only matches specific kernel-log indicators and entry_count
continues to represent log entries rather than broad probe matches.
In `@isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py`:
- Around line 15-27: Replace the duplicated module-guard logic with a shared
typed helper in isvctl/configs/providers/nico/scripts/breakfix/_common.py that
accepts the skip reason, gap ID, and operation payload, parses the existing
--org, --site-id, and --api-base arguments, emits the skip result, and returns
an exit code. In
isvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.py#L15-L27,
call it from main() -> int with gap BFX01-05 and operation {"requested": False,
"node_removed_from_pool": False}; apply the same main() structure in
reset_gpus.py#L15-L27 with BFX01-01 and {"requested": False, "completed":
False}, return_node_maintenance.py#L15-L27 with BFX01-02 and {"requested":
False, "accepted": False}, and return_rack_maintenance.py#L15-L27 with BFX01-03
and {"requested": False, "accepted": False}.
- Around line 15-27: Move the argument parsing and stub-result construction from
the __main__ guard into a typed main() -> int function, then invoke it with
sys.exit(main()). Within main(), replace the abbreviated p, a, and r variables
with descriptive names while preserving the existing required arguments,
skip_result call, operation payload, and emitted exit status.
In `@isvctl/configs/providers/nico/scripts/breakfix/reset_gpus.py`:
- Around line 15-27: Refactor the __main__ block into a typed main() function,
matching the structure used by query_bmc_kernel_logs.py. Replace abbreviated
variables such as p, a, and r with descriptive names, preserve the existing
argument parsing, skip_result call, operation payload, and emitted exit status,
and invoke main() from the __main__ guard.
In `@isvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.py`:
- Around line 15-27: Refactor the module-level CLI logic in the __main__ block
into a typed main() function, using descriptive names instead of p, a, and r
while preserving the existing arguments, skip_result call, operation payload,
and emitted exit status. Update the entry point to invoke main(), matching the
structure used by query_bmc_kernel_logs.py.
In `@isvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.py`:
- Around line 15-27: Refactor the __main__ block into a typed main() function,
moving argument parsing and result construction into it. Replace abbreviated
variables such as p, a, and r with descriptive names, matching the structure and
naming style used by query_bmc_kernel_logs.py, while preserving the existing
arguments, skip_result call, operation values, and exit behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 802eaa2e-d6ae-47d0-bea7-ce76f3e17235
📒 Files selected for processing (38)
docs/test-plan.yamlisvctl/configs/providers/my-isv/config/bare_metal.yamlisvctl/configs/providers/my-isv/config/k8s.yamlisvctl/configs/providers/my-isv/scripts/breakfix/_stub.pyisvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/config/key_access.yamlisvctl/configs/providers/nico/scripts/breakfix/_common.pyisvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/nico/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/nico/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/nico/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/nico/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/nico/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/nico/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/nico/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/nico/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/suites/README.mdisvctl/configs/suites/bare_metal.yamlisvctl/configs/suites/k8s.yamlisvtest/src/isvtest/validations/breakfix.pyisvtest/tests/test_breakfix.py
💤 Files with no reviewable changes (1)
- isvctl/configs/providers/nico/config/key_access.yaml
Correctness: - CordonNodeCheck passed when `existing_workloads_running` was absent, since the guard only rejected an explicit False. BFX01-04 needs positive evidence that existing workloads kept running, so require exactly True. - NodeHealthAgentCheck passed with "0 node(s)" when `agents` was empty. BFX04-01 asks whether GPUd/Sentinel is running; no agent records means none is. - `list_site_machines` caught only NicoAuthError, so an HTTPError or a decoding failure from `forge_get_all` escaped and killed the script before `emit()`, leaving the orchestrator with no JSON at all. Catch URLError and ValueError (covering HTTPError and JSONDecodeError) and return a structured failure. Tests: - `test_planned_notification_passes` instantiated FailureNotificationCheck, so PlannedMaintenanceNotificationCheck had no coverage. Test both, and add cases for the two correctness fixes above. Docstrings: add the PEP 257 docstrings required by python-standards.mdc to the break-fix validations and the my-isv/NICo provider scripts. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
isvctl/configs/providers/nico/scripts/breakfix/_common.py (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the stale exception-handling comment.
The
except (URLError, ValueError)block converts these failures into a structured result. The preceding comment says that the failures escape and produce no JSON. Replace it with wording that describes the current handled path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py` around lines 32 - 34, Update the stale comment immediately before the except (URLError, ValueError) block to state that these errors are caught and converted into a structured failure result, preserving the existing exception-handling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@isvctl/configs/providers/nico/scripts/breakfix/_common.py`:
- Around line 32-34: Update the stale comment immediately before the except
(URLError, ValueError) block to state that these errors are caught and converted
into a structured failure result, preserving the existing exception-handling
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12913a45-cf7d-4776-a6e1-2cdc24e60bb7
📒 Files selected for processing (23)
isvctl/configs/providers/my-isv/scripts/breakfix/_stub.pyisvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_node_health_agents.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_planned_notifications.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.pyisvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.pyisvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.pyisvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.pyisvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.pyisvctl/configs/providers/nico/scripts/breakfix/_common.pyisvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.pyisvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.pyisvctl/configs/providers/nico/scripts/breakfix/query_repair_history.pyisvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.pyisvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.pyisvtest/src/isvtest/validations/breakfix.pyisvtest/tests/test_breakfix.py
🚧 Files skipped from review as they are similar to previous changes (18)
- isvctl/configs/providers/nico/scripts/breakfix/query_maintenance_events.py
- isvctl/configs/providers/nico/scripts/breakfix/query_retirement_notices.py
- isvctl/configs/providers/my-isv/scripts/breakfix/cordon_node.py
- isvctl/configs/providers/nico/scripts/breakfix/query_switch_firmware.py
- isvctl/configs/providers/nico/scripts/breakfix/query_bmc_kernel_logs.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_failure_notifications.py
- isvctl/configs/providers/nico/scripts/breakfix/query_repair_history.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_serial_numbers.py
- isvctl/configs/providers/my-isv/scripts/breakfix/reset_gpus.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_maintenance_events.py
- isvctl/configs/providers/my-isv/scripts/breakfix/_stub.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_bmc_kernel_logs.py
- isvctl/configs/providers/my-isv/scripts/breakfix/return_node_maintenance.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_switch_firmware.py
- isvctl/configs/providers/my-isv/scripts/breakfix/request_host_replacement.py
- isvtest/tests/test_breakfix.py
- isvctl/configs/providers/my-isv/scripts/breakfix/return_rack_maintenance.py
- isvctl/configs/providers/my-isv/scripts/breakfix/query_retirement_notices.py
Docstring coverage across the PR's Python files was 78%, below the 80% threshold. The remaining gaps were all in test_breakfix.py: 7 test classes and 8 test methods. Every changed Python file is now at 100%. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
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
2 participants
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
Adds the break-fix validation suite covering BFX01–BFX06, wired into the existing
bare_metalandk8ssuites rather than a standalone suite.isvtest/validations/breakfix.py— 12 new validation classes (BFX03-01 reuses the existingBmHardwareSerialCheck)suites/bare_metal.yaml— 11 checks;suites/k8s.yaml— GPU reset + cordonmy-isv— demo stubs for every step, exercised bymake demo-testnico— 4 read-only implementations; the rest emit a structured skip carrying agap: BFXxx-yymarker rather than a vacuous passValidated against live NICo
Run on two independent NICo deployments:
az51-dev3(6 CPU-only machines) andaz51-dev3-dh1(18 GPU hosts).Errorhistory on 18/18 machinesThe 8 gaps reproduced with identical reasons on both deployments, so they are platform gaps rather than one site's configuration. On NICo only BFX03-01 and BFX02-03 currently prove anything; the rest document missing API surface.
Every break-fix step reads only the
machineresource, so the suite runs with infrastructure-scope access and needs no tenant-level API permissions.Also in this PR
nico/config/key_access.yamlfolded intobare_metal.yaml— both imported the same suite, which made--provider nico --suite bare_metalunresolvable. The key-provisioning steps carryskip: trueso a routine run stays read-only.make test).InUseno longer counts as repair history.Testing
make test(124 passed) ·make demo-test·make lint·uvx pre-commit run -a·scripts/validate_suite_wiring.py --checkNew checks are unreleased; run with
ISVTEST_INCLUDE_UNRELEASED=1.Issues
Closes #213 — observed passing against live NICo, on 18/18 machines at
az51-dev3-dh1Closes #211 — implemented against the NICo API; skipped only because no test site had a machine in maintenance
The rest get the test and wiring, but NICo cannot satisfy them yet, so they stay open:
Part of #207
Part of #208
Part of #210
Part of #212
Part of #214
Part of #215
#206 and #209 also stay open — both are wired into
k8s.yamland have not been exercised end-to-end.#567 tracks each remaining NICo gap. #568 tracks the orphaned
reset_gpus.py.Summary by CodeRabbit