fix(pxe): seed NoCloud network-config for tenant-provided network settings#3392
Open
RajMandaliya wants to merge 4 commits into
Open
fix(pxe): seed NoCloud network-config for tenant-provided network settings#3392RajMandaliya wants to merge 4 commits into
RajMandaliya wants to merge 4 commits into
Conversation
Keeping Pace with original repo
…tings NICo seeds NoCloud's user-data and meta-data files, but never seeds the separate network-config file. A top-level 'network:' key inside user-data is not a valid user-data format (confirmed via cloud-init's own schema validator) and is silently ignored, regardless of the change in NVIDIA#2814 that correctly moved user-data to the canonical NoCloud seed location. Adds a new /network-config PXE route that extracts an existing 'network:' key from the tenant's custom_cloud_init (if present) and serves it as its own document, plus one line in disk_imaging.sh to fetch it into the seed directory. No API changes needed; existing 'network:' key placement keeps working as-is. Verified end-to-end against real cloud-init: a populated network-config applies correctly, and an empty one (the common case, no custom network settings) falls back cleanly to default DHCP with no errors. Addresses NVIDIA#3303. Signed-off-by: Raj <rajmandaliya2249@gmail.com>
Contributor
|
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
WalkthroughAdds PXE support for serving the ChangesCloud-init network configuration delivery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PXEClient
participant network_config
participant extract_network_config
participant network_config_template
PXEClient->>network_config: GET /network-config
network_config->>extract_network_config: Parse custom_cloud_init YAML
extract_network_config-->>network_config: Serialized network YAML or empty value
network_config->>network_config_template: Render network_config
network_config_template-->>PXEClient: Network configuration response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/pxe/src/routes/cloud_init.rs (3)
624-652: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for these input variants.
These tests exercise one total operation with different inputs. Consolidate them with
value_scenarios!orcheck_values, keeping the valid-case structural assertions explicit.As per coding guidelines, tests covering multiple input variants should prefer table-driven scenarios.
🤖 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 624 - 652, The three tests for extract_network_config should be consolidated into a table-driven test using value_scenarios! or check_values, with cases for valid network YAML, missing network, and invalid YAML. Keep the valid case’s explicit version and eth0 structural assertions while preserving the expected result for each scenario.Source: Coding guidelines
272-273: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the extracted network configuration.
value.get("network")already returns a reference, so cloning the full YAML tree adds unnecessary allocation before serialization.Proposed fix
- let network = value.get("network")?.clone(); - serde_yaml::to_string(&network).ok() + serde_yaml::to_string(value.get("network")?).ok()As per coding guidelines, avoid needless clones and prefer borrowing where ownership is not required.
🤖 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 272 - 273, In the network serialization logic, avoid cloning the YAML value returned by value.get("network"). Pass the borrowed reference directly to serde_yaml::to_string, preserving the existing optional flow and removing the unnecessary allocation.Source: Coding guidelines
624-652: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a route-level contract test.
The current tests only validate extraction; they do not exercise
network_config, template rendering, or the registered/network-configroute. A focused request test should verify both rendered network YAML and the empty fallback.🤖 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 624 - 652, Add a route-level contract test covering the registered `/network-config` endpoint and `network_config` template rendering, verifying a request with network data returns the expected rendered YAML and a request without a network key returns the empty fallback. Keep the existing extraction tests and use the route’s actual test setup and handler symbols to exercise the full request path.
🤖 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/pxe/src/routes/cloud_init.rs`:
- Around line 624-652: The three tests for extract_network_config should be
consolidated into a table-driven test using value_scenarios! or check_values,
with cases for valid network YAML, missing network, and invalid YAML. Keep the
valid case’s explicit version and eth0 structural assertions while preserving
the expected result for each scenario.
- Around line 272-273: In the network serialization logic, avoid cloning the
YAML value returned by value.get("network"). Pass the borrowed reference
directly to serde_yaml::to_string, preserving the existing optional flow and
removing the unnecessary allocation.
- Around line 624-652: Add a route-level contract test covering the registered
`/network-config` endpoint and `network_config` template rendering, verifying a
request with network data returns the expected rendered YAML and a request
without a network key returns the empty fallback. Keep the existing extraction
tests and use the route’s actual test setup and handler symbols to exercise the
full request path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54970ee9-9f67-48f0-b908-caa27b6e8de1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/pxe/Cargo.tomlcrates/pxe/src/routes/cloud_init.rspxe/common_files/disk_imaging.shpxe/templates/network-config
- Avoid cloning the extracted network YAML value before serializing. - Consolidate the three extract_network_config tests into a single table-driven test, moved into the existing tests module. Signed-off-by: Raj <rajmandaliya2249@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following up on the discussion in #3303 with a working fix.
Root cause
NICo seeds NoCloud's
user-dataandmeta-datafiles but never seeds the separatenetwork-configfile. A top-levelnetwork:key insideuser-datais not a valid user-data format — confirmed directly against real cloud-init, whose own schema validator rejects it:Error: Cloud config schema errors: network: Additional properties are not allowed ('network' was unexpected)
This is unrelated to #2814 specifically — that change correctly moved
user-datato the canonical NoCloud seed location; thenetwork:key was never valid there before or after, it just happened to "work" pre-#2814 because content was being written into cloud-init's owncloud.cfg.d/config-merge directory instead of a real user-data document.Fix
/network-configPXE route that extracts an existingnetwork:key from the tenant'scustom_cloud_init(if present) and serves it as its own document.disk_imaging.shto fetch it into the seed directory, alongside the existinguser-data/meta-datafetches.No API changes — existing
network:key placement in tenant userdatakeeps working as-is.Testing
invalid YAML).
reproduced both the old and new seed-directory layouts): a populated
network-configapplies correctly (netplan reflects the customconfig), and an empty one (the common case, no custom network
settings) falls back cleanly to default DHCP with no errors or
warnings.
cargo test -p carbide-pxe— 18/18 passing.Addresses #3303.