feat: DHCP every network interface by default when no custom network-config is set#3708
feat: DHCP every network interface by default when no custom network-config is set#3708RajMandaliya wants to merge 12 commits into
Conversation
Keeping Pace with original repo
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe PXE cloud-init network-config route now serves a default DHCP-enabled NoCloud configuration when tenant network data is absent, while preserving custom network configuration when provided. Unit tests cover fallback, custom, and absent cloud-init inputs. ChangesNoCloud network configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…config is set cloud-init's own default behavior (no network-config at all) only DHCPs the first network interface it finds. For multi-NIC hosts using qcow-imager (or any tenant not providing a custom network: key), this leaves every other interface unconfigured. Adds DEFAULT_NETWORK_CONFIG, served whenever a tenant hasn't supplied their own network: key, that DHCPs every interface matching either the predictable (en*) or legacy (eth*) naming convention. Verified against real cloud-init: confirmed the exact constant string (byte-for-byte, written to a real seed directory) produces the expected netplan config covering both naming conventions. Addresses NVIDIA#3665. Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.com>
3eba752 to
647746b
Compare
|
This is now rebased cleanly on top of #3392 (merged), so the diff is scoped to just the new default-network-config logic. Ready for review whenever convenient. Quick summary: cloud-init's own default (no network-config at all) only DHCPs the first interface it finds. This adds a DEFAULT_NETWORK_CONFIG served whenever a tenant hasn't supplied their own Addresses #3665. |
![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/pxe/src/routes/cloud_init.rs (1)
837-855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the resolver tests table-driven.
These tests exercise one input-to-output mapping. Consolidate the
Some/missing-key, custom-key, andNonecases intocheck_valuesorscenarios!; also assert the expected parsed custom configuration rather than only usingassert_ne!and a substring check.As per coding guidelines, use table-driven tests for functions mapping inputs to outputs, errors, or observable results.
🤖 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/pxe/src/routes/cloud_init.rs` around lines 837 - 855, Convert the three resolve_network_config tests into a single table-driven test using check_values or scenarios!, covering missing network key, custom network configuration, and None input. Define each case with its expected parsed configuration and assert exact equality with the resolver result, replacing the assert_ne! and substring checks while preserving DEFAULT_NETWORK_CONFIG expectations.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/pxe/src/routes/cloud_init.rs`:
- Around line 338-342: Update the documentation for resolve_network_config to
state that when no custom network configuration is present, it returns
DEFAULT_NETWORK_CONFIG rather than an empty document. Keep the description of
extracting the tenant’s network configuration and serving NoCloud’s
network-config document accurate.
---
Nitpick comments:
In `@crates/pxe/src/routes/cloud_init.rs`:
- Around line 837-855: Convert the three resolve_network_config tests into a
single table-driven test using check_values or scenarios!, covering missing
network key, custom network configuration, and None input. Define each case with
its expected parsed configuration and assert exact equality with the resolver
result, replacing the assert_ne! and substring checks while preserving
DEFAULT_NETWORK_CONFIG expectations.
🪄 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: 3cce5db1-60ac-437c-b2ed-9ff105b7fa98
📒 Files selected for processing (1)
crates/pxe/src/routes/cloud_init.rs
- Consolidate the three resolve_network_config tests into one table-driven test, asserting the actual parsed YAML structure for the custom-key case (version, ethernets.eth0.addresses) instead of a substring check. - Fix resolve_network_config's stale doc comment, which was a leftover from before DEFAULT_NETWORK_CONFIG existed and incorrectly said it falls back to cloud-init's own default DHCP behavior. Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.com>
|
/ok to test 7855e13 |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3708.docs.buildwithfern.com/infra-controller |
Signed-off-by: RajMandaliya <rajmandaliya2249@gmail.com>
|
Sorry for this formatting issue. It's been happening few times now so created my own hook so with every commit it will automatically run for both rust and go files. |
2 participants
Addresses #3665.
Draft/stacked on #3392 — this branch is built on top of the
network_config/extract_network_configwork in #3392, which hasn't merged yet, so this diff currently includes those commits too. Will rebase ontomainand clean this up once #3392 lands, at which point this should shrink down to just the new default-config logic below.Problem
cloud-init's own default behavior (no network-config at all) only DHCPs the first network interface it finds. For multi-NIC hosts (e.g. via qcow-imager) where a tenant hasn't provided a custom
network:key, every other interface is left unconfigured.Fix
Adds
DEFAULT_NETWORK_CONFIG, served whenever a tenant hasn't supplied their ownnetwork:key, that DHCPs every interface matching either the predictable (en*) or legacy (eth*) naming convention.Testing
Verified against real cloud-init: confirmed the exact constant string (byte-for-byte, written to a real seed directory) produces the
expected netplan config covering both naming conventions. Three new unit tests cover the decision logic directly (default when absent, custom value used when present, default when no cloud-init at all).
Opening as a draft for early visibility/feedback since #3665 was just filed - happy to adjust the interface-matching approach if there's a preference (e.g. whether to include dhcp6, or a different naming convention assumption) before this is ready to merge.