From 88cf987beed4a803163daf5d8dd91f09da085571 Mon Sep 17 00:00:00 2001 From: Alex Yuskauskas Date: Thu, 17 Sep 2026 16:51:30 -0700 Subject: [PATCH 1/2] feat(upgrade): nodewright v0.18.0 record; relax rule 2 to safe only Add recipes/components/nodewright-operator/upgrades.yaml describing the v0.18.0 Skyhook -> NodeWright rename: manual, with per-deployer steps transcribed from upstream's docs/getting-started/migration.md, and a to ceiling at v0.18.0 so any higher target reports blocked and is told to cross the rename on its own. Authoring it required relaxing ADR-021 rule 2, which held every verdict's to ceiling at or below the pinned version. Only safe vouches, so only safe is held to what AICR ships; manual and blocked are warnings carrying instructions and cannot read as a pass. Holding them to the pin forced the wrong order of work - bump first, document after - when reading the migration notes is what qualifies the bump. The obligation stays self-renewing through rule 3, which fails any bump leaving a from hole below the new pin. Signed-off-by: Alex Yuskauskas --- .claude/CLAUDE.md | 2 +- AGENTS.md | 2 +- docs/contributor/upgrade-records.md | 44 +++-- docs/design/021-component-upgrade-safety.md | 12 +- docs/user/cli-reference.md | 4 +- docs/user/component-catalog.md | 43 ++++- pkg/upgrade/wellformed.go | 24 ++- pkg/upgrade/wellformed_test.go | 45 +++-- .../nodewright-operator/upgrades.yaml | 170 ++++++++++++++++++ recipes/registry.yaml | 2 + 10 files changed, 307 insertions(+), 41 deletions(-) create mode 100644 recipes/components/nodewright-operator/upgrades.yaml diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index e6e3fda100..82b9d9a400 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -282,7 +282,7 @@ slog.Error("operation failed", "error", err, "component", "gpu-collector") **After any change to `recipes/registry.yaml`, a component's values file, or a chart version pin (in registry, overlay, or mixin):** run `make bom-docs` and commit the regenerated `docs/user/container-images.md` in the same PR. The BOM is rendered fresh from each Helm chart's actual templates, so an unbumped pin can still pick up upstream image drift — running it locally is the only reliable way to know whether the doc needs an update. The BOM's **version column and component set are gated**: `TestCommittedBOMVersionsMatchRegistry` (run by `make test` → `make qualify`, and by the `bom-freshness` merge-gate job on docs-only PRs) fails CI when a pinned version or the component set drifts from the registry, so a version change that forgets `make bom-docs` is caught. Not gated at PR time is *rendered-image drift* — an unbumped pin picking up a new image inside a chart's templates; `make bom-check` (a full re-render comparison) is its **opt-in** blocking check and is not wired into `make qualify`, `make lint`, or the merge gate, while the scheduled BOM-refresh workflow (`.github/workflows/bom-refresh.yaml`) auto-detects that drift weekly and opens a PR. So still run `make bom-docs` on any chart-touching change. -**After any chart version pin bump (`defaultVersion` / `defaultTag`, in registry, overlay, or mixin):** the component's ADR-021 transition record must describe the boundary you just crossed. Records live at `recipes/components//upgrades.yaml`, referenced from the registry entry's `upgrades.file`. Two rules make this self-renewing rather than deferrable: a record's `to` ceiling may not reach past the current pin (so a record only ever describes ground already covered, and every bump lands outside it), and the `from` domains may not leave a hole below the pin (so no operator version matches nothing). `make lint` runs `check-upgrade-records` over every referenced record. +**After any chart version pin bump (`defaultVersion` / `defaultTag`, in registry, overlay, or mixin):** the component's ADR-021 transition record must describe the boundary you just crossed. Records live at `recipes/components//upgrades.yaml`, referenced from the registry entry's `upgrades.file`. Two rules govern reach: a **`safe`** record's `to` ceiling may not reach past the current pin (only `safe` vouches, so only `safe` is held to what AICR ships — `manual` and `blocked` may describe a boundary above the pin, which is how upgrade guidance lands *before* the bump it describes), and the `from` domains may not leave a hole below the pin (so no operator version matches nothing). The second is what makes the obligation self-renewing: a bump leaving a hole below the new pin fails whatever any ceiling says. `make lint` runs `check-upgrade-records` over every referenced record. Author `safe` only with a `verifiedBy` naming a UAT lane, KWOK run, or upstream release note; `manual` and `blocked` need at least one step for every one of the five deployers. A wrong `safe` is worse than no record, because it converts uncertainty into false confidence; when you do not know, leave it unwritten and let it report `unknown`. `summary` is not a changelog or release note: it names what breaks and by what mechanism, tightly, because it is read beside the steps at the moment someone decides whether to upgrade. Put upstream migration guides, release notes, and the component's own docs in `references` instead. Never add a Go test asserting a verdict for a real component: verdicts are validated empirically by KWOK and UAT, and pinning them in the unit suite turns "keep tests green" into pressure to weaken records. Full authoring guide: `docs/contributor/upgrade-records.md`. diff --git a/AGENTS.md b/AGENTS.md index c804fc5c86..6ac51e68bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -282,7 +282,7 @@ slog.Error("operation failed", "error", err, "component", "gpu-collector") **After any change to `recipes/registry.yaml`, a component's values file, or a chart version pin (in registry, overlay, or mixin):** run `make bom-docs` and commit the regenerated `docs/user/container-images.md` in the same PR. The BOM is rendered fresh from each Helm chart's actual templates, so an unbumped pin can still pick up upstream image drift — running it locally is the only reliable way to know whether the doc needs an update. The BOM's **version column and component set are gated**: `TestCommittedBOMVersionsMatchRegistry` (run by `make test` → `make qualify`, and by the `bom-freshness` merge-gate job on docs-only PRs) fails CI when a pinned version or the component set drifts from the registry, so a version change that forgets `make bom-docs` is caught. Not gated at PR time is *rendered-image drift* — an unbumped pin picking up a new image inside a chart's templates; `make bom-check` (a full re-render comparison) is its **opt-in** blocking check and is not wired into `make qualify`, `make lint`, or the merge gate, while the scheduled BOM-refresh workflow (`.github/workflows/bom-refresh.yaml`) auto-detects that drift weekly and opens a PR. So still run `make bom-docs` on any chart-touching change. -**After any chart version pin bump (`defaultVersion` / `defaultTag`, in registry, overlay, or mixin):** the component's ADR-021 transition record must describe the boundary you just crossed. Records live at `recipes/components//upgrades.yaml`, referenced from the registry entry's `upgrades.file`. Two rules make this self-renewing rather than deferrable: a record's `to` ceiling may not reach past the current pin (so a record only ever describes ground already covered, and every bump lands outside it), and the `from` domains may not leave a hole below the pin (so no operator version matches nothing). `make lint` runs `check-upgrade-records` over every referenced record. +**After any chart version pin bump (`defaultVersion` / `defaultTag`, in registry, overlay, or mixin):** the component's ADR-021 transition record must describe the boundary you just crossed. Records live at `recipes/components//upgrades.yaml`, referenced from the registry entry's `upgrades.file`. Two rules govern reach: a **`safe`** record's `to` ceiling may not reach past the current pin (only `safe` vouches, so only `safe` is held to what AICR ships — `manual` and `blocked` may describe a boundary above the pin, which is how upgrade guidance lands *before* the bump it describes), and the `from` domains may not leave a hole below the pin (so no operator version matches nothing). The second is what makes the obligation self-renewing: a bump leaving a hole below the new pin fails whatever any ceiling says. `make lint` runs `check-upgrade-records` over every referenced record. Author `safe` only with a `verifiedBy` naming a UAT lane, KWOK run, or upstream release note; `manual` and `blocked` need at least one step for every one of the five deployers. A wrong `safe` is worse than no record, because it converts uncertainty into false confidence; when you do not know, leave it unwritten and let it report `unknown`. `summary` is not a changelog or release note: it names what breaks and by what mechanism, tightly, because it is read beside the steps at the moment someone decides whether to upgrade. Put upstream migration guides, release notes, and the component's own docs in `references` instead. Never add a Go test asserting a verdict for a real component: verdicts are validated empirically by KWOK and UAT, and pinning them in the unit suite turns "keep tests green" into pressure to weaken records. Full authoring guide: `docs/contributor/upgrade-records.md`. diff --git a/docs/contributor/upgrade-records.md b/docs/contributor/upgrade-records.md index 6eb1bb0585..ad1a3e7e9d 100644 --- a/docs/contributor/upgrade-records.md +++ b/docs/contributor/upgrade-records.md @@ -21,15 +21,23 @@ moved nvsentinel from `v1.9.0` to `v1.20.0` in one step, skipping eleven minor versions, typed as `Build/CI/tooling` with the breaking-change box unchecked. Nothing claimed the upgrade was safe, because nothing asked. -Two rules in `pkg/upgrade` make the obligation self-renewing rather than -something you can defer: - -- A record's `to` ceiling **may not reach past the currently pinned version**. - An author cannot have read the migration notes for a version nobody has - released, so a record can only ever describe ground already covered. Every pin - bump therefore lands outside the existing record and forces you back into it. +Three rules in `pkg/upgrade` govern how far a record may reach, and keep the +obligation self-renewing rather than something you can defer: + +- A **`safe`** record's `to` ceiling **may not reach past the currently pinned + version**. `safe` is the vouching verdict, and an author cannot have read the + migration notes for a version nobody has released. +- **`manual` and `blocked` may reach past the pin.** They are warnings carrying + instructions, so reaching forward over-warns rather than passing something + unassessed — and holding them to the pin forced the wrong order of work. The + order that qualifies an upgrade is: read the migration notes, write the + record, *then* bump. A component deliberately held below a known-breaking + release is the case that proves it, since the record most worth having is the + one describing the release you are not shipping yet. - The `from` domains **may not leave a hole** below the pin. An operator sitting - on a version in the hole would match no transition at all. + on a version in the hole would match no transition at all. This is the rule + that makes the obligation self-renewing: a bump that leaves a hole below the + new pin fails, whatever any record's ceiling says. ## Where a record lives @@ -127,7 +135,7 @@ kind: ComponentUpgrades component: # must match the registry entry transitions: - from: "<0.18.0" # needs an upper bound, or it cannot be shown forward-only - to: ">=0.18.0 <=0.19.0" # needs both bounds; ceiling at or below the pin + to: ">=0.18.0 <=0.19.0" # needs both bounds; if safe, ceiling at or below the pin verdict: manual summary: >- What breaks, in one or two sentences. Not a changelog. @@ -242,11 +250,19 @@ the ceiling you assessed and re-run, or get the record widened. They get no verdict and no steps, for the same reason as the floor case, and the report names your ceiling as the place to stop. -That is not a limitation to route around. It is the match-time form of the rule -that already stops you writing a ceiling above the current pin: you cannot have -read the migration notes for a release nobody has cut. A ceiling is your claim -about how far forward you actually looked, so put it where you looked, and widen -it deliberately later rather than reaching for headroom now. +That is not a limitation to route around. A ceiling is your claim about how far +forward you actually looked, so put it where you looked, and widen it +deliberately later rather than reaching for headroom now. + +Note what the ceiling is *not* pinned to. Authoring-time rule 2 holds only a +`safe` ceiling at or below the current pin; a `manual` or `blocked` ceiling may +sit above it. So the two ends of a held component work like this: the ceiling +says how far you read, and the pin says how far AICR ships. When AICR +deliberately stays below a breaking release, those diverge, and the record that +describes the release you are *not* shipping is exactly the one an operator +needs. Put the ceiling at the boundary you assessed and let a target above it +land on `beyond-record-ceiling`, which is what turns "this jump has to be taken +on its own" into something the tool says rather than something prose asks for. ## Checking your work diff --git a/docs/design/021-component-upgrade-safety.md b/docs/design/021-component-upgrade-safety.md index 4372214c9d..581cba8191 100644 --- a/docs/design/021-component-upgrade-safety.md +++ b/docs/design/021-component-upgrade-safety.md @@ -225,7 +225,7 @@ This is what makes [Decision 10](#decision-10-a-coverage-gate-keeps-records-curr It costs little on the quiet path: a minor bump moves the rolling-head transition's `to` ceiling from `<=25.3.0` to `<=25.4.0` and its paired `from` ceiling to match, per the corrected example below — a small, mechanical edit, though a two-field one rather than the one-character edit this decision originally claimed. The point is not the edit, it is that the edit happens while the author is deciding whether the bump is safe. -**Together with coverage, this forces a continuous chain.** The gate requires the current pin to be covered, and no record may reach past it, so some record must end exactly at the pin. Extending that record backward, or adding one behind it, is then the only way to keep the chain whole. A gap can only appear if an author deliberately leaves one. +**Together with coverage, this forces a continuous chain.** The gate requires the current pin to be covered, and no `safe` record may reach past it, so some record must end at or above the pin. Extending that record backward, or adding one behind it, is then the only way to keep the chain whole. A gap can only appear if an author deliberately leaves one. Coverage is what makes the obligation self-renewing: a bump that leaves a `from` hole below the new pin fails regardless of any record's ceiling. That is mechanically checkable inside the record file alone, with no git history and no external data: sort the transitions by their `from` floor, and require the `from` domains to cover every version up to the pin with no interior hole. A hole between them is a version range the component could be running that no record describes, which is precisely what would later surface as `unknown` to whoever is furthest behind. @@ -264,7 +264,7 @@ What it is good for is the thing an operator needs *before* upgrading rather tha | `apiVersion` | yes | `aicr.run/v1beta1`, per [ADR-022 §2](022-artifact-maturity-and-deprecation.md). The loader fails closed on anything else. | | `kind`, `component` | yes | `ComponentUpgrades`; `component` must match the registry entry. | | `transitions[]` | yes | One or more. | -| `.from`, `.to` | yes | Semver ranges. `to`'s lower bound is the boundary the record describes, and a jump crosses it whenever the source is below it and the target reaches it; `from` decides whether this record's guidance was authored for that starting point (see [Decision 5](#decision-5-one-matcher-three-independent-axes)). `to` MUST NOT reach past the currently pinned version; widen `from` backward instead. | +| `.from`, `.to` | yes | Semver ranges. `to`'s lower bound is the boundary the record describes, and a jump crosses it whenever the source is below it and the target reaches it; `from` decides whether this record's guidance was authored for that starting point (see [Decision 5](#decision-5-one-matcher-three-independent-axes)). A `safe` `to` MUST NOT reach past the currently pinned version; widen `from` backward instead. `manual` and `blocked` may reach past it, so upgrade guidance can be landed before the bump it describes (amended in [#2424](https://github.com/NVIDIA/aicr/issues/2424)). | | `.verdict` | yes | `safe`, `manual`, or `blocked`. `unknown` and `unversioned` are computed, never authored. | | `.verifiedBy` | for `safe` | What backs the claim: a UAT lane, a KWOK run, or an upstream release note. Required on `safe` so the gate measures assessment rather than coverage. | | `.summary` | yes | One sentence on what changes. | @@ -355,7 +355,7 @@ That shared code path was aspirational when written and is now real: only `helm` **Whether a cluster scan runs.** The at-risk scan for unmanaged resources ([Decision 3](#decision-3-ownership-classes-and-what-aicr-can-see)) needs a cluster no matter where the `from` table came from, so it is its own axis rather than a property of `--from`. It is implied by `--from cluster` and available alongside artifact comparison via `--scan-cluster`. Comparing two bundles while scanning a live cluster for unmanaged `Skyhook` objects is a legitimate combination, and the two-mode framing had no name for it. -**A record's verdict reaches only as far as its own `to` ceiling** (narrowed in [#2760](https://github.com/NVIDIA/aicr/pull/2760), which previously tested only `to`'s lower bound and never compared the target against the ceiling, so a record claiming `to: ">=0.18.0 <0.19.0" verdict: safe` lent `safe` to `0.17.0 -> 0.25.0`). `to`'s floor is the boundary a jump crosses; its ceiling is how far the verdict carries. This is the same principle [Rule 2](#decision-2-transition-records) enforces at authoring time, where a `to` may not reach past the current pin because an author cannot have read the migration notes for a version nobody has released. A record vouching past its own ceiling is that identical forward reach, moved to match time. +**A record's verdict reaches only as far as its own `to` ceiling** (narrowed in [#2760](https://github.com/NVIDIA/aicr/pull/2760), which previously tested only `to`'s lower bound and never compared the target against the ceiling, so a record claiming `to: ">=0.18.0 <0.19.0" verdict: safe` lent `safe` to `0.17.0 -> 0.25.0`). `to`'s floor is the boundary a jump crosses; its ceiling is how far the verdict carries. This is the same principle [Rule 2](#decision-2-transition-records) enforces at authoring time, where a `safe` `to` may not reach past the current pin because an author cannot have read the migration notes for a version nobody has released. A record vouching past its own ceiling is that identical forward reach, moved to match time. Both are scoped to vouching: a `manual` or `blocked` ceiling may sit above the pin, because over-warning cannot read as a pass. **`blocked` means AICR can name where to stop.** That is what unifies its four routes, and what separates it from `unknown`: `blocked` hands the operator information to act on, while `unknown` says AICR has none and the investigation is theirs. Neither is a pass. [Decision 6](#decision-6-non-zero-exit-by-default-on-anything-but-safe) carries the gradient of `unknown` and the failure rule that follows from it. @@ -483,7 +483,7 @@ A downgrade stays `unknown` rather than becoming `blocked` because `blocked` nam **`unversioned` fails for a third reason.** There is no boundary to classify at all, so it is a blind spot in the *inputs* rather than a gap in the data, and the remedy is in the operator's hands: pinning a comparable ref resolves it. -**Most runs are red today, and that is the honest signal.** Exactly one registry component ships a record, so nearly every component whose version changes reports `unknown` and the check exits non-zero. The original text treated that as a reason to calibrate: "a matrix that starts at zero coverage would fail on every component, and strict mode would sit disabled forever." That reads near-zero coverage as a permanent constraint to design around. It is a starting condition to design *out of*, and the strict exit is part of what forces that. Anyone who accepts the gap deliberately has `--fail-on-error=false`, which prints the full report without the gate. +**Most runs are red today, and that is the honest signal.** Only two registry components ship a record, so nearly every component whose version changes reports `unknown` and the check exits non-zero. The original text treated that as a reason to calibrate: "a matrix that starts at zero coverage would fail on every component, and strict mode would sit disabled forever." That reads near-zero coverage as a permanent constraint to design around. It is a starting condition to design *out of*, and the strict exit is part of what forces that. Anyone who accepts the gap deliberately has `--fail-on-error=false`, which prints the full report without the gate. **This decision and [Decision 10](#decision-10-a-coverage-gate-keeps-records-current) justify each other, and neither is sound alone.** The strict exit is only reasonable because coverage is being made mandatory: the coverage gate ([#2535](https://github.com/NVIDIA/aicr/issues/2535)) drives `unknown` toward the exception rather than the rule, which is what turns this into something a pipeline can rely on. And the coverage gate is only enforceable because the strict exit creates the pressure to author records: a gate nobody feels is a gate nobody clears. Read either one on its own and it looks like a bad trade. @@ -778,7 +778,7 @@ The governing discipline is a design constraint, not a test-plan detail: **no te |---|---| | Unit (Go) | Table-driven matcher tests over synthetic records: verdict selection, semver range edges, and that a forward record never matches in reverse. Never reads the real registry. | | Golden (Go) | Rendered table and JSON report compared byte-for-byte against checked-in goldens with an `-update` flag, not by substring match. Synthetic input. | -| Registry well-formedness (Go) | Runs against **real** `recipes/components/*/upgrades.yaml`. Asserts files parse, ranges are valid semver, no record matches its own reverse, the referenced component exists, `safe` carries `verifiedBy`, no `to` reaches past the pin, and no gap sits between records. **Deliberately pin-sensitive**: a bump that does not touch the record fails it. | +| Registry well-formedness (Go) | Runs against **real** `recipes/components/*/upgrades.yaml`. Asserts files parse, ranges are valid semver, no record matches its own reverse, the referenced component exists, `safe` carries `verifiedBy`, no `safe` `to` reaches past the pin, and no gap sits between records. **Deliberately pin-sensitive**: a bump that does not touch the record fails it. | | KWOK (new, this ADR) | Synthetic fixture component with two trivial chart versions. Install, upgrade, roll back, and confirm the check reads the right versions back. No network chart pull, per-PR speed, unaffected when a real pin moves. | | UAT (new, [Decision 9](#decision-9-uat-covers-upgrade-and-rollback)) | Release-to-release against real clusters. The only layer that tests a real component's `safe` verdict, at the version pair it happens to run. The rollback leg is a smoke check, not validation of `reversible`. | @@ -798,7 +798,7 @@ It deliberately proves nothing about real component upgrades. It proves the mech 6. Wrapper charts expose the payload version in `aicr.run/component-version`, and a `dev` build still produces a Helm-valid `Chart.yaml`. 7. A malformed or reverse-matching record in `recipes/components/*/upgrades.yaml` fails `make lint` rather than being silently skipped. 8. A `safe` record with no `verifiedBy` fails the well-formedness check, so a blanket `safe` cannot satisfy the coverage gate. -9. A record whose `to` reaches past the currently pinned version fails the well-formedness check, so no record can make a claim about a version that does not exist yet. +9. A `safe` record whose `to` reaches past the currently pinned version fails the well-formedness check, so no record can *vouch* for a version AICR does not ship. `manual` and `blocked` may describe one, which is how upgrade guidance lands before the bump it describes; coverage (rule 3), not this rule, is what forces a record back open on a bump. 10. A record file whose transitions leave an interior hole in the `from` domains, a version between the lowest `from` floor and the pin that no transition's `from` covers, fails the well-formedness check. 11. A record carrying an unrecognized `apiVersion` fails with `ErrCodeInvalidRequest` naming both values, and is neither skipped nor reported as `unknown`. 12. A pinned component version with no record whose `to` range covers it fails `make test`, unless the component is on the coverage allowlist. diff --git a/docs/user/cli-reference.md b/docs/user/cli-reference.md index 8a80ca635d..1cdc342355 100644 --- a/docs/user/cli-reference.md +++ b/docs/user/cli-reference.md @@ -1553,7 +1553,7 @@ The report still reports a **breaking boundary** (a major bump, a minor bump whi `blocked` and `unknown` say opposite things. `blocked` means AICR has something to tell you and a version to stop at: read it and act on it. `unknown` means AICR has nothing for you: read the component's own upstream release notes and decide. Neither is a pass. -**Rollout note: expect red today.** Exactly one registry component ships a transition record so far, so most components that change version report `unknown` and the check exits non-zero on most comparisons. That is a coverage problem being worked ([#2535](https://github.com/NVIDIA/aicr/issues/2535) makes records mandatory per pin bump), not a tool limitation, and it shrinks as records are authored. Use `--fail-on-error=false` if you want the report without the gate in the meantime. +**Rollout note: expect red today.** Only two registry components ship a transition record so far, so most components that change version report `unknown` and the check exits non-zero on most comparisons. That is a coverage problem being worked ([#2535](https://github.com/NVIDIA/aicr/issues/2535) makes records mandatory per pin bump), not a tool limitation, and it shrinks as records are authored. Use `--fail-on-error=false` if you want the report without the gate in the meantime. Components whose version is identical on both sides produce no row. Added components are reported with nothing to do; removed components are reported and **stay installed**, because AICR does not uninstall them. @@ -1568,7 +1568,7 @@ Components whose version is identical on both sides produce no row. Added compon The first renders its record's steps, deployer-scoped, exactly as a `manual` row does: the author marked the move `blocked` and then wrote what to do instead. The other three render none, because the record that carries them describes a different move than the one you asked about. All four name a stopping point. -The fourth exists because a record vouches only as far as its own `to` ceiling. An author cannot have read the migration notes for a release nobody had cut, so a record claiming `>=0.18.0 <0.19.0` says nothing about `0.25.0`, and letting it lend its verdict there would report eight minors as safe on the strength of a two-minor claim. Stop at the assessed ceiling and re-run, or have the record widened. +The fourth exists because a record vouches only as far as its own `to` ceiling. A record claiming `>=0.18.0 <0.19.0` says nothing about `0.25.0`, and letting it lend its verdict there would report eight minors as safe on the strength of a two-minor claim. Stop at the assessed ceiling and re-run, or have the record widened. This is also how a component deliberately held below a breaking release reports: its record's ceiling sits at that release, so any target above it is told to stop there and take the boundary on its own. Every row states its reason in the detail block under the table, and `--format json` carries the same thing as `reason` (a stable code: `recorded`, `record-blocks`, `multiple-boundaries`, `undefined-origin`, `beyond-record-ceiling`, `no-record`, `no-boundary-crossed`, `downgrade`, `not-comparable`) plus `explanation`, the sentence naming your versions. diff --git a/docs/user/component-catalog.md b/docs/user/component-catalog.md index 654d22e4eb..0c47b9d8fc 100644 --- a/docs/user/component-catalog.md +++ b/docs/user/component-catalog.md @@ -1464,8 +1464,43 @@ requires, at minimum: 4. Deployment-phase validation passes on a live cluster carrying `nodewright-customizations`. +Upstream's own account of the rename is +[`docs/getting-started/migration.md`](https://github.com/NVIDIA/nodewright/blob/main/docs/getting-started/migration.md), +which sets a hard operational prerequisite for the upgrade itself: +[every `Skyhook` must be `complete` with no nodes in progress](https://github.com/NVIDIA/nodewright/blob/main/docs/getting-started/migration.md#prerequisite-all-skyhooks-must-be-complete) +before the operator is upgraded. It is a requirement rather than a +recommendation — the migration relabels the operator's package and per-node +ConfigMaps so the post-rename operator adopts them, and that flow assumes no +in-flight package work to disrupt. `paused` and `disabled` objects are fine to +leave as they are. Check with: + +```bash +kubectl get skyhooks.skyhook.nvidia.com \ + -o custom-columns=NAME:.metadata.name,STATUS:.status.status,INPROGRESS:.status.nodesInProgress +``` + Tracked in [#2593](https://github.com/NVIDIA/aicr/issues/2593) and -[#2594](https://github.com/NVIDIA/aicr/issues/2594). Once the pin moves, this -entry becomes a transition record at -`recipes/components/nodewright-operator/upgrades.yaml` and `aicr upgrade-check` -reports it directly. +[#2594](https://github.com/NVIDIA/aicr/issues/2594). + +`aicr upgrade-check` reports all of this. The transition record at +`recipes/components/nodewright-operator/upgrades.yaml` describes the `v0.18.0` +boundary itself — the rename, the prerequisite above, and per-deployer steps — +and stops its `to` ceiling there. So crossing `v0.18.0` is `manual` with steps, +while any target above it is `blocked` and told to take the rename on its own: + +```console +$ aicr upgrade-check --from old.yaml --to new.yaml --deployer helm +COMPONENT FROM TO VERDICT NOTES +nodewright-operator v0.17.1 v0.18.0 manual 1 minor, 5 steps + +$ aicr upgrade-check --from old.yaml --to newer.yaml --deployer helm +COMPONENT FROM TO VERDICT NOTES +nodewright-operator v0.17.1 v0.19.0 blocked 2 minors, stops at >=0.18.0 <=0.18.0 +``` + +The record sits above the pin deliberately. Only a `safe` verdict is held to the +pinned version, so upgrade guidance can be written before the bump it describes +— which is the order that qualifies a bump in the first place. The steps +therefore tell you how upstream's migration works *and* that AICR's own +readiness gate does not yet survive it; the prerequisites above are what moving +the pin needs. diff --git a/pkg/upgrade/wellformed.go b/pkg/upgrade/wellformed.go index 51307b0a86..e363e59bc4 100644 --- a/pkg/upgrade/wellformed.go +++ b/pkg/upgrade/wellformed.go @@ -250,6 +250,23 @@ func checkStepGroups(where string, t *Transition) []string { // checkPinCeiling implements rule 2. It fires per transition, so a record // carrying only a replaces block is untouched: it has no `to` to compare. // +// Only safe is held to the pin. ADR-021 originally held every verdict to it, +// reasoning that an author cannot have read the migration notes for a version +// nobody has released. That reasoning covers safe and only safe: safe is the +// vouching verdict, and vouching past what AICR ships is the false-confidence +// failure the ADR exists to prevent. manual and blocked are warnings carrying +// instructions, and holding those to the pin forced the wrong order of work — +// bump first, document after — when the order that actually qualifies an +// upgrade is to read the migration notes, write the record, then bump. A +// component AICR deliberately holds below a known-breaking release is the case +// that exposed this: the record most worth having described the release AICR +// would not ship, so the rule forbade exactly the guidance operators needed. +// Over-warning is the safe direction; a manual or blocked record reaching +// forward can never read as a pass. +// +// The obligation stays self-renewing without this: rule 3 fails any bump that +// leaves a from gap below the new pin, which is what forces a record back open. +// // The non-comparable-pin failure is an addition to ADR-021 rather than a // transcription of it. The ADR assigns such a pin the unversioned verdict at // check time and does not make it an authoring error; failing closed here means @@ -287,6 +304,11 @@ func checkPinCeiling(where string, t *Transition, pin string) []string { if b.upper.unbounded { return v } + // Everything below compares the ceiling against the pin, which only a + // vouching verdict owes an answer to. + if t.Verdict != VerdictSafe { + return v + } if strings.Contains(pin, "+") { return append(v, fmt.Sprintf( "%s is pinned at %q, which carries build metadata; semver orders build metadata as equal, so such a bump would move past no ceiling", @@ -300,7 +322,7 @@ func checkPinCeiling(where string, t *Transition, pin string) []string { } if b.upper.ver.Compare(pinVer) > 0 { v = append(v, fmt.Sprintf( - "%s has a to ceiling of %s which reaches past the pinned version %s; widen from backward instead", + "%s is safe with a to ceiling of %s which reaches past the pinned version %s; widen from backward instead, or say manual or blocked if this is guidance written ahead of the bump", where, b.upper.ver, pinVer)) } return v diff --git a/pkg/upgrade/wellformed_test.go b/pkg/upgrade/wellformed_test.go index 21d25abed1..8d0e686a11 100644 --- a/pkg/upgrade/wellformed_test.go +++ b/pkg/upgrade/wellformed_test.go @@ -410,29 +410,47 @@ func TestValidatePinCeiling(t *testing.T) { name string to string pin string + verdict Verdict wantErr bool wantText string }{ - {"ceiling equals the pin", ">=0.18.0 <=0.18.0", "v0.18.0", false, ""}, - {"ceiling below the pin", ">=0.17.0 <=0.17.9", "v0.18.0", false, ""}, - {"ADR ordinary idiom", ">=25.0.0 <=25.3.0", "v25.3.0", false, ""}, - {"ceiling above the pin", ">=0.18.0 <=0.20.0", "v0.18.0", true, "reaches past"}, - {"exclusive ceiling above the pin", ">=0.18.0 <0.20.0", "v0.18.0", true, "reaches past"}, - {"unbounded above fails", ">=0.18.0", "v0.18.0", true, "upper bound"}, - {"unbounded below fails", "<=0.18.0", "v0.18.0", true, "lower bound"}, - {"non-semver pin fails", ">=0.18.0 <=0.18.0", "main", true, "not a comparable version"}, - {"commit sha pin fails", ">=0.18.0 <=0.18.0", "9f8e7d6c5b4a", true, "not a comparable version"}, - {"build metadata pin fails", ">=0.18.0 <=0.18.0", "v0.18.0+build.5", true, "build metadata"}, + {"ceiling equals the pin", ">=0.18.0 <=0.18.0", "v0.18.0", VerdictSafe, false, ""}, + {"ceiling below the pin", ">=0.17.0 <=0.17.9", "v0.18.0", VerdictSafe, false, ""}, + {"ADR ordinary idiom", ">=25.0.0 <=25.3.0", "v25.3.0", VerdictSafe, false, ""}, + {"safe above the pin", ">=0.18.0 <=0.20.0", "v0.18.0", VerdictSafe, true, "reaches past"}, + {"safe with an exclusive ceiling above the pin", + ">=0.18.0 <0.20.0", "v0.18.0", VerdictSafe, true, "reaches past"}, + // Guidance written ahead of the bump. Only safe vouches, so only safe + // is held to the pin; a warning reaching forward cannot read as a pass. + {"manual above the pin", ">=0.18.0 <=0.20.0", "v0.17.1", VerdictManual, false, ""}, + {"blocked above the pin", ">=0.18.0 <=0.20.0", "v0.17.1", VerdictBlocked, false, ""}, + {"manual against a non-semver pin", ">=0.18.0 <=0.18.0", "main", VerdictManual, false, ""}, + {"manual against a build-metadata pin", + ">=0.18.0 <=0.18.0", "v0.18.0+build.5", VerdictManual, false, ""}, + // The `to` shape itself is not a claim about the pin, so these hold + // whatever the verdict is. + {"unbounded above fails", ">=0.18.0", "v0.18.0", VerdictManual, true, "upper bound"}, + {"unbounded below fails", "<=0.18.0", "v0.18.0", VerdictManual, true, "lower bound"}, + {"non-semver pin fails", ">=0.18.0 <=0.18.0", "main", VerdictSafe, true, "not a comparable version"}, + {"commit sha pin fails", + ">=0.18.0 <=0.18.0", "9f8e7d6c5b4a", VerdictSafe, true, "not a comparable version"}, + {"build metadata pin fails", + ">=0.18.0 <=0.18.0", "v0.18.0+build.5", VerdictSafe, true, "build metadata"}, {"prerelease pin with matching prerelease ceiling passes", - ">=0.1.0-alpha.1 <=0.1.0-alpha.12", "v0.1.0-alpha.12", false, ""}, + ">=0.1.0-alpha.1 <=0.1.0-alpha.12", "v0.1.0-alpha.12", VerdictSafe, false, ""}, {"prerelease pin with release ceiling fails", - ">=0.1.0-alpha.1 <=0.1.0", "v0.1.0-alpha.12", true, "reaches past"}, + ">=0.1.0-alpha.1 <=0.1.0", "v0.1.0-alpha.12", VerdictSafe, true, "reaches past"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { set, comps := rec(tt.pin, tr(func(x *Transition) { x.To = tt.to x.From = "<0.0.1" + x.Verdict = tt.verdict + if tt.verdict == VerdictSafe { + x.VerifiedBy = "the UAT lane" + x.StepsByDeployer = nil + } })) err := set.Validate(comps) if got := mentions(err, rule2Fragments...); got != tt.wantErr { @@ -949,6 +967,9 @@ func TestValidatePinCeilingRejectsReleaseCeilingAtPrereleasePin(t *testing.T) { set, comps := rec("v0.1.0-alpha.12", tr(func(x *Transition) { x.From = "<0.1.0" x.To = ">=0.1.0 <=0.1.0" + x.Verdict = VerdictSafe + x.VerifiedBy = "the UAT lane" + x.StepsByDeployer = nil })) err := set.Validate(comps) if !mentions(err, rule2Fragments...) { diff --git a/recipes/components/nodewright-operator/upgrades.yaml b/recipes/components/nodewright-operator/upgrades.yaml new file mode 100644 index 0000000000..13f586fc01 --- /dev/null +++ b/recipes/components/nodewright-operator/upgrades.yaml @@ -0,0 +1,170 @@ +# Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# This record describes a boundary above the current pin, which rule 2 permits +# for manual and blocked because only safe vouches. AICR holds the pin at +# v0.17.1 until #2593 and #2594 land, and the guidance for crossing v0.18.0 is +# worth having before the bump rather than after it: reading the migration +# notes is what qualifies the bump in the first place. +# +# The ceiling is v0.18.0 itself, so a jump from below it to v0.19.0 or beyond +# reports blocked and is told to stop at v0.18.0 — the rename has to be crossed +# on its own, not composed with whatever follows it. +# +# Transcribed from NVIDIA/nodewright docs/getting-started/migration.md, which +# remains the authoritative account. AICR's own reasons for holding are +# "Upgrade Notes" > nodewright-operator in docs/user/component-catalog.md. +apiVersion: aicr.run/v1beta1 +kind: ComponentUpgrades +component: nodewright-operator +transitions: + - from: "<0.18.0" + to: ">=0.18.0 <=0.18.0" + verdict: manual + summary: >- + v0.18.0 renames skyhook.nvidia.com/v1alpha1 Skyhook to + nodewright.nvidia.com/v1alpha1 NodeWright, moves DeploymentPolicy to the + same group, and shifts the on-node annotation, label and finalizer + prefix. An operator-side mirror migrates the objects for you, but + completion status is then written only on the new kind and the attempt + to mirror it back to the legacy object fails in a reconcile conflict + loop, so Skyhook.status stays empty forever. AICR's deployment readiness + gate and the nodewright-customizations health check both poll the legacy + kind, so they wait on a status that never populates and time out on a + cluster where tuning has genuinely finished. + precondition: >- + Every Skyhook is complete with no nodes in progress. Upstream makes this + a requirement rather than a recommendation: the migration relabels the + operator's package and per-node ConfigMaps so the post-rename operator + adopts them instead of recreating them, and that flow assumes no + in-flight package work to disrupt. Check with `kubectl get + skyhooks.skyhook.nvidia.com -o + custom-columns=NAME:.metadata.name,STATUS:.status.status,INPROGRESS:.status.nodesInProgress` + and proceed only when nothing is actively rolling out. paused and + disabled objects are fine to leave as they are — the mirror carries that + state onto the NodeWright — but do not unpause or enable one until its + pre-upgrade package pods are gone. + reversible: false + affectedResources: + - group: skyhook.nvidia.com + kinds: + - Skyhook + - DeploymentPolicy + stepsByDeployer: + - deployers: [argocd, argocd-helm] + steps: + - id: upgrade-operator + description: >- + Bump the operator's chart version in its own Application and + sync it, before touching any CR. + reason: >- + The chart ships both groups' CRDs and RBAC, and the mirror + imports each Skyhook into a NodeWright. Per-node state is copied + to the nodewright.nvidia.com/* prefix, so packages are not + re-run. The new objects are untracked by Argo at this point; + it neither prunes nor flags them. + - id: rename-crs-in-one-commit + description: >- + In a single git commit, remove each Skyhook manifest and add the + NodeWright equivalent: apiVersion skyhook.nvidia.com/v1alpha1 + becomes nodewright.nvidia.com/v1alpha1, kind Skyhook becomes + NodeWright, metadata.name is unchanged. DeploymentPolicy changes + apiVersion only. Then sync. + reason: >- + Argo adopts the mirror-created NodeWright and prunes the Skyhook + in one sync. Leaving both in git opens a window where the mirror + and Argo both write the NodeWright and a stale Skyhook re-import + stomps the git edit. Rewrite apiVersion and kind only: a blanket + substitution also rewrites nodeSelectors and + podNonInterruptLabels, which name your labels rather than the + operator's, and pointing them at keys nothing carries makes the + CR match no node. + - id: verify-migration + description: >- + kubectl get nodewrights.nodewright.nvidia.com lists a NodeWright + for each former Skyhook, node annotations are present under the + nodewright.nvidia.com/ prefix, and no package re-ran. + reason: >- + Confirms the mirror adopted live lifecycle position rather than + handing the new object a fresh rollout. + - id: expect-aicr-readiness-to-fail + description: >- + Do not read a failing AICR deployment phase on this cluster as a + migration error. Until #2593 and #2594 land, the readiness gate + and the nodewright-customizations health check poll the legacy + Skyhook and will time out. + reason: >- + This is why AICR pins v0.17.1 and why this record's ceiling + stops here. Crossing it is supported by upstream, not yet by + AICR's own validation path. + - steps: + - id: upgrade-operator + description: >- + helm upgrade the operator chart (flux: reconcile the operator's + HelmRelease onto the new chart version), before touching any CR. + reason: >- + The chart ships both groups' CRDs and RBAC, and the mirror + imports each Skyhook into a NodeWright. Per-node state is copied + to the nodewright.nvidia.com/* prefix, so packages are not + re-run. + - id: rename-crs + description: >- + Rewrite your CR manifests and apply them: + sed -e '/^ *apiVersion:/ s|skyhook\.nvidia\.com/|nodewright.nvidia.com/|' + -e 's|^\( *kind: *\)Skyhook[[:space:]]*$|\1NodeWright|' + my-skyhook.yaml > my-nodewright.yaml + reason: >- + The mirror already created the object, so this applies as a + no-op adoption rather than a fresh create. Rewrite apiVersion + and kind only: a blanket substitution also rewrites + nodeSelectors and podNonInterruptLabels, which name your labels + rather than the operator's, and that turns a no-op adoption into + a real spec change against keys nothing carries. + - id: delete-legacy-crs + description: >- + Once the NodeWright exists and reconciles: kubectl delete + skyhook.skyhook.nvidia.com , then kubectl delete + deploymentpolicy.skyhook.nvidia.com . + reason: >- + Skyhooks must go before the DeploymentPolicy they reference. The + NodeWright survives, because the mirror never deletes it. Legacy + per-node state and pre-rename package pods are kept for the + LEGACY_CLEANUP_DELAY rollback window and pruned later, not now. + Do not remove the skyhook.nvidia.com CRD itself until every + legacy object is gone, since that cascade-deletes whatever + remains. + - id: verify-migration + description: >- + kubectl get nodewrights.nodewright.nvidia.com lists a NodeWright + for each former Skyhook, node annotations are present under the + nodewright.nvidia.com/ prefix, and no package re-ran. + reason: >- + Confirms the mirror adopted live lifecycle position rather than + handing the new object a fresh rollout. + - id: expect-aicr-readiness-to-fail + description: >- + Do not read a failing AICR deployment phase on this cluster as a + migration error. Until #2593 and #2594 land, the readiness gate + and the nodewright-customizations health check poll the legacy + Skyhook and will time out. + reason: >- + This is why AICR pins v0.17.1 and why this record's ceiling + stops here. Crossing it is supported by upstream, not yet by + AICR's own validation path. + references: + - https://github.com/NVIDIA/nodewright/blob/main/docs/getting-started/migration.md + - https://github.com/NVIDIA/nodewright/blob/main/docs/getting-started/migration.md#prerequisite-all-skyhooks-must-be-complete + - https://github.com/NVIDIA/aicr/blob/main/docs/user/component-catalog.md#nodewright-operator-staying-on-v017x + - https://github.com/NVIDIA/aicr/issues/2593 + - https://github.com/NVIDIA/aicr/issues/2594 diff --git a/recipes/registry.yaml b/recipes/registry.yaml index 4a417cda0f..ed0df288f0 100644 --- a/recipes/registry.yaml +++ b/recipes/registry.yaml @@ -308,6 +308,8 @@ components: - skyhook healthCheck: assertFile: checks/nodewright-operator/health-check.yaml + upgrades: + file: components/nodewright-operator/upgrades.yaml helm: defaultRepository: oci://ghcr.io/nvidia/nodewright/charts defaultChart: nodewright From 8bb84340321b47e613c2327b42fc98804772e8d9 Mon Sep 17 00:00:00 2001 From: Alex Yuskauskas Date: Thu, 17 Sep 2026 17:02:53 -0700 Subject: [PATCH 2/2] refactor(upgrade): write the nodewright to range as =0.18.0 ">=0.18.0 <=0.18.0" is a degenerate range only 0.18.0 satisfies, written the long way. parseBounds gives "=" the identical inclusive lower and upper bounds, so this is the same boundary said plainly, and the blocked report now reads "stops at =0.18.0" instead of restating both comparators. Signed-off-by: Alex Yuskauskas --- docs/user/component-catalog.md | 2 +- recipes/components/nodewright-operator/upgrades.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/user/component-catalog.md b/docs/user/component-catalog.md index 0c47b9d8fc..3a89dac3c5 100644 --- a/docs/user/component-catalog.md +++ b/docs/user/component-catalog.md @@ -1495,7 +1495,7 @@ nodewright-operator v0.17.1 v0.18.0 manual 1 minor, 5 steps $ aicr upgrade-check --from old.yaml --to newer.yaml --deployer helm COMPONENT FROM TO VERDICT NOTES -nodewright-operator v0.17.1 v0.19.0 blocked 2 minors, stops at >=0.18.0 <=0.18.0 +nodewright-operator v0.17.1 v0.19.0 blocked 2 minors, stops at =0.18.0 ``` The record sits above the pin deliberately. Only a `safe` verdict is held to the diff --git a/recipes/components/nodewright-operator/upgrades.yaml b/recipes/components/nodewright-operator/upgrades.yaml index 13f586fc01..a5b32cad65 100644 --- a/recipes/components/nodewright-operator/upgrades.yaml +++ b/recipes/components/nodewright-operator/upgrades.yaml @@ -30,7 +30,7 @@ kind: ComponentUpgrades component: nodewright-operator transitions: - from: "<0.18.0" - to: ">=0.18.0 <=0.18.0" + to: "=0.18.0" verdict: manual summary: >- v0.18.0 renames skyhook.nvidia.com/v1alpha1 Skyhook to