docs: site NTP server config and extension service name resuse#3353
docs: site NTP server config and extension service name resuse#3353hanyux-nv wants to merge 1 commit into
Conversation
Summary by CodeRabbit
WalkthroughThe changes update NTP setup and fallback documentation across quick-start and provisioning guides, and clarify DPU extension-service termination, naming, deletion, and instance-readiness semantics. ChangesNTP configuration documentation
DPU extension-service documentation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3353.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/getting-started/quick-start.md`:
- Line 145: The `ntp_servers` documentation incorrectly implies that setting it
to `[]` automatically enables the legacy fallback. Update the `ntp_servers`
table entry in the quick-start documentation to state that an empty list only
disables NICo-managed NTP configuration; operators must separately configure the
Kea `nico-ntpserver` fallback and/or the `nico-ntp.nico` DNS record.
- Line 158: Update the TOML configuration guidance near the field list to remove
the claim that missing keys cause a startup crash. Explain that ntp_servers may
be omitted because its serde default produces an empty vector, and accurately
distinguish optional fields from any genuinely required fields while keeping the
instructions operator-usable.
In `@docs/manuals/dpu_extension_service.md`:
- Line 205: Update the lifecycle summary near the “every DPU” wording to state
that provisioning waits for configured extension services to reach a running
state on every DPU selected by the Instance network configuration, excluding
attached but unselected DPUs; keep the wording consistent with the readiness
description.
In `@docs/provisioning/ip-and-network-configuration.md`:
- Around line 178-181: The documentation incorrectly instructs operators to
configure nico-ntpserver with the Unbound resolver VIP. Update the listed Kea
hook parameters so nico-nameserver points to the unbound.nico recursive resolver
VIP, while nico-ntpserver explicitly requires operator-supplied NTP server IPs;
retain the note that nico-ntpserver is only a fallback when
siteConfig.ntp_servers is empty.
🪄 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: 315fcf95-efaa-420e-bc09-1eaa1d2f202a
📒 Files selected for processing (3)
docs/getting-started/quick-start.mddocs/manuals/dpu_extension_service.mddocs/provisioning/ip-and-network-configuration.md
| | `sitename` | Short identifier matching `siteName` in `values.yaml` | | ||
| | `initial_domain_name` | Base DNS domain for the site (e.g. `mysite.example.com`) | | ||
| | `dhcp_servers` | List of DHCP server IPs reachable from bare-metal hosts, or `[]` | | ||
| | `ntp_servers` | List of enterprise NTP server IPs for BMC time setup and DHCP option 42, or `[]` to use the legacy DHCP/DNS fallback | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clarify that [] does not configure the fallback by itself.
An empty ntp_servers list only disables NICo-managed NTP configuration. Operators must separately configure the Kea nico-ntpserver fallback and/or the legacy nico-ntp.nico DNS record. Otherwise, clients receive no usable NTP server.
As per path instructions, documentation must be technically correct and operator-usable.
🤖 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 `@docs/getting-started/quick-start.md` at line 145, The `ntp_servers`
documentation incorrectly implies that setting it to `[]` automatically enables
the legacy fallback. Update the `ntp_servers` table entry in the quick-start
documentation to state that an empty list only disables NICo-managed NTP
configuration; operators must separately configure the Kea `nico-ntpserver`
fallback and/or the `nico-ntp.nico` DNS record.
Source: Path instructions
| - **Required fields--do not leave empty:** `[networks.admin]`, `prefix`, and `gateway` must be set to real values. `nico-api` crashes at startup with a parse error if these are empty strings. Similarly, `[pools.lo-ip]`, `[pools.vlan-id]`, and `[pools.vni]` ranges must be non-empty. | ||
|
|
||
| These fields are safe to leave as empty arrays: `dhcp_servers`, `site_fabric_prefixes`, `deny_prefixes`. Do not delete any field from the TOML block; missing keys cause a different crash than empty ones. | ||
| These fields are safe to leave as empty arrays: `dhcp_servers`, `ntp_servers`, `site_fabric_prefixes`, `deny_prefixes`. Do not delete any field from the TOML block; missing keys cause a different crash than empty ones. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the incorrect claim about missing TOML keys crashing startup.
ntp_servers is declared with #[serde(default)], so an omitted key deserializes to an empty vector. The documentation should distinguish omission from fields that are genuinely required, rather than claiming that omission causes a crash.
As per path instructions, documentation must be technically correct and operator-usable.
🤖 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 `@docs/getting-started/quick-start.md` at line 158, Update the TOML
configuration guidance near the field list to remove the claim that missing keys
cause a startup crash. Explain that ntp_servers may be omitted because its serde
default produces an empty vector, and accurately distinguish optional fields
from any genuinely required fields while keeping the instructions
operator-usable.
Source: Path instructions
| ``` | ||
|
|
||
| During Instance provisioning, NICo waits for all configured extension services to reach a running state before the Instance becomes ready. If no extension services are configured, this step is skipped. | ||
| During Instance provisioning, NICo waits for all configured extension services to reach a running state on every DPU used by the Instance before the Instance becomes ready. On multi-DPU hosts, DPUs that are attached to the host but not selected by the Instance network configuration do not block the extension-service readiness check. If no extension services are configured, this step is skipped. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the readiness wording with the lifecycle summary.
This change says DPUs not selected by the Instance network do not block readiness, but line 318 still says provisioning waits for services on “every DPU.” Update the lifecycle summary to use the same selected-DPU scope.
As per path instructions, documentation must remain technically correct and consistent with current behavior.
Suggested documentation fix
-**Provisioning:** After network configuration completes, the Instance waits in `WaitingForExtensionServicesConfig` until all configured extension services are running on every DPU.
+**Provisioning:** After network configuration completes, the Instance waits in `WaitingForExtensionServicesConfig` until all configured extension services are running on every DPU selected by the Instance network configuration.🤖 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 `@docs/manuals/dpu_extension_service.md` at line 205, Update the lifecycle
summary near the “every DPU” wording to state that provisioning waits for
configured extension services to reach a running state on every DPU selected by
the Instance network configuration, excluding attached but unselected DPUs; keep
the wording consistent with the readiness description.
Source: Path instructions
| The values that `nico-dhcp` returns in DHCP options (nameservers, NTP servers, next-server, boot file, etc.) are sourced from: | ||
|
|
||
| - The Kea hook parameters in the `nico-dhcp` Helm chart (`nico-nameserver`, `nico-ntpserver`, etc.) — set these to the `unbound.nico` (or `unbound.nico`, see [section 3](#3-dns-configuration)) recursive resolver VIP and your enterprise NTP server addresses. | ||
| - The `ntp_servers` list in `siteConfig` — preferred for DHCP option 42 when non-empty. | ||
| - The Kea hook parameters in the `nico-dhcp` Helm chart (`nico-nameserver`, `nico-ntpserver`, etc.) — set these to the `unbound.nico` (or `unbound.nico`, see [section 3](#3-dns-configuration)) recursive resolver VIP. `nico-ntpserver` is used only as a fallback when `siteConfig.ntp_servers` is empty. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not point nico-ntpserver at the Unbound resolver.
nico-nameserver should use the unbound VIP, but nico-ntpserver must contain operator-supplied NTP server IPs. Sending DHCP option 42 with the DNS resolver address would cause clients to attempt NTP against a DNS service and fail time synchronization.
As per path instructions, documentation must be technically correct and operator-usable.
🤖 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 `@docs/provisioning/ip-and-network-configuration.md` around lines 178 - 181,
The documentation incorrectly instructs operators to configure nico-ntpserver
with the Unbound resolver VIP. Update the listed Kea hook parameters so
nico-nameserver points to the unbound.nico recursive resolver VIP, while
nico-ntpserver explicitly requires operator-supplied NTP server IPs; retain the
note that nico-ntpserver is only a fallback when siteConfig.ntp_servers is
empty.
Source: Path instructions
1 participant
Adds docs for NICo 2.0 release changes including:
Type of Change
Testing