feat: add NICo hardware ingestion and DPU health validations (#133, #136) - #450
Conversation
) Re-home the NICo (formerly Carbide/Forge) hardware lifecycle validations onto the current suites + providers config model. - HardwareIngestionCheck, DpuHealthCheck, DpuNetworkCheck validation classes using labels (not markers) and Apache-2.0 SPDX headers - Provider-agnostic suite contracts: suites/hardware_ingestion.yaml and suites/dpu_health.yaml - NICo provider wiring under providers/nico/ (config + scripts), with a shared NICo API client and verify_ingestion / check_dpu_health scripts - Register the HARDWARE platform in the test catalog - Document the new suites in the suites README Closes #133, #136 (validation code ready; live testing blocked on NGC access)
|
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. |
📝 WalkthroughWalkthroughAdds NICo provider support: shared auth/API client, hardware ingestion and DPU health scripts, validation classes, doctor CLI checks, bare-metal config wiring, and tests. ChangesNICo Provider Integration
sequenceDiagram
participant CLI as Validation CLI
participant ResolveAuth as resolve_auth
participant Issuer as OIDC Issuer
participant TokenEP as TokenEndpoint
participant NICoAPI as NICo API
participant Validator as isvtest Validations
CLI->>ResolveAuth: request token (NICO_BEARER_TOKEN or OIDC)
alt bearer token present
ResolveAuth-->>CLI: token (NICO_BEARER_TOKEN)
else OIDC path
ResolveAuth->>Issuer: GET /.well-known/openid-configuration
Issuer-->>ResolveAuth: token_endpoint
ResolveAuth->>TokenEP: POST client_credentials (Basic auth)
TokenEP-->>ResolveAuth: access_token
ResolveAuth-->>CLI: token (oidc_client_credentials)
end
CLI->>NICoAPI: forge_get_all (Bearer) for machines/expectedMachines
NICoAPI-->>CLI: paginated JSON
CLI->>Validator: provide step outputs for HardwareIngestion/DpuHealth/DpuNetwork
Validator-->>CLI: pass/fail + subtest results
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Michail Resvanis <mresvanis@nvidia.com>
Signed-off-by: Michail Resvanis <mresvanis@nvidia.com>
Use the carbide API path for NICo requests and filter the DPU health query to DPU machines. Normalize nullable NICo health and capability fields before iterating or lowercasing them, and avoid a hardcoded Base64 auth fixture that triggers secret scanners. Signed-off-by: Michail Resvanis <mresvanis@nvidia.com>
Signed-off-by: Michail Resvanis <mresvanis@nvidia.com>
Use the machine id (UUID) as the human-facing identifier in hardware validations instead of chassis_serial, which is only populated for provider-scoped tokens and otherwise falls back to the machine id upstream. Drop the redundant chassis_serial field from the DPU health output; keep it in ingestion where it is a real matching key. Also filter non-DPU machines client-side (the server-side type=DPU filter is ignored by API versions keyed on capabilityType) and treat a null MachineCapability.count as 1 to avoid a TypeError. Isolate cached `common` modules when loading nico scripts in tests so `from common.nico_client import ...` resolves correctly under the full suite regardless of test order. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…btests Join expected machines to discovered machines via the authoritative expectedMachine.machineId link instead of reconstructing the match from chassis serial. This drops the includeMetadata fetch (only dmiData needed it) and the serial-collision handling. missing = machineId is null; extra = discovered machines not referenced by any expected machine. chassis serial is retained as a manifest-sourced debug field. HardwareIngestionCheck now emits a passing subtest per healthy machine (status + health), matching DpuHealthCheck, so good machines show up instead of only failures. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Re-add chassis_serial to check_dpu_health.py, sourced honestly from metadata.dmiData.chassisSerial (empty when absent, never falling back to machine_id). This makes the existing includeMetadata fetch meaningful again and mirrors verify_ingestion's manifest-sourced debug serial. Display and matching still key off machine_id. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…ntract Move HardwareIngestionCheck and DpuHealthCheck into the canonical suites/bare_metal.yaml and replace the four standalone YAMLs (two suites + two nico configs) with a single providers/nico/config/bare_metal.yaml. NICo imports the BM suite but implements only the verify_ingestion and check_dpu_health steps, so the rest of the bare_metal validations are skipped automatically (step_not_configured). Retire the separate HARDWARE platform: drop it from PLATFORM_CONFIGS and the hardware->HARDWARE label mapping, and relabel the three validation classes from "hardware" to "bare_metal" so the catalog platform invariant stays consistent. Update the nico provider test, suites/README.md, and the script usage docstrings accordingly. Wiring NICo to the full bare_metal lifecycle is left for a follow-up. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
🔐 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-06-05 20:00:02 UTC | Commit: 8aaaafe |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
isvctl/configs/providers/nico/scripts/hardware_ingestion/verify_ingestion.py (1)
162-162: ⚡ Quick winStabilize
capabilitiesordering for deterministic output.Line 162 converts a set to a list, so output order can vary run-to-run. Sorting here avoids noisy diffs and brittle output assertions.
Proposed fix
- cap_types = list({c.get("type", "") for c in capabilities}) + cap_types = sorted({c.get("type", "") for c in capabilities if c.get("type")})🤖 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/hardware_ingestion/verify_ingestion.py` at line 162, The conversion of capabilities to cap_types is nondeterministic because it builds a list from a set; replace list({c.get("type", "") for c in capabilities}) with a deterministic sorted call—e.g., cap_types = sorted({c.get("type", "") for c in capabilities})—so that cap_types is stable across runs; ensure any downstream code expecting list order still works with the sorted order.
🤖 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/nico/scripts/common/nico_client.py`:
- Around line 242-266: The pagination logic clamps the requested page size to
100 when building page_params but still compares len(items) against the original
page_size, causing early termination when page_size > 100; fix by computing an
effective_page_size = min(page_size, 100), use str(effective_page_size) when
setting page_params["pageSize"] and use effective_page_size in the end-of-loop
check (replace len(items) < page_size with len(items) < effective_page_size) so
paging continues correctly; update references in the function that builds
page_params/handles items (page_params, pageSize, page_number, items,
all_items).
In `@isvctl/configs/providers/nico/scripts/dpu/check_dpu_health.py`:
- Around line 78-101: Tighten the loose dict typings and add a docstring for
main: update helper signatures _lower_field, _is_dpu_alert, _has_dpu_heartbeat,
and _extract_health_successes to use PEP 585 annotations (e.g., dict[str, Any]
for JSON-like inputs and list[str] for string lists) instead of plain dict, and
type any local result variables accordingly; also add a concise PEP 257
docstring to main() describing its purpose, parameters (if any) and return
value. Ensure you import Any from typing if needed and keep existing function
names (_lower_field, _is_dpu_alert, _has_dpu_heartbeat,
_extract_health_successes, main) unchanged so the changes are only to
annotations and docstring.
In
`@isvctl/configs/providers/nico/scripts/hardware_ingestion/verify_ingestion.py`:
- Line 85: Add a PEP 257 docstring to the main() function describing its
purpose, parameters (if any) and return value, and update all broad container
type hints to PEP 585 parameterized forms (e.g., replace dict with dict[str,
Any] or the specific key/value types, replace list[dict] with list[dict[str,
Any]] or list[SpecificType]) so typing is explicit; apply the same change to the
other functions in this module that currently use unparameterized containers
(the ones flagged around the later annotations) and import typing.Any if needed.
In `@isvctl/tests/test_nico_provider.py`:
- Around line 56-63: Add PEP 257 docstrings to the helper functions
_load_nico_client and _load_dpu_health_script describing their purpose,
parameters (if any), and return value, and update the _isolated_common_imports
signature to include an explicit return type (e.g.,
typing.ContextManager[ModuleType] or the appropriate ContextManager[...] type
used in the file); locate these symbols in the test file and ensure the
docstrings are concise and the return type annotation is added to the function
definition.
In `@isvtest/src/isvtest/validations/hardware.py`:
- Line 29: The helper _machine_label currently types its parameter as a plain
dict and several run() methods lack docstrings; update _machine_label signature
to use PEP 585 annotations (e.g., machine: dict[str, Any]) and add the necessary
"from typing import Any" import if missing, and add concise PEP 257-compatible
docstrings to each run() method (describe purpose, parameters, return value) so
all functions and methods follow the repo's typing and docstring standards;
locate symbols named _machine_label and any methods named run to apply these
changes.
---
Nitpick comments:
In
`@isvctl/configs/providers/nico/scripts/hardware_ingestion/verify_ingestion.py`:
- Line 162: The conversion of capabilities to cap_types is nondeterministic
because it builds a list from a set; replace list({c.get("type", "") for c in
capabilities}) with a deterministic sorted call—e.g., cap_types =
sorted({c.get("type", "") for c in capabilities})—so that cap_types is stable
across runs; ensure any downstream code expecting list order still works with
the sorted order.
🪄 Autofix (Beta)
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: 980a1860-2290-4b87-9b17-f2f5ac650ed3
📒 Files selected for processing (13)
isvctl/configs/providers/nico/config/bare_metal.yamlisvctl/configs/providers/nico/scripts/common/__init__.pyisvctl/configs/providers/nico/scripts/common/nico_client.pyisvctl/configs/providers/nico/scripts/dpu/check_dpu_health.pyisvctl/configs/providers/nico/scripts/hardware_ingestion/verify_ingestion.pyisvctl/configs/suites/README.mdisvctl/configs/suites/bare_metal.yamlisvctl/src/isvctl/doctor/checks/env.pyisvctl/src/isvctl/redaction.pyisvctl/tests/test_doctor_cli.pyisvctl/tests/test_nico_provider.pyisvtest/src/isvtest/validations/hardware.pyisvtest/tests/test_hardware.py
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
- forge_get_all: compare page length against the effective (clamped) page size so a requested page_size > 100 no longer stops after the first page. - Add PEP 585 parameterized dict types and missing docstrings (main(), validation run() methods, test helpers) to satisfy the repo's Python standards, plus an explicit return type on the test contextmanager. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Michail Resvanis <mresvanis@nvidia.com>
Quality cleanups on the NICo hardware validations: - check_dpu_health: drop the dead `if dpu_caps:` guard (machines without a DPU are already skipped) and reuse the shared classify_health() for health_summary instead of re-deriving the alerts->healthy/unhealthy rule. - hardware.py: compute the missing-machine label list once in HardwareIngestionCheck, and track DpuHealthCheck failures with an ordered label->serial dict instead of a list plus repeated dedup guards and an all-machines serial_by_label rebuild. forge_get_all keeps handling bare-list, result_key-wrapped, and single-object responses (the live NICo API returns a top-level list for some endpoints). Add direct forge_get_all tests for the list and wrapped shapes, which the existing forge_get_all-mocking tests never exercised. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…alidations (CAP05-01, CAP05-02) Adds two provider-agnostic Unified Health API checks as a follow-up to the NICo work in #450: - HostHealthCheck (CAP05-01): asserts the per-host health API surfaces GPU state, thermal status, and memory health for every host, with optional real-time freshness enforcement. - HealthAggregationCheck (CAP05-02): asserts host health can be rolled up to a primitive (nodegroup/reservation) with internally consistent counts. Wires two NICo scripts (query_host_health.py, query_health_aggregation.py) that map machine health probes into categories and aggregate by InstanceType, adds them to the bare_metal suite + NICo config, documents the steps, and adds unit + end-to-end tests. Ships unreleased (released_tests.json untouched). Closes #254 Closes #255 Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com>
…alidations (CAP05-01, CAP05-02) (#453) * feat(nico): add unified host health and primitive-level aggregation validations (CAP05-01, CAP05-02) Adds two provider-agnostic Unified Health API checks as a follow-up to the NICo work in #450: - HostHealthCheck (CAP05-01): asserts the per-host health API surfaces GPU state, thermal status, and memory health for every host, with optional real-time freshness enforcement. - HealthAggregationCheck (CAP05-02): asserts host health can be rolled up to a primitive (nodegroup/reservation) with internally consistent counts. Wires two NICo scripts (query_host_health.py, query_health_aggregation.py) that map machine health probes into categories and aggregate by InstanceType, adds them to the bare_metal suite + NICo config, documents the steps, and adds unit + end-to-end tests. Ships unreleased (released_tests.json untouched). Closes #254 Closes #255 Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com> * fix(nico): align HostHealthCheck with NICo's alert-driven health model A live NICo run hard-failed all healthy hosts on 'missing memory': NICo's machine-health API is alert-driven (per the health_probe_ids.md catalog) and reports BMC sensors under a single BmcSensor probe, so it does not enumerate a passing per-category probe (notably memory). - HostHealthCheck now verifies, per host, that the health API returns a report (require_report, default true) and that any surfaced GPU/thermal/memory category is not alerting. Category coverage is opt-in via require_present (default false) instead of mandatory, so healthy real sites pass. - query_host_health.py emits health_present and folds the probe message into category matching (BmcSensor carries the sensor entity in target/message). - Suite default uses require_report (coverage left unenforced) with an explanatory comment; README documents health_present. Verified end-to-end against a mock NICo API using BmcSensor-style probes: the absent memory category is now SKIPPED and HostHealthCheck PASSES. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com> * refactor(nico): compute probe match text once per probe in categorize_health categorize_health rebuilt each probe's lowercased id+target+message string once per category, making the match cost O(probes x categories). Precompute each probe's text up front and reuse it across all categories, dropping it to O(probes). Behavior is unchanged; the renamed _matches_keywords helper takes the precomputed text instead of recomputing it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(nico): rework HostHealthCheck to NICo probe-ID/classification model Replaces the invented gpu/thermal/memory coverage requirement (derived from the CAP05-01 issue wording, not NICo's API) with NICo's real alert-driven health model, grounded in the upstream health_probe_ids.md / health_alert_classifications.md catalogs and the Probe/Classification enums. - HostHealthCheck now asserts, per host: a health report is returned (require_report) and the host carries no blocking alerts. By default ANY alert is blocking (parity with DpuHealthCheck/HardwareIngestionCheck); fail_on_classifications narrows to specific severities, and require_probes enforces presence of specific probe IDs. The failure summary names the offending probe IDs. - query_host_health.py now emits probe_ids, alerts (with classifications), a top-level healthy flag, and an informational gpu/thermal/memory/cooling component breakdown. Leak detection (BmcLeakDetection / Leak) is recognized. - Suite/README/config updated for the new contract. Verified end-to-end against a mock NICo API: a healthy site passes; an injected BmcLeakDetection/Leak alert fails the host (host_health: m-leak (alerts: BmcLeakDetection)) and degrades its nodegroup aggregation. Relates to #254 (CAP05-01); folds in leak detection as health since no leak-specific issue is open. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com> * test(nico): hoist datetime import and annotate function-local imports Fold the function-local `datetime`/`timedelta` import into the top-level import and add reason comments to the function-local isvtest validation imports, per the project's Python import standards (CodeRabbit nitpicks). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * test(nico): hoist validation imports to module top Move GovernanceMetricsCheck, HostHealthCheck, and HealthAggregationCheck to top-level imports per the repo's import guidelines (isvtest is a hard dependency of isvctl, so no lazy/cycle reason applies). Removes the duplicate HostHealthCheck import. Addresses CodeRabbit review feedback. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> --------- Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Alexandre Begnoche <abegnoche@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(catalog): compose suite checks into uniquely named tests A wiring name is a test's identity in the catalog, the report, and the service, so duplicates are not cosmetic: _build_suite_map last-wins, which silently drops entries (bare_metal's GpuCheck lost to vm's). Wiring generic checks under their class names both creates those duplicates and spends two catalog names on one idea, leaving the second holding test_id: "N/A". Add a `compose:` form so a suite names the property under test and lists the generic checks that establish it. The composite is one catalog entry with one test_id; members run against the group's step output as subtests, so failures still name the part that broke and every member runs even after one fails. Migrate iam.yaml as the first suite: five wiring entries become three named ones, clearing all 4 of its uniqueness violations (99 -> 95 repo-wide). This also maps IAM02-01 (Delete user), which no check claimed before, so the plan entry gains the `iam` label the label-sync guardrail requires on both sides. Composites ship unreleased until a release commit adds them to released_tests.json; run with ISVTEST_INCLUDE_UNRELEASED=1 to exercise them. Global uniqueness enforcement stays behind ISVCTL_ENFORCE_UNIQUE_WIRING until the remaining suites are migrated. docs/test-plan.adoc is regenerated by `make plan`; it was already stale, so the diff also picks up a `storage` label on K8S23-04/05 that was in the source YAML but missing from the rendered doc. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(catalog): reserve catalog identities for named tests The generic checks name a mechanism, not a property under test, so wiring one directly spends a catalog identity on "StepSuccessCheck" and forces several unrelated tests to share it. Mark them compose_only and have the wiring validator reject them outside a composite's compose list, so a suite has to say what the test proves before it can assert it. Resolving the wiring name first means variant spellings such as StepSuccessCheck-teardown are rejected too; they leave the same mechanism as the test's public identity. The ban is scoped to the canonical suites because the catalog only reads suites/, so provider wiring never becomes a catalog entry. Enforcement joins the existing uniqueness deferral under one flag, since migrating a suite satisfies both rules at once. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(image-registry): name the tests the suite actually proves Every validation group here spent two or three catalog names on one idea, leaving the generic members holding test_id "N/A" and making the plan id land on whichever member happened to carry it. Compose each group into the test it proves, so BOOT01-01..05 and BOOT03-02 attach to a name a reader recognises and each group is one catalog entry. The instance-state members fold in too: "boots from image" is only true if the instance reaches running, and that also stops image-registry from claiming the InstanceStateCheck name that vm and bare_metal already wire. vm_ssh keeps ConnectivityCheck and OsCheck, which already say what they prove. Provider docs that listed the old per-step wiring are updated. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): finish naming the tests each suite proves Migrates the remaining six suites off directly wired generic checks, so no canonical suite spends a catalog identity on StepSuccessCheck and friends any more. Teardown groups become the thing they verify was released, the storage and object-storage groups become the volume or object behaviour they exercise, and CP10-01 and SEC21-01 now attach to a name a reader recognises instead of a bare step-success assertion. This also retires StepSuccessCheck-delete_access_key and StepSuccessCheck-delete_tenant, the variant spellings that put the mechanism in the test's public identity. bare_metal's topology_placement and host_status_logs drop their extra StepSuccessCheck rather than compose it: TopologyPlacementCheck and BmHostStatusLog both fail when the step produces no usable output, so the generic check asserted nothing they did not, and keeping their names holds CNP01-04 and BMAAS07-01 steady. Two catalog tests used StepSuccessCheck as a stand-in for a catalog entry; they now use a name that still has one, and the unreleased-filter test asserts on a composite, which is what actually ships unreleased. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(k8s): name each node-pool operation as its own test The four node-pool assertions all shared the K8sNodePoolCheck wiring name, so the catalog kept only the last one and K8S06-01, K8S06-02 and K8S06-03 had no distinct entry to attach to. Each operation now names what it proves and composes the class, which also removes the reason list form was mandatory (ordering keeps it). Adds a test for a composite forwarding its params to a member listed with none of its own: wrapping a single purpose-built check depends on it, and until now every composite passed params per member. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(storage): name the volume fixture's host check storage wired InstanceStateCheck for the instance that carries the test volume, colliding with bare_metal's setup check of the same name and letting the catalog keep only one of them. Name it for the role the instance plays here so both survive. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(image-registry): name what the booted VM's SSH checks prove vm_ssh wired ConnectivityCheck and OsCheck under their class names, which bare_metal and vm also claim, so the catalog kept one entry for all three suites. Both checks tell one story here - the image boots into an OS you can log into and it is the OS asked for - so they become one named test. This is the group deferred when image-registry was first migrated, and it sets the shape for the ssh groups still to come in vm and bare_metal. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): name each host lifecycle stage as its own test The suite asserted the same five properties at five points in the host lifecycle by wiring the same check names repeatedly. The catalog keys on the wiring name, so those repeats collapsed into one entry each and the frontend showed a single row where the suite proves five distinct things. Give every wiring a name that says which stage it proves, composing the generic check underneath. Reachability and OS are merged per stage since they share labels and only one carries a plan ID; the three host_os checks stay separate because only the PCI one is gpu-labelled and merging would pull the other two into gpu label filtering. AWS overrides start_gpu, power_cycle_gpu, and serial_console by check name, and NICo excludes SSH checks by name - both are retargeted at the new names, otherwise the override merges as a second check instead of configuring the intended one. Test IDs are unchanged, so plan coverage holds. The new names ship unreleased until a release commit adds them to released_tests.json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(vm): name each VM lifecycle stage as its own test Last suite wiring the same check names at several points in the lifecycle, so the catalog collapsed those repeats into one entry each and hid what the suite proves at start and after reboot. Name every wiring for the stage it proves, composing the generic check underneath. Unlike bare_metal, reachability and the OS image are distinct plan items here (VMAAS-XX-01 and VMAAS-XX-05), so they stay two tests per stage rather than merging into one ready check. Test IDs are unchanged. The new names ship unreleased until a release commit adds them to released_tests.json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(catalog): enforce unique wiring names and compose-only generics Both rules were written opt-in behind ISVCTL_ENFORCE_WIRING_RULES while the suites were migrated one at a time. Every suite now names what it proves, so the toggle has no remaining use and only offers a way to reintroduce the duplicates it was meant to catch. Enforce unconditionally: the validator reports reused wiring names and directly wired generic checks, and the catalog raises on a duplicate rather than dropping a test through last-wins. Uniqueness had no test of its own while it was opt-in, so add one. Drop the test asserting unmigrated suites still pass, since nothing may be unmigrated now. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(catalog): show suite/capability correctly in catalog list Platform rows were rendering as bare_metal / bare_metal because the CLI treated entry capability as a requirement. Align the column with the post-refactor model: platform suites show the capability alone; plain suites show requires (or core). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): shorten the bare-metal prefix to Bm The suite's new composite names spelled out BareMetal while the checks already in the tree use Bm (BmHostStatusLog, BmGpuHealth) and the vm suite uses Vm, so the catalog listed three spellings of the same idea. Settle on Bm. BareMetalOutput keeps its name: it is the provider step output model, not a check, and renaming it would change the JSON contract stubs implement. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor: tighten composite check wiring Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(nico): give NICo's checks a home in the suites The nico control-plane, iam, and network configs declared their own named checks, so the catalog - which reads only suites/ - never saw them, and a release would have left them permanently unreleased. Two of them restated assertions a suite already made, and the two network probes proved the same property twice behind step names inherited from Carbide that promise traffic tests neither one runs. Name each property in the suite that owns it and let nico import that suite. Checks whose steps nico does not wire skip as step_not_configured, the way nico/config/bare_metal.yaml already works. - control-plane: nico's api_health composite was byte-identical to ControlPlaneApiHealthCheck, so it is gone. That check now declares CP03-01, which an authenticated control-plane request is what proves; DATASVC-XX-01 remains covered end to end by the object-lifecycle check. - iam: add IamCallerIdentityCheck, a whoami on the credentials the run is configured with, for platforms whose IAM is read-only. - network: add a network_inventory group (VpcListedCheck, VpcInfoCheck, SubnetAssignedCheck) for platforms that hand out pre-provisioned VPCs instead of letting a tenant create them. VpcInfoCheck reclaims SDN01-02, which #452 declared and #526 had to drop to the N/A sentinel because an SDN id cannot carry the network label from a provider config. Collapse the duplicate subnet probes into check_subnet_assignment.py and take off the tenant costume: list_vpcs/get_vpc emit vpc_id and vpc_name rather than faking tenant fields so the IAM tenant checks would accept them. list_vpcs now always emits found_target, which closes a hole where a requested VPC absent from the listing still passed. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * docs: explain why the single-node on-host checks are unreleased The catalog became suite-derived in #561, so the six on-host NVIDIA-stack checks wired only in k3s/microk8s/minikube stopped being catalog entries. They still sit in released_tests.json because that file is a pre-#561 snapshot, so the next version bump regenerates it without them and they begin skipping as "unreleased". These are local-dev tools no ISV runs, so staying out of the catalog is correct; record that intent and point developers at the existing ISVTEST_INCLUDE_UNRELEASED escape hatch instead of inventing a mechanism. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(bare_metal): wire DpuNetworkCheck into the suite DpuNetworkCheck arrived in #450 alongside DpuHealthCheck but was never wired, so it has never run and the next manifest regen would drop it from released_tests.json. Publish the contract it expects next to its sibling instead of losing the class. No provider emits interfaces/bgp_enabled/dpu_extension_deployments yet, so it skips as step_not_configured exactly as DpuHealthCheck already does for providers without check_dpu_health. Claims no plan id: BMAAS06-01 is proven over SSH by the nvlink, infiniband, and ethernet checks, and an API reporting an interface "Ready" does not establish that nodes can communicate over it. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): give AWS's image check a suite home, drop the dry run Both AWS-local checks predate the rule that checks live in suites, and both overlapped coverage the image-registry suite already owns. BmImageInstallationVerifiedCheck moves into the bare-metal suite claiming BOOT01-03. AWS proves that id from its bare-metal run because its image-registry run launches a VM, not a metal host, so it implements neither install_image_bm nor install_config_bm. BOOT01-03 now has two implementers, so the plan item and both checks carry the union of their labels, as the label-sync guardrail requires. BmInstallConfigUsableCheck is deleted with its step and script: an EC2 DryRun proves only that EC2 would accept the launch template, which is not BOOT01-04's "installed on a BMaaS system" and adds little over BOOT01-02's launch-template CRUD coverage. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): name properties, not mechanisms, in the remaining wirings Review found the branch violating its own README rule: VpcCrudCheck-* and SgCrudCheck-* spent eight catalog identities on a class name plus a suffix while each maps to a distinct plan id. They become named composites over VpcCrudCheck/SgCrudCheck. The other variant names are parameter variants of one property and stay as they are. BmImageInstallationVerifiedCheck overclaimed: verify_image_installed.py describes an already-running host's image id and installs nothing, so it is BmHostRunsExpectedImageCheck. That also stops it competing with BmHostBootedFromCustomImageCheck for BOOT01-03. SDN01-02's two evidence paths now say which is which: VpcReadCheck for the CRUD lifecycle, VpcReadFromInventoryCheck for a standalone read of a pre-provisioned VPC. Also: the teardown pair carries claimed-vs-confirmed in its names and descriptions, boot names take the past tense of their neighbours, and CompositeCheck now fails on a malformed compose member instead of silently running fewer checks than the config names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): prefix every bare-metal and VM check consistently Review noted the prefix rule was "prefix when it would otherwise collide", so a reader could not guess whether a bare-metal test is X or BmX: BmGpusPresentCheck sat beside GpuStressCheck, NcclCheck and NvlinkCheck, and vm.yaml called its subject both Vm and Instance. All 34 classes were wired by exactly one suite, so each is a plain class rename rather than a composite wrapper. Two names change more than their prefix: SpecifiedKeyAccessCheck becomes BmComponentKeyAccessCheck so the AUTH03-01 pair reads as siblings instead of colliding with AUTH02-01's VmLaunchedWithSpecifiedKeyCheck, and DpuNetworkCheck becomes BmDpuNetworkReadinessCheck. bare_metal.yaml and vm.yaml now have no unprefixed wiring names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(iam): prove IAM01-01's login instead of only creating the user IAM01-01 is "create user and log in as that user", but the id sat on a check that only proved the create step succeeded and returned a username and access key. Issuing a key is not logging in. The test_credentials step already reports identity and access as separate probes, and IamCredentialAccessCheck already takes required_tests, so each plan item can prove its own half over the same output: logging in (IAM01-01) and reaching an authorized resource once logged in (IAM03-01). Creation drops to N/A as the supporting check it is, keeping min_req so a min_req-only run still creates the user the credential checks need. The check's pass message named authorized-resource access unconditionally, which was wrong once identity could be required alone; it now reports the probes it actually required. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(bare_metal): keep Bm off checks that are not about a host The prefix sweep stamped Bm onto nine checks whose plan items are not bare-metal-scoped: CAP01-01 fleet-wide governance counts, CAP05-02 aggregation at cluster/nodegroup/reservation level, SDN04-04/05 IB fabric, STG02/03/04/05 storage service, and HWING01-01 fleet inventory. They sit in bare_metal.yaml because that is where NICo's config imports from, not because the requirement is per-host, so the name asserted a scope the plan does not have. Our own labels already said so - the IB pair carries network, the STG four carry sds_controller. It would also have cost us later: names are globally unique and a wiring holds one test_id, so the first of these to need a network.yaml or storage.yaml home would have produced the two-names-one-id split we just undid for BOOT01-03. The prefix rule is now stated in the suites README: it follows the subject (one host, one VM), not the suite and not the test_id's requirement family, which is why BmHardwareSerialCheck (BFX03-01) and BmCloudInitCheck (BOOT02-01) keep theirs. Suite placement for these nine is a separate question, since moving them means NICo importing those suites too. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(suites): enforce compose_only at runtime and fold the VM launch rows compose_only was authoring-time only: validate_suite_wiring rejects a generic check wired under its own name, but an ISV's config is never linted, so it would run and report a pass under a name that says nothing about the property proven. parse_validations now rejects it too, as an invalid-config error naming the fix. Config parsing is the right place, not class resolution: the rule is about how a check is wired, so putting it there leaves internal pre-resolved entries alone. Four fixtures wired the generics directly and now compose them instead, which is what the rule asks of any config. vm.yaml also had two rows on launch_instance for one event - VmRunningCheck at N/A beside VmCreatedCheck at CNP01-09 - where bare_metal.yaml proves the same launch with one. The class becomes VmInstanceIdReportedCheck so the VmCreatedCheck composite can claim CNP01-09 over both the returned id and the running state. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(cli): apply the config's exclude block in the dry-run plan --dry-run read only the CLI's --exclude-label, ignoring the exclude block in the config itself, so it promised checks a real run then skips. The AWS bare-metal plan listed BmSerialConsoleRetentionCheck as [RUN] even though that config excludes it by name, and k3s listed ten workload checks its own exclude labels always drop. Both halves of the block now apply, as the orchestrator already does: labels union with the CLI's, and names are matched first to mirror resolve_entries' precedence. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(orchestrator): say when a step is skipped rather than unconfigured A step carrying skip: true is deliberately left out of the step-phase map so its validations skip with it, but that reused the "no such step" path, so AWS's four reinstall checks reported step_not_configured for a step the config declares. The two cases differ in what an operator can do: skip: true is one flag away from coverage, a step the provider never declares is not. resolve_entries now takes the skipped step names and reports step_skipped with "configured but skipped (skip: true)". Checks bound to a step the provider genuinely lacks - AWS has no check_dpu_health or verify_ingestion - keep reporting step_not_configured. Note for downstream: skip reasons become the type attribute on JUnit <skipped> elements, so step_skipped is a new value there. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * chore(aws): exclude the credential-readiness check from the IAM suite IamCredentialsAuthenticateCheck is a whoami on the run's own credentials, for platforms whose read-only IAM cannot create a user and log in as them. AWS does exactly that, so IAM01-01 and IAM03-01 already prove the stronger property and the check only added a skipped line to every AWS IAM run. Excluded by name rather than removed from the suite: NICo implements check_credentials and still needs it. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(catalog): close composite review gaps Keep dry-run filtering and compose-only enforcement aligned with runtime behavior, and synchronize the related suite and operator documentation. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(iam): drop the duplicate NICo credential probe from the suite check_credentials made the same two calls as NICo's control-plane check_api - forge_get on the site, then forge_get_all over sites - with the probes renamed. CP03-01 already covers "an authenticated request scoped to the account reaches the platform API", and it maps to a plan item where the IAM copy was N/A. Hoisting it into the shared suite also made every AWS IAM run report a skipped row for a check AWS has no step for, which needed muting. Removing it from the suite drops both the row and the mute: AWS's IAM run is four checks and no skips again. NICo's script stays on disk unwired, in case its IAM story changes. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * docs(my-isv): stop listing check names in coverage comments Enumerating every validation in header comments drifts as suites change; describe JSON-contract coverage and SSH exclusions instead. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): drop prefixes the subject rule does not earn The prefix sweep and the walk-back after it left four names claiming more than they prove. Each was globally unique before the sweep, so the prefix bought consistency only, and each cost a released_tests.json entry. VirtualDeviceHardeningCheck (CNP01-17) already carries its subject: VmVirtualDeviceHardening stutters, and USB/clipboard/virtual-device passthrough has no bare-metal reading to distinguish it from. SerialConsoleRetentionCheck (CNP06-02) asserts that the console archive keeps a month of history, which belongs to the logging service rather than to a host. It is also the one console plan item that is not platform-scoped, unlike CNP06-01 (bare metal) and CNP06-03 (VM). HostHealthCheck (CAP05-01) is per-host by subject, but it is one half of a pair whose other half - HealthAggregationCheck (CAP05-02) - is fleet-level and already unprefixed. Prefixing one half hid that the two are one requirement seen at two scopes. BmDpuNetworkCheck keeps its prefix, since a DPU is per-host, and drops "Readiness": the check asserts interface status, BGP, and extension deployments, which the vaguer word described less well. It is free to change, carrying test_id N/A with no implementation yet. Three of the four land on names already in released_tests.json, so unreleased wiring names drop 125 -> 122 and dangling manifest entries 25 -> 22. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> --------- Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Summary
Re-homes the NICo (formerly Carbide/Forge) hardware-lifecycle validations onto
main's current conventions (labelsnotmarkers, Apache-2.0 SPDX headers, thesuites/+providers/<name>/{config,scripts}/model). Fresh re-implementation, not a rebase of #228.HardwareIngestionCheck,DpuHealthCheck, andDpuNetworkCheckvalidation classes, labeledbare_metal.suites/bare_metal.yamlcontract (no separate per-feature suites orHARDWAREplatform).providers/nico/: a singleconfig/bare_metal.yamlthat imports the BM suite and implements theverify_ingestion+check_dpu_healthsteps, a shared NICo API client, and the two scripts. NICo implements only those two steps today, so the rest of the BM suite is skipped automatically (step_not_configured); wiring the full lifecycle is a follow-up.isvctl doctor(env/auth/API reachability) and redaction for NICo secrets.configs/suites/README.md.Behavior notes / decisions
expectedMachine.machineId(not chassis serial):missing= unlinked,extra= discovered-but-unexpected.chassis_serialis retained only as a debug field (manifest serial for ingestion;dmiData.chassisSerialfor DPU health, never falling back to the id).check_dpu_healthfilters non-DPU machines client-side so a missing/ignored server-side filter can't cause false failures; nullablecountand list fields are handled defensively.Test plan
uv run pytest isvtest/tests/test_hardware.py isvctl/tests/test_nico_provider.py-- passmake test(all packages) +make lint-- passISVTEST_INCLUDE_UNRELEASED=1 isvctl test run -f providers/nico/config/bare_metal.yaml-- onlyingestion_check+dpu_healthrun, the rest of the BM suite is skipped (step_not_configured)Error-state machine; DPU health passed across 20 machinesNotes
DpuNetworkCheckis unit-tested but not yet wired into a suite (it needs a live instance with EVPN overlay).released_tests.jsonuntil a version bump; run locally withISVTEST_INCLUDE_UNRELEASED=1.Closes #133
Closes #136
Summary by CodeRabbit
doctornow includes NICo readiness and authentication checks (bearer token or OIDC) and probes NICo API.