fix(admin-cli): delete machine interfaces by MAC address (#3046) - #4466
fix(admin-cli): delete machine interfaces by MAC address (#3046)#4466hatamzad-nv wants to merge 1 commit into
Conversation
Extend DeleteInterface to accept a MAC address, so an operator can clear a leftover interface record when they have the BMC MAC but not the interface id -- the case that blocks re-ingestion of a replacement host. A MAC is unique only per network segment, so it can match several interfaces. All matches are validated before any is deleted, and the request is refused if any of them still belongs to a live machine, DPU, switch or power shelf. Also clear machine_boot_override in the shared machine_interface::delete. That foreign key has no ON DELETE CASCADE, so a leftover override previously failed the delete with a foreign-key violation; fixing it in the shared path also fixes the same latent bug for machine, switch and power-shelf force-delete. Exploration reports are intentionally left to site explorer, which prunes them on its next run. Part of NVIDIA#3046 Signed-off-by: Amir Hatamzad <ahatamzad@nvidia.com>
Summary by CodeRabbit
WalkthroughThe deletion flow now accepts an interface ID or MAC address. MAC selection can resolve multiple interfaces. The API validates all matches before deletion, clears boot overrides, and returns not-found errors for unknown selectors. ChangesInterface deletion selection and execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant APIClient
participant APIHandler
participant Database
AdminCLI->>APIClient: Submit InterfaceDeleteQuery with ID or MAC
APIClient->>APIHandler: Forward deletion request
APIHandler->>Database: Resolve matching interfaces
APIHandler->>Database: Validate associations
APIHandler->>Database: Clear boot overrides and delete interfaces
Database-->>APIHandler: Return deletion result
APIHandler-->>APIClient: Return success or validation error
APIClient-->>AdminCLI: Display result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Matthias247 @kensimon this is the smaller version we discussed—it extends delete_interface with a MAC selector instead of adding a separate flow, and leaves explored_endpoints to Site Explorer. |
🔐 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-31 22:29:03 UTC | Commit: fc9285a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
crates/admin-cli/src/machine_interfaces/tests.rs (1)
96-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a scenario for no selector.
The table covers ID only, MAC only, both selectors, and a malformed MAC. It does not cover the case with no selector. That case is the sole reason for
.required(true)on theArgGroup, so a regression there would pass unnoticed. Theadmin-clicrate is excluded from the CI test job, which raises the value of explicit coverage.♻️ Proposed additional scenario
"a malformed MAC is rejected at parse time" { &["machine-interface", "delete", "--mac-address", "not-a-mac"][..] => Fails, } + + "no selector is rejected" { + &["machine-interface", "delete"][..] => Fails, + }🤖 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/machine_interfaces/tests.rs` around lines 96 - 113, Add a table-driven scenario in the machine-interface delete argument tests for invoking the command with no selector, and assert that parsing fails. Place it alongside the existing selector cases to cover the required ArgGroup behavior.Sources: Coding guidelines, Learnings
crates/api-core/src/tests/machine_interfaces.rs (2)
1631-1654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the reason for the refusal, not only its category.
The test asserts
Code::InvalidArgument. The handler returns that same code for six distinct conditions, including both selectors set and every association check. An unrelated validation change could therefore keep this test green while the switch-ownership check stops working. Assert a fragment of the switch message to pin the actual cause.♻️ Proposed assertion
.expect_err("a switch-owned match must refuse the whole request"); assert_eq!(status.code(), Code::InvalidArgument); + assert!( + status.message().contains("belongs to switch"), + "the refusal must name switch ownership as the cause, got: {}", + status.message() + );🤖 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/api-core/src/tests/machine_interfaces.rs` around lines 1631 - 1654, Strengthen the refusal assertion in the delete_interface test by also checking that the returned status message contains the switch-ownership-specific text, not just Code::InvalidArgument. Update the assertion around the status from delete_interface and preserve the existing all-or-nothing database checks.
1660-1681: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for both invalid selector combinations.
delete_interfacemust reject requests with both selectors and requests with neither selector. AssertCode::InvalidArgumentfor both cases.🤖 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/api-core/src/tests/machine_interfaces.rs` around lines 1660 - 1681, Extend test_delete_interface_by_unknown_id_is_not_found with coverage for delete_interface requests containing both id and mac_address, and requests containing neither selector. Assert that each call fails with tonic::Code::InvalidArgument while preserving the existing unknown-id NotFound assertion.Source: Path instructions
crates/api-db/src/machine_interface.rs (1)
3544-3548: 🗄️ Data Integrity & Integration | 🔵 TrivialThe placement and the reasoning are correct.
clearruns before the row DELETE, which the FK requires. The operation is idempotent, so callers with no override are unaffected. No caller can have depended on the override outliving its interface, because the FK made that delete fail.One forward-looking note, outside the scope of this PR: an
ON DELETE CASCADEon themachine_boot_overrideFK would enforce the same invariant at the schema level. That would protect any future code path that deletes amachine_interfacesrow without going through this function. Consider it for a separate migration.🤖 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/api-db/src/machine_interface.rs` around lines 3544 - 3548, The current placement and behavior of machine_boot_override::clear before deleting the interface are correct; no code changes are required for this review. Leave the existing cleanup in place and defer any ON DELETE CASCADE schema migration to a separate change.crates/api-core/src/handlers/machine_interface.rs (1)
137-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider reporting every blocking association at once.
The loop returns on the first interface that fails validation. When a MAC matches several owned interfaces, the operator clears one blocker, retries, and then discovers the next one. The all-or-nothing contract already refuses the whole request, so the handler holds enough information to report every blocker in a single response. Collect the reasons and return one
InvalidArgumentthat lists them. This is an ergonomics improvement, not a correctness defect; defer it if the multi-match case is rare in practice.♻️ Sketch of an aggregated refusal
let mut blockers = Vec::new(); for interface in &interfaces { if let Some(machine_id) = interface.machine_id { blockers.push(if interface.interface_type == InterfaceType::Bmc { format!("{}: BMC interface attached to machine {machine_id}", interface.id) } else { format!("{}: machine {machine_id} is attached", interface.id) }); continue; } if let Some(dpu_machine_id) = interface.attached_dpu_machine_id { blockers.push(format!("{}: attached to DPU machine {dpu_machine_id}", interface.id)); continue; } // switch_id, power_shelf_id, BMC-IP checks follow the same shape. } if !blockers.is_empty() { return Err(CarbideError::InvalidArgument(format!( "these interfaces are still in use, delete their owners first: {}", blockers.join("; ") )) .into()); }🤖 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/api-core/src/handlers/machine_interface.rs` around lines 137 - 190, Update the interface validation loop to collect every blocking association in a blockers collection instead of returning immediately from each check. Apply this to machine, DPU, switch, power-shelf, and BMC-IP ownership checks, then return one InvalidArgument after the loop containing all interface-specific reasons joined together; preserve the existing all-or-nothing refusal when any blocker exists.
🤖 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/machine_interfaces/tests.rs`:
- Around line 96-113: Add a table-driven scenario in the machine-interface
delete argument tests for invoking the command with no selector, and assert that
parsing fails. Place it alongside the existing selector cases to cover the
required ArgGroup behavior.
In `@crates/api-core/src/handlers/machine_interface.rs`:
- Around line 137-190: Update the interface validation loop to collect every
blocking association in a blockers collection instead of returning immediately
from each check. Apply this to machine, DPU, switch, power-shelf, and BMC-IP
ownership checks, then return one InvalidArgument after the loop containing all
interface-specific reasons joined together; preserve the existing all-or-nothing
refusal when any blocker exists.
In `@crates/api-core/src/tests/machine_interfaces.rs`:
- Around line 1631-1654: Strengthen the refusal assertion in the
delete_interface test by also checking that the returned status message contains
the switch-ownership-specific text, not just Code::InvalidArgument. Update the
assertion around the status from delete_interface and preserve the existing
all-or-nothing database checks.
- Around line 1660-1681: Extend test_delete_interface_by_unknown_id_is_not_found
with coverage for delete_interface requests containing both id and mac_address,
and requests containing neither selector. Assert that each call fails with
tonic::Code::InvalidArgument while preserving the existing unknown-id NotFound
assertion.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 3544-3548: The current placement and behavior of
machine_boot_override::clear before deleting the interface are correct; no code
changes are required for this review. Leave the existing cleanup in place and
defer any ON DELETE CASCADE schema migration to a separate change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 493ca68f-269a-4841-aeee-9a5e02675734
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (8)
crates/admin-cli/src/machine_interfaces/delete/args.rscrates/admin-cli/src/machine_interfaces/delete/cmd.rscrates/admin-cli/src/machine_interfaces/tests.rscrates/api-core/src/handlers/machine_interface.rscrates/api-core/src/tests/machine_interfaces.rscrates/api-db/src/machine_interface.rscrates/rpc/proto/forge.protorest-api/proto/core/src/v1/nico_nico.proto
1 participant
Extends DeleteInterface to accept a MAC address, so an operator can clear a leftover interface record when they have the BMC MAC but not the interface ID, the case that blocks re-ingestion of a replacement host.
A MAC is only unique per network segment, so it can match several interfaces. All matches are validated before anything is deleted, and the request is refused if any of them still belong to a live machine, DPU, switch, or power shelf.
Also clears machine_boot_override in the shared machine_interface::delete. That FK has no ON DELETE CASCADE, so a leftover override used to fail the delete with a foreign-key violation, fixing it in the shared path also fixes the same latent bug for machine, switch, and power-shelf force-delete.
Exploration reports are intentionally left to Site Explorer, which prunes them on its next run.
Supersedes #4013, reimplemented per @Matthias247's review to extend delete_interface rather than add a parallel deletion flow, which also addresses @kensimon's point about the regression cost of a second order-sensitive DB path.
Part of #3046: this covers the machine-interface blocker. Expected-machines records remain out of scope per @ajf's comment on the issue.
Note: integration tests weren't run locally, relying on CI.