docs: add troubleshooting guide for test runs stuck in STARTED - #4
Merged
Conversation
aanimasaun-nvda
commented
Feb 11, 2026
aanimasaun-nvda
commented
Collaborator
- Add docs/guides/troubleshooting-started-tests.md with RCA, fix (single-command flow), split-flow guidance, and cleanup steps for existing STARTED runs
- Link from getting-started.md (--lab-id note) and isvreporter.md
- Add guide to docs/README.md under Guides
abegnoche
reviewed
Feb 11, 2026
Signed-off-by: aanimasaun-nvda <aanimasaun@nvidia.com>
- Add docs/guides/troubleshooting-started-tests.md with RCA, fix (single-command flow), split-flow guidance, and cleanup steps for existing STARTED runs - Link from getting-started.md (--lab-id note) and isvreporter.md - Add guide to docs/README.md under Guides Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: aanimasaun-nvda <aanimasaun@nvidia.com>
aanimasaun-nvda
force-pushed
the
docs/troubleshooting-started-tests
branch
from
February 11, 2026 21:44
89c95a5 to
47544bc
Compare
Remove Mirantis-specific references; guide applies to all ISV Lab partners. Signed-off-by: aanimasaun-nvda <aanimasaun@nvidia.com> Co-authored-by: Cursor <cursoragent@cursor.com>
saiyam1814
added a commit
to saiyam1814/ISV-NCP-Validation-Suite
that referenced
this pull request
May 16, 2026
… GKE 4 new conformance test skips found during live run (total now 10): - HostPort validates that there is no conflict: HostPort binding not supported in vCluster (same root cause as HostPorts skip NVIDIA#1). - should be able to create a functioning NodePort service: NodePort reachability fails because virtual node InternalIPs (34.118.x.x on GKE) differ from the host VPC IP where kube-proxy binds (same root cause as ExternalName-to-NodePort skip NVIDIA#4). - should run through the lifecycle of a PV and a PVC: PV patch races with vCluster's PV sync, causing a spurious not-found error. - should schedule a Pod requesting a RuntimeClass and initialize its Overhead: RuntimeClass overhead is not factored into virtual node allocatable resources on synced nodes. K8sDriverVersionCheck fix (belt-and-suspenders): - setup.sh: unconditionally clear driver_version on GKE (was conditional on 'unknown'; GKE never publishes nvidia.com/cuda.driver.* labels). - k8s.yaml: add kubernetes.checks.K8sDriverVersionCheck.driver_version="" override so the check skips even if setup.sh output changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
saiyam1814
added a commit
to saiyam1814/ISV-NCP-Validation-Suite
that referenced
this pull request
May 16, 2026
New failures caught by continuous monitoring: - should be able to switch session affinity for NodePort service: same NodePort IP mismatch root cause as the other NodePort failures (NVIDIA#4). - CustomResourceConversionWebhook should be able to convert a non homogeneous list of CRs: the virtual control plane (syncer pod) cannot reach conversion webhook pods in the tenant because the virtual apiserver runs in the host namespace, separated from tenant workloads by vCluster's network isolation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
saiyam1814
added a commit
to saiyam1814/ISV-NCP-Validation-Suite
that referenced
this pull request
May 16, 2026
Two new conformance failures discovered in the fresh clean run: 1. "should support a Service with multiple ports specified in multiple EndpointSlices" — same NodePort IP mismatch root cause as the existing "multiple endpoint IPs" skip; two distinct EndpointSlice conformance tests both exercise the same broken GKE vCluster network path. 2. "should apply changes to a pv/pvc status" — same PV sync race as the existing PV lifecycle skip; the test fetches a PV that hasn't synced back to the tenant cluster yet, causing a "not found" error. Also updated comments for NVIDIA#4 (EndpointSlice) and NVIDIA#5 (PV) to cover both variants of each failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9 tasks
sinorga
referenced
this pull request
in sinorga/ai-cloud-validation
May 18, 2026
Three P1 fixes to clear phase2 review (p1=0 p2=1; the lone P2 is an oracle-mirroring cleanup-ordering weakness, not a target regression): 1. isvctl/configs/providers/gcp/config/vm.yaml: trim start_instance step timeout from 900s -> 600s (2x the AWS oracle's 300s cap rather than 3x). Internal cloud-init / SSH waits are bounded by deadlines inside the stub, so the cap is not a sum of internal waits. Live evidence: start_instance ran in ~74s in the full-chain live gate (test-b9cc97cd), well under the new 600s cap. 2. isvctl/configs/providers/shared/teardown_nim.py: add a sentinel-skip path (success=True, skipped=True, rc=0) when --host or --key-file is the canonical 'none' / 'null' / '' sentinel that vm.yaml forwards on upstream skip/failure. Mirrors deploy_nim's NGC_API_KEY skip shape and the docs/existing-patterns/common.md 'forwarded inter-step Jinja args' rule. AWS teardown_nim path forwards real values so this change is a no-op for AWS. 3. isvctl/pyproject.toml + uv.lock: tighten google-cloud-compute floor from >=1.29.0 to >=1.30.0,<2.0.0 to match the reviewed GCP knowledge floor at .factory/context/ncp-knowledge/gcp/vm.yaml:887-888. Locked resolution stays at 1.47.0; uv.lock refreshed offline. Gates: live (full chain incl. console_rbac, 1419s), phase2 review (attempt #4, p1=0) Changed paths: isvctl/configs/providers/gcp/config/vm.yaml, isvctl/configs/providers/shared/teardown_nim.py, isvctl/pyproject.toml, uv.lock
sinorga
referenced
this pull request
in sinorga/ai-cloud-validation
May 18, 2026
… probe POST-PATCH FIX #4 on integration branch agent/gcp-workers-286265d1-…, addressing the only iteration-5 review P1 that's a real bug worth fixing under the operator's "fix critical, skip concern-cases" criterion. ## Trigger Post-fix final-run review test-d31fe561 (P1=2) flagged `vm.yaml:112` `console_rbac` as missing `--network` flow-through from `launch_instance.vpc_id`. Reviewer's required action: thread the launched-network through to the probe so probe VMs in non-default- network projects validate against the same network as the launched instance. ## Why fix this one (and not the other iteration-5 finding) The other iteration-5 P1 (start_instance step timeout vs SSH stack) was identified as a FALSE POSITIVE — the reviewer interpreted `wait_for_ssh_stable` + `wait_for_cloud_init` as a single "SSH stability gate" (semantic grouping), but the audit rule at `test_harness/review/agents/ncp-review-contract-config.md:80-89` explicitly says "longest SINGLE wait" means one `wait_for_X(timeout=K)` call, not a sum of sequential waits forming a logical readiness check. The longest single wait in start_instance.py is `wait_for_cloud_init(timeout=600)` = 600s, equal to the step cap. NOT under-cap by the rule's literal definition. F-073 + F-103 in docs/tracking/FACTORY-ISSUE-TRACKING.md (closed/RETIRED 2026-05-15) established that step timeout is a CAP, not a SUM — operator's "we don't need to sum up all operation's timeout" guidance reaffirms this. Tracker entry for the false-positive prevention rule extension lands separately in factory inputs (common.md + ncp-review-contract-config.md clarifying language). ## What this commit changes `vm.yaml:112-122` adds two args to `console_rbac`: - "--network" - "{{steps.launch_instance.vpc_id | default(network, true)}}" mirroring the pattern used by `list_instances` (line 72 in the same file). `console_rbac.py:925` already accepts `--network` in its argparse and threads it through to `network=args.network` at `console_rbac.py:990` — the gap was solely in the provider config's wiring. ## Iteration lineage on this branch - 7ed96e6 (post-patch fix #1, 2026-05-17): cleanup-bool capture + iam-retry envelope. Driven by 05-17 post-review P1#1. - 0d985d6 (post-patch fix #2, 2026-05-18): broaden URLError catch with TimeoutError/OSError. Driven by code-review against #1. - c135fc7 (post-patch fix #3, 2026-05-18): zone-walk contract compliance — `other_in_region` semantics + structured error handling in `_list_region_zones` + PREFERRED_ZONES drift. Driven by post-fix final-run review iterations #1 + #2. - THIS COMMIT (post-patch fix #4, 2026-05-18): thread vpc_id to console_rbac. Driven by post-fix final-run review iteration #3 (the non-false-positive half of the P1=2 verdict). The PR doc's §5 Human-in-loop edit log will render these as four separate rows with distinct triggers. ## Validation - static: test-1c462ac1 PASS (46/46 incl. no_internal_issue_ids 10/0/10). - Live gate not re-run for this single yaml change — the wiring adds one arg pair the consuming script already supports. Prior live gates on this branch lineage (test-489a0efc, test-9fbc9ac7, test-b36f66d1) all PASSED with default-network setups; the new arg falls back to `network` (default) via `default(network, true)` on operator envs that don't set vpc_id. ## Factory-side rule landing (separate, NOT in this commit) The reviewer's iteration-3 finding for this class did NOT have a matching reviewer audit before today; the rule will land alongside F-105/F-106/F-107 as "operator argument flow-through to dependent probes" in a subsequent factory commit. False-positive prevention for the under-cap rule (clarify "longest SINGLE wait" definition) also lands separately as a `common.md` + `ncp-review-contract-config.md` edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
abegnoche
added a commit
to abegnoche/ai-cloud-validation
that referenced
this pull request
Jul 24, 2026
…path The same config ran a different set of checks depending on how it was named: `--suite network` gave 26 checks (core default), `-f .../network.yaml` gave 35 (unfiltered), and `--provider X --label network` gave 35 as well. The label path is the one requirements.md NVIDIA#4 calls out by name - label selection must not run checks gated on a capability the ISV does not support - and it did. Resolve the context after config validation instead of inside the `--suite` branch, so one rule covers `--suite`, `-f` and `--label` discovery alike: a plain suite with no `--capability` runs its core checks. Platform suites declare no `requires:`, so they keep the unfiltered context. Label discovery inherits this through its recursive call, which closes NVIDIA#4 without special casing it. This drops the "no filtering" pseudo-context as a default. It modelled no real ISV situation - nobody runs on vm and kubernetes at once - so a plain suite now always carries exactly one context. Warn when `--capability` names something no check in the suite requires (`--suite iam --capability kubernetes`): the run proceeds with core checks, but a flag that silently does nothing reads as a typo. UI-composed commands are unaffected. `-f` used to hand the demos their gated checks for free, so the Makefile now names a capability per suite to keep exercising those stubs; without it `make demo-test` would quietly shrink by ~29 checks and stop covering the launch_instance / install_image_bm / install_config_bm scripts while still printing all-green. image-registry splits its gated checks between vm and bare_metal, so it runs once per context. Document the rule in configs/suites/README.md, which described neither `requires:` nor the selection grammar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
added a commit
that referenced
this pull request
Jul 28, 2026
* feat(isvtest): add requires capability expand and filter Introduce declarable capability vocabulary, expand_capabilities() for compute inheritance from vm/bare_metal, per-check requires filtering with explicit capability_requirement skip reasons, and validation for requires values on plain-suite checks. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(isvctl): validate requires and reject legacy suite axes Reject tests.module, tests.platform=compute, requires on platform suites, and legacy per-check platforms fields. Rewrite validate_suite_wiring for plain vs platform suite rules and globally unique wiring names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(isvctl): add --suite/--capabilities CLI and step gating Resolve platform or plain suite configs via unified --suite selection, parse optional --capabilities filter context (rejecting compute), apply capability filtering in orchestration with human-readable dry-run plans, and skip setup steps when all bound checks are filtered out. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(catalog): migrate envelope to schema v2 requires model Build catalog entries from canonical suite YAML with source, suite, platform, and requires metadata. Bump schemaVersion to 2, replace platforms with capabilities in the envelope, and remove the legacy catalog_platforms registry module. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): migrate plain suites to requires model Remove tests.platform from plain capability suites, add per-check requires from requires-worksheet, move kubernetes storage checks into storage.yaml with ensure_cluster wiring, add my-isv demo stub and AWS reference setup, and drop provider platform keys from plain-suite command groups. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * test: update provider merge and catalog expectations Align merger, nico provider, and capacity config tests with plain-suite capability resolution and catalog schema v2 entry shape. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(make): filter storage demo to vm capability context my-isv storage demo exercises compute-path checks only; pass --capabilities vm so kubernetes requires checks are skipped with explicit capability_requirement reasons. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(catalog): align upload envelope with backend platforms/suites Emit declarable capabilities as `platforms` and plain suite names as `suites` in catalog documents and upload payloads so backend catalog ingestion matches the feat/test-structure contract. * feat: add --dry-run alias for catalog push --no-upload Both flags now build and save the catalog locally without uploading, using --dry-run as the primary name for consistency with other commands. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> (cherry picked from commit 1550697) * fix(docs): sync test-plan labels with storage suite wiring K8S23-04..07 gained the storage label in the suite YAML; the test-plan coverage guardrail requires the doc to carry the same label union. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * chore(make): say suites, not domains, in demo output Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): use any-match requirement semantics Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(storage): tear down cluster fixture Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(suites): defer unique wiring names Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(cli): reject unknown test options Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(cli): make --capability single-valued The four capabilities (kubernetes, slurm, vm, bare_metal) are mutually exclusive execution environments — you run on one, never a combination — so a comma-separated multi-value context never made sense. - Rename the flag --capabilities -> --capability (single value); reject comma-separated input with an explanatory error. - parse_capabilities -> parse_capability (returns str | None). - requirements_satisfied / resolve_entries take one capability instead of a set: `not requires or capability in requires`. - Thread a single capability through the orchestrator and dry-run. `requires` (per-check) stays a list: any-match applicability across the isolated contexts, not a request to combine them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(catalog): enforce disjoint capability/suite namespaces A plain suite named after a declarable capability (vm/bare_metal/kubernetes/slurm) would collapse the merged frontend test-target list and cross-wire the backend's flat-selection re-split (selection ∩ catalog.platforms). Reject the collision at the upload chokepoint (catalog_document) and in the wiring lint (validate_suite_wiring.py), reusing DECLARABLE_CAPABILITIES, so the capability-vs-suite distinction stays recoverable by construction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(config): harden requires validation at validate and lint time test validate now rejects unknown/duplicate requires values via the Pydantic suite-shape validator, matching the run-time and wiring checks. validate_suite_wiring flags a requires naming a capability with no platform suite (no ISV can declare it, so the check is unreachable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(storage): gate EKS lifecycle by capability Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(selection): default plain suites to core checks Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(reporting): upload complete catalog envelope Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(selection): compose suite and label filters Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(selection): gate teardown steps by capability Capability gating skips a step when it carries an explicit `requires:` or when every validation bound to it is requirement-filtered. Teardown steps have no bound validations, so inference cannot reach them: under the core-by-default `--suite` path they kept running against fixtures that were never created. Only storage carried explicit gates; three other suites had the same hole. - image-registry: teardown deleted both the core upload_image resources and the vm-only launch_instance ones, so it referenced a skipped step and was abandoned whole with "missing step reference" - leaking the image, disks and bucket. Split by owner into teardown_instance (requires: [vm]) and teardown_image (core); the scripts already guard each resource independently, so no script change was needed. Rebind the suite's teardown check to teardown_image, the half that always runs. - observability: gate launch_host/teardown_host on [vm, bare_metal] so a core run no longer launches the reference metal host only to skip every check that needed it. - security: gate topology_block_teardown/capacity_teardown on [vm, bare_metal], mirroring the gate inferred on the test-phase steps that allocate those resources. StorageCapacityTelemetryCheck moves from core to [vm, bare_metal]. It probes the volumes attached to a running instance via the same script and --instance-id as its StoragePerformanceTelemetryCheck twin, which already declared that gate; with no instance it reports "no volumes attached", so it could never pass as a core check. Needs a catalog re-upload. Add test_capability_step_gating.py: instead of three point tests it walks every plain-suite provider config across every context and asserts no surviving step reads a gated-off step's output without a `default(...)`, so new suites inherit the guarantee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(my-isv): align the scaffold with the reference lifecycle my-isv is the folder ISVs copy, so a guarantee that holds only in aws/ teaches the wrong lifecycle. Three divergences, all in the capability-gated paths added on this branch. - storage: setup_cluster carried no `requires: [kubernetes]` and had no teardown_cluster at all, so the scaffold demonstrated acquiring a cluster and never releasing it - the opposite of the setup_cluster/teardown_cluster contract aws/ has carried since 607c121. Add both, with STORAGE_SKIP_TEARDOWN mirroring AWS_SKIP_TEARDOWN, and a teardown_cluster.py stub in the house style. A no-op teardown is a valid answer when the stub reuses a long-lived cluster; the step still has to exist so a standalone storage run cannot leak a cluster it provisioned. - kubernetes/slurm: no config existed, so `--suite kubernetes --provider my-isv` errored - and that is exactly what the frontend emits for its default provider. The scripts were already there; only the wiring was missing. Steps are overridden rather than inherited because the canonical suites point at ../providers/my-isv/scripts/*, which resolves wrong from a provider config directory. Neither joins `make demo-test`: both drive real kubectl/sinfo, so a dummy-success stub has nothing to return. Also fixes a pre-existing scaffold break found while verifying the above: `--suite storage --capability kubernetes` failed with "'cluster_name' is a required property". The step name auto-detects the platform-suite `cluster` schema, which demands cluster_name/node_count - inventory no storage check reads. Set `output_schema: generic` on both providers; scoping it to my-isv would have recreated the divergence this commit removes, and per the JSON contract discipline storage must not require cluster inventory of an ISV. test_storage_cleanup_steps_have_explicit_capability_gates encoded the divergence as `if provider == "aws":`; both providers now assert the same gates. Add coverage that every declarable capability resolves to a my-isv platform suite, so a UI-emitted `--suite <capability>` cannot silently become an error again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(selection): resolve the capability context once, for every entry path The same config ran a different set of checks depending on how it was named: `--suite network` gave 26 checks (core default), `-f .../network.yaml` gave 35 (unfiltered), and `--provider X --label network` gave 35 as well. The label path is the one requirements.md #4 calls out by name - label selection must not run checks gated on a capability the ISV does not support - and it did. Resolve the context after config validation instead of inside the `--suite` branch, so one rule covers `--suite`, `-f` and `--label` discovery alike: a plain suite with no `--capability` runs its core checks. Platform suites declare no `requires:`, so they keep the unfiltered context. Label discovery inherits this through its recursive call, which closes #4 without special casing it. This drops the "no filtering" pseudo-context as a default. It modelled no real ISV situation - nobody runs on vm and kubernetes at once - so a plain suite now always carries exactly one context. Warn when `--capability` names something no check in the suite requires (`--suite iam --capability kubernetes`): the run proceeds with core checks, but a flag that silently does nothing reads as a typo. UI-composed commands are unaffected. `-f` used to hand the demos their gated checks for free, so the Makefile now names a capability per suite to keep exercising those stubs; without it `make demo-test` would quietly shrink by ~29 checks and stop covering the launch_instance / install_image_bm / install_config_bm scripts while still printing all-green. image-registry splits its gated checks between vm and bare_metal, so it runs once per context. Document the rule in configs/suites/README.md, which described neither `requires:` nor the selection grammar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * docs: document the capability model `requires:`, `--suite` and `--capability` appeared in no user-facing doc. The model existed only in the code and in decision.md, an internal design record, so an ISV had no way to learn what governs which checks they owe. - configs/suites/README.md already gained the rule when the default changed; this adds the full model to the two guides an ISV actually reads. - configuration.md: a "Capabilities and requires" section (the four mutually exclusive capabilities, platform vs plain suites, any-match semantics and why AND is deliberately inexpressible, the one selection rule, opt-in scope, the shared namespace), plus `requires` in the step field table with the gate-both-halves-of-a-fixture guidance. - external-validation-guide.md: the same model stated for an ISV audience, and a Running Validations section that shows the --suite/--capability grammar, --dry-run, label composition and single-check rerun by pytest passthrough. - my-isv scaffold README: real invocations rather than only the -f form, plus what governs which checks run. Also corrects drift found in the scaffold's Domains table: 7 of 11 script counts were wrong, and k8s/slurm listed no provider YAML - true until those configs landed in 3735674. Both now note they need a real cluster and so sit outside `make demo-test`. Drops a stale MY_ISV_DOMAINS reference for the MY_ISV_SUITES / DEMO_CAP_<suite> variables that replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * feat(reporting): report the (suite, capability) pair on test runs A run's signal is the pair, not the suite alone: `network` and `network --capability vm` execute different checks, so recording only one axis loses the distinction at the point where it matters most. Neither axis was transmitted at all -- the service inferred a single test_target_type from the first command, which collapsed both. Two client-side spellings have to be translated before they leave the process. CORE_REQUIREMENT_CONTEXT is this module's word for "no capability", which the service records as NULL rather than a sentinel. And a platform suite carries no explicit context because its own platform *is* the capability it runs under, so report that instead of dropping the axis for every platform-suite run. resolve_suite_name recovers the suite from every entry path, including `-f lab.yaml -f commands.yaml -f suites/k8s.yaml` where the first config is not the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(suites): share the capability vocabulary and stop re-parsing suites The pre-commit validator carried a hand-copied DECLARABLE_CAPABILITIES literal, so the hook that exists to enforce the vocabulary was the one place that could silently drift from the schema it enforces. It now imports the shared constant, and the REQUIREMENT_VOCABULARY alias (a pure alias with no external importers) is gone. Each suite YAML was also read three to four times and parsed two to three times per run: _build_suite_map parsed a file and then called iter_config_checks, which re-read and re-parsed the same file, and wiring_errors globbed the directory twice while reading each file twice more in its main loop. Both now expose iter_checks_from_data so a caller that has already parsed the document reuses it, and wiring_errors reads and parses each file exactly once. The suite vocabularies are cached: resolve_suite_name alone scanned the suites directory twice per invocation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * chore: move tests Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor: state the requires rule and suite-name canonicalization once Two rules had each been written out four times. `requires` validation (a list, drawn from DECLARABLE_CAPABILITIES, no duplicates) lived in StepConfig.validate_requires, in ValidationConfig.validate_suite_shape, in _validate_entry_shape, and in the wiring script -- with four separately maintained message strings, so "requires must contain only" and "requires must be a list containing only" were the same verdict spelled two ways. requires_error() states it next to the vocabulary it validates; each caller adds only its own prefix. The pre-commit hook and the runtime validator can no longer disagree about what a valid suite file is. Suite-name canonicalization was worse, because suite name is the join key between a test run and its catalog entries: `_normalize_name` did lower + `-`/`_` + the k8s alias, `catalog.py` did the replace alone in two places, and the wiring script hardcoded `if stem == "k8s"`. Adding a second aliased filename meant finding all four. canonical_suite_name() in isvtest.core.resolution is now the single authority -- isvctl already depends on isvtest, so nothing new is coupled. Alongside those, three smaller consolidations: iter_checks_from_data existed twice, differing only in whether the category is yielded and in a branch the script's copy had dropped (dict-form categories without a `checks` key). The three-tuple version in isvtest.catalog is now the only walker of the wiring shape; no suite YAML uses the missing branch today, so the script only gains coverage. catalog_document parsed the suites directory three times -- once via _build_suite_map, then again in each vocabulary builder. _iter_suite_docs makes it one pass, and suite_vocabularies returns both lists from it. CONFIGS_ROOT moves from cli/test.py to config/suite_resolution.py, so `deploy` no longer imports the whole `test` command module -- typer app, orchestrator and all -- to obtain one Path. _suite_name gains @cache, since classifying a config merges it with the suite it imports and a single run asks both resolve_suite and resolve_suite_name for it. test_capability_step_gating copied the run/skip cascade out of _apply_capability_step_gates into its own _gated_step_names, so it would have kept passing against the old rule if the real gate changed. It now calls the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(tests): strip rich styling before matching the rejected option name Typer forces a terminal under GITHUB_ACTIONS, so the reported option name arrives spliced with escape codes and the substring match fails in CI while passing locally. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix: name only the four declarable capabilities in schema and docs tests.platform now rejects anything outside vm, bare_metal, kubernetes and slurm, but its field description (and the generated JSON schema) still listed the old vocabulary, and four config examples declared values that raise at load. Plain suites omit the key entirely. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * test: gate every discovered provider and document the new helpers The gating invariant listed aws and my-isv by name, so nico's plain suites escaped it; discovering the config directories keeps a new provider covered. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * build: stop warning about the overridden demo-image-registry recipe The explicit two-capability rule overrode the static pattern rule, so every make invocation printed an overriding-recipe warning. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(vocab): rename tests.platform to tests.capability `capability` was already the shared word across the client, backend, and frontend (`--capability`, `DECLARABLE_CAPABILITIES`, `isv_version.capabilities`, `CapabilityChip`). `platform` was the odd one out, and it meant three different things: the capability a platform suite declares (`tests.platform:`), the lifecycle command groups (`commands.<key>`, which also span plain suites like `iam` and `security`), and the capability-valued half of the catalog namespace. Delete it as a synonym for capability. `requires` stays as-is: it is the predicate on a check, `capability` is the value, and collapsing the two would lose the distinction between "this check requires kubernetes" and "this run ran under kubernetes". - `tests.platform:` -> `tests.capability:` in the four platform suites and every reader (schema, suite_resolution, cli/test, orchestrator/loop, isvtest catalog, isvreporter platform detection, the suite wiring validator). - The legacy key now raises rather than being ignored. `ValidationConfig` allows extras, so an unmigrated `tests.platform:` would have landed in `model_extra`, demoted the suite to a plain suite, and gated every check on a capability it no longer declared. Both validators report the rename, matching how `tests.module` is handled. - `commands.<key>` keys are unchanged; their descriptions and KeyError messages now say "test target", since the keys span capabilities and plain suites. - `isvctl test validate` prints `Capability:` instead of `Platform:`. - Regenerated config.schema.json. The wire format is deliberately untouched: the catalog envelope still carries `platforms`/`suites` and entries still carry `platform` xor `suite` plus `requires`. Renaming those columns is a separate migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(vocab): emit capabilities/capability in the catalog payload Client half of the catalog wire rename. The document envelope's `platforms` becomes `capabilities` and a per-entry `platform` becomes `capability`, matching the renamed backend columns and the word the CLI already uses for this axis (`--capability`, tests.capability). - isvtest.catalog: the envelope key and the per-check suite map. - isvreporter.client.upload_test_catalog: the `capabilities` kwarg and the JSON body it POSTs. - isvctl catalog push/list read the renamed keys. The backend accepts both spellings on upload (@JsonAlias), so a lab still running an older isvctl is unaffected; this only changes what a current client sends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(reporting): upload the renamed catalog envelope, and pin the fixture to it Addresses review feedback on #561. The first item is a live break I introduced in 02acfc0. **Catalog upload was silently dead.** `isvctl.reporting.update_test_run` still did `catalog_document["platforms"]` and passed `platforms=` to `upload_test_catalog`. The envelope key is now `capabilities` and the parameter was renamed in the same commit, so every automatic upload raised KeyError before the call was even made. It is wrapped in `except Exception -> logger.warning`, so nothing failed loudly: coverage data just stopped arriving. `isvreporter report update --test-catalog` had the same break reading a saved catalog file; that path now prefers `capabilities` and falls back to `platforms`, since v1 files on disk outlive the rename. **The test suite could not have caught it.** The fixture in test_forwards_complete_catalog_document is a literal that kept the old `platforms` key, so it exercised a shape no producer emits and stayed green while the real path raised. Updated, and pinned: a new assertion compares the envelope keys against `catalog_document()` itself, so the next rename fails here instead of in production. Also from the review: - `_declared_platform` and `build_catalog` docstrings still described the entry key as `platform`; it is `capability`. - `docs/packages/isvreporter.md` documented `--platform` as taking three values and omitted vm. It takes all four; the flag keeps its spelling because it maps to the service's deprecated `test_target_type`. - `PlatformCommands` claimed skip works "at phase level" -- there is no per-phase skip field, only target and step -- and contradicted the corrected sentence four lines below it. - Two lines over the 120-char guideline, and the regenerated schema. - `resolve_suite` classified every YAML in a provider's config dir without catching SuiteResolutionError, so one malformed file broke `--suite <anything>` for that whole provider. `resolve_suite_name` already skipped such files; this now matches it. Pre-existing, not from the rename. Verified both upload call sites bind against the real signature and a real envelope, including the v1 legacy fallback. 1452 + 58 + 1279 + 113 tests pass, lint and pre-commit clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * refactor(vocab): rename leftover platform locals after capability rename The YAML key and catalog envelope already say capability/capabilities; align the local names in the suite wiring validator and catalog builder so the post-rename vocabulary is consistent. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * style: shorten the comments added in this branch Rationale blocks belong in commit messages, not above the code. Also drops a duplicated skip sentence from the PlatformCommands docstring and regenerates the schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * test: scope the default(...) guard per Jinja expression Addresses the last open finding on #561. `_unguarded_references` returned nothing as soon as `default(` appeared anywhere in a string, so a mixed value like {{ steps.a.id | default('') }} {{ steps.b.id }} reported no dependency at all -- the guarded expression vouched for its unguarded neighbour, and the gating test would have passed a step that reads a skipped step's output. Now each `{{ ... }}` is checked on its own. Latent, not masking anything: every provider config still passes under all five contexts with the stricter helper. Adds unit tests for the helper itself, including the mixed-string case, since it had none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix: preserve platform-suite reporting identity Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> * fix(schema): reject empty tests.capability as invalid Truthiness treated capability: "" as an omitted plain-suite axis, so an empty value bypassed the vocabulary check and could carry requires. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> --------- Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.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.