bug: 2822: Use interface associated with boot interface id if host is booted with non-DPU interface.#3416
bug: 2822: Use interface associated with boot interface id if host is booted with non-DPU interface.#3416abvarshney-nv wants to merge 1 commit into
Conversation
… booted with non-DPU interface.
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-core/src/handlers/machine_discovery.rs (1)
162-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed
find_oneerror.The fallback correctly gates on
!is_dpu()and avoids a redundant lookup when the cmdline id matches the resolved interface. However,db::machine_interface::find_one(&mut txn, cmdline_id).awaiterrors are silently discarded by thelet Ok(...)chain — this masks genuine DB failures (not just the expected "not found" case), making such failures invisible in logs.Based on coding guidelines: "Avoid using `let _unused = foo();` to discard errors... If you don't care about the errors a function produces, prefer using `.ok()`."🩹 Proposed fix: log the failure before discarding it
if !hardware_info.is_dpu() && interface.machine_id.is_none() && let Some(cmdline_id) = interface_id.filter(|id| *id != interface.id) - && let Ok(cmdline_interface) = db::machine_interface::find_one(&mut txn, cmdline_id).await + && let Ok(cmdline_interface) = db::machine_interface::find_one(&mut txn, cmdline_id) + .await + .inspect_err(|e| tracing::debug!(%cmdline_id, error=%e, "kernel-cmdline interface lookup failed during host fallback")) && cmdline_interface.machine_id.is_some() {🤖 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_discovery.rs` around lines 162 - 186, Update the fallback lookup around find_one in the non-DPU interface reassignment block to capture and log any database error before treating the lookup as unavailable. Preserve the existing behavior of only replacing interface when the returned cmdline interface has a machine, while making genuine find_one failures visible through tracing.Source: Coding guidelines
crates/api-core/src/tests/machine_discovery.rs (1)
270-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid regression test for the happy path — consider covering the negative branches too.
This locks in the case where the cmdline interface exists and is machine-associated. The handler's fallback also has two branches this doesn't exercise: the cmdline interface not existing/lookup failing, and the cmdline interface existing but itself being unassociated (
cmdline_interface.machine_id.is_none()) — in both cases discovery should still fail with the original "machine_id not found" behavior. All the fixtures needed (host_discover_machine_from_remote_ip,validate_existing_mac_and_create) are already in place, so this is a cheap addition to guard against regressions in the fallback's guard conditions.🤖 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_discovery.rs` around lines 270 - 317, Extend test_host_discovery_falls_back_to_cmdline_interface_when_ip_unassociated with negative cases covering a missing/unresolvable cmdline interface and an existing cmdline interface whose machine_id is None. Assert both discovery attempts fail with the original “machine_id not found” behavior, reusing host_discover_machine_from_remote_ip and validate_existing_mac_and_create fixtures.crates/api-core/src/tests/common/api_fixtures/host.rs (1)
155-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared
DiscoveryInfo/AttestKeyInfoboilerplate.This is now the third fixture (
host_discover_machine,host_discover_machine_with_reporter, and this one) that builds an identicalDiscoveryInfo+AttestKeyInfoblock. A small shared helper would remove the triplicated setup and keep each fixture focused on what makes it distinct (reporter fields vs. remote-IP metadata).Based on path instructions (CONTRIBUTING.md, matched by `**`): "Does an existing workspace dependency or local helper already solve it?"♻️ Proposed refactor
+fn discovery_info_with_attest_key(host_config: &ManagedHostConfig) -> DiscoveryInfo { + let mut discovery_info = DiscoveryInfo::try_from(HardwareInfo::from(host_config)).unwrap(); + discovery_info.attest_key_info = Some(AttestKeyInfo { + ek_pub: EK_PUB_SERIALIZED.to_vec(), + ak_pub: AK_PUB_SERIALIZED.to_vec(), + ak_name: AK_NAME_SERIALIZED.to_vec(), + }); + discovery_info +} + pub async fn host_discover_machine_from_remote_ip( env: &TestEnv, host_config: &ManagedHostConfig, machine_interface_id: MachineInterfaceId, remote_ip: std::net::IpAddr, ) -> Result<MachineId, tonic::Status> { - let mut discovery_info = DiscoveryInfo::try_from(HardwareInfo::from(host_config)).unwrap(); - discovery_info.attest_key_info = Some(AttestKeyInfo { - ek_pub: EK_PUB_SERIALIZED.to_vec(), - ak_pub: AK_PUB_SERIALIZED.to_vec(), - ak_name: AK_NAME_SERIALIZED.to_vec(), - }); + let discovery_info = discovery_info_with_attest_key(host_config); let mut request = Request::new(MachineDiscoveryInfo {🤖 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/common/api_fixtures/host.rs` around lines 155 - 186, Extract the repeated DiscoveryInfo and AttestKeyInfo construction into a shared helper in the host fixture module, then reuse it from host_discover_machine, host_discover_machine_with_reporter, and host_discover_machine_from_remote_ip. Keep each fixture’s distinct reporter fields and X-Forwarded-For metadata setup unchanged.Source: Path instructions
🤖 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/api-core/src/handlers/machine_discovery.rs`:
- Around line 162-186: Update the fallback lookup around find_one in the non-DPU
interface reassignment block to capture and log any database error before
treating the lookup as unavailable. Preserve the existing behavior of only
replacing interface when the returned cmdline interface has a machine, while
making genuine find_one failures visible through tracing.
In `@crates/api-core/src/tests/common/api_fixtures/host.rs`:
- Around line 155-186: Extract the repeated DiscoveryInfo and AttestKeyInfo
construction into a shared helper in the host fixture module, then reuse it from
host_discover_machine, host_discover_machine_with_reporter, and
host_discover_machine_from_remote_ip. Keep each fixture’s distinct reporter
fields and X-Forwarded-For metadata setup unchanged.
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Around line 270-317: Extend
test_host_discovery_falls_back_to_cmdline_interface_when_ip_unassociated with
negative cases covering a missing/unresolvable cmdline interface and an existing
cmdline interface whose machine_id is None. Assert both discovery attempts fail
with the original “machine_id not found” behavior, reusing
host_discover_machine_from_remote_ip and validate_existing_mac_and_create
fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3be88b59-2384-4bac-92e7-495e4f6184fb
📒 Files selected for processing (3)
crates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/tests/common/api_fixtures/host.rscrates/api-core/src/tests/machine_discovery.rs
|
I just left a comment in the GH issue -- just double checking the background. I think the answer might be that as of today you need to set |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
3 participants
Use interface associated with boot interface id if host is booted with non-DPU interface
Related issues
Issue 2822
Type of Change
Breaking Changes
Testing
Additional Notes