feat(dhcp): include the host's IPv6 DNS resolvers in the agent's DHCP config#2686
feat(dhcp): include the host's IPv6 DNS resolvers in the agent's DHCP config#2686chet wants to merge 1 commit into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
WalkthroughIPv6 DNS nameserver support is added to the DHCP configuration stack. A new ChangesIPv6 Nameserver Propagation through DHCP Stack
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai PTAL, thank you! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Fetch PR details
gh pr view 2686 --repo NVIDIA/infra-controller --json title,body,files,commits,baseRefName,headRefName
echo "---FILES CHANGED---"
gh pr diff 2686 --repo NVIDIA/infra-controllerLength of output: 20399
1. Duplicated nameserver-splitting logic (maintainability)The /// Splits a dual-stack nameserver list into its IPv4 and IPv6 components.
fn split_nameservers(addrs: &[IpAddr]) -> (Vec<Ipv4Addr>, Vec<Ipv6Addr>) {
addrs.iter().fold((vec![], vec![]), |(mut v4, mut v6), addr| {
match addr {
IpAddr::V4(a) => v4.push(*a),
IpAddr::V6(a) => v6.push(*a),
}
(v4, v6)
})
}Then at each call site: let (nameservers_v4, nameservers_v6) = split_nameservers(&service_addrs.nameservers);This is not a blocker, but it is the cleaner production-grade form. 2. Variable shadowing in
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/agent/src/ethernet_virtualization.rs (1)
1069-1085: ⚡ Quick winExtract nameserver family splitting into one helper to prevent path drift.
The same split logic is duplicated in Line 1069+ and Line 1487+. Centralizing it avoids divergence between gRPC and file-write paths.
♻️ Proposed refactor
+fn split_nameservers_by_family(nameservers: &[IpAddr]) -> (Vec<Ipv4Addr>, Vec<Ipv6Addr>) { + nameservers.iter().fold((Vec::new(), Vec::new()), |mut acc, addr| { + match addr { + IpAddr::V4(v4) => acc.0.push(*v4), + IpAddr::V6(v6) => acc.1.push(*v6), + } + acc + }) +}- let nameservers_v4 = service_addrs - .nameservers - .iter() - .filter_map(|x| match x { - IpAddr::V4(x) => Some(*x), - _ => None, - }) - .collect::<Vec<Ipv4Addr>>(); - - let nameservers_v6 = service_addrs - .nameservers - .iter() - .filter_map(|x| match x { - IpAddr::V6(x) => Some(*x), - _ => None, - }) - .collect::<Vec<Ipv6Addr>>(); + let (nameservers_v4, nameservers_v6) = + split_nameservers_by_family(&service_addrs.nameservers);As per coding guidelines, prefer simple/explicit implementations with justified abstractions; this shared helper keeps dual-stack behavior consistent.
Also applies to: 1487-1503
🤖 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/agent/src/ethernet_virtualization.rs` around lines 1069 - 1085, Extract the nameserver IPv4 and IPv6 filtering logic that appears around line 1069 (where nameservers_v4 and nameservers_v6 are created using filter_map operations) into a single helper function. This same filtering logic is duplicated around line 1487 in the file-write path. Create a helper function that takes the service_addrs.nameservers collection and returns both IPv4 and IPv6 nameserver lists, then use this helper in both locations to prevent the gRPC and file-write paths from diverging.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.
Nitpick comments:
In `@crates/agent/src/ethernet_virtualization.rs`:
- Around line 1069-1085: Extract the nameserver IPv4 and IPv6 filtering logic
that appears around line 1069 (where nameservers_v4 and nameservers_v6 are
created using filter_map operations) into a single helper function. This same
filtering logic is duplicated around line 1487 in the file-write path. Create a
helper function that takes the service_addrs.nameservers collection and returns
both IPv4 and IPv6 nameserver lists, then use this helper in both locations to
prevent the gRPC and file-write paths from diverging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ce8b94d5-2b5a-46bb-8415-78093e586a79
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/agent/src/dhcp.rscrates/agent/src/dhcp_server_grpc_client.rscrates/agent/src/ethernet_virtualization.rscrates/dhcp-server/Cargo.tomlcrates/dhcp-server/proto/dhcp_server_control.protocrates/dhcp-server/src/grpc_server.rscrates/rpc-utils/src/dhcp.rs
|
@coderabbitai Thanks for the eyes. Updated -- extracted a |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 2686 --repo NVIDIA/infra-controller | grep -A 60 "split_nameservers"Length of output: 4564
🐇 ✨ [resolve] |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/agent/src/ethernet_virtualization.rs (2)
143-153: ⚡ Quick winPrefer iterator-based construction in
split_nameservers_by_family.This helper is correct, but building the result vectors via a
forloop diverges from repo guidance to prefer iterator-based Vec construction.♻️ Proposed refactor
fn split_nameservers_by_family(nameservers: &[IpAddr]) -> (Vec<Ipv4Addr>, Vec<Ipv6Addr>) { - let mut v4 = Vec::new(); - let mut v6 = Vec::new(); - for addr in nameservers { - match addr { - IpAddr::V4(v4_addr) => v4.push(*v4_addr), - IpAddr::V6(v6_addr) => v6.push(*v6_addr), - } - } - (v4, v6) + nameservers + .iter() + .copied() + .fold((Vec::new(), Vec::new()), |(mut v4, mut v6), addr| { + match addr { + IpAddr::V4(v4_addr) => v4.push(v4_addr), + IpAddr::V6(v6_addr) => v6.push(v6_addr), + } + (v4, v6) + }) }As per coding guidelines, "When building a Vec or a HashMap in Rust, prefer using iterators to building them from a for-loop."
🤖 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/agent/src/ethernet_virtualization.rs` around lines 143 - 153, The split_nameservers_by_family function currently uses a for loop with manual vector mutation to build the result vectors, which diverges from the repo's coding guidelines to prefer iterator-based Vec construction. Refactor this function to use iterator methods (such as filter_map or filter combined with collect) to separate the nameservers by IP version (IPv4 and IPv6) rather than using explicit for loops and push operations. This will make the code more idiomatic and align with project conventions.Source: Coding guidelines
3313-3328: ⚡ Quick winUse a table-driven test for nameserver family partitioning.
This operation maps inputs to observable outputs, so table-driven style is preferred here; it will also make it easy to cover empty, v4-only, and v6-only cases.
🧪 Proposed refactor
#[test] -fn split_nameservers_by_family_partitions_both_families() { - let (v4, v6) = split_nameservers_by_family(&[ - IpAddr::from([10, 0, 0, 1]), - "2001:db8::1".parse().unwrap(), - IpAddr::from([10, 0, 0, 2]), - ]); - assert_eq!( - v4, - vec![Ipv4Addr::new(10, 0, 0, 1), Ipv4Addr::new(10, 0, 0, 2)] - ); - assert_eq!( - v6, - vec!["2001:db8::1".parse::<std::net::Ipv6Addr>().unwrap()] - ); +fn split_nameservers_by_family_partitions_by_family() { + use carbide_test_support::value_scenarios; + + value_scenarios!( + run = |input: Vec<IpAddr>| split_nameservers_by_family(&input); + "split nameservers" { + vec![ + IpAddr::from([10, 0, 0, 1]), + "2001:db8::1".parse().unwrap(), + IpAddr::from([10, 0, 0, 2]), + ] => ( + vec![Ipv4Addr::new(10, 0, 0, 1), Ipv4Addr::new(10, 0, 0, 2)], + vec!["2001:db8::1".parse().unwrap()], + ), + vec![IpAddr::from([10, 0, 0, 1])] => (vec![Ipv4Addr::new(10, 0, 0, 1)], vec![]), + vec!["2001:db8::1".parse().unwrap()] => (vec![], vec!["2001:db8::1".parse().unwrap()]), + vec![] => (vec![], vec![]), + } + ); }As per coding guidelines, "Prefer table-driven tests for any function that maps inputs to outputs" and "Use
carbide_test_support::value_scenarios!macro for total operations."🤖 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/agent/src/ethernet_virtualization.rs` around lines 3313 - 3328, The test `split_nameservers_by_family_partitions_both_families` should be refactored from a single-case test to a table-driven test format using the `carbide_test_support::value_scenarios!` macro. Define multiple test scenarios to cover all edge cases: empty nameserver list, IPv4-only addresses, IPv6-only addresses, and mixed IPv4/IPv6 addresses. Since `split_nameservers_by_family` is a total operation that maps inputs to outputs, the table-driven approach will provide better coverage and maintainability.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.
Nitpick comments:
In `@crates/agent/src/ethernet_virtualization.rs`:
- Around line 143-153: The split_nameservers_by_family function currently uses a
for loop with manual vector mutation to build the result vectors, which diverges
from the repo's coding guidelines to prefer iterator-based Vec construction.
Refactor this function to use iterator methods (such as filter_map or filter
combined with collect) to separate the nameservers by IP version (IPv4 and IPv6)
rather than using explicit for loops and push operations. This will make the
code more idiomatic and align with project conventions.
- Around line 3313-3328: The test
`split_nameservers_by_family_partitions_both_families` should be refactored from
a single-case test to a table-driven test format using the
`carbide_test_support::value_scenarios!` macro. Define multiple test scenarios
to cover all edge cases: empty nameserver list, IPv4-only addresses, IPv6-only
addresses, and mixed IPv4/IPv6 addresses. Since `split_nameservers_by_family` is
a total operation that maps inputs to outputs, the table-driven approach will
provide better coverage and maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 342fddd1-a72a-4d98-9693-6fa4a84eb037
📒 Files selected for processing (3)
crates/agent/src/dhcp.rscrates/agent/src/ethernet_virtualization.rscrates/rpc-utils/src/dhcp.rs
The dual-stack `DhcpConfig` added in NVIDIA#2672 has a `carbide_nameservers_v6` field, but nothing fills it -- `from_forge_dhcp_config` and the agent's config builders are IPv4-only, so a host's IPv6 DNS resolvers never reach the config. The agent already learns both families from the DPU's `ServiceAddresses`; this splits them by family and fills `carbide_nameservers_v6` at both DHCP-config build sites (the gRPC control path and the on-disk config path), threading the IPv6 set through `from_forge_dhcp_config` and `build_server_config`. A `split_nameservers_by_family` helper does the split once, so the two build sites cannot diverge. The IPv4 DHCPv4 option-6 path is unchanged; the IPv6 set stays inert until a delivery channel consumes it (DHCPv6 or RA, tracked by the broader IPv6 effort). Tests cover `from_forge_dhcp_config` populating the v6 field and the family split itself. This supports NVIDIA#2640. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks for the eyes. Updated -- |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 2686 --repo NVIDIA/infra-controller | grep -A 80 "split_nameservers_by_family"Length of output: 6837
🐇 ✨ [resolve] |
1 participant
Summary
The dual-stack
DhcpConfigfieldcarbide_nameservers_v6landed in #2672, but nothing fills it —from_forge_dhcp_configand the agent's config builders are still IPv4-only. This has the agent populate it from the DPU'sServiceAddresses(which already holds both families):split_nameservers_by_familyhelper splits the nameservers once, used at both DHCP-config build sites (the gRPC control path and the on-disk config path) so they can't diverge.from_forge_dhcp_config/build_server_configthread the IPv6 set through into the config.Tests cover
from_forge_dhcp_configpopulating the field and the family split.Implements #2640. Part of the #2628 epic (dual-stack DNS resolver advertisement).
Draft pending review.