feat: preserve MachineBootInterface information across machine/interface deletion

[chet]

Description

A machine's boot target is the (MAC, EthernetInterface.Id) pair on its primary interface row -- also known as a MachineBootInterface -- and the ID half can be impossible to re-derive from a MAC address later; after a DPU flips to NIC mode, the BMC can report the interface ID without its MAC.

This means anything that deletes the machine_interfaces (e.g. force-delete --delete-interfaces) can leave the re-ingested machine with no boot target until it happens to PXE on a leftover UEFI boot entry (if one exists).

Soooo... this PR introduces a new retained_boot_interfaces table as a place to stash MachineBootInterface pairings in between ingestion(s), adjusting things such that the MachineBootInterface doesn't get lost. machine_interface::delete itself records each deleted MachineBootInterface into our small retained_boot_interfaces table (keyed by MAC), so every deletion path retains them (whether it's force-delete, interface-delete etc).

And to connect even more, predicted_machine_interfaces gains a boot_interface_id column, so a host with unmanaged/no DPUs explored before its first DHCP also has a prepared MachineBootInterface which can be picked up the moment it exists -- again, whether created by predicted-interface promotion, or directly by DHCP.

To be clear, retention applies to ANY interface -- a DPU in DPU mode, a DPU in NIC mode, an integrated NIC, etc. Rows created any other way -- plain DHCP, or the proactive interface a DPU-mode ingest creates -- recover straight from retained_boot_interfaces at creation; predicted interfaces are just the way hosts with unmanaged/zero DPUs are managed.

One more thing worth calling out (if it wasn't obvious): rows in retained_boot_interfaces are temporary. As soon as the MachineBootInterface is dropped into a machine_interfaces row, the record is removed (via take_by_mac). How long a record stays retained is configurable via a new top-level retained_boot_interface_window config; the default is forever -- if the machine eventually comes back, the pair will be waiting. Setting a window means a MAC reappearing on different hardware months later won't inherit an obsolete Redfish interface ID (e.g. the old NIC.Slot.X resource may not even exist there); a too-old record just gets swept on the spot, and the new row starts without a boot id -- exactly as if retention never existed -- until exploration fills it in from a live report.

Tests added to check force-delete boot interface retention, and that a predicted interface hands its ID to the real row at promotion (and is removed from the retained interfaces), and that a DHCP-derived row is populated with a retained id and removes the retained record. Existing zero-DPU, backfill, and force-delete tests still pass.

Signed-off-by: Chet Nichols III chetn@nvidia.com

Type of Change

• Add - New feature or capability
• Change - Changes in existing functionality
• Fix - Bug fixes
• Remove - Removed features or deprecated functionality
• Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

Breaking Changes

• This PR contains breaking changes

Testing

• Unit tests added/updated
• Integration tests added/updated
• Manual testing performed
• No testing required (docs, internal refactor, etc.)

Additional Notes

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f930383c-6a81-47b0-be22-d1f8e8ae1e6d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chet]

@coderabbitai PTAL. Thank you!

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details and diff
gh pr view 2448 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions,commits 2>/dev/null | jq '{title, additions, deletions, files: [.files[].path]}'

Length of output: 2703

---

🏁 Script executed:

#!/bin/bash
# Fetch the full diff
gh pr diff 2448 --repo NVIDIA/infra-controller 2>/dev/null | head -4000

Length of output: 105614

---

---

@chet — this is a well-considered feature. The single-point retention at delete and single-point recovery inside create_with_type give a clean invariant: every creation path participates automatically. Below are the issues I identified:

---

1. Stale retained_boot_interfaces rows accumulate without a sweep

find_by_mac and take_by_mac enforce the window at read time, but rows older than the window are never physically removed unless take_by_mac is called for that specific MAC. An interface deleted, never re-ingested, and older than the window will occupy the table permanently.

This is bounded by the number of deleted interfaces (not traffic-proportional), so it's unlikely to be urgent — but for completeness a periodic cleanup job, or at minimum a DELETE … WHERE recorded_at < NOW() - $1 sweep triggered on each site-explorer pass, would prevent indefinite accumulation:

DELETE FROM retained_boot_interfaces
WHERE $1::bigint IS NOT NULL
  AND recorded_at <= NOW() - ($1::bigint * INTERVAL '1 second');

---

2. as_option_duration should use an explicit chrono::Duration type

In crates/utils/src/config.rs the new serializer:

pub fn as_option_duration<S>(d: &Option<Duration>, serializer: S) -> Result<S::Ok, S::Error>

The bare Duration is ambiguous at a glance — std::time::Duration has as_secs(), not num_seconds(), so a future reader or refactor could get confused. Prefer an unambiguous signature:

pub fn as_option_duration<S>(d: &Option<chrono::Duration>, serializer: S) -> Result<S::Ok, S::Error>

---

3. Double take_by_mac call in move_predicted_machine_interface_to_machine

When row_created_here = true, create_with_type already consumes the retention record via take_by_mac and sets boot_interface_id on the row. The explicit second take_by_mac immediately after will always return None:

// row_created_here = true → create() already consumed this
let retained_boot_interface_id = crate::retained_boot_interface::take_by_mac(
    &mut *txn,
    predicted_machine_interface.mac_address,
    retained_window,
)
.await?;

The priority resolution is therefore correct (current_boot_interface_id already carries the retained value), but the redundant call is a latent readability hazard. Consider short-circuiting:

let retained_boot_interface_id = if row_created_here {
    None // already consumed and reflected in current_boot_interface_id
} else {
    crate::retained_boot_interface::take_by_mac(
        &mut *txn,
        predicted_machine_interface.mac_address,
        retained_window,
    )
    .await?
};

---

4. #[serde(skip)] injection is silent for directly-constructed SiteExplorerConfig

Any site that constructs SiteExplorerConfig directly without flowing through setup.rs gets retained_boot_interface_window: None silently (the Default impl always sets None). This is the correct safe default (forever-retain), but it does mean a misconfigured retained_boot_interface_window in the top-level config would have no effect if initialize_and_start_controllers is bypassed — worth a #[cfg(test)]-time assertion or at minimum a tracing::warn! in setup.rs if the window was injected, so operators can confirm it's taking effect.

---

5. Minor: the migration recorded_at column is not indexed

find_by_mac and take_by_mac both filter by mac_address (the PK), so range scans on recorded_at never happen without a preceding MAC lookup. This is fine — just confirming the design is intentional.

---

The core approach — retention keyed by MAC with no foreign keys, consumed on first re-use — is clean and correct. The priority logic in move_predicted_machine_interface_to_machine is subtle but the comments explain each case clearly. Test coverage for the key paths looks solid.

🐇 ✨

[chet]

@coderabbitai Thanks for the eyes. Updated -- (1) added retained_boot_interface::delete_expired plus a once-per-pass sweep at the end of explore_site (a no-op when no window is configured, since records then wait forever by design), with a test covering expired vs in-window vs no-window; (3) short-circuited the redundant take_by_mac when the row was created during promotion, per your suggestion; (4) added a tracing::info! in setup.rs when the window is injected so operators can confirm it's taking effect. (2) is per the file's existing convention -- as_duration right above uses the same bare chrono::Duration import, with as_std_duration as the explicitly-named std variant -- so leaving as-is. (5) yep, intentional: both reads are MAC-PK lookups, so recorded_at is only evaluated per-row after the index hit. PTAL. Resolve if good, thanks!

[coderabbitai]

@chet — all five points are accounted for:

• (1) delete_expired + end-of-pass sweep with test coverage is exactly the right approach; the no-op behaviour when no window is set preserves the forever-retain semantics cleanly.
• (2) Convention argument accepted — consistent with as_duration in the same file.
• (3) Short-circuit confirmed; eliminates the latent readability hazard.
• (4) tracing::info! gives operators a clear signal without being intrusive.
• (5) MAC-PK-first lookup design confirmed; no index needed.

Implementation looks solid across all paths. Resolving comments now.

🐇 ✨

✅ Action performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.