fix(setup): seed the REST site record and fix the worker DB target#3284
Open
shayan1995 wants to merge 2 commits into
Open
fix(setup): seed the REST site record and fix the worker DB target#3284shayan1995 wants to merge 2 commits into
shayan1995 wants to merge 2 commits into
Conversation
Two defects left a fresh site with an empty `nicocli site list` despite a healthy, handshaked site-agent: 1. setup.sh minted a site UUID and the site-agent bootstrap Job POSTed /v1/site to site-manager, which creates the Site CR + OTP — but never the site row in the REST database. The agent handshakes under an identity no REST site matches, so its inventory is dropped. 2. The nico-pg-cluster consolidation (NVIDIA#3081) missed the workflow subchart: site/cloud workers stayed on the legacy postgres.postgres/nico database (zero tables) and failed every DB activity with SQLSTATE 42P01 (relation "site" does not exist), so no site could leave Pending. Both fixes are contained in setup.sh — the bootstrap Job's existing POST /v1/site flow (Site CR + OTP + temporal certs) is untouched and simply runs after the DB row exists, under the same UUID: - Resolve the site UUID instead of blindly minting: explicit NICO_SITE_UUID (bind to a pre-existing site) -> CLUSTER_ID from a prior install's site-agent StatefulSet (stable reruns) -> an existing REST site row with our name (adopt; idempotent reprovision) -> mint. Then seed the REST DB directly with the same record shape as forged's per-env envs/*/carbide-rest/site.sql: a 'default' infrastructure_provider for the org (NICO_ORG, default ncx) plus a Pending site row named after NICO_SITE_NAME / values.yaml siteName, id = created_by = the resolved UUID. All inserts are guarded (WHERE NOT EXISTS) and the seed waits for the REST migrations to have created the tables. IdP-agnostic: no API token required. - Delete a site-registration secret bound to a stale UUID so rebinding CLUSTER_ID re-bootstraps instead of silently keeping the old identity. - Inject worker DB values at install time via the existing REST creds temp file: nico-rest-workflow now targets nico-pg-cluster/nico_rest as nico-rest.nico with the db-creds secret, aligned with nico-rest-api. Based on Parham Armani's proposed fix, minimized to avoid chart changes: the adopt-only bootstrap.yaml rewrite is intentionally not taken — the CR and DB record are independent artifacts, and ordering the seed before the existing bootstrap achieves the same invariant.
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 (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughUpdates ChangesNICo REST bootstrap changes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant setup.sh
participant nico-rest-site-agent StatefulSet
participant Patroni primary
participant REST PostgreSQL
setup.sh->>setup.sh: Resolve NICO_SITE_UUID
setup.sh->>nico-rest-site-agent StatefulSet: Read prior CLUSTER_ID
setup.sh->>REST PostgreSQL: Look up existing site by name and org
setup.sh->>setup.sh: Mint UUID with python3 if needed
setup.sh->>Patroni primary: Discover primary for REST DB writes
setup.sh->>REST PostgreSQL: Seed site records when primary is available
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm-prereqs/setup.sh (1)
1016-1018: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuppressing
stderrhere defeats the very diagnostics the error handlers promise.
_rest_sqlroutespsqlstderr to/dev/null, yet the seeding guards (Lines 1052, 1059) emit only a genericfailed to seed …. When an insert genuinely fails, the operator loses the underlyingpsql/ON_ERROR_STOPmessage — precisely the actionable detail needed to diagnose a fresh-site provisioning failure. Consider dropping2>/dev/nullon the write path (or capturing and echoing stderr in the failure branch) so the real cause surfaces.As per path instructions:
helm-prereqs/**scripts should provide "clear failure messages."🤖 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 `@helm-prereqs/setup.sh` around lines 1016 - 1018, The _rest_sql helper is suppressing psql stderr, which hides the real failure details from the seeding guards that call it. Update _rest_sql in setup.sh so diagnostics from kubectl exec/su/psql are preserved on the write path, or capture stderr and surface it in the existing seed-failure handling near the _rest_sql call sites. Keep the existing helpers and failure branches, but ensure the operator sees the actual psql/ON_ERROR_STOP error instead of only a generic message.Source: Path instructions
🤖 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 `@helm-prereqs/setup.sh`:
- Around line 1016-1019: The _rest_sql helper currently runs raw SQL strings,
and the adoption/seeding queries are interpolating NICO_SITE_NAME, NICO_ORG, and
NICO_SITE_UUID directly into those strings, which is unsafe. Update the SQL
execution flow in _rest_sql and its callers to pass these values as psql
variables using -v and reference them in the SQL with :'var' placeholders
instead of shell interpolation. Make sure the affected adoption and seeding
query construction paths use the existing _rest_sql wrapper so the site/org/UUID
values are safely quoted by psql.
---
Nitpick comments:
In `@helm-prereqs/setup.sh`:
- Around line 1016-1018: The _rest_sql helper is suppressing psql stderr, which
hides the real failure details from the seeding guards that call it. Update
_rest_sql in setup.sh so diagnostics from kubectl exec/su/psql are preserved on
the write path, or capture stderr and surface it in the existing seed-failure
handling near the _rest_sql call sites. Keep the existing helpers and failure
branches, but ensure the operator sees the actual psql/ON_ERROR_STOP error
instead of only a generic message.
🪄 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: 986a4190-7170-4251-9906-916f39066df8
📒 Files selected for processing (1)
helm-prereqs/setup.sh
Comment on lines
+1016
to
+1019
| _rest_sql() { # runs SQL against the nico_rest DB on the Patroni primary | ||
| kubectl exec -n postgres "${_REST_PG_PRIMARY}" -- \ | ||
| su postgres -c "psql -d nico_rest -v ON_ERROR_STOP=1 -tAc \"$1\"" 2>/dev/null | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section with line numbers.
sed -n '990,1085p' helm-prereqs/setup.sh | cat -n
# Find _rest_sql usage and nearby variable handling.
rg -n "_rest_sql|NICO_SITE_NAME|NICO_ORG|NICO_SITE_UUID" helm-prereqs/setup.shRepository: NVIDIA/infra-controller
Length of output: 9513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '990,1085p' helm-prereqs/setup.sh | cat -n
rg -n "_rest_sql|NICO_SITE_NAME|NICO_ORG|NICO_SITE_UUID" helm-prereqs/setup.shRepository: NVIDIA/infra-controller
Length of output: 9513
Pass site/org/UUID values as psql variables The adoption and seeding queries splice NICO_SITE_NAME, NICO_ORG, and NICO_SITE_UUID directly into SQL literals, so an apostrophe will break the query and arbitrary SQL can be injected. Use psql -v with :'var' placeholders instead of interpolating shell values into the SQL string.
🤖 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 `@helm-prereqs/setup.sh` around lines 1016 - 1019, The _rest_sql helper
currently runs raw SQL strings, and the adoption/seeding queries are
interpolating NICO_SITE_NAME, NICO_ORG, and NICO_SITE_UUID directly into those
strings, which is unsafe. Update the SQL execution flow in _rest_sql and its
callers to pass these values as psql variables using -v and reference them in
the SQL with :'var' placeholders instead of shell interpolation. Make sure the
affected adoption and seeding query construction paths use the existing
_rest_sql wrapper so the site/org/UUID values are safely quoted by psql.
Source: Path instructions
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3284.docs.buildwithfern.com/infra-controller |
Contributor
There was a problem hiding this comment.
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 `@docs/getting-started/quick-start.md`:
- Line 98: Update the NICO_SITE_UUID documentation row to match the actual
bootstrap flow by replacing the reference to the site-agent ConfigMap with the
site-agent StatefulSet environment source. Use the quick-start table entry for
NICO_SITE_UUID as the anchor, and align the wording with setup.sh behavior so
operators know the prior UUID is read from CLUSTER_ID in the site-agent
StatefulSet env before falling back to an existing REST site or minting a new
one.
🪄 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: 3c669252-765d-43e6-a92b-61e3fe03a72d
📒 Files selected for processing (3)
docs/getting-started/quick-start.mdhelm-prereqs/README.mdhelm-prereqs/setup.sh
✅ Files skipped from review due to trivial changes (1)
- helm-prereqs/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- helm-prereqs/setup.sh
| | `NICO_REST_IMAGE_TAG` | **Yes** | NICo REST image tag (e.g. `v1.0.4`). | | ||
| | `KUBECONFIG` | **Yes** | Path to your cluster kubeconfig. | | ||
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` generates a random UUID each run. | | ||
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent ConfigMap), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the UUID source with the bootstrap flow.
This row says the prior UUID comes from the site-agent ConfigMap, but the supplied setup.sh context reads CLUSTER_ID from the site-agent StatefulSet env. Please update the docs so operators look at the right object.
-| `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent ConfigMap), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. |
+| `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent StatefulSet env), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent ConfigMap), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. | | |
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent StatefulSet env), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. | |
🤖 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 98, Update the NICO_SITE_UUID
documentation row to match the actual bootstrap flow by replacing the reference
to the site-agent ConfigMap with the site-agent StatefulSet environment source.
Use the quick-start table entry for NICO_SITE_UUID as the anchor, and align the
wording with setup.sh behavior so operators know the prior UUID is read from
CLUSTER_ID in the site-agent StatefulSet env before falling back to an existing
REST site or minting a new one.
Post-review hardening of the site-registration seeding, plus seed the site
config with the v2 networking posture:
- Seed config as {"native_networking": true, "network_security_group": true,
"flow": true} instead of '{}' — the REST create handler defaults the first
two to true ("new sites default to the v2 networking posture", site.go);
flow is enabled because these deployments run NICo Flow. Previously all
capabilities read false via the API.
- Also seed the status_detail row the create handler writes atomically with
the site row, so status endpoints don't return an empty array (non-fatal).
- Read the prior install's CLUSTER_ID from the nico-rest-site-agent-config
ConfigMap: the chart delivers it via envFrom, never as an inline env entry,
so the previous StatefulSet jsonpath always returned empty — resolution
step 2 and the stale-secret guard were dead code. Fetched once, reused.
- Guard the Patroni-primary lookup with || true: under set -euo pipefail a
kubectl failure killed the script before the emptiness check that was
meant to make seeding optional.
- Validate inputs before SQL interpolation: NICO_SITE_UUID must be a UUID,
NICO_SITE_NAME/NICO_ORG restricted to [A-Za-z0-9][A-Za-z0-9._-]* (they are
embedded in a double-quoted shell string; a quote would break psql or the
remote shell).
- Parse siteName from values.yaml regardless of quoting style (bare, single-
or double-quoted), and don't die under set -e when values.yaml is missing.
- Docs: NICO_SITE_UUID no longer "generates a random UUID each run" — it is
resolved (prior ConfigMap, adopt-by-name, mint+seed).
shayan1995
force-pushed
the
fix/site-registration-seed
branch
from
eeb42ab to
e668832
Compare
thossain-nv
approved these changes
Jul 8, 2026
| fi | ||
| if [[ -z "${NICO_SITE_UUID:-}" && -n "${_REST_PG_PRIMARY}" ]]; then | ||
| # 3. adopt an existing site row with our name | ||
| NICO_SITE_UUID="$(_rest_sql "SELECT id FROM site WHERE name='${NICO_SITE_NAME}' AND org='${NICO_ORG}' AND deleted IS NULL LIMIT 1;" || true)" |
Contributor
There was a problem hiding this comment.
If for some reason there's more than 1 Site in DB, we can print Site ID, name and org and print a warning that this is not expected under normal disconnected Site workflow.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
polarweasel
reviewed
Jul 8, 2026
| | `NICO_REST_IMAGE_TAG` | **Yes** | NICo REST image tag (e.g. `v1.0.4`). | | ||
| | `KUBECONFIG` | **Yes** | Path to your cluster kubeconfig. | | ||
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` generates a random UUID each run. | | ||
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent ConfigMap), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. | |
Contributor
There was a problem hiding this comment.
If this change works, apply it to the README too :)
Suggested change
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` reuses the UUID from a prior install (site-agent ConfigMap), else adopts an existing REST site with the same name, else mints one and seeds the site record itself. | | |
| | `NICO_SITE_UUID` | No | Stable UUID for this site. If unset, `setup.sh` tries to reuse the UUID from a prior install (site-agent ConfigMap). If that fails, it adopts an existing REST site with the same name, or mints a UUID and seeds the site record itself. | |
parmani-nv
approved these changes
Jul 9, 2026
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
4 participants
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.
Two defects left a fresh site with an empty
nicocli site listdespite a healthy, handshaked site-agent:Both fixes are contained in setup.sh — the bootstrap Job's existing POST /v1/site flow (Site CR + OTP + temporal certs) is untouched and simply runs after the DB row exists, under the same UUID:
Related issues
Type of Change
Breaking Changes
Testing
tested this end to end on dev6
Additional Notes