feat(api): NodeWright API group + Skyhook->NodeWright migration base#311
feat(api): NodeWright API group + Skyhook->NodeWright migration base#311lockwobr wants to merge 35 commits into
Conversation
github-actions
Bot
added
doc
Coverage Report for CI Build 29453787244Coverage decreased (-3.3%) to 79.075%Details
Uncovered Changes
Coverage Regressions15 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThis change introduces the NodeWright v1alpha1 API, CRDs, validation and defaulting webhooks, conversion helpers, and generated deep-copy code. It adds mirror controllers that copy legacy Skyhook and DeploymentPolicy objects into the new API group while preserving user-owned targets. Manager wiring, RBAC, webhook manifests, generation scripts, and tests are updated. Legacy admission paths now emit deprecation warnings, and a migration guide documents Argo CD, Helm, manual migration, verification, and future removal procedures. Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/config/webhook/manifests.yaml (1)
23-170: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSynchronize these webhook additions with the Helm chart.
The PR explicitly leaves
chart/unsynchronized, so Helm installations will not deploy admission configuration equivalent to kustomize.As per coding guidelines, every
operator/config/change must be mirrored intochart/in the same PR.🤖 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 `@operator/config/webhook/manifests.yaml` around lines 23 - 170, Mirror the webhook additions from the MutatingWebhookConfiguration and ValidatingWebhookConfiguration in operator/config/webhook/manifests.yaml into the corresponding Helm chart templates or manifests. Add equivalent webhook entries for the nodewright and skyhook DeploymentPolicy and Nodewright/Skyhook resources, preserving their paths, API groups, operations, names, and failure policies so Helm deployments match kustomize.Source: Coding guidelines
🤖 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/nodewright-migration.md`:
- Around line 23-24: Update the migration instructions in the step 2/3 flow and
the referenced sections to require deleting all Skyhooks that reference legacy
DeploymentPolicies before deleting those DeploymentPolicies, then creating the
renamed replacements; explicitly include legacy DeploymentPolicy cleanup in the
manual procedure and preserve the requirement to perform the operator upgrade
during a quiet window.
- Line 10: Update all documented `kubectl nodewright` command references in the
migration guide to use the currently supported CLI command name; do not document
`kubectl nodewright migrate` or imply the CLI has already been renamed. Ensure
the affected command examples and references consistently match the existing CLI
until the rename is implemented.
- Around line 20-21: Update the migration procedure to defer deleting legacy
Skyhook/DeploymentPolicy CRs until NodeWright support is implemented in the
primary reconciler and any required state migration is complete. Apply the same
gating requirement to every repeated legacy-CR deletion step, while preserving
the existing mirror-controller behavior.
- Line 9: Update the migration documentation to remove claims that node state,
node annotations, finalizers, or package resources are migrated or preserved,
including the related no-rerun/reboot and complete-migration assurances.
Describe only the API-object mirroring performed by the mirror controller, and
revise the referenced sections consistently.
- Around line 81-83: Update the migration documentation sections around the
separate Argo Application guidance, removal steps, and referenced chart details
so they do not claim that this PR ships synchronized Helm chart/operator
resources or a removal-safety hook. Mark those CRD, RBAC, and hook steps as
future work unless the corresponding artifacts are added and verified in the
chart.
- Around line 131-145: Remove the statements claiming the operator performs the
rollout check during upgrade or warns/blocks when nodes are in progress. Keep
the documented requirement and kubectl preflight instructing users to verify
every Skyhook has STATUS complete and INPROGRESS 0 before upgrading.
- Around line 55-62: Update the Argo adoption guidance around the pre-created
NodeWright workflow to qualify that adoption depends on the configured Argo CD
resource-tracking method and applicable AppProject settings. Either document the
required argocd-cm and AppProject configuration or explicitly present adoption
as an assumption, and distinguish orphaned-resource monitoring as affecting
warnings only.
In `@operator/api/nodewright/v1alpha1/deployment_policy_types.go`:
- Around line 374-382: Update CalculateBatchSize in FixedStrategy and the
corresponding CalculateBatchSize implementations for the other two strategies to
return 0 immediately when remaining is less than or equal to zero. Only apply
the existing minimum batch-size enforcement when nodes remain, preserving the
capped batch size behavior otherwise.
In `@operator/api/nodewright/v1alpha1/deployment_policy_webhook.go`:
- Around line 109-125: Replace the list-and-admit deletion check in the
deployment-policy webhook with a finalizer-based flow: add the policy finalizer
on admission, reject NodeWright references when the target DeploymentPolicy has
a deletion timestamp, and retain deletion while the finalizer exists. Update the
controller to recheck references and remove the finalizer only when none remain,
using the existing webhook and reconciliation symbols.
- Around line 159-162: Update the compartment validation call in the generator
source to wrap any error from compartment.Validate() with context identifying
the affected compartment, using an error format that preserves the original
error via %w.
- Around line 35-37: Replace uses of the package-level deploymentPolicylog in
the webhook methods Default and Validate* with request-scoped loggers obtained
via log.FromContext(ctx). Remove the context-free logger usage, then regenerate
the generated deployment policy webhook file.
- Around line 164-170: Update the identical-selector validation around the
selectors loop to convert each compartment’s metav1.LabelSelector with
metav1.LabelSelectorAsSelector and compare the resulting normalized selectors or
requirements instead of using reflect.DeepEqual. Preserve the existing
duplicate-compartment error behavior, handle conversion errors consistently, and
add a test covering semantically equivalent selector forms.
In `@operator/api/nodewright/v1alpha1/nodewright_types.go`:
- Around line 799-806: Update PackageStatus.Equal to compare the Image field
alongside Name, Version, ContainerSHA, Stage, State, and Restarts, ensuring
statuses differing only by image are considered unequal and match the method
comment.
- Around line 418-421: Update the generator/source implementation of
Interrupt.ToArgs so the json.Marshal(clone) failure is wrapped with the
requested contextual message using %w before returning it; do not edit the
generated nodewright_types.go file directly.
In `@operator/api/nodewright/v1alpha1/nodewright_webhook_test.go`:
- Around line 42-47: The defaulting test under the “When creating NodeWright
under Defaulting Webhook” context is only a placeholder and must not remain
always-passing. Either implement it with a real NodeWright creation/defaulting
invocation and assertion for a specified default value, or remove the test until
the intended defaulting behavior is defined.
In `@operator/api/nodewright/v1alpha1/nodewright_webhook.go`:
- Around line 94-98: Update ValidateCreate, ValidateUpdate, Validate, and
validateDeploymentPolicyExists to wrap returned validation and deployment-policy
lookup errors with contextual messages using %w, preserving the original errors
for unwrapping; replace any plain-string or bare error returns in these flows
without changing successful behavior.
- Around line 41-43: Replace package-global skyhooklog usage in Default,
ValidateCreate, ValidateUpdate, and ValidateDelete with logf.FromContext(ctx),
using each method’s admission context so request-scoped fields are preserved.
- Line 44: Update the validPackageName regular expression to remove multiline
mode so validation applies to the entire package name rather than any individual
line. Add a regression test covering newline-separated prefix or suffix text,
ensuring values such as invalid\nfoo are rejected.
In `@operator/api/nodewright/v1alpha1/zz_generated.deepcopy.go`:
- Around line 1-5: Add the repository’s standard Apache-2.0/SPDX license header
to the generated file before the existing build constraint, using the project’s
license-formatting or code-generation workflow rather than manual edits.
Preserve the generated marker and package content unchanged.
In `@operator/api/v1alpha1/deployment_policy_webhook.go`:
- Around line 88-90: Replace the unavailable kubectl nodewright migrate
reference in the deprecation warnings, including
operator/api/v1alpha1/deployment_policy_webhook.go lines 88-90 and
operator/api/v1alpha1/skyhook_webhook.go lines 88-90, with the shipped migration
guide or accurate manual migration steps. Update the corresponding expected
warning text in operator/api/v1alpha1/deployment_policy_webhook_test.go lines
97-115 to match; no separate behavior change is needed there.
In `@operator/api/v1alpha1/nodewright_conversion.go`:
- Around line 107-120: The rekeyPrefix function has nondeterministic collisions
when legacy and NodeWright metadata share a key. Update rekeyPrefix to copy
existing newPrefix entries before rewriting oldPrefix entries, or explicitly
reject conflicts, ensuring nodewright.nvidia.com/* values always take
precedence.
In `@operator/api/v1alpha1/skyhook_webhook_test.go`:
- Around line 68-70: Strengthen the warning assertion in the ValidateUpdate test
for skyhookWebhook so it verifies the warning is explicitly a deprecation
warning, while still checking for the “nodewright.nvidia.com” API group text.
Keep the existing nil-error assertion unchanged.
In `@operator/cmd/manager/main.go`:
- Line 76: Register nwv1.NodeWright and nwv1.DeploymentPolicy webhook handlers
in the main manager setup, not only their schemes. Update
operator/cmd/manager/main.go lines 76-76 to invoke both SetupWebhookWithManager
registrations, and update operator/config/webhook/manifests.yaml lines 23-42,
63-82, 109-129, and 151-170 to include the DeploymentPolicy and NodeWright
mutating and validating handlers respectively.
In `@operator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yaml`:
- Around line 1-7: Synchronize the CRD represented by
deploymentpolicies.nodewright.nvidia.com between the Kustomize configuration and
Helm chart. Run the documented Helm generation target and commit the generated
Helm CRD output, preserving the API schema and metadata so both installation
paths expose the same NodeWright API.
In `@operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml`:
- Around line 1-7: Generate the matching Helm CRD from the updated operator CRD
using the documented Helm generation target, then review and commit the
resulting chart changes so the corresponding resource definition is synchronized
under chart/. Ensure the generated Helm CRD matches
nodewrights.nodewright.nvidia.com and includes the API changes introduced here.
In `@operator/internal/controller/mirror_controller_test.go`:
- Around line 43-46: Update the AfterEach cleanup to restore Gomega’s default
Consistently duration instead of setting it to 2 seconds. Preserve the existing
Eventually timeout reset and ensure later specs do not inherit the widened
duration.
In `@operator/internal/controller/mirror_controller.go`:
- Around line 78-80: Update SkyhookMirrorReconciler and the related reconcilers
to depend on the repository’s internal/dal or wrapper interface instead of
controller-runtime client.Client. Move all Kubernetes get, create, and update
calls in these reconcilers and the generic core behind that abstraction, and
update construction/injection sites to provide it.
- Around line 96-99: Update the mirror controller setup to watch both
mirror-owned target kinds and enqueue reconciliation when those targets are
created, modified, or deleted. In the reconciliation generation-stamp shortcut,
compare each target’s current spec and metadata with the converted desired
object before accepting the source generation as steady state. Preserve
level-triggered convergence so missing or drifted targets are recreated or
corrected.
In `@operator/Makefile`:
- Around line 121-123: Update verify-nodewright-gen to run the controller-gen
generation and manifest-generation targets before checking git diff, including
deepcopy outputs and generated CRDs/RBAC. Extend the verification diff to cover
all regenerated output directories, while preserving the existing stale-output
error and nonzero exit behavior.
---
Outside diff comments:
In `@operator/config/webhook/manifests.yaml`:
- Around line 23-170: Mirror the webhook additions from the
MutatingWebhookConfiguration and ValidatingWebhookConfiguration in
operator/config/webhook/manifests.yaml into the corresponding Helm chart
templates or manifests. Add equivalent webhook entries for the nodewright and
skyhook DeploymentPolicy and Nodewright/Skyhook resources, preserving their
paths, API groups, operations, names, and failure policies so Helm deployments
match kustomize.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 9a15856a-e8ca-4b55-8d92-106596851dec
📒 Files selected for processing (27)
docs/nodewright-migration.mdoperator/Makefileoperator/api/nodewright/v1alpha1/deployment_policy_types.gooperator/api/nodewright/v1alpha1/deployment_policy_webhook.gooperator/api/nodewright/v1alpha1/deployment_policy_webhook_test.gooperator/api/nodewright/v1alpha1/groupversion_info.gooperator/api/nodewright/v1alpha1/nodewright_types.gooperator/api/nodewright/v1alpha1/nodewright_webhook.gooperator/api/nodewright/v1alpha1/nodewright_webhook_test.gooperator/api/nodewright/v1alpha1/webhook_suite_test.gooperator/api/nodewright/v1alpha1/zz_generated.deepcopy.gooperator/api/v1alpha1/deployment_policy_webhook.gooperator/api/v1alpha1/deployment_policy_webhook_test.gooperator/api/v1alpha1/nodewright_conversion.gooperator/api/v1alpha1/nodewright_conversion_test.gooperator/api/v1alpha1/skyhook_webhook.gooperator/api/v1alpha1/skyhook_webhook_test.gooperator/cmd/manager/main.gooperator/config/crd/bases/nodewright.nvidia.com_deploymentpolicies.yamloperator/config/crd/bases/nodewright.nvidia.com_nodewrights.yamloperator/config/crd/kustomization.yamloperator/config/rbac/role.yamloperator/config/webhook/manifests.yamloperator/internal/controller/mirror_controller.gooperator/internal/controller/mirror_controller_test.gooperator/internal/controller/suite_test.goscripts/gen_nodewright.sh
| > **BREAKING CHANGE.** The primary CRD moves group and Kind: `skyhook.nvidia.com/v1alpha1 Skyhook` -> | ||
| > `nodewright.nvidia.com/v1alpha1 NodeWright`. `DeploymentPolicy` moves group (same Kind name). The | ||
| > on-node annotation/label/finalizer prefix moves `skyhook.nvidia.com/* -> nodewright.nvidia.com/*`, and | ||
| > the kubectl plugin is renamed `kubectl skyhook -> kubectl nodewright`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not document kubectl nodewright migrate until the CLI exists.
This PR does not rename the CLI, so these commands will fail for users. Use the currently supported command or land the CLI rename before publishing this guide.
Also applies to: 19-19, 70-70, 98-99, 122-123
🤖 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/nodewright-migration.md` at line 10, Update all documented `kubectl
nodewright` command references in the migration guide to use the currently
supported CLI command name; do not document `kubectl nodewright migrate` or
imply the CLI has already been renamed. Ensure the affected command examples and
references consistently match the existing CLI until the rename is implemented.
| The key property: because the operator **pre-creates** the `NodeWright`, when your git later declares it, | ||
| Argo finds it already present and **adopts** it (applies its tracking label, shows in-sync) rather than | ||
| creating a duplicate. | ||
|
|
||
| 1. **Upgrade the operator** through its own Argo Application (bump the chart/image). The mirror imports | ||
| your existing `Skyhook`s into `NodeWright`s. At this point the `NodeWright`s exist but are not part of | ||
| any Argo Application (untracked) - Argo neither prunes nor flags them, and your `Skyhook` Application | ||
| stays in-sync because the mirror never writes to your `Skyhook`s. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target doc and nearby references first.
git ls-files docs/nodewright-migration.md
wc -l docs/nodewright-migration.md
sed -n '1,140p' docs/nodewright-migration.md
# Look for Argo tracking / orphaned-resource references in the repo.
rg -n "orphan|tracking label|resource-tracking|adopt|in-sync|untracked|orphaned" docs . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'Repository: NVIDIA/nodewright
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== docs/nodewright-migration.md ==="
wc -l docs/nodewright-migration.md
sed -n '1,140p' docs/nodewright-migration.md
echo "=== related references ==="
rg -n "orphan|tracking label|resource-tracking|adopt|in-sync|untracked|orphaned" docs . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || trueRepository: NVIDIA/nodewright
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '50,70p' docs/nodewright-migration.mdRepository: NVIDIA/nodewright
Length of output: 1251
🌐 Web query:
Argo CD resource tracking orphaned resources adoption existing object tracking label orphaned-resource monitoring documentation
💡 Result:
In Argo CD, managing existing objects and monitoring orphaned resources are distinct but related processes. Orphaned Resource Monitoring An "orphaned resource" is defined as a top-level, namespaced Kubernetes resource present in the cluster that is not managed by any Argo CD Application [1]. 1. Enabling Monitoring: This feature is configured at the Project level using the AppProject custom resource [1][2]. You can enable warnings for orphaned resources by setting orphanedResources.warn: true in your AppProject manifest [1][3]. 2. Visibility: Once enabled, Argo CD provides warnings for applications with orphaned resources in their target namespace [1]. You can view these resources in the Argo CD UI by using the "Show Orphaned" filter on the application details page [1][2]. 3. Exceptions: Some resources are excluded by default (e.g., the default ServiceAccount, the 'kubernetes' Service, and common system ConfigMaps) [1][3]. You can add custom ignore rules for specific resources using Group, Kind, and Name (supports glob patterns) in the orphanedResources.ignore section of the AppProject [1][2]. Resource Tracking and Adoption Argo CD uses specific methods to track resources and reconcile them with Git state. If resources are missing tracking metadata (labels or annotations), they may be incorrectly identified as orphaned or fail to sync [4][5]. 1. Tracking Methods: You can configure the global tracking method in the argocd-cm ConfigMap using the application.resourceTrackingMethod key [4][5]. - annotation (default): Uses the argocd.argoproj.io/tracking-id annotation [4]. - annotation+label: Uses the annotation for tracking and the app.kubernetes.io/instance label for informational purposes [4][5]. - label: Uses the app.kubernetes.io/instance label [4][5]. 2. Adopting Existing Resources: When migrating existing objects to Argo CD management, ensure they possess the necessary tracking identifier required by your chosen method [4]. If an object is not being tracked correctly, forcing a sync—sometimes after temporarily adjusting ignore settings—can help trigger the reconciliation of the required tracking annotations or labels [6]. In some cases, manual application of the argocd.argoproj.io/tracking-id annotation may be required to bring legacy resources under Argo CD control without deletion and recreation [6]. For best practices when moving existing infrastructure into Argo CD, prioritize using the annotation tracking method to avoid conflicts with other tools that may rely on standard Kubernetes labels [4][5].
Citations:
- 1: https://argo-cd.readthedocs.io/en/stable/user-guide/orphaned-resources/
- 2: https://argo-cd.readthedocs.io/en/release-3.4/user-guide/orphaned-resources/
- 3: https://argo-cd.readthedocs.io/en/release-3.1/user-guide/orphaned-resources/
- 4: https://argo-cd.readthedocs.io/en/stable/user-guide/resource_tracking/
- 5: https://argo-cd.readthedocs.io/en/release-2.12/user-guide/resource_tracking/
- 6: FAQ: Use Annotation-Based Tracking by Default (
argocd.argoproj.io/tracking-id) doesn't explain what to do (especially withRespectIgnoreDifferences) argoproj/argo-cd#23334
🌐 Web query:
site:argo-cd.readthedocs.io resource tracking orphaned resources existing resources adoption Argo CD
💡 Result:
Argo CD manages resources through specific tracking mechanisms and provides features to monitor or adopt existing resources [1][2]. Resource Tracking Argo CD identifies and manages resources using one of three configured tracking methods, which can be set in the argocd-cm ConfigMap [2][3]: - annotation (default): Uses the argocd.argoproj.io/tracking-id annotation [2][3]. - annotation+label: Uses the argocd.argoproj.io/tracking-id annotation for tracking while maintaining the app.kubernetes.io/instance label for compatibility with other tools [2][3]. - label: Uses the app.kubernetes.io/instance label [2][3]. Orphaned Resources An orphaned resource is a namespaced resource that exists in the cluster but does not belong to any Argo CD Application [1]. Argo CD provides an Orphaned Resources Monitoring feature that can be enabled in Project settings to detect these resources and provide warnings in the UI [1][4]. You can also define ignore rules in the AppProject custom resource to prevent specific resources from being flagged as orphaned [1]. Adopting Existing Resources To "adopt" or bring existing cluster resources under Argo CD management, you generally ensure your Git manifests match the existing resource's properties [5]. Key considerations include: - Server-Side Apply (SSA): Using the ServerSideApply=true sync option is recommended for adopting or patching resources, particularly when migrating from client-side apply or handling complex metadata [6][7]. SSA allows Argo CD to manage fields without the limitations of traditional tracking annotations [6][7]. - Namespace Adoption: When adopting a namespace, you may use managedNamespaceMetadata to set tracking annotations; however, it is advised to upgrade to server-side apply first to avoid unintended modification or deletion of labels/annotations by kubectl [8][6][7]. - Precedence: If you include a manifest in your Argo CD application for a resource already in the cluster, Argo CD will reconcile the live state to match your Git definition [5][8]. Caution is required to ensure that existing, non-managed configurations are not overwritten during this adoption process [8][6].
Citations:
- 1: https://argo-cd.readthedocs.io/en/stable/user-guide/orphaned-resources/
- 2: https://argo-cd.readthedocs.io/en/stable/user-guide/resource_tracking/
- 3: https://argo-cd.readthedocs.io/en/stable/user-guide/resource%5Ftracking/
- 4: https://argo-cd.readthedocs.io/en/latest/user-guide/orphaned-resources/
- 5: https://argo-cd.readthedocs.io/en/latest/user-guide/tracking%5Fstrategies/
- 6: https://argo-cd.readthedocs.io/en/release-3.2/user-guide/sync-options/
- 7: https://argo-cd.readthedocs.io/en/release-3.0/user-guide/sync-options/
- 8: https://argo-cd.readthedocs.io/en/stable/user-guide/sync-options/
Qualify Argo adoption here.
Adoption depends on Argo CD’s resource-tracking method; orphaned-resource monitoring is separate and only affects warnings. Call out the required argocd-cm / AppProject settings, or rephrase this as an assumption.
🤖 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/nodewright-migration.md` around lines 55 - 62, Update the Argo adoption
guidance around the pre-created NodeWright workflow to qualify that adoption
depends on the configured Argo CD resource-tracking method and applicable
AppProject settings. Either document the required argocd-cm and AppProject
configuration or explicitly present adoption as an assumption, and distinguish
orphaned-resource monitoring as affecting warnings only.
| --- | ||
| apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.21.0 | ||
| name: nodewrights.nodewright.nvidia.com |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Generate and commit the matching Helm CRD before merge.
The PR explicitly omits Helm synchronization, leaving Helm installations without the API added here. Run the documented Helm generation target and review the resulting chart changes.
Based on the PR objective, Helm synchronization is currently omitted.
As per coding guidelines, changes to operator/config/ must be mirrored to chart/ in the same PR.
🤖 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 `@operator/config/crd/bases/nodewright.nvidia.com_nodewrights.yaml` around
lines 1 - 7, Generate the matching Helm CRD from the updated operator CRD using
the documented Helm generation target, then review and commit the resulting
chart changes so the corresponding resource definition is synchronized under
chart/. Ensure the generated Helm CRD matches nodewrights.nodewright.nvidia.com
and includes the API changes introduced here.
Source: Coding guidelines
| AfterEach(func() { | ||
| SetDefaultEventuallyTimeout(time.Second) | ||
| SetDefaultConsistentlyDuration(2 * time.Second) | ||
| }) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'SetDefaultConsistentlyDuration|DefaultConsistentlyDuration' \
operator vendor/github.com/onsi/gomega 2>/dev/nullRepository: NVIDIA/nodewright
Length of output: 3382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- mirror_controller_test.go ---'
sed -n '1,220p' operator/internal/controller/mirror_controller_test.go
echo
echo '--- gomega defaults/docs ---'
sed -n '480,520p' operator/vendor/github.com/onsi/gomega/gomega_dsl.go
echo
sed -n '110,135p' operator/vendor/github.com/onsi/gomega/internal/gomega.goRepository: NVIDIA/nodewright
Length of output: 11395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'SetDefaultConsistentlyDuration|SetDefaultEventuallyTimeout|DisableDefaultTimeoutsWhenUsingContext|EnforceDefaultTimeoutsWhenUsingContexts' operator/internal operator/test operator -g '!**/vendor/**'Repository: NVIDIA/nodewright
Length of output: 2008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'SetDefaultConsistentlyDuration|SetDefaultEventuallyTimeout|SetDefaultEventuallyPollingInterval|SetDefaultConsistentlyPollingInterval|EnforceDefaultTimeoutsWhenUsingContexts|DisableDefaultTimeoutsWhenUsingContext' operator -g '!**/vendor/**'Repository: NVIDIA/nodewright
Length of output: 578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'GetDefaultConsistentlyDuration|ConsistentlyDuration|DefaultConsistentlyDuration|DurationBundle' operator/vendor/github.com/onsi/gomegaRepository: NVIDIA/nodewright
Length of output: 3946
Restore Gomega’s Consistently duration in AfterEach AfterEach leaves it at 2s, so later specs in the shared Ginkgo process inherit the widened window instead of Gomega’s default.
🤖 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 `@operator/internal/controller/mirror_controller_test.go` around lines 43 - 46,
Update the AfterEach cleanup to restore Gomega’s default Consistently duration
instead of setting it to 2 seconds. Preserve the existing Eventually timeout
reset and ensure later specs do not inherit the widened duration.
7f21500 to
584d732
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
5be0275 to
4f536e4
Compare
…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>
…ode image - The cache restore-keys used a bare `-bin-` prefix, so on an exact-key miss (any version bump in deps.mk/versions.yaml) it restored an older commit's bin/, leaving a stale tool (e.g. ctlptl v0.9.3, which rejects kind v0.32.0) that the make `test -s` guards never replaced. Make restore-keys match the full key so only an exact hit restores; a bump now forces a fresh binary download. - kind v0.32.0 does not publish a kindest/node:v1.32.x image, so drop 1.32.14 from the CI matrix (validate-kind-node-image failed on it). The matrix is now the published 1.33.12/1.34.8/1.35.5/1.36.1; docs/kubernetes-support.md updated to match. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…rt policy Formalize the support window as "the four most recent Kubernetes minor versions," bounded by the kindest/node images the pinned kind publishes: as a new minor lands and kind drops the oldest node image, we drop the oldest and add the newest. Record the currently tested set (1.36/1.35/1.34/1.33 on kind v0.32.0) and update the version-selection guidance to match. The historical per-release compatibility table is left as-is. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…compatibility framing Drop the stale per-Skyhook-version recommendation table (cited v0.12.x as latest and contradicted itself). State plainly: the operator uses only core Kubernetes APIs, so it likely runs back to ~1.23 untested, while CI tests and officially supports the latest four minors (1.33-1.36). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The Helm chart shipped only the legacy skyhook.nvidia.com surface, so an operator deployed via the chart could not create or reconcile NodeWright objects and the helm-chart / helm-chart-webhook e2e tests failed. Mirror the new group into the chart, keeping the legacy entries for the migration window: - nodewright-crd.yaml + nodewright-deploymentpolicy-crd.yaml, spec bodies copied verbatim from operator/config/crd/bases/nodewright.* (single-version, no conversion) - nodewright.nvidia.com RBAC rules in manager-rbac.yaml, mirroring config/rbac/role.yaml - nodewright mutating + validating webhook entries mirroring config/webhook/manifests.yaml - pre-delete cleanup job also deletes nodewrights (strips the finalizer so CRD removal during helm uninstall does not hang) and fully-qualifies the now-ambiguous deploymentpolicies plural across both groups Transform the helm-chart and helm-webhook e2e pools to NodeWright, pointing the webhook-rejection assertions at the new validating-webhook names. Verified: helm lint + helm template (webhook on/off) render clean with the nodewright CRDs/RBAC/webhooks present; nodewright CRD specs diff-identical to config; go build green. The helm chainsaw e2e itself is validated in CI. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
NodeOrderOffset is json:",omitempty", so a reset (0) value is absent from the NodeWright status and the strict 'nodeOrderOffset: 0' assert failed with 'field not found'. Use a chainsaw default expression ((nodeOrderOffset || 0): 0). Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…inity The selector-migration pre-upgrade hook and the cleanup pre-delete hooks inherited controllerManager.nodeSelector/nodeAffinity. These jobs only talk to the API server, so pinning them to the operator's placement makes the hook pod unschedulable and time out (DeadlineExceeded) whenever that placement is restrictive or in flux (e.g. the helm node-affinity e2e), which then fails the upgrade and cascades across the sequential helm tests via the chart's fixed-name resources. Drop the node selector/affinity from the three maintenance jobs; keep tolerations. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…node affinity" This reverts commit 9d5f6dd.
The pre-delete cleanup and pre-upgrade selector-migration hooks only talk to the API server, so they must stay schedulable anywhere. Inheriting the controller-manager's nodeSelector/nodeAffinity pins them to a restrictive placement and makes the hooks unschedulable: the selector-migration pre-upgrade hook then fails with DeadlineExceeded (breaking helm-node-affinity), and the leftover release leaks its NetworkPolicy into the shared skyhook namespace, cascading into helm-scale and helm-chart-webhook. Keep tolerations (so hooks can land on tainted nodes) but drop the nodeSelector/nodeAffinity blocks. Update the helm-template unit tests that asserted the jobs inherit selectors/affinity to assert they are absent (structurally, via == null), which is why the earlier fix was reverted. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The operator's skyhook->nodewright mirror migrates node labels, so by the time helm-node-affinity runs the shared kind-worker carries only nodewright.nvidia.com/test-node; the skyhook.nvidia.com/test-node label set at cluster setup is gone. This test still pinned the operator's nodeAffinity to the old skyhook.nvidia.com/test-node key, so nothing matched and the manager pods stayed Pending (assert-scheduled timed out), which in turn leaked the release NetworkPolicy and cascaded into helm-scale and helm-chart-webhook. Migrate the node-affinity test (values + asserts + README) to the canonical nodewright.nvidia.com/test-node key, matching the rest of the chainsaw suite and the label that actually survives on the node. Verified locally against a cluster labelled only with nodewright.nvidia.com/test-node. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The chart ships webhook configs with no caBundle; the operator self-manages certs and patches the CA in after startup. helm-chart-webhook asserted only that the webhook configs exist, then slept a fixed 5s before applying an invalid CR to exercise admission rejection. On this branch the operator runs more controllers (dual CRDs + skyhook->nodewright mirror), so it patches the caBundle slightly later; the CR then raced ahead of the CA sync and failed with a TLS 'unknown authority' error instead of the expected admission denial. Assert every webhook's clientConfig.caBundle is populated (chainsaw polls until the operator injects it) so the test only proceeds once the webhook is actually callable. Verified the JMESPath assertion locally. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
…hooks The webhook cert controller only reconciled webhooks whose names it owns (skyhook / deploymentpolicy); getX WebhookRules returns nil for any other name and the update loop skipped those entirely, leaving their caBundle unset. The chart's webhook configs now also carry the nodewright mirror webhooks (mutate/validate-nodewright.nvidia.com and the -deploymentpolicy- nodewright variants), so those never received the CA and calls to them failed with 'x509: certificate signed by unknown authority' (surfaced by helm-chart-webhook once the node-affinity fix stopped masking it). Every webhook in these configs targets the operator's service, so always inject the caBundle; only reconcile rules for webhooks the operator owns (expectedRules != nil), leaving the chart-defined mirror webhooks' rules intact. Adds a unit test for the unowned-webhook path. Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
The operator-agent chainsaw suite was the one test suite the rename missed: it still applied legacy skyhook.nvidia.com/Skyhook CRs and asserted on the legacy Skyhook status, but the operator now reconciles the mirrored NodeWright and writes status there, so the legacy status stayed empty (status: Required value: field not found) and the packages never read complete. reap_old_logs also hard-failed because it calls the CLI without '|| true' and the binary was renamed bin/skyhook -> bin/nodewright. Migrate skyhook.nvidia.com -> nodewright.nvidia.com (api group, node status/ condition annotations, test-node selector), kind: Skyhook -> NodeWright, and bin/skyhook -> bin/nodewright, matching the already-migrated chainsaw/skyhook suite. Filesystem paths (/var/lib/skyhook), SKYHOOK_* agent env, and the skyhook-packages images are the agent contract and stay unchanged. 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>
Migration guide needs to match what ships — and the rollback guidance is currently unsafe
1. Reconcile the guide with the shipped behavior. It repeatedly says the per-node state migration (annotations + package pods + ConfigMaps), the chart-installed 2. The rollback FAQ is a data-loss trap — please add a warning. The FAQ (lines ~255–258) says rollback "is possible while the Please add explicit guidance built around one principle: migrate on a steady-state (quiesced) clusterThe following sharp edges all disappear if the cluster is quiet during the cutover, so the guide should lead with that and call each one out:
Net: the recommended runbook should be "quiesce the cluster (all Skyhooks |
3 participants
Summary
Shared API base for the Skyhook -> NodeWright rename. Introduces the new
nodewright.nvidia.com/v1alpha1group (NodeWright,DeploymentPolicy) as a fork of thelegacy
skyhook.nvidia.comtypes, plus the operator-side migration scaffolding.Part of #310.
What's in this PR
nodewright.nvidia.com/v1alpha1group: types + CRDs (fork of the legacy types).operator/api/v1alpha1/nodewright_conversion.go) withexhaustive fidelity + a reflection-based zero-value guard (fails loudly if the schema drifts).
Skyhook/DeploymentPolicyinto thenew group. Read-only on the source (no GitOps drift), ownership-stamped, backs off on
user-managed targets, and never deletes the target.
Skyhook/DeploymentPolicywrites.docs/nodewright-migration.md(Argo CD / Helm / manual).All unit + envtest suites pass.
Not in this PR (follow-ups)
NodeWright; reduce the legacy controller to a no-op.kubectl nodewright migrate+ backward-compatible Kind detection.operator/config/+ Helm chart sync + the pre-upgrade safety hook.nodewrighttype generator (automated re-fork from legacy so the copy can't drift)scripts/gen_nodewright.shis a WIP prototype, intentionally excluded here.For collaborators
This is a shared base branch. Please branch off
fix/api-renameand keep changesadditive - it will not be force-pushed.
Upgrade model (see migration guide)
Update operator (auto-mirrors CRs + migrates node/pod/ConfigMap naming) -> user renames CRs in
git (Argo adopts the pre-created
NodeWright) -> user deletes the old CRs. Hard prerequisite:all Skyhooks complete (no rollout in progress) at upgrade time.