fix: claim machine interface addresses before publishing hostnames - #4659
fix: claim machine interface addresses before publishing hostnames#4659chet wants to merge 1 commit into
Conversation
Summary by CodeRabbit
WalkthroughMachine-interface creation now inserts addresses in deterministic order before assigning hostname and domain metadata. A SQLx integration test enforces this ordering and verifies dual-stack creation results. ChangesMachine-interface creation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant create_inner
participant Database
participant DNS_metadata
create_inner->>Database: Insert interface without DNS metadata
create_inner->>Database: Insert allocated addresses in sorted order
create_inner->>DNS_metadata: Assign hostname and domain metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
| for address in &sorted_addresses { | ||
| insert_machine_interface_address(txn, &interface_id, address, allocation_type).await?; | ||
| } | ||
| update_hostname_and_domain(txn, interface_id, &hostname, domain_id).await?; |
There was a problem hiding this comment.
the description implies this was moved, but it looks like a new call. is it now a duplicate call?
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
`create_inner` currently inserts the final `(domain_id, hostname)` before its address rows. Existing-interface replacements claim the address first, so the site-wide address constraint in NVIDIA#4150 would make the two paths take their address and FQDN locks in opposite orders. Create the interface outside its DNS domain, sort and insert its addresses, and then apply the final hostname and domain. This keeps successful creates unchanged while every writer uses the same address-before-FQDN order. A PostgreSQL regression installs a test-only trigger to verify the intermediate row stays DNS-silent and dual-stack address inserts stay ordered. This supports NVIDIA#4654 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4659.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-db/src/machine_interface.rs (1)
2125-2135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the lint suppression with a private parameter struct.
create_innernow accepts eight positional parameters.target_domain_idmakes incorrect argument ordering easier. Replace#[allow(clippy::too_many_arguments)]with a private request struct for the creation inputs.As per coding guidelines, “Avoid
#[allow(...)]” and “Design APIs to be hard to misuse.”🤖 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 2125 - 2135, Replace the #[allow(clippy::too_many_arguments)] suppression on create_inner with a private request struct containing the creation inputs currently passed positionally. Update create_inner and all call sites to accept and use that struct, preserving the existing transaction, segment, and creation behavior while making argument ordering explicit and harder to misuse.Source: Coding guidelines
🤖 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.rs`:
- Around line 2181-2186: Update create_inner to sort allocated_addresses before
constructing NamingContext, then reuse that canonical sorted slice for both
hostname derivation and insert_machine_interface_address calls. Remove the later
duplicate sorting so address ordering is consistent across naming and
synchronization.
---
Nitpick comments:
In `@crates/api-db/src/machine_interface.rs`:
- Around line 2125-2135: Replace the #[allow(clippy::too_many_arguments)]
suppression on create_inner with a private request struct containing the
creation inputs currently passed positionally. Update create_inner and all call
sites to accept and use that struct, preserving the existing transaction,
segment, and creation behavior while making argument ordering explicit and
harder to misuse.
🪄 Autofix
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: b55df442-95e4-4b00-8d51-3baab1fc93cc
📒 Files selected for processing (2)
crates/api-db/src/machine_interface.rscrates/api-db/src/machine_interface/tests.rs
3 participants
New-interface creation and existing-interface replacement used opposite write orders.
create_innerpublished a new interface's(domain_id, hostname)before inserting its address rows. The replacement path claimed the address first, then updated(domain_id, hostname).That was fine with the existing database constraints, but #4150 will add site-wide uniqueness to
machine_interface_addresses.addressalongsidefqdn_must_be_unique. With both constraints in place, a create could hold the FQDN while waiting for the address, while a replacement could hold the address and wait for the FQDN.So, this makes new-interface creation follow the same address-before-FQDN order.
create_innernow inserts the interface with its final hostname anddomain_id = NULL, sorts and inserts its addresses, and then callsupdate_hostname_and_domainto publish the FQDN.domain_idused to receive its configured value in the initialINSERT; the helper now applies that value after the address rows exist.For callers, this is a noop. There are no API, config, or database schema changes, successful creates store the same interface data as before, and the surrounding transaction still rolls everything back on failure. This just gives #4150 one lock order to build on.
And, there's a PostgreSQL regression test that uses a test-only trigger to make the intermediate writes visible. It verifies that the interface stays outside its DNS domain until its addresses are claimed, and that dual-stack addresses are inserted in a deterministic order.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes