feat(breakfix): implement BFX01-01 as a provider repair-API test - #608
Open
abegnoche wants to merge 5 commits into
Open
feat(breakfix): implement BFX01-01 as a provider repair-API test#608abegnoche wants to merge 5 commits into
abegnoche wants to merge 5 commits into
Conversation
…PU reset BFX01-01 asked to reset GPUs "via the Breakfix API". No provider break-fix API exposes an on-demand GPU reset: NICo has none, and the repair automation behind it discovers work by polling a flag every few minutes and then waits on human ticket workflows. There is no request/response for a GPU reset to test. What a tenant can do synchronously is report a GPU fault and watch the provider move the node into a repair state. BFX01-01 now tests that, through NICo online repair (PATCH machine with onlineRepair + healthIssue), asserting the assigned instance goes Ready -> Repairing and back again. The step emits a structured skip when the site has no GPU machine with a Ready instance, since that is an unmet precondition rather than a provider defect. Online repair is cleared in a finally so a node cannot be stranded out of the allocatable pool, and a failure to clear it fails the step. The in-cluster tenant-side reset keeps its own check (GpuResetCheck) under the new id BFX01-06, with a summary stating what it actually proves: it exercises the tenant's own node access and passes with or without a provider. Its former description claimed "via the breakfix API", which it never did. The two checks cannot share one test_id because coverage requires shared ids to carry identical labels. Verified against a live NICo v2.2.0 deployment: the request contract is accepted, and the online-repair guard rejects a machine with no instance before opening a transaction. The happy-path transition is not yet exercised; it needs a site with a tenant instance. Refs #206 Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…node The worst outcome this step can produce is a node left in Repairing, out of the allocatable pool and needing a human. Three changes remove the ways that happened. Re-mint the access token before restoring. Auth was resolved once and reused in the finally, so a run that outlived a short-lived token (NICo SSA tokens are minutes) would 401 exactly when clearing online repair -- failing the step and stranding the node in the same breath. Fall back to deleting the request-online-repair health override when clearing online repair does not take effect. Needing the fallback is reported as a cleanup_warnings finding rather than a failure, since nothing was left behind; a node still in Repairing after both attempts remains a hard error. Raise the state-poll deadline from 180s to 300s, because NICo applies the override through a site workflow that may queue, and a false "never entered repair" is worse than waiting. The step timeout goes to 900s so two full polls plus overhead cannot be killed mid-run -- being killed is now the only remaining way to strand a node, since no finally executes. Test note: _await_status binds its deadline as a default argument, so tests advance a fake clock instead of lowering the constant. Patching only sleep left monotonic real and spun for the full deadline. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Follow-up cleanup on the BFX01-01 online-repair step. No behavior change beyond the two noted below. Reuse: - Collapse _clear_online_repair/_remove_repair_override, two copies of the same mutate/await/compare skeleton, into one _attempt_exit taking the mutation as a callable. - Promote _delete_if_present from auth/_key_access.py into nico_client as delete_if_present, and use it for the override-removal fallback. A 404 there means the override is already gone, which the bare forge_delete reported as a cleanup failure. - Read instance status through common.inventory.first_string, which also tolerates the state/instanceState spellings NICo uses elsewhere. Simplification: - Drop `entered` (a duplicate of operation["requested"]) and the unreachable-false `auth is not None` guard from the restore finally. - Build the operation contract literal once instead of twice. - Fetch status in one place in the poll loop; inline _restore_token. - Trim the module docstring's duplicated JSON contract and restore narrative, both already stated at _restore and in the suite README. Efficiency: - list_site_machines accepts a pre-resolved auth and an include_metadata flag, so the step mints one token instead of two and skips metadata it never reads. - Give the override-removal fallback a shorter deadline. It previously re-armed a full 300s, so enter+clear+fallback could reach the 900s step timeout and be killed mid-restore -- the outcome the finally exists to prevent. Altitude: - cleanup_warnings was a top-level key no validation read, so a provider whose documented exit path silently failed still passed with a clean message. Carry the finding on operation.message, which _OperationCheck already surfaces, replacing the constant (restored=True) suffix. - Point the my-isv reset_gpus scaffold at BFX01-06 after the renumbering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…pair The step auto-selected the first eligible machine and mutated it. On a shared lab site that is dangerous: such sites routinely carry exactly one tenant instance, and it is usually somebody else's. Pointing the NICo config at one would have put a stranger's workload into Repairing. An auto-discovered target is now reported in a structured skip that names the node instead of mutating it, which also makes a plain run a dry run. The operator opts in with --machine-id <id> to confirm that node, or NICO_ALLOW_ONLINE_REPAIR=1 to accept whichever eligible node is found. This inverts the sibling query_key_access.py convention, which mutates by default and opts out via --no-provision. The difference is blast radius: minting a throwaway SSH key affects nobody, moving a stranger's instance into a repair state does. Noted at the constant so the divergence reads as deliberate. Also drop the speculative state/instanceState fallbacks when reading instance status. Only DpuMachine spells it `state`; Instance has just `status`, so the extra keys were flexibility nothing asked for. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
|
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 (16)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe PR separates provider GPU repair validation from tenant-side GPU reset validation. It adds a NICo GPU repair workflow with polling and restoration, shared cleanup support, suite wiring, test coverage, and updated test-plan metadata. ChangesGPU repair validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change adds an opt-in provider repair test with guarded target selection and restoration handling, while preserving the existing tenant-side check under a corrected identifier. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant GPURepairCLI
participant NicoAPI
participant RepairRestoration
GPURepairCLI->>NicoAPI: Discover eligible GPU machines
GPURepairCLI->>NicoAPI: Report GPU health issue
NicoAPI-->>GPURepairCLI: Return Repairing state
GPURepairCLI->>RepairRestoration: Begin cleanup
RepairRestoration->>NicoAPI: Clear online repair
NicoAPI-->>RepairRestoration: Return Ready state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 96.30% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 54 functions across 9 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🔐 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-27 16:00:41 UTC | Commit: cbb8868 |
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
1 participant
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.
Reworks BFX01-01. Supersedes the approach in #603. Refs #206.
Why
BFX01-01 asked to reset GPUs "via the Breakfix API", but no provider break-fix API exposes an on-demand GPU reset. NICo's machine sub-resources are only
bmc/reset,dpu/reprovision,health-report,power,decommission,validation/run— nothing GPU-scoped, andpowerisPROVIDER_ADMIN-only. The repair automation behind it discovers work by polling and then waits on human tickets, so there is no GPU-reset request/response anywhere to test.What a tenant can do synchronously is report a GPU fault and watch the provider move the node into a repair state. That is what this tests.
What changed
request_gpu_repairstep uses NICo online repair (PATCH machinewithonlineRepair+healthIssue) and asserts the instance goesReady → Repairing → Ready. NewGpuRepairRequestCheckvalidates it. NICo's own runbook uses a GPU fault as its worked example.GpuResetCheckkeeps its in-cluster SSH behaviour under a new id, with a summary saying what it proves: it exercises the tenant's own node access and passes with or without a provider. The two can't share atest_id— coverage requires shared ids to carry identical labels.my-isvstub that returns "Not implemented" outside demo mode.Safety
Mutating requires an opt-in. An auto-discovered target is reported in a skip naming the node (so a plain run is a dry run); the operator confirms with
--machine-idorNICO_ALLOW_ONLINE_REPAIR=1. Shared lab sites often carry exactly one tenant instance and it usually isn't yours.Restore is belt-and-braces: re-mint the token first (a long run can outlive it, and a 401 there would strand the node), fall back to deleting the
request-online-repairoverride, and report needing that fallback onoperation.messagerather than passing silently. Deadlines are sized so enter + clear + fallback fit the step timeout.Skips rather than fails when no GPU machine has a
Readyinstance — unmet precondition, not a provider defect.Verification
Against a live NICo
v2.2.0-pr-242deployment: both request bodies accepted (enter, and the flag-only exit), credentials clear the permission gate, the rejection path is side-effect-free (machine.goreturns on!machine.IsAssignedbefore the transaction opens), andincludeMetadata=falsestill returnsmachineCapabilitiesso GPU detection survives.make lint,make test,isvctl test validate, plan coverage, suite wiring, anduvx pre-commit run -apass.Not verified: the happy-path transition. It needs a site with a tenant instance; ours has none (no OS registered,
imageBasedOperatingSystem: false). Skip path, request bodies, polling, and restore are unit-tested — the live transition is not.Also here
delete_if_presentpromoted intonico_client, fixing a bug in the fallback: a 404 means the override is already gone (success) but was reported as a cleanup failure.list_site_machinestakes an optional pre-resolved auth andinclude_metadataflag, so the step mints one token instead of two. Defaults keep its three other callers unchanged.my-isv/scripts/breakfix/reset_gpus.pyrenumbered to BFX01-06 — the file external ISVs copy.Open question
Whether BFX01-06 should exist at all: it has an honest summary but no real implementation, so if #603 is abandoned it's an empty shell. Out of scope here.