fix(dhcp): resync hostname on static assign/remove-address (#3383)#3515
Open
hatamzad-nv wants to merge 4 commits into
Open
fix(dhcp): resync hostname on static assign/remove-address (#3383)#3515hatamzad-nv wants to merge 4 commits into
hatamzad-nv wants to merge 4 commits into
Conversation
assign-address and remove-address mutated the interface address but skipped the hostname resync every other allocation path runs, leaving a stale IP-derived hostname that collided with fqdn_must_be_unique and wedged DHCP allocation on the whole segment.
Contributor
Summary by CodeRabbit
WalkthroughStatic address assignment and removal now resynchronize IP-derived interface hostnames. Address deletion returns all owning interfaces, DHCP expiry adapts to the new result, and regression tests cover normal and mismatched-owner removal paths. ChangesAddress hostname synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AddressHandler
participant AddressDatabase
participant InterfaceDatabase
AddressHandler->>AddressDatabase: Assign or remove address
AddressDatabase-->>AddressHandler: Deleted owner interface IDs
AddressHandler->>InterfaceDatabase: Synchronize IP-derived hostname
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-core/tests/integration/static_address_management.rs (1)
455-529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid regression coverage for the hostname resync fix.
The three-stage flow (DHCP → static assign → static remove) correctly exercises hostname re-derivation and the dormant-placeholder reset, matching the
sync_hostname_after_address_assignment/sync_hostname_after_address_changecontracts.One optional enhancement: since
domain_idclearing is also part of the same sync contract (cleared toNoneonce addresses are empty), consider assertingiface_after_remove.domain_id.is_none()alongside the hostname check to more fully pin down the fix's behavior.🧪 Optional: assert domain clearing too
assert!( iface_after_remove.hostname.to_lowercase().starts_with("noip"), "hostname should reset to dormant format after removal, got: {}", iface_after_remove.hostname, ); + assert!( + iface_after_remove.domain_id.is_none(), + "domain should be cleared once the interface has no addresses" + );🤖 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/tests/integration/static_address_management.rs` around lines 455 - 529, Extend test_assign_and_remove_resync_hostname to also assert that iface_after_remove.domain_id is None after removing the final static address, covering the domain-clearing behavior of the hostname/address synchronization contract while preserving the existing hostname assertion.
🤖 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/api-core/src/handlers/machine_interface_address.rs`:
- Around line 194-201: The deleted address may belong to a different interface
than the supplied interface_id, causing hostname resynchronization for the wrong
owner. Update remove_static_address to either constrain deletion by interface_id
or return the deleted row’s owning interface_id, then pass that confirmed owner
to sync_hostname_after_address_change only when a row was deleted.
---
Nitpick comments:
In `@crates/api-core/tests/integration/static_address_management.rs`:
- Around line 455-529: Extend test_assign_and_remove_resync_hostname to also
assert that iface_after_remove.domain_id is None after removing the final static
address, covering the domain-clearing behavior of the hostname/address
synchronization contract while preserving the existing hostname assertion.
🪄 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: ac5f066d-24c9-4d7a-ae33-643662831f38
📒 Files selected for processing (2)
crates/api-core/src/handlers/machine_interface_address.rscrates/api-core/tests/integration/static_address_management.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Address review feedback: remove-address deletes by IP, so resync the interface returned by delete_by_address instead of the request's interface_id, which may not own the row. Also wrap an assert to satisfy nightly rustfmt.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-core/tests/integration/static_address_management.rs (1)
576-596: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the regression by asserting
other_id's hostname is untouched.The test proves the owner gets resynced, but doesn't assert that
other_id(the mismatched interface passed in the request) is left alone. Adding that check would positively confirm the fix targets the real owner rather than merely happening to not throw.🧪 Suggested addition
// The owner (which actually lost the address) must be resynced to dormant. let mut txn = env.db_txn().await; let owner_after = db::machine_interface::find_one(&mut *txn, owner_id).await?; assert!( owner_after.hostname.to_lowercase().starts_with("noip"), "the address owner should be resynced to dormant, got: {}", owner_after.hostname, ); + // The mismatched request interface must be left untouched. + let other_after = db::machine_interface::find_one(&mut *txn, other_id).await?; + assert_eq!( + other_after.hostname, other.hostname, + "the interface named in the request but not owning the address must not be resynced" + ); txn.commit().await?;🤖 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/tests/integration/static_address_management.rs` around lines 576 - 596, Extend the regression test around the existing owner_after assertion to fetch the interface identified by other_id and assert its hostname remains unchanged from before removal. Keep the existing owner resync assertion, and use the test’s established pre-removal hostname or expected value for the untouched-interface comparison.
🤖 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/api-db/src/machine_interface_address.rs`:
- Around line 209-223: Update delete_by_address to preserve every interface_id
returned by the DELETE ... RETURNING query instead of taking only the first
owner; change its return type and callers as needed to return and resync all
affected interfaces, or enforce a database-level uniqueness guarantee if the
operation must remain singular.
---
Nitpick comments:
In `@crates/api-core/tests/integration/static_address_management.rs`:
- Around line 576-596: Extend the regression test around the existing
owner_after assertion to fetch the interface identified by other_id and assert
its hostname remains unchanged from before removal. Keep the existing owner
resync assertion, and use the test’s established pre-removal hostname or
expected value for the untouched-interface comparison.
🪄 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: 43100277-158b-49d0-a77a-f82649d19e0d
📒 Files selected for processing (5)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-core/tests/integration/static_address_management.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/api-core/src/handlers/machine_interface_address.rs
delete_by_address can match multiple rows since (address, allocation_type) is not unique on its own; return every affected interface and resync each instead of dropping all but the first.
Contributor
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/api-core/src/dhcp/expire.rs`:
- Around line 80-86: Update the deletion flow around
db::machine_interface_address::delete_by_address to retain its returned owner
IDs instead of converting the Vec to a boolean. Resynchronize every returned
owner, while preserving the existing synchronization behavior for the interface
found by find_by_ip and the branch’s result semantics.
🪄 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: cbd2d2cb-6762-42f3-ab1c-bb8be013e714
📒 Files selected for processing (4)
crates/api-core/src/dhcp/expire.rscrates/api-core/src/handlers/machine_interface_address.rscrates/api-db/src/machine_interface/tests.rscrates/api-db/src/machine_interface_address.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-db/src/machine_interface/tests.rs
- crates/api-core/src/handlers/machine_interface_address.rs
chet
approved these changes
Jul 15, 2026
chet
left a comment
chet
left a comment
Contributor
There was a problem hiding this comment.
Nice, this is great. Thank you! Fun how it ultimately all wires through HostNamingStrategy too so we can do stuff like this. Appreciate the fix!
Contributor
Author
|
@chet Thanks! Yeah, the HostNamingStrategy wiring made this a lot cleaner than I expected, once I traced how the other allocation paths resync through it, the fix was really just making assign/remove do the same thing. Appreciate the review! |
…NVIDIA#3383) Address review feedback: the DHCP lease-expiry path now resyncs every interface returned by delete_by_address instead of only the find_by_ip owner. Also apply nightly rustfmt to the query literal and test setup.
Contributor
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/api-core/src/dhcp/expire.rs`:
- Around line 73-86: The DHCP expiration path around delete_by_address_and_mac
must derive the resync target from the row actually deleted, not the IP-only
interface lookup. Update delete_by_address_and_mac to return the affected
interface_id using RETURNING, use that returned ID when building resync_targets
in the mac_address branch, and add a regression test covering a mismatched owner
between lookup and deletion.
🪄 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: 110f2ad4-c296-4af1-963d-5af28b8f5b6b
📒 Files selected for processing (3)
crates/api-core/src/dhcp/expire.rscrates/api-core/tests/integration/static_address_management.rscrates/api-db/src/machine_interface_address.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/api-core/tests/integration/static_address_management.rs
- crates/api-db/src/machine_interface_address.rs
Comment on lines
+73
to
+86
| let (deleted, resync_targets) = match mac_address { | ||
| Some(mac) => { | ||
| db::machine_interface_address::delete_by_address_and_mac( | ||
| let deleted = db::machine_interface_address::delete_by_address_and_mac( | ||
| &mut txn, | ||
| ip_address, | ||
| mac, | ||
| model::allocation_type::AllocationType::Dhcp, | ||
| ) | ||
| .await? | ||
| .await?; | ||
| let targets = match (deleted, &interface) { | ||
| (true, Some(iface)) => vec![iface.id], | ||
| _ => Vec::new(), | ||
| }; | ||
| (deleted, targets) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Derive the MAC-scoped resync target from the deleted row.
interface was looked up by IP alone, while deletion is scoped by (IP, MAC, allocation type). If multiple rows share the address, or ownership changes between the lookup and delete, this can resynchronize the wrong interface—or none—leaving the actual owner’s hostname stale. Make delete_by_address_and_mac return the affected interface_id via RETURNING, then use that authoritative ID here and add a mismatched-owner regression test.
🤖 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/dhcp/expire.rs` around lines 73 - 86, The DHCP expiration
path around delete_by_address_and_mac must derive the resync target from the row
actually deleted, not the IP-only interface lookup. Update
delete_by_address_and_mac to return the affected interface_id using RETURNING,
use that returned ID when building resync_targets in the mac_address branch, and
add a regression test covering a mismatched owner between lookup and deletion.
Source: Path instructions
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.
Problem
carbide-admin-cli machine-interfaces assign-addressandremove-addressmutate an interface's address but skip the hostname-resync step that every
other allocation path runs. With the default IP-derived naming strategy, this
leaves the interface's
hostnamepinned to an IP it no longer holds — e.g. theinterface is moved to
7.243.174.25but stays named7-243-174-20.That single stale row then wedges all new DHCP allocation on the segment:
just freed).
fqdn_must_be_unique (domain_id, hostname)constraint.selection is deterministic, so all 128 retries pick the same IP and fail.
DISCOVERon the segment queues behind that IP and is dropped.Observed on
ytl-dev1: after a repave, no host BMC or DPU BMC could get anaddress and site-explorer could not discover attached DPUs, until the stale
interface was deleted.
Fixes #3383.
Fix
Both handlers now run the same resync the other paths already use, before
committing:
assign_static_address→sync_hostname_after_address_assignment,mirroring the static-preallocation path.
remove_static_address→sync_hostname_after_address_changefor theinterface that actually owned the removed address.
remove_static_addressdeletes by IP, so the owning interface can differ fromthe caller-supplied
interface_id.delete_by_addressnow returns the affectedinterface_id(viaRETURNING interface_id) and the handler resyncs thatowner, so the real owner can't be left with a stale hostname. The DHCP
lease-expiry path is unchanged (it already resyncs the interface it looked up).
Testing
test_assign_and_remove_resync_hostname— hostname follows the new static IPon assign, and resets to the dormant
noip-…placeholder on remove.test_remove_resyncs_the_address_owner_not_the_request_id— removing anaddress while passing a different
interface_idstill resyncs the trueowner.
static_address_managementsuite (18 tests) — passes, no regressions.Run on Linux (the crate pulls in
tss-esapi-sys, which does not build on macOS):DATABASE_URL="postgresql://…@…:5432/postgres"
cargo test -p carbide-api-core --test integration static_address_management
Notes / follow-ups (out of scope)
fqdn_must_be_uniquemeans oneinconsistent row can still block allocation of unrelated IPs. This PR removes
the cause; hardening the allocator against any stale row is a separate change.
num_free_ips: 0diagnostic on a mostly-empty prefix is filedseparately.