feat(admin-cli): erase site metadata for a BMC MAC (#3046) - #4013
feat(admin-cli): erase site metadata for a BMC MAC (#3046)#4013hatamzad-nv wants to merge 1 commit into
Conversation
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Forge RPC and admin CLI command to erase NICo-owned host metadata by BMC MAC, with dry-run reporting, ownership safeguards, transactional cleanup, credential removal, authorization, and integration tests. ChangesBMC metadata erasure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant ForgeAPI
participant EraseHandler
participant Database
participant Vault
AdminCLI->>ForgeAPI: Submit BMC MAC and dry-run flag
ForgeAPI->>EraseHandler: Authorize and forward request
EraseHandler->>Database: Inspect interfaces and explored records
EraseHandler->>Database: Delete eligible metadata when not dry-run
EraseHandler->>Vault: Clear BMC credentials after commit
EraseHandler-->>ForgeAPI: Return affected IDs, IPs, and credential status
ForgeAPI-->>AdminCLI: Return response for formatted output
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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-23 20:14:50 UTC | Commit: 0a089a6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/admin-cli/src/managed_host/tests.rs (1)
350-395: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the confirmation guard, not only argument parsing.
These cases validate Clap wiring but never exercise
cmd::erase_metadata; add a command-level test asserting that a non-dry run without--confirmreturns a failure, while dry-run remains allowed. This protects the destructive-operation safety contract from regressions.🤖 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 `@crates/admin-cli/src/managed_host/tests.rs` around lines 350 - 395, Add a command-level test alongside parse_erase_metadata_routes_to_erase_metadata that invokes cmd::erase_metadata with a non-dry-run configuration lacking --confirm and asserts failure, plus a dry-run case without confirmation that succeeds. Keep the existing Clap argument-parsing scenarios unchanged.
🤖 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 `@crates/admin-cli/src/managed_host/erase_metadata/args.rs`:
- Around line 23-31: Update the MAC address placeholder in both examples within
the erase-metadata command’s after_long_help text to the canonical
00:11:22:33:44:55 value, preserving the existing dry-run and confirm example
commands.
In `@crates/admin-cli/src/managed_host/erase_metadata/cmd.rs`:
- Around line 24-34: Update erase_metadata so a non-dry-run without --confirm
returns the existing Carbide CLI error type after printing the refusal message,
allowing callers to observe failure; preserve successful Ok(()) behavior for
--dry-run invocations.
In `@crates/rpc/proto/forge.proto`:
- Around line 334-342: Update the EraseHostMetadataByBmcMac RPC documentation to
describe every cleanup protection: refuse erasure when interfaces or BMC IPs
remain associated with live or ingested devices, and preserve records whose
advertised MAC differs from the requested BMC MAC. Keep the existing
live-machine ownership, expected-machine, and dry_run behavior documented.
---
Nitpick comments:
In `@crates/admin-cli/src/managed_host/tests.rs`:
- Around line 350-395: Add a command-level test alongside
parse_erase_metadata_routes_to_erase_metadata that invokes cmd::erase_metadata
with a non-dry-run configuration lacking --confirm and asserts failure, plus a
dry-run case without confirmation that succeeds. Keep the existing Clap
argument-parsing scenarios unchanged.
🪄 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: 6d83dd08-c9ff-45c2-9d91-48628ae6e81d
📒 Files selected for processing (14)
crates/admin-cli/src/managed_host/erase_metadata/args.rscrates/admin-cli/src/managed_host/erase_metadata/cmd.rscrates/admin-cli/src/managed_host/erase_metadata/mod.rscrates/admin-cli/src/managed_host/mod.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/erase_host_metadata.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/tests/common/endpoint.rscrates/api-core/src/tests/erase_host_metadata.rscrates/api-core/src/tests/mod.rscrates/rpc/build.rscrates/rpc/proto/forge.proto
0a089a6 to
5f8383d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/admin-cli/src/managed_host/tests.rs`:
- Around line 349-396: Regenerate and commit the admin-cli reference
documentation for the new managed-host erase-metadata command surface introduced
by parse_erase_metadata_routes_to_erase_metadata. Follow the repository’s
established documentation-generation process and ensure the generated output
reflects the required --bmc-mac argument and dry-run/confirm options.
🪄 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: 7bfbbfd9-27b8-4a1e-8a07-da401290a311
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (15)
crates/admin-cli/src/managed_host/erase_metadata/args.rscrates/admin-cli/src/managed_host/erase_metadata/cmd.rscrates/admin-cli/src/managed_host/erase_metadata/mod.rscrates/admin-cli/src/managed_host/mod.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/erase_host_metadata.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/tests/common/endpoint.rscrates/api-core/src/tests/erase_host_metadata.rscrates/api-core/src/tests/mod.rscrates/rpc/build.rscrates/rpc/proto/forge.protorest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/api-core/src/api.rs
- crates/api-core/src/tests/mod.rs
- crates/admin-cli/src/managed_host/erase_metadata/mod.rs
- crates/admin-cli/src/managed_host/mod.rs
- crates/admin-cli/src/managed_host/erase_metadata/cmd.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/rpc/proto/forge.proto
- crates/api-core/src/tests/common/endpoint.rs
- crates/admin-cli/src/managed_host/erase_metadata/args.rs
- crates/api-core/src/handlers/erase_host_metadata.rs
- crates/api-core/src/tests/erase_host_metadata.rs
|
|
||
| // erase-metadata routes to the EraseMetadata variant. Each row yields | ||
| // (bmc_mac, dry_run, confirm); the required --bmc-mac is missing in the failing row. | ||
| #[test] | ||
| fn parse_erase_metadata_routes_to_erase_metadata() { | ||
| scenarios!( | ||
| run = |argv| { | ||
| Cmd::try_parse_from(argv.iter().copied()) | ||
| .map(|cmd| match cmd { | ||
| Cmd::EraseMetadata(args) => (args.bmc_mac, args.dry_run, args.confirm), | ||
| _ => panic!("expected EraseMetadata variant"), | ||
| }) | ||
| .map_err(drop) | ||
| }; | ||
| "with bmc mac" { | ||
| &[ | ||
| "managed-host", | ||
| "erase-metadata", | ||
| "--bmc-mac", | ||
| "aa:bb:cc:dd:ee:ff", | ||
| ][..] => Yields(("aa:bb:cc:dd:ee:ff".to_string(), false, false)), | ||
| } | ||
|
|
||
| "with bmc mac and dry-run" { | ||
| &[ | ||
| "managed-host", | ||
| "erase-metadata", | ||
| "--bmc-mac", | ||
| "aa:bb:cc:dd:ee:ff", | ||
| "--dry-run", | ||
| ][..] => Yields(("aa:bb:cc:dd:ee:ff".to_string(), true, false)), | ||
| } | ||
|
|
||
| "with bmc mac and confirm" { | ||
| &[ | ||
| "managed-host", | ||
| "erase-metadata", | ||
| "--bmc-mac", | ||
| "aa:bb:cc:dd:ee:ff", | ||
| "--confirm", | ||
| ][..] => Yields(("aa:bb:cc:dd:ee:ff".to_string(), false, true)), | ||
| } | ||
|
|
||
| "missing required bmc mac" { | ||
| &["managed-host", "erase-metadata"][..] => Fails, | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include regenerated admin-cli reference documentation before merge.
This adds the managed-host erase-metadata command surface, but the PR still leaves generated CLI documentation pending. Without the regenerated reference, operator-facing help remains stale.
As per path instructions, crates/admin-cli/** command-surface changes require regenerated reference documentation.
🤖 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 `@crates/admin-cli/src/managed_host/tests.rs` around lines 349 - 396,
Regenerate and commit the admin-cli reference documentation for the new
managed-host erase-metadata command surface introduced by
parse_erase_metadata_routes_to_erase_metadata. Follow the repository’s
established documentation-generation process and ensure the generated output
reflects the required --bmc-mac argument and dry-run/confirm options.
Source: Path instructions
There was a problem hiding this comment.
The command moved to machine erase-metadata (per @ericpretzel's review above), so this file path is outdated, the test now lives in crates/admin-cli/src/machine/tests.rs.
On the generated docs: regenerating locally rewrites ~580 unrelated pages because my pandoc version differs from whatever produced the committed docs, which would bury the real diff. I don't see a check-cli-docs job in CI to pin the expected output against either. Happy to include the regenerated reference if someone can confirm the pandoc version used for the committed docs, otherwise I'll file it as a follow-up.
5f8383d to
3bb3666
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4013.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/admin-cli/src/managed_host/tests.rs (1)
372-390: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the combined destructive-execution flags.
The test verifies
--dry-runand--confirmindependently, but not together. Add a scenario containing both flags to ensure the parser preserves the exact invocation required for destructive execution.🤖 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 `@crates/admin-cli/src/managed_host/tests.rs` around lines 372 - 390, Add a case to the managed-host erase-metadata argument parsing tests that supplies both --dry-run and --confirm with the existing BMC MAC, and assert the parser returns that MAC with both corresponding flags enabled.
🤖 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.
Nitpick comments:
In `@crates/admin-cli/src/managed_host/tests.rs`:
- Around line 372-390: Add a case to the managed-host erase-metadata argument
parsing tests that supplies both --dry-run and --confirm with the existing BMC
MAC, and assert the parser returns that MAC with both corresponding flags
enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4eb2d4c2-8769-4013-8c5c-90cd2c4e41b1
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (15)
crates/admin-cli/src/managed_host/erase_metadata/args.rscrates/admin-cli/src/managed_host/erase_metadata/cmd.rscrates/admin-cli/src/managed_host/erase_metadata/mod.rscrates/admin-cli/src/managed_host/mod.rscrates/admin-cli/src/managed_host/tests.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/erase_host_metadata.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/tests/common/endpoint.rscrates/api-core/src/tests/erase_host_metadata.rscrates/api-core/src/tests/mod.rscrates/rpc/build.rscrates/rpc/proto/forge.protorest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/api-core/src/tests/mod.rs
- crates/api-core/src/api.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/api-core/src/handlers/mod.rs
- crates/rpc/build.rs
- crates/api-core/src/tests/common/endpoint.rs
- crates/admin-cli/src/managed_host/erase_metadata/cmd.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/admin-cli/src/managed_host/erase_metadata/args.rs
- crates/rpc/proto/forge.proto
- crates/api-core/src/handlers/erase_host_metadata.rs
- crates/admin-cli/src/managed_host/mod.rs
- crates/admin-cli/src/managed_host/erase_metadata/mod.rs
- crates/api-core/src/tests/erase_host_metadata.rs
|
Since this feature request is part of the broader decomissioning epic, I think these changes should be re-worked to integrate into it better. Please work with @ericpretzel to see where it could fit. The decomissioning design doc is here: #3659 |
ericpretzel
left a comment
ericpretzel
left a comment
There was a problem hiding this comment.
This isn't intended to be a direct feature of decommissioning but decommissioning definitely would like to use the erase metadata endpoint as a part of final deletion. Something like delete machine record -> erase metadata -> success. We haven't gotten to this point in implementation quite yet so I apologize if it's a bit fuzzy.
However, instead of being a managed-host subcommand this should be a machine subcommand because we will want to be able to clean up managed switches and power shelves as well as managed hosts.
I think the general functionality is good as it is now but we might want to have a follow-up task (probably outside the scope of this PR) that bundles up some of the shared logic between this feature, decommissioning, and force deletion.
3bb3666 to
93e0f0d
Compare
|
Thanks @ericpretzel moved it under machine (nico-admin-cli machine erase-metadata) as you suggested, since we'll want to cover switches/power shelves too. Just pushed that. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/admin-cli/src/machine/erase_metadata/cmd.rs`:
- Around line 36-44: Update the response handling in the erase metadata command
after erase_host_metadata_by_bmc_mac returns to unwrap the tonic response with
into_inner(). Use the resulting EraseHostMetadataByBmcMacResponse for the
dry_run check and serde_json serialization, while preserving the existing output
messages.
🪄 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: 7d03b35a-287d-47fa-b65d-a7994dfe5b76
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (15)
crates/admin-cli/src/machine/erase_metadata/args.rscrates/admin-cli/src/machine/erase_metadata/cmd.rscrates/admin-cli/src/machine/erase_metadata/mod.rscrates/admin-cli/src/machine/mod.rscrates/admin-cli/src/machine/tests.rscrates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/erase_host_metadata.rscrates/api-core/src/handlers/mod.rscrates/api-core/src/tests/common/endpoint.rscrates/api-core/src/tests/erase_host_metadata.rscrates/api-core/src/tests/mod.rscrates/rpc/build.rscrates/rpc/proto/forge.protorest-api/proto/core/src/v1/nico_nico.proto
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/api-core/src/handlers/mod.rs
- crates/api-core/src/auth/internal_rbac_rules.rs
- crates/rpc/proto/forge.proto
- crates/rpc/build.rs
- crates/api-core/src/tests/mod.rs
- crates/api-core/src/tests/common/endpoint.rs
- rest-api/proto/core/src/v1/nico_nico.proto
- crates/api-core/src/handlers/erase_host_metadata.rs
93e0f0d to
5d7f377
Compare
5d7f377 to
f3d4689
Compare
f3d4689 to
d43a289
Compare
d43a289 to
adb7ef1
Compare
adb7ef1 to
05a27ed
Compare
Add managed-host erase-metadata --bmc-mac <MAC> [--dry-run] [--confirm], removing all NICo-owned records for a server BMC MAC (machine interfaces and their addresses/DHCP/boot-override, retained boot rows, site-explorer exploration reports, explored managed hosts, and vault BMC credentials plus convergence markers) so an operator has a clean slate to re-ingest a replacement host. Deletion is MAC-driven from cached state (no live BMC call), so an off or relocated host is still cleaned. Refuses when an interface is still owned by a live device (machine, DPU, switch, power shelf) or a BMC IP still belongs to an ingested machine; protects a managed-host row whose IP now belongs to a different BMC. Safety for the destructive path is dry-run plus explicit --confirm. Expected machines are never touched. Closes NVIDIA#3046 Signed-off-by: Amir Hatamzad <ahatamzad@nvidia.com>
05a27ed to
9c71025
Compare
Looks good now that the subcommand was moved to machine
kensimon
left a comment
kensimon
left a comment
There was a problem hiding this comment.
Comments are just nitpicks, can be ignored unless you find yourself needing to push again and feel like making tweaks.
| required(true), | ||
| help = "Server BMC MAC address whose lingering site records should be erased" | ||
| )] | ||
| pub bmc_mac: String, |
There was a problem hiding this comment.
Nit: This can be a MacAddress directly and you'll get free argument validation before it gets sent to the server. Not a huge deal though.
| let mut bmc_ips: BTreeSet<IpAddr> = interfaces | ||
| .iter() | ||
| .flat_map(|i| i.addresses.iter().copied()) | ||
| .collect(); | ||
| bmc_ips.extend(endpoints.iter().map(|e| e.address)); |
There was a problem hiding this comment.
Nit: You can avoid a mut here with
| let mut bmc_ips: BTreeSet<IpAddr> = interfaces | |
| .iter() | |
| .flat_map(|i| i.addresses.iter().copied()) | |
| .collect(); | |
| bmc_ips.extend(endpoints.iter().map(|e| e.address)); | |
| let bmc_ips: BTreeSet<IpAddr> = interfaces | |
| .iter() | |
| .flat_map(|i| i.addresses.iter().copied()) | |
| .chain(endpoints.iter().map(|e| e.address)) | |
| .collect(); |
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
thossain-nv
removed
the
rest-api
|
Closing this in favor of a smaller implementation, per review feedback. Rather than a new erase-metadata command with its own deletion flow, I've extended delete_interface to accept a MAC address, as @Matthias247 suggested, same operator outcome, no parallel order-sensitive DB code (which also addresses @kensimon's regression-cost concern). Exploration reports are left to Site Explorer. Replacement PR coming shortly. I'll link it here. |
|
Replacement: #4466 |
5 participants
Implements #3046:
nico-admin-cli managed-host erase-metadata --bmc-mac <MAC> [--dry-run] [--confirm]removes all NICo-owned records for a server BMC MAC (machine interfaces + addresses/DHCP/boot-override, retained boot rows, site-explorer exploration reports, explored managed hosts, vault BMC credentials + convergence markers), giving an operator a clean slate to re-ingest a replacement host.Deletion is MAC-driven from cached state (no live BMC call), so an off/relocated host is still cleaned. Refuses when an interface is still owned by a live device (machine, DPU, switch, power shelf) or a BMC IP still belongs to an ingested machine; protects a managed-host row or exploration endpoint whose BMC advertises a different MAC. Safety is dry-run + explicit --confirm. Expected machines are never touched.
11 integration tests added (verified 10/10 locally against Postgres before adding the endpoint-protection test). Draft: relying on CI for the full test suite + clippy-flow; generated CLI docs to follow from CI's canonical output.