feat: rename the Skyhook API to NodeWright with a rollback-safe migration bridge - #349
Merged
Conversation
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
…ation base Introduce the nodewright.nvidia.com/v1alpha1 API group (NodeWright, DeploymentPolicy) as a fork of the legacy skyhook.nvidia.com types, plus the operator-side migration scaffolding: - typed field-by-field conversion (api/v1alpha1/nodewright_conversion.go) with exhaustive fidelity + reflection-based zero-value-guard tests - new-group validating/mutating webhooks - a level-triggered mirror controller importing legacy Skyhook/DeploymentPolicy objects into the new group (read-only on source, ownership-stamped, never deletes the target) - deprecation warnings on legacy Skyhook/DeploymentPolicy writes - migration guide (docs/nodewright-migration.md) Shared base for the rename. Reconciler repoint, CLI, chart sync, and the nodewright type generator (re-fork automation) follow as separate changes. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
- use reflect.Pointer instead of the deprecated reflect.Ptr in the conversion zero-value guard (govet inline check) - fix markdownlint in docs/nodewright-migration.md: merge adjacent blockquotes (MD028) and repoint the dead #timing anchor to the renamed prerequisite section (MD051) Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The mirror controller creates nodewright.nvidia.com objects, so the operator needs the new-group CRDs installed and RBAC to write them. Add the two nodewright CRDs to config/crd/kustomization.yaml and kubebuilder:rbac markers on the mirror reconciler (regenerating config/rbac/role.yaml). Without this a deployed operator's mirror is denied / hits a missing CRD. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Add scripts/gen_nodewright.sh, which re-derives the nodewright.nvidia.com API group (types + webhooks) from the legacy skyhook.nvidia.com source via a deterministic rename transform, dropping the legacy-only deprecation warning that a pure rename would wrongly carry onto the new group. Wire it into 'make generate'/'make manifests' so the frozen mirror can never silently drift from the legacy schema again (as it did with DrainConfig and the image-parsing rewrite). 'make verify-nodewright-gen' fails if the checked-in copy is stale. Generated files carry a DO-NOT-EDIT marker; tests stay hand-maintained (test drift fails loudly in the suites). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Repoint the operator's primary reconcile path from skyhook.nvidia.com/Skyhook to nodewright.nvidia.com/NodeWright. Swapping the api import in the reconcile-path packages (dal, wrapper, controller, drain) flips METADATA_PREFIX to nodewright.nvidia.com, so per-node state annotations/labels/finalizers move to the new prefix, and the primary Watches now targets NodeWright. Mocks regenerated. Unit + envtest suites updated and green (build clean, lint 0 issues). Does NOT yet migrate the chainsaw e2e tests (they still create Skyhook objects), so this lands on a feature branch off the api base, not the base itself, until e2e is migrated. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
- register NodeWright and DeploymentPolicy webhook handlers in main.go so the fail-closed nodewright.nvidia.com webhooks accept writes (including mirror-created objects) instead of rejecting them - PackageStatus.Equal now compares Image, matching its doc contract - batch-size strategies return 0 when no nodes remain - wrap compartment validation errors with the compartment identity - strengthen the deprecation-warning update test - mark not-yet-shipped pieces in the migration guide as planned The nodewright/v1alpha1 changes are regenerated from the legacy source via make generate-nodewright, not hand-edited. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
… API The operator reconciles NodeWright and writes nodewright.nvidia.com/* annotations, so the CLI and the e2e suite are moved to match: - flip the operator finalizer to nodewright.nvidia.com/nodewright - repoint the kubectl plugin to the nodewright.nvidia.com group and the NodeWright kind; it now reads and writes nodewright.nvidia.com/* node annotations - migrate the skyhook and deployment-policy chainsaw pools to NodeWright, preserving the runtime-required taint key, the skyhook namespace, the agent image, SKYHOOK_* env vars, and skyhook_* metric names - add a mirror-legacy-skyhook e2e test covering the Skyhook -> NodeWright mirror bridge BREAKING CHANGE: the CLI now targets nodewright.nvidia.com and no longer works against operators that only serve the legacy skyhook.nvidia.com group and Skyhook kind. Upgrade the operator to a NodeWright-capable version before using this CLI. See operator/cmd/cli/RELEASE_NOTES.md and docs/cli.md. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…egacy Skyhooks complete On upgrade from the pre-rename operator, two things protect existing state: - node.Migrate now re-keys a pre-rename node's skyhook.nvidia.com/* annotations, labels, and condition types to the nodewright.nvidia.com/* prefix before the stored version/state are read, so an upgraded node is adopted instead of being treated as fresh and re-running every package. Idempotent and collision-safe; the bare runtime-required taint key is left untouched. - the NodeWright reconcile holds (requeue 20s) with a loud warning while any legacy Skyhook status is set and not complete, so the operator never takes over a node the pre-rename operator may still be mutating. Empty-status legacy Skyhooks (created post-upgrade and mirrored) do not trigger the hold. This is a fail-safe stop, not an auto-resume: the hold clears only when the legacy Skyhooks read complete. Documents the runtime hold in the migration guide. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
- migrate: reads legacy Skyhook/DeploymentPolicy from -f/stdin (offline) or the cluster and emits converted nodewright.nvidia.com YAML via the api converters, stripping status/server fields and rekeying metadata, for kubectl apply / git. - preflight: cluster commands now detect via discovery when only the legacy skyhook.nvidia.com group is served and fail with an actionable error naming the required NodeWright-capable operator, instead of a confusing NotFound. migrate and version are exempt (migrate must read the legacy group by design). Documents both in docs/cli.md and the CLI RELEASE_NOTES. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…kdownlint - setup-kind-cluster now labels the e2e worker with both skyhook.nvidia.com/test-node and nodewright.nvidia.com/test-node. The chainsaw skyhook and deployment-policy pools were migrated to select the nodewright label, but the node was still only labelled with the legacy key, so those NodeWrights matched no node and the operator never acted (every node/status assert timed out). The legacy label stays for the not-yet-migrated cli pool; drop it when that pool moves. - merge the two intro blockquotes in the migration guide so there is no blank line inside a blockquote (markdownlint MD028), and refresh the "scope of the current release" list now that the migrate command, prefix re-keying, and runtime hold ship. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
versions.yaml pins kind.binary v0.32.0, but the ctlptl pin was left at v0.9.3,
which predates kind v0.32.0 and rejects it ("unsupported Kind version v0.32.0")
when creating the local kind cluster. v0.9.4 vendored the newer kind support and
is backward compatible, so create-kind-cluster works against the pinned kind again.
Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Bump kind.binary to v0.32.0 so the pinned kind matches the ctlptl v0.9.4 bump (v0.9.3 rejected v0.32.0). Node image stays 1.35.0 (the v1.36 image has local boot issues; CI uses primaryKindNodeImage 1.35.0 regardless). Also normalize the license header to the SPDX form the license check expects. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…hook drift verify-nodewright-gen only diffed api/nodewright, but generate-nodewright intentionally excludes zz_generated.deepcopy.go and does not touch the CRDs, RBAC, or webhook config. A legacy type change could therefore pass verification with a stale deepcopy or stale generated manifests. Depend on manifests + generate and diff the full generated surface so drift in any of them fails the check. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…vent kind The chainsaw->NodeWright transform missed `kubectl annotate` (not in its verb list), so the delete-blocked-when-paused/disabled tests annotated the legacy `skyhook` resource, which no longer exists for these NodeWright-based tests, so the pause/disable was never applied and deletion was not blocked. Also flip the DeletionBlocked event field-selector from involvedObject.kind=Skyhook to NodeWright, since the operator records that event on the NodeWright object. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…li e2e pool Completes the CLI side of the Skyhook -> NodeWright rename: - the plugin is invoked as `kubectl nodewright` (binary bin/nodewright, built by make build-cli); the version banner, root description, and docs/cli.md examples use NodeWright. The Makefile build/coverage/symlink plumbing and every chainsaw test's `../skyhook-cli` reference move to `nodewright-cli`. - the cli chainsaw pool is migrated to NodeWright (create NodeWright, assert nodewright.nvidia.com/* metadata, point kubectl resource references at nodewright) so it is consistent with the nodewright-only CLI. The skyhook namespace, agent image, and runtime-required taint key are unchanged. BREAKING CHANGE: the CLI is now invoked as `kubectl nodewright` instead of `kubectl skyhook`; reinstall the plugin and update scripts/aliases. Recorded in operator/cmd/cli/RELEASE_NOTES.md. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The build-created nodewright-cli symlinks were accidentally committed; ignore them like the old skyhook-cli symlinks (make ensure-test-symlinks recreates them). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…gration After the rename the reconcile path only queries nodewright.nvidia.com labels, so package pods and per-node metadata ConfigMaps the pre-rename operator stamped with skyhook.nvidia.com labels would be orphaned. HandleMigrations now graceful-deletes the legacy-labeled pods (respawned under the new labels; a still-running package shuts down cleanly first) and relabels the reused per-node ConfigMaps to the nodewright prefix. Level-triggered and idempotent. Tagged MIGRATION-SHIM so it is removed with the legacy group in the removal release. Envtest covers deletion, scoping, relabel, and idempotence. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…elease
The legacy skyhook.nvidia.com surface (legacy api/v1alpha1 package + converters,
the mirror controller, the node-prefix and workload migrations, the legacy
webhook names/rules, and the legacy CRDs/RBAC/webhook entries in the chart) is
kept only for the transition and deleted in the adoption-gated removal release.
Tag every such site with a MIGRATION-SHIM comment so `grep -rn MIGRATION-SHIM
operator chart` is the removal checklist. Chart markers use {{/* */}} so they do
not render. Comments only; no behavior change.
Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
Rename the product/CR/operator name and API surface (kind: NodeWright, nodewright.nvidia.com group, node annotation/condition keys) in README, the domain docs, and docs/cli.md, and add the BREAKING rename note to the operator RELEASE_NOTES linking docs/nodewright-migration.md. Preserves what did not move: the skyhook install namespace, SKYHOOK_* env vars, /var/lib/skyhook paths, skyhook_* metric names, the skyhook-packages images, and legacy skyhook.nvidia.com references that intentionally name the old group. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The CR is now NodeWright, so the CLI's --skyhook flag becomes --nodewright. --skyhook still works as a deprecated back-compat alias (prints a migration warning, hidden from help, required commands accept either, both is an error) via a shared utils.RegisterNodeWrightNameFlag helper. Rename the CLI's examples, <nodewright-name> positionals, flag help, and prose/output to NodeWright, and fix operator-version discovery, which matched the operator by "skyhook" in the image name (now nodewright/operator) and only worked via a legacy label fallback; it now matches nodewright and skyhook. migrate.go keeps its legacy Skyhook references. Updates the go unit tests + cli-e2e (--skyhook -> --nodewright) and adds tests for the alias helper and operator detection. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
- Webhooks: use the request-scoped logger (logf.FromContext) instead of a package-global logger; drop the unintended (?m) multiline flag from the package-name regexp so embedded newlines are rejected; wrap validation errors with %w. Applied to the legacy api/v1alpha1 sources and regenerated the api/nodewright/v1alpha1 mirrors. - nodewright_conversion: make skyhook->nodewright metadata re-keying deterministic when both the legacy and new key are present (explicit nodewright.nvidia.com/* entries win). - mirror_controller_test: restore Gomega's default Consistently duration in AfterEach so the widened window does not leak into later specs. - config/crd/bases: add the missing Apache/SPDX license headers (via license-fmt). - groupversion_info: drop the MIGRATION-SHIM comment from the legacy source; the gen script mirrors source comments into the generated (permanent) nodewright package, where that marker is wrong. The legacy package removal is still tracked via the MIGRATION-SHIM in nodewright_conversion.go. - docs/nodewright-migration.md: correct the guide to match what this release actually ships (node-state migration, workload cleanup, and kubectl nodewright migrate are implemented; only the removal-release pre-upgrade guard is future). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The per-node state migration dropped the legacy skyhook.nvidia.com keys the moment a Skyhook completed, so rolling back to the pre-rename operator lost node state and re-applied every package. Migration now converges additively (copies legacy metadata to the nodewright prefix, keeps the legacy copies and pods) and defers the destructive prune until LEGACY_CLEANUP_DELAY (default 24h) elapses, tracked by a nodewright.nvidia.com/legacy-migrated-at stamp on the NodeWright. Set the delay to 0 to prune immediately. Also fixes correctness issues found in review: - the mirror now strips the stranded skyhook.nvidia.com/skyhook finalizer, and its watch predicate delivers the deletion transition, so deleting a legacy Skyhook no longer hangs forever on a finalizer nothing owns - the migration hold treats a missing legacy CRD as "no legacy objects" rather than holding forever, so the operator does not wedge once the legacy CRD is removed - the mirror re-sync preserves target-only control annotations (pause, disable, legacy-migrated-at) instead of clobbering them wholesale Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…ring migration Once the post-rename operator is running, a legacy Skyhook/DeploymentPolicy is a deprecated snapshot and its mirrored nodewright.nvidia.com object is the single writable source of truth. Make that explicit. - The legacy admission webhooks now reject any spec or pause/disable change to a legacy object. Deletions, finalizer edits, and identical re-applies are still allowed, so a steady-state GitOps sync is not broken; a real edit is rejected and forces the rename. The reject logic lives in a file that is never mirrored into the nodewright group, so the NodeWright webhook stays writable and a stray manual regenerate fails to compile rather than silently freezing NodeWright. - The migration hold no longer waits on paused/disabled Skyhooks. It holds only on genuinely in-flight rollouts (in_progress, erroring, blocked, waiting, unknown); complete/paused/disabled migrate in that state (the mirror carries pause/disable onto the NodeWright, which then does not roll out). Users are no longer forced to unpause or re-enable a Skyhook to migrate it. - Stop auto-running scripts/gen_nodewright.sh from make (kept as a manual target). The migration schemas have converged and the legacy group is frozen, so NodeWright is now the hand-maintained source and can evolve independently. - Move the downgrade/upgrade/uninstall validation test coverage to the NodeWright webhook, where that logic is now live and writable (it previously had none). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
From a local CodeRabbit CLI review of the full diff (code, docs, chart, k8s-tests, and Go tests): - cli-release-build archived the wrong binary: it builds nodewright_* but the tar/zip step still cp'd the nonexistent skyhook_*, which would break the CI release build. - Finalizer/deletion race: reconcile wrote NodeWright status AFTER removing the last finalizer, racing the apiserver delete into a spurious NotFound. Write status before removing the finalizer. - Admission now validates podNonInterruptLabels (an invalid selector was admitted and then failed later in controllers). - Migration hold honors the pause/disable annotations directly, so a stale in_progress status no longer wedges a paused/disabled Skyhook. The read-only no-op check uses apiequality.Semantic.DeepEqual so a re-serialized resource.Quantity is not a false-positive edit; read-only rejections keep the deprecation warning; preflight tolerates partial discovery; migrate closes each input file per iteration. - Legacy read-only webhooks no longer re-validate a frozen object on an allow-listed update. A finalizer strip or GitOps no-op re-apply of a migrated Skyhook/DeploymentPolicy went on to run Validate()/validateDeploymentPolicyExists, which could reject the update (e.g. once a stricter rule ships or the referenced legacy DeploymentPolicy is gone) and strand the object in Terminating. The frozen object is now gated on the read-only check alone; all substantive validation runs on the writable NodeWright, and the now-dead legacy uninstall/downgrade path is removed. - Mirror controller watches its target kind: once a legacy object is frozen read-only its generation never changes again, so the legacy-source watch alone cannot recover from an out-of-band deletion of a mirror-owned NodeWright or DeploymentPolicy. A target-deletion watch re-enqueues and rebuilds the bridge without waiting for a controller restart. - Maintenance hook jobs pin kubernetes.io/os=linux; NodeWright CRD descriptions, user-facing strings, CLAUDE.md, docs, and RELEASE_NOTES use NodeWright naming and reflect the read-only behavior. Tests: exact-version, quantity-equality, stale-status pause, and mirror target-recreation regressions; resource-override and webhook-rule cases de-aliased. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
enabled auto-merge (squash)
July 21, 2026 19:54
ayuskauskas
previously approved these changes
Jul 21, 2026
Coverage Report for CI Build 29788416275Warning No base build found for commit Coverage: 78.22%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
The Skyhook -> NodeWright conversion is mechanical: the spec body is unchanged,
so migration is just an apiVersion group swap plus, for Skyhook, a kind rename
(DeploymentPolicy keeps its kind), and a rewrite of any skyhook.nvidia.com/*
metadata keys to nodewright.nvidia.com/*. The operator's mirror controller
already converts the live cluster objects automatically; the only remaining need
is to move source manifests forward, which a one-line sed handles:
sed -e 's|skyhook\.nvidia\.com/|nodewright.nvidia.com/|g' \
-e 's|kind: Skyhook|kind: NodeWright|' skyhook.yaml
So the CLI migrate command carried its weight only as a convenience. Remove it
(command, tests, wiring) and document the sed in docs/cli.md and the migration
guide. The legacy deprecation warnings now state the swap inline instead of
pointing at the removed command.
Also folds in two CodeRabbit CLI-review fixes:
- preflight: when the nodewright group itself is the casualty of a partial
discovery failure, return the discovery error instead of the misleading
"upgrade to a NodeWright-capable operator" message (it is absent from the
group list even though it may well be served). Covered by new tests.
- lifecycle: run the served-group preflight before the --dry-run short-circuit,
matching node/package/reset, so a dry-run against a legacy-only operator does
not print "Would ..." for a NodeWright the cluster cannot serve.
Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
rice-riley
approved these changes
Jul 21, 2026
lockwobr
added a commit
that referenced
this pull request
Jul 21, 2026
…354) Rename the chainsaw/operator-agent fixture files and the chainsaw suite directory from skyhook to nodewright, following the operator API rename (#349). This is a filename/directory rename only; YAML resource bodies (metadata.name, condition types, annotation values) are left untouched. The two legacy-compatibility fixtures keep the skyhook name on purpose: chainsaw/nodewright/mirror-legacy-skyhook/ and deployment-policy/legacy-compatibility/skyhook.yaml exercise the read-only skyhook.nvidia.com path. Also updates everything that referenced the old paths so nothing dangles: same-dir chainsaw `file:` refs, per-test READMEs and comments, the operator Makefile e2e `--test-dir` and ensure-test-symlinks paths, and docs (ci-test-pools, k8s-tests readme, uninstall-enhancement link). An accidentally-committed __pycache__/*.pyc under the old suite dir is dropped (its new location is gitignored). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
6 tasks
lockwobr
added a commit
that referenced
this pull request
Jul 29, 2026
Adds k8s-tests/migration, the first test in the repo that performs a real operator upgrade, and fixes the three defects it found in the rename migration introduced by #349. The suite installs the last pre-rename release (chart/v0.17.1 at operator v0.17.0), runs a real package to completion, upgrades to the working tree, and asserts per-node state migrates without re-running packages. Existing coverage was unit and envtest only; nothing exercised state written by a previous operator, which is where all three defects lived. Defects found and fixed: * Every upgrade wedged on the package ConfigMap. The converge swept only skyhook.nvidia.com/skyhook-node-meta, but package ConfigMaps carry skyhook.nvidia.com/name. UpsertConfigmaps lists by the new nodewright.nvidia.com/name label, so it could not see the existing object, called Create, and failed permanently on AlreadyExists behind exponential backoff. Both legacy label keys now converge. * Deleting a migrated legacy Skyhook cascade-deleted the NodeWright's ConfigMaps. The converge relabelled them but left their ownerReferences on the legacy Skyhook, so the guide's own "delete the old CRs" step garbage collected the package and per-node metadata ConfigMaps out from under the live NodeWright. The converge now re-parents them onto the NodeWright. * The migration claimed the entire skyhook.nvidia.com/ prefix. It copied every prefixed node key to nodewright.nvidia.com/* and deleted every prefixed key at prune, so a user's own label under the prefix (the prefix is the product's domain name) was duplicated into the operator's namespace and then destroyed 24h after an unrelated upgrade. It now migrates only operator-owned keys and leaves everything else untouched. Docs: the documented sed one-liner for migrating manifests was a global prefix replace that also rewrote nodeSelectors and podNonInterruptLabels keys, which name the user's own nodes and pods rather than anything the operator owns; rewriting them silently selects zero nodes. Narrowed to apiVersion and kind. RELEASE_NOTES corrected: per-node state is copied with the legacy keys kept for the rollback window, not "re-keyed in place". CI: adds a migration row to the operator-ci tests matrix. It builds its cluster with ctlptl like helm-tests, since only ctlptl provides the local registry that serves the operator image under test. Gated by the existing ci-gate with no branch-protection change. Removes k8s-tests/time_travel_tests, which was unrunnable: it referenced a deleted directory, a stale branch, and a pre-scheme tag, and switched versions with git checkout over the working tree. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
added a commit
that referenced
this pull request
Jul 29, 2026
Adds k8s-tests/migration, the first test in the repo that performs a real operator upgrade, and fixes the three defects it found in the rename migration introduced by #349. The suite installs the last pre-rename release (chart/v0.17.1 at operator v0.17.0), runs a real package to completion, upgrades to the working tree, and asserts per-node state migrates without re-running packages. Existing coverage was unit and envtest only; nothing exercised state written by a previous operator, which is where all three defects lived. Defects found and fixed: * Every upgrade wedged on the package ConfigMap. The converge swept only skyhook.nvidia.com/skyhook-node-meta, but package ConfigMaps carry skyhook.nvidia.com/name. UpsertConfigmaps lists by the new nodewright.nvidia.com/name label, so it could not see the existing object, called Create, and failed permanently on AlreadyExists behind exponential backoff. Both legacy label keys now converge. * Deleting a migrated legacy Skyhook cascade-deleted the NodeWright's ConfigMaps. The converge relabelled them but left their ownerReferences on the legacy Skyhook, so the guide's own "delete the old CRs" step garbage collected the package and per-node metadata ConfigMaps out from under the live NodeWright. The converge now re-parents them onto the NodeWright. * The migration claimed the entire skyhook.nvidia.com/ prefix. It copied every prefixed node key to nodewright.nvidia.com/* and deleted every prefixed key at prune, so a user's own label under the prefix (the prefix is the product's domain name) was duplicated into the operator's namespace and then destroyed 24h after an unrelated upgrade. It now migrates only operator-owned keys and leaves everything else untouched. Docs: the documented sed one-liner for migrating manifests was a global prefix replace that also rewrote nodeSelectors and podNonInterruptLabels keys, which name the user's own nodes and pods rather than anything the operator owns; rewriting them silently selects zero nodes. Narrowed to apiVersion and kind. RELEASE_NOTES corrected: per-node state is copied with the legacy keys kept for the rollback window, not "re-keyed in place". CI: adds a migration row to the operator-ci tests matrix. It builds its cluster with ctlptl like helm-tests, since only ctlptl provides the local registry that serves the operator image under test. Gated by the existing ci-gate with no branch-protection change. Removes k8s-tests/time_travel_tests, which was unrunnable: it referenced a deleted directory, a stale branch, and a pre-scheme tag, and switched versions with git checkout over the working tree. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
added a commit
that referenced
this pull request
Jul 29, 2026
Adds k8s-tests/migration, the first test in the repo that performs a real operator upgrade, and fixes the three defects it found in the rename migration introduced by #349. The suite installs the last pre-rename release (chart/v0.17.1 at operator v0.17.0), runs a real package to completion, upgrades to the working tree, and asserts per-node state migrates without re-running packages. Existing coverage was unit and envtest only; nothing exercised state written by a previous operator, which is where all three defects lived. Defects found and fixed: * Every upgrade wedged on the package ConfigMap. The converge swept only skyhook.nvidia.com/skyhook-node-meta, but package ConfigMaps carry skyhook.nvidia.com/name. UpsertConfigmaps lists by the new nodewright.nvidia.com/name label, so it could not see the existing object, called Create, and failed permanently on AlreadyExists behind exponential backoff. Both legacy label keys now converge. * Deleting a migrated legacy Skyhook cascade-deleted the NodeWright's ConfigMaps. The converge relabelled them but left their ownerReferences on the legacy Skyhook, so the guide's own "delete the old CRs" step garbage collected the package and per-node metadata ConfigMaps out from under the live NodeWright. The converge now re-parents them onto the NodeWright. * The migration claimed the entire skyhook.nvidia.com/ prefix. It copied every prefixed node key to nodewright.nvidia.com/* and deleted every prefixed key at prune, so a user's own label under the prefix (the prefix is the product's domain name) was duplicated into the operator's namespace and then destroyed 24h after an unrelated upgrade. It now migrates only operator-owned keys and leaves everything else untouched. Docs: the documented sed one-liner for migrating manifests was a global prefix replace that also rewrote nodeSelectors and podNonInterruptLabels keys, which name the user's own nodes and pods rather than anything the operator owns; rewriting them silently selects zero nodes. Narrowed to apiVersion and kind. RELEASE_NOTES corrected: per-node state is copied with the legacy keys kept for the rollback window, not "re-keyed in place". CI: adds a migration row to the operator-ci tests matrix. It builds its cluster with ctlptl like helm-tests, since only ctlptl provides the local registry that serves the operator image under test. Gated by the existing ci-gate with no branch-protection change. Removes k8s-tests/time_travel_tests, which was unrunnable: it referenced a deleted directory, a stale branch, and a pre-scheme tag, and switched versions with git checkout over the working tree. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
added a commit
that referenced
this pull request
Jul 29, 2026
Adds k8s-tests/migration, the first test in the repo that performs a real operator upgrade, and fixes the three defects it found in the rename migration introduced by #349. The suite installs the last pre-rename release (chart/v0.17.1 at operator v0.17.0), runs a real package to completion, upgrades to the working tree, and asserts per-node state migrates without re-running packages. Existing coverage was unit and envtest only; nothing exercised state written by a previous operator, which is where all three defects lived. Defects found and fixed: * Every upgrade wedged on the package ConfigMap. The converge swept only skyhook.nvidia.com/skyhook-node-meta, but package ConfigMaps carry skyhook.nvidia.com/name. UpsertConfigmaps lists by the new nodewright.nvidia.com/name label, so it could not see the existing object, called Create, and failed permanently on AlreadyExists behind exponential backoff. Both legacy label keys now converge. * Deleting a migrated legacy Skyhook cascade-deleted the NodeWright's ConfigMaps. The converge relabelled them but left their ownerReferences on the legacy Skyhook, so the guide's own "delete the old CRs" step garbage collected the package and per-node metadata ConfigMaps out from under the live NodeWright. The converge now re-parents them onto the NodeWright. * The migration claimed the entire skyhook.nvidia.com/ prefix. It copied every prefixed node key to nodewright.nvidia.com/* and deleted every prefixed key at prune, so a user's own label under the prefix (the prefix is the product's domain name) was duplicated into the operator's namespace and then destroyed 24h after an unrelated upgrade. It now migrates only operator-owned keys and leaves everything else untouched. Docs: the documented sed one-liner for migrating manifests was a global prefix replace that also rewrote nodeSelectors and podNonInterruptLabels keys, which name the user's own nodes and pods rather than anything the operator owns; rewriting them silently selects zero nodes. Narrowed to apiVersion and kind. RELEASE_NOTES corrected: per-node state is copied with the legacy keys kept for the rollback window, not "re-keyed in place". CI: adds a migration row to the operator-ci tests matrix. It builds its cluster with ctlptl like helm-tests, since only ctlptl provides the local registry that serves the operator image under test. Gated by the existing ci-gate with no branch-protection change. Removes k8s-tests/time_travel_tests, which was unrunnable: it referenced a deleted directory, a stale branch, and a pre-scheme tag, and switched versions with git checkout over the working tree. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
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
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.
Summary
Completes the
skyhook.nvidia.com->nodewright.nvidia.comAPI rename and ships the full, rollback-safe migration bridge. The operator reconcilesNodeWrightas the source of truth, automatically mirrors existingSkyhook/DeploymentPolicyobjects into the new group, and migrates per-node state in place without re-running packages. The legacy group is kept, deprecated, and frozen read-only for a bounded migration window.Part of #310. Supersedes #311 (closed to reset the automated review; same branch and diff).
Breaking changes
skyhook.nvidia.com/v1alpha1 Skyhook->nodewright.nvidia.com/v1alpha1 NodeWright.DeploymentPolicymoves group (Kind unchanged).skyhook.nvidia.com/*->nodewright.nvidia.com/*.kubectl skyhook->kubectl nodewright(--skyhookkept as a deprecated alias of--nodewright).Migration is automated and documented in
docs/nodewright-migration.md; no manual node surgery required.What's in this PR
New API group + mirror -
nodewright.nvidia.com/v1alpha1(NodeWright,DeploymentPolicy): types, CRDs, webhooks; field-by-field conversion with a reflection zero-value guard. Level-triggered mirror controller (read-only on the source, ownership-stamped, backs off on user-managed targets, never deletes the target, preserves target-only pause/disable on re-sync). Primary reconciler repointed toNodeWright.Safe in-place node migration - per-node state is copied to the new prefix on upgrade so packages are not re-run. The copy is additive (legacy keys/pods/ConfigMap labels kept); the destructive prune is deferred until
LEGACY_CLEANUP_DELAY(default 24h) elapses, tracked by alegacy-migrated-atstamp. Rollback within the window resumes cleanly;0prunes immediately.Migration interlocks - the hold waits only on genuinely in-flight legacy Skyhooks (
in_progress/erroring/blocked/waiting/unknown);complete/paused/disabledmigrate in place (no forced unpause/enable). Legacy read-only: the legacy webhooks reject spec + pause/disable edits (deletions, finalizer edits, no-op re-applies still allowed), forcing edits onto theNodeWright; the NodeWright stays writable.CLI -
kubectl nodewright,kubectl nodewright migrate,--nodewrightwith deprecated--skyhookalias; backward-compatible with older operators where possible.Chart/config/docs -
operator/config/and the Helm chart ship both groups' CRDs/RBAC/webhooks plus theLEGACY_CLEANUP_DELAYknob;docs/nodewright-migration.mdcovers Argo/Helm/manual runbooks, the rollback window, and read-only behavior.Tooling -
scripts/gen_nodewright.shis kept as a manual target but unhooked frommake: schemas have converged and the legacy group is frozen, so NodeWright is now hand-maintained.Upgrade model
Quiesce (no in-flight rollout) -> upgrade the operator (auto-mirrors CRs + migrates node state) -> rename CRs in git atomically (Argo adopts the pre-created
NodeWright) -> delete the legacy CRs.Testing
Unit + envtest suites pass (legacy api, nodewright api, controller, wrapper, CLI) plus helm/chainsaw e2e in CI.