fix: Align v2.0 behavior and documentation#3321
Conversation
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe changes update CLI and MCP documentation, clarify health and lifecycle REST workflows, revise OpenAPI examples and descriptions, and restrict machine power control for provider viewers with test coverage. ChangesAPI and Documentation Alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3321.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/architecture/health_aggregation.md`:
- Around line 293-305: Update the operator-facing prose around the health-report
API to consistently use “NICo-managed host” and “built-in” instead of “NICo
managed host” and “builtin NICo tools,” including the Merge and Replace mode
descriptions.
In `@docs/configuration/tenant_management.md`:
- Line 105: Label the fenced code block in tenant_management.md with a language
identifier, such as text, by changing the opening fence to ```text while
preserving the block contents and closing fence.
In `@docs/manuals/building_nico_containers.md`:
- Line 49: Update the command comment describing machine-validation and
boot-artifact images to use the exact architecture name “x86_64” instead of
“x86”, resulting in “x86_64/aarch64”.
🪄 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: 72a719ed-4eeb-4a8a-8754-8b8a592194b8
⛔ Files ignored due to path filters (7)
rest-api/sdk/standard/api_bmc_credential.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_bmc_reset.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_dpu_reprovision.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_machine.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/model_bmc_reset_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_dpu_reprovision_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_machine_power_control_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (11)
docs/architecture/health_aggregation.mddocs/configuration/org-permissions.mddocs/configuration/tenant_management.mddocs/dpu-management/dpu-lifecycle-management.mddocs/manuals/building_nico_containers.mddocs/manuals/nicocli-reference.mddocs/provisioning/ingesting-hosts-rest-api.mdrest-api/api/pkg/api/handler/bmccredential.gorest-api/api/pkg/api/handler/machinepower.gorest-api/api/pkg/api/handler/machinepower_test.gorest-api/openapi/spec.yaml
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Signed-off-by: Kyle Felter <kfelter@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-07-10 04:32:52 UTC | Commit: 2a151c3 |
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
thossain-nv
left a comment
There was a problem hiding this comment.
Thanks for the fixes @kfelternv
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
What this PR does
Aligns the v2.0 REST behavior, OpenAPI contract, generated SDK, and operator documentation with the current implementation.
HostUpdateInProgress/PreventAllocationshealth report before DPU reprovisioning, the Set/Clear/Restart modes, attached-Instance acknowledgements, and current firmware behaviorcurrentandstatsHow we verified it
Current revision:
2a151c30919d561cb12626554fe863f692acac2eHands-on REST/MCP revision:
8766edc644fa5ce53d75eb83d30039e08289ba02. The current head adds only the generated SDK update after that runtime exercise.DevSpace integration base: PR #3304 at
1a0d78b9454ad5fbf841f3886a0451efb4ad2e98. The tested composite revision was3615be5437aa43a74ef2b98f07f47f37df9a8bc5.Environment: Linux x86_64 NICo development VM, Docker 29.1.3, kind 0.27.0, kubectl 1.30.8, and DevSpace 6.3.21 using the
kind-kindcontext.The full integrated stack was built and deployed with:
DevSpace rolled out the REST API image
localhost:5000/nico-rest-api:3615be54-1783656294(sha256:934ecca2c644115eadeec3654cd708de31203f948852f1e884d826ab44ac375a) and MCP imagelocalhost:5000/nico-mcp:3615be54-1783656294(sha256:6bdbe7c15b547da210b9c0091bdc1e84db14c1525de8aab7c7a6b328ac77aee9). The post-deploy integration check reported the registered site online with seven current Core machines.Hands-on results:
The authorization comparison used the same nonexistent Machine ID in both revisions. The old 404 proves the Provider Viewer passed authorization and reached Machine lookup; the patched 403 proves it is now rejected at the intended role gate. The Provider Admin still reached lookup and returned 404 in both revisions.
The DPU request was cleared, the temporary health report was deleted, the BMC credential was restored, and the temporary Provider Viewer role was removed after verification.
Supporting checks:
All listed checks passed. A broad
go test ./cli/pkgalso reaches the unrelatedTestSiteBootstrapCommandWritesReplayableManifestfailure; the same failure reproduces on upstreammainat2c3bf2f9b, while the focused current/stats command tests above pass.How to reproduce the verification
Prerequisites:
kind-kinddevelopment clusterStart from a clean checkout and assemble the exact product and DevSpace revisions:
git clone https://github.com/NVIDIA/infra-controller.git cd infra-controller git fetch origin pull/3321/head:pr-3321 pull/3304/head:pr-3304 git checkout --detach 2a151c30919d561cb12626554fe863f692acac2e git merge --no-commit --no-ff 1a0d78b9454ad5fbf841f3886a0451efb4ad2e98 kubectl config use-context kind-kind devspace deploy -n nico-system --no-colors --max-concurrent-builds 3Expected checkpoint: the deploy succeeds, all application pods become Ready, the registered site is online, and the post-deploy check reports at least one current Core Machine.
Forward the REST API and Keycloak in separate terminals:
Acquire the local Provider Admin token:
Verify that Provider Viewer is rejected before Machine lookup while Provider Admin still reaches it:
Expected result:
viewer=403andadmin=404.Verify the BMC credential response contract while preserving the seeded local credential:
Expected result:
invalid=400,valid=200, the response omitspassword, Vault contains the temporary value during the check, and cleanup restores the original local value.Verify the documented health precondition and DPU Set/Clear path with a current unassigned simulated host:
Expected statuses are 200 for the health report, 202 for Set, 202 for Clear, and 204 for cleanup.
Verify the deployed nicocli action names against the live API:
Expected result: both live tenant commands return JSON successfully, and the service-account
currentaction is present.Finally, forward the MCP service and exercise an authenticated GET tool:
Expected result: the tool list is non-empty and the site call returns a successful MCP result without an error.