fix(labels): declare check labels only in suites - #526
Merged
Conversation
Per-check `labels:` wiring belongs in isvctl/configs/suites/*.yaml; the
nico provider configs redeclared control_plane/iam/network/min_req labels
in their standalone wiring, so `isvctl catalog labels --files` attributed
labels to provider files.
- Drop `labels:` from nico/config/{control-plane,iam,network}.yaml. Also
change nico's TenantInfoCheck test_id SDN01-02 to the "N/A" sentinel:
SDN01-02 is owned by VpcCrudCheck-read in suites/network.yaml, and the
provider-local attribution now trips the plan-coverage domain guardrail
(SDN implies label 'network', which TenantInfoCheck no longer carries).
- Document the rule in AGENTS.md so agents never reintroduce provider
labels. Exception: the single-node local providers
(k3s/microk8s/minikube.yaml), which wire host-level Bm* checks that
exist in no suite.
Accepted behavior change: `isvctl test run --provider nico --label <x>`
no longer discovers these standalone configs (they carry no labels and
import no suite); direct `-f <file>` runs are unaffected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Michail Resvanis <mresvanis@nvidia.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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-07-03 09:27:10 UTC | Commit: 106c643 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
abegnoche
approved these changes
Jul 6, 2026
abegnoche
added a commit
that referenced
this pull request
Jul 30, 2026
* 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>
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
labels:from the Nico provider configs (control-plane.yaml,iam.yaml,network.yaml) so labels are declared only inisvctl/configs/suites/*.yaml;isvctl catalog labels --filesnow attributes every label to suite files, with the single-node local providers (k3s/microk8s/minikube) as the only allowed exception.TenantInfoChecktest_idSDN01-02to the"N/A"sentinel:SDN01-02is owned byVpcCrudCheck-readinsuites/network.yaml, and the provider-local attribution trips the plan-coverage domain guardrail once thenetworklabel is gone.exclude.labels:filtering blocks remain fine).Accepted behavior change:
isvctl test run --provider nico --label <x>no longer discovers these standalone configs (they carry no labels and import no suite); direct-f <file>runs are unaffected.Test plan
uv run isvctl catalog labels --fileslists onlysuites/*.yaml(plus the exemptedproviders/{k3s,microk8s,minikube}.yaml) for every labelmake test(all packages + scripts)make demo-testuvx pre-commit run -a🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes