fix(cfg): allow partial DpfServiceConfig overrides in site config by shayan1995 · Pull Request #4604 · NVIDIA/infra-controller

shayan1995 · 2026-08-05T18:07:59Z

Root cause

PR #4511 fixed the YAML-level parse error (TOML appended at file level) so that [dpf.services.*] sections now land inside nicoApiSiteConfig correctly. However nico-api still crashes when NICO_DPF_*_CHART_VERSION overrides are set:

Error: failed to load configuration files
Caused by:
    missing field `name` for key "default.dpf.services.dhcp_server"
    in /etc/forge/carbide-api/site/carbide-api-site-config.toml

Three conditions combine to produce this:

DpfServiceConfig had no #[serde(default)] — every field was required by serde.
#[serde(default = "default_dhcp_server_service")] on DpfMandatoryServicesConfig.dhcp_server only fires when the key is completely absent, not when it is partially specified.
The compiled-in service defaults (name, helm_repo_url, etc.) live only in Rust code, not in any TOML file that Figment can deep-merge. So when the site config provides dhcp_server = {helm_version: "2.1.0"}, Figment sees only that one field and serde fails on name.

Fix

Two changes:

crates/api-core/src/cfg/file.rs — add #[serde(default)] at the DpfServiceConfig struct level so a partial config (e.g. only helm_version) deserialises without error (missing string fields become ""). Add DpfServiceConfig::fill_from_defaults() to restore any empty field from a reference config.

crates/api-core/src/cfg/load.rs — call fill_from_defaults() for each mandatory service immediately after Figment extraction, using the compiled-in service default as the reference. This restores name, helm_repo_url, etc. from the Rust defaults, so a site-config partial override of only helm_version produces a fully-populated config.

The same fill is applied to per-deployment service overrides (bf3, bf4_generic, bf4_astra).

Type of Change

Fix - Bug fixes

Testing

Manual testing performed

Verified with NICO_DPF_DHCP_SERVER_CHART_VERSION=2.1.0-test in the site config — nico-api starts cleanly and dhcp_server.name is populated from the compiled default.

coderabbitai · 2026-08-05T18:08:55Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0d03a765-2dbd-4ff4-b5c6-25f65e6ced0a

📥 Commits

Reviewing files that changed from the base of the PR and between e030aa1 and 9e62a8c.

📒 Files selected for processing (2)

crates/api-core/src/cfg/file.rs
crates/api-core/src/cfg/load.rs

🚧 Files skipped from review as they are similar to previous changes (2)

crates/api-core/src/cfg/load.rs
crates/api-core/src/cfg/file.rs

Summary by CodeRabbit

New Features
- Partial service configurations are now supported, with omitted settings automatically populated from built-in defaults.
- Defaults are applied consistently across supported services and deployment types.
Bug Fixes
- Improved configuration loading when optional service settings are missing.

Walkthrough

DpfServiceConfig now supports partial overrides. The configuration loader fills missing top-level and per-deployment DPF service fields from compiled-in defaults.

Changes

DPF service default filling

Layer / File(s)	Summary
Partial configuration default contract `crates/api-core/src/cfg/file.rs`	`DpfServiceConfig` uses struct-level Serde defaults. `fill_from_defaults` fills empty scalar fields and missing pull-secret values. Extra services and tests cover partial and complete configurations.
Configuration loader integration `crates/api-core/src/cfg/load.rs`	The loader applies compiled-in defaults to top-level and per-deployment DPF services while preserving existing non-empty values.

Estimated code review effort: 2 (Simple) | ~15 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the fix for partial DpfServiceConfig overrides in site configuration.
Description check	✅ Passed	The description explains the root cause, implementation, affected configurations, and testing for the partial override fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

coderabbitai

Actionable comments posted: 1

🤖 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/api-core/src/cfg/file.rs`:
- Around line 1624-1625: The serde defaulting in DpfServiceConfig and the loader
path around DpfDeploymentConfig::extra_services currently allow partial
extra-service overrides to replace built-in definitions with empty fields. Scope
partial deserialization to mandatory-service overrides or, preferably, merge
each partial extra_services entry with its matching DpfExtraService default in
the loader; add a table-driven regression test covering partial extra-service
overrides, updating both crates/api-core/src/cfg/file.rs:1624-1625 and
crates/api-core/src/cfg/load.rs:121-147 as needed.

🪄 Autofix

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: 088da268-d818-4ce4-a899-826e7e931762

📥 Commits

Reviewing files that changed from the base of the PR and between f3360e2 and 034e79a.

📒 Files selected for processing (2)

crates/api-core/src/cfg/file.rs
crates/api-core/src/cfg/load.rs

When setup.sh sets NICO_DPF_*_CHART_VERSION, _dpf_inject_service_overrides inserts e.g. [dpf.services.dhcp_server] with only helm_version into the site config TOML. This caused nico-api to crash with 'missing field name' because: 1. DpfServiceConfig had no serde default — all fields were required. 2. The #[serde(default = "default_dhcp_server_service")] on DpfMandatoryServicesConfig.dhcp_server fires only when the key is completely absent, not when it is partially specified. 3. The compiled-in service defaults (name, helm_repo_url, etc.) live in Rust code, not in any TOML file that Figment can deep-merge. Fix: add #[serde(default)] at the DpfServiceConfig struct level so partial configs deserialise without error (missing fields become empty strings), then call fill_from_defaults() for each mandatory service immediately after Figment extraction. This restores any empty field from the service's compiled-in default, so a site-config partial override of only helm_version produces a fully-populated config with the correct name, helm_repo_url, etc. Signed-off-by: Shayan Namaghi <snamaghi@nvidia.com>

coderabbitai · 2026-08-05T18:38:00Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

shayan1995 · 2026-08-06T19:10:27Z

Closing — PR #4563 fixed the same root cause through a cleaner mechanism. It replaced the per-field #[serde(default = "...")] attributes with a custom Deserialize impl on DpfMandatoryServicesConfig that starts from compiled-in defaults and Figment-merges the user's partial config over them, so a partial [dpf.services.*] entry correctly inherits all unset fields without any post-processing. The fill_from_defaults() approach in this PR would be redundant on top of that. Confirmed by QA — the pipeline passes against main without this branch.

shayan1995 requested a review from a team as a code owner August 5, 2026 18:08

shayan1995 requested a review from abvarshney-nv August 5, 2026 18:09

coderabbitai Bot reviewed Aug 5, 2026

View reviewed changes

Comment thread crates/api-core/src/cfg/file.rs

shayan1995 force-pushed the fix/dpf-service-config-partial-override branch from 034e79a to 9e62a8c Compare August 5, 2026 18:37

spydaNVIDIA approved these changes Aug 5, 2026

View reviewed changes

shayan1995 closed this Aug 6, 2026

Navigation Menu

Uh oh!

fix(cfg): allow partial DpfServiceConfig overrides in site config - #4604

fix(cfg): allow partial DpfServiceConfig overrides in site config#4604
shayan1995 wants to merge 1 commit into
NVIDIA:mainfrom
shayan1995:fix/dpf-service-config-partial-override

shayan1995 commented Aug 5, 2026

Uh oh!

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 5, 2026

Uh oh!

shayan1995 commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

shayan1995 commented Aug 5, 2026

Root cause

Fix

Related

Type of Change

Testing

Uh oh!

coderabbitai Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 5, 2026

Uh oh!

shayan1995 commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading