Skip to content

docs: site NTP server config and extension service name resuse #3353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hanyux-nv wants to merge 1 commit into
base: main
Choose a base branch
from
Open

docs: site NTP server config and extension service name resuse #3353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/getting-started/quick-start.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ Open `helm-prereqs/values/nico-core.yaml` and update the following values:
| `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 |

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

| `site_fabric_prefixes` | CIDRs that are part of the site fabric (instance-to-instance traffic) |
| `deny_prefixes` | CIDRs instances must not reach (OOB, control plane, management) |
| `[pools.lo-ip]` ranges | Loopback IP range allocated to bare-metal hosts |
Expand All @@ -154,7 +155,7 @@ All fields are documented with inline comments in the file.

- **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.

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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


### 3d. NICo REST source tree

Expand Down Expand Up @@ -214,7 +215,7 @@ envConfig:

MetalLB provides LoadBalancer IPs for NICo Core services (nico-api, DHCP, DNS, PXE, SSH console). Without it, those services stay in `<pending>` state and the site is unreachable.

> **NTP note:** NICo does not run a standalone NTP service. Instead, NTP server addresses are provided to managed hosts via DHCP option 42--configured in the `nico-dhcp` chart Kea hook parameters (`nico-ntpserver`). Point this to your enterprise NTP servers.
> **NTP note:** NICo does not run a standalone NTP service. Configure enterprise NTP server IPs in `siteConfig.ntp_servers`; NICo uses that list to configure BMC NTP during pre-ingestion and to advertise DHCP option 42 to managed hosts. The `nico-dhcp` chart Kea hook parameter (`nico-ntpserver`) remains a fallback when `ntp_servers` is empty.

Edit `helm-prereqs/values/metallb-config.yaml`--this file ships pre-populated with example values. Replace all values labeled `# EXAMPLE` with your site-specific configuration before running `setup.sh`.

Expand Down
8 changes: 4 additions & 4 deletions docs/manuals/dpu_extension_service.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ Currently, the only supported service type is **Kubernetes Pod** (`KubernetesPod
| **Version** | An immutable snapshot of the Pod manifest (`data`), optional registry credentials, and optional observability configuration. Versions use NICo config version strings (for example, `V1-T1761856992374052`). |
| **Deployment** | An association between an Instance and a specific `(service_id, version)` pair. |
| **Active versions** | All non-deleted versions available for deployment. Older versions remain until explicitly deleted. |
| **Terminating services** | When a service is removed from an Instance, NICo tracks it until all DPUs confirm termination. |
| **Terminating services** | When a service is removed from an Instance, NICo tracks it until the affected DPUs confirm termination. |

**Important Constraints:**

* Service names must be case-insensitive unique within a tenant.
* Service names must be case-insensitive unique among non-deleted services within a tenant. After a service is deleted, a new service in the same tenant may reuse the same name.
* Service type and tenant ownership are immutable after creation.
* An Instance may deploy multiple services but at most one version of each service at a time.
* Instance extension service changes are only accepted while the Instance is in `Ready` state.
Expand Down Expand Up @@ -154,7 +154,7 @@ Use `--if-version-ctr-match` (CLI only) to prevent concurrent update conflicts.

### Delete a DPU Extension Service or Version

Deletion succeeds only when no Instance is using the version being deleted. If a version is still deployed, remove it from affected Instances first. When all versions are deleted, the service itself is removed automatically.
Deletion succeeds only when no Instance is using the version being deleted. If a version is still deployed, remove it from affected Instances first. When all versions are deleted, the service itself is removed automatically. Deleted service names are released and can be reused by new services in the same tenant.

**REST API:**

Expand Down Expand Up @@ -202,7 +202,7 @@ Include `dpuExtensionServiceDeployments` in the request body:
}
```

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.

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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


### Upgrade a Deployed DPU Extension Service for an Instance

Expand Down
11 changes: 7 additions & 4 deletions docs/provisioning/ip-and-network-configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,15 @@ To configure these flows:
2. **Configure the DHCP relay on every OOB switch** to forward DHCP traffic to the `nico-dhcp` LoadBalancer VIP (the IP assigned to the `nico-dhcp` service by MetalLB in [Quick Start Step 3h](../getting-started/quick-start.md#3h-assign-service-vips)). The relay must be on the same L2 broadcast domain as the BMCs and DPUs it serves.
3. **For predefined IPs**, upload `expected_machines.json` with `bmc_ip_address` populated **before** the host first powers on. Uploading after the BMC has already received a dynamic lease will not retroactively change its IP — release the lease (`nico-admin-cli ... em ...`) and power-cycle the BMC.
4. **Set `dhcp_servers`** in `siteConfig` to the list of DHCP server IPs reachable from bare-metal hosts. This list is informational and is passed through to agents; it does not change how `nico-dhcp` itself serves leases. May be left as `[]`.
5. **Set `ntp_servers`** in `siteConfig` to your NTP server IPs. NICo uses this list to configure BMC NTP through Redfish during pre-ingestion, includes it in `DiscoverDhcp` responses, and passes it to DPU agents so their DHCP server advertises the same NTP servers to managed hosts.

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.
Comment on lines 178 to +181

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

- The per-segment definitions in `siteConfig` `[networks.<name>]` blocks — gateway, MTU, additional routes.

> **NTP note:** NICo does not run a standalone NTP service. NTP server addresses are distributed to managed hosts via DHCP option 42, configured in the `nico-dhcp` chart Kea hook parameters (`nico-ntpserver`). Point this to your enterprise NTP servers.
> **NTP note:** NICo does not run a standalone NTP service. Point `siteConfig.ntp_servers` to your enterprise NTP servers. NICo also attempts to set those servers on host BMCs during pre-ingestion; if the Redfish operation fails repeatedly, pre-ingestion continues and the failure is logged.

### 2.3 How to Verify DHCP Is Working

Expand Down Expand Up @@ -289,7 +291,7 @@ The required A records (shown for `.nico`; substitute `.nico` if your binaries u
| `nico-api.nico` | 443 | `nico-api` external LoadBalancer VIP | NICo gRPC API | Yes — `NICO_API_URL` env var on most clients |
| `nico-pxe.nico` | 80 | `nico-pxe` LoadBalancer VIP | iPXE scripts, cloud-init, internal APT, TLS root CA | **No** — hardcoded in the compiled DPU agent |
| `nico-static-pxe.nico` | 80 | Static PXE asset server VIP | `scout.squashfs`, `scout.efi`, BFB images, and other static boot artifacts | **No** — hardcoded in the host boot scripts that ship inside boot images |
| `nico-ntp.nico` | 123 | Operator-supplied NTP server IP(s) — the record points at your existing NTP infrastructure, not a NICo-deployed service | NTP time sync; agent reads this and re-advertises via DHCP option 42 | **No** — hostname is hardcoded in the compiled DPU agent; multiple A records recommended |
| `nico-ntp.nico` | 123 | Operator-supplied NTP server IP(s) — the record points at your existing NTP infrastructure, not a NICo-deployed service | Legacy NTP fallback for DPU agents when `siteConfig.ntp_servers` is empty | **Fallback only** — prefer `siteConfig.ntp_servers`; keep this DNS record if any deployed agent still relies on it |
| `unbound.nico` | 53 | `unbound` LoadBalancer VIP | Recursive DNS resolver | Yes — the resolver address itself is distributed via DHCP option 6 |
| `otel-receiver.nico` | 443 | OTel receiver VIP on the site controller | OTLP ingestion endpoint for DPU otel-collector sidecars | Yes — set in the otel-collector configuration YAML and re-deployed |

Expand Down Expand Up @@ -343,10 +345,11 @@ Use this checklist as the final gate before powering on the first host BMC:
- [ ] One or more OOB network segments declared in `[networks.<name>]`, sized for `1 + 2 × DPU_count` IPs per host.
- [ ] `initial_domain_name` set in `siteConfig`.
- [ ] `dhcp_servers` set in `siteConfig` (or left as `[]`).
- [ ] `ntp_servers` set in `siteConfig`, or the legacy `nico-ntp` DNS / `nico-dhcp` `nico-ntpserver` fallback is intentionally configured.
- [ ] `expected_machines.json` uploaded for every host; `bmc_ip_address` populated for any host that needs a predefined BMC IP.
- [ ] OOB switches configured with a DHCP relay pointing to the `nico-dhcp` LoadBalancer VIP.
- [ ] LoadBalancer VIPs assigned for `nico-api`, `nico-dhcp`, `nico-pxe`, `nico-dns` (one per replica), `nico-ssh-console-rs`, and `unbound`.
- [ ] `unbound`'s `local_data.conf` ConfigMap contains A records for `nico-api`, `nico-pxe`, `nico-static-pxe`, `nico-ntp`, `unbound`, and `otel-receiver` in the `.nico` (or `.nico`) zone; the `nico-ntp` record points to your operator-supplied NTP server.
- [ ] `unbound`'s `local_data.conf` ConfigMap contains A records for `nico-api`, `nico-pxe`, `nico-static-pxe`, `unbound`, and `otel-receiver` in the `.nico` (or `.nico`) zone; include `nico-ntp` pointing at your operator-supplied NTP server if you use the legacy fallback.
- [ ] `nico-dns` zone for `initial_domain_name` is delegated from upstream DNS, or `unbound` forwards the zone to the `nico-dns` VIPs.
- [ ] `unbound.nico` resolves every NICo service hostname (verified with the `dig` loop in [section 3.4](#34-how-to-verify-dns-is-working)).
- [ ] `nico-dhcp` logs show DISCOVER → OFFER for a test BMC power-on.
Expand Down
Loading