fix(operator): rename skyhook -> nodewright in event and condition text - #410
Conversation
github-actions
Bot
added
component/operator
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated controller event and deletion-blocking messages to use Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/internal/controller/skyhook_controller.go (1)
120-120: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-494): Download of Code Without Integrity Check
Reachability: Internal
Pin
AgentImageto an immutable release.When
AGENT_IMAGEis unset, the operator uses:latestfor privileged agent Pods. Use a released tag with its digest, such asv6.4.2@sha256:7cd80f5ef351266dc08c3979e802f9dc936a1ed9fd02e199233e7968a3a0de3e. Updateoperator/config/manager/manager.yamlif it remains a supported deployment manifest.🤖 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/skyhook_controller.go` at line 120, Pin the AgentImage default in the controller configuration to the specified immutable release tag and digest instead of :latest. Update operator/config/manager/manager.yaml as well if it defines or overrides the supported default deployment image, keeping the AGENT_IMAGE override behavior unchanged.
🤖 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.
Outside diff comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Line 120: Pin the AgentImage default in the controller configuration to the
specified immutable release tag and digest instead of :latest. Update
operator/config/manager/manager.yaml as well if it defines or overrides the
supported default deployment image, keeping the AGENT_IMAGE override behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f94d611c-0ea7-4b62-babc-9455d4f8d21f
📒 Files selected for processing (3)
operator/internal/controller/skyhook_controller.gooperator/internal/controller/skyhook_controller_test.gooperator/internal/wrapper/node.go
Part 1/3 of NVIDIA#403 -- event messages and Condition messages users see via kubectl describe. Errorf text and log messages/keys follow in separate PRs. Refs NVIDIA#403 Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
7adf51c to
4cf4a92
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/internal/wrapper/node.go`:
- Line 650: The condition comparison in the cond.Type branch must detect updates
when either the reason or message differs. Replace the current AND-based
mismatch check with OR logic, while preserving the existing found-condition
handling and setting node.updated so SaveNodesAndSkyhook applies the new
NodeWright message.
🪄 Autofix
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: 54658a4e-a416-4aaa-b255-5f8ec22e30e6
📒 Files selected for processing (4)
operator/internal/controller/pod_controller.gooperator/internal/controller/skyhook_controller.gooperator/internal/controller/skyhook_controller_test.gooperator/internal/wrapper/node.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
operator/internal/controller/skyhook_controller.go (4)
668-669: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winWrap propagated migration errors.
Line 669 and Line 799 return bare
errvalues after multi-operation migration paths. This removes operation context and violates the required error-wrapping rule. Wrap each error withfmt.Errorf("...: %w", err).As per coding guidelines, use standard-library
errorsand wrap propagated errors withfmt.Errorf("...: %w", err); do not return a bareerrfrom functions that perform more than one operation.Proposed fix
- return false, err + return false, fmt.Errorf("reconciling legacy migration stamp: %w", err) - return false, false, err + return false, false, fmt.Errorf("reparenting legacy ConfigMap [%s]: %w", cm.Name, err)Also applies to: 797-800
🤖 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/skyhook_controller.go` around lines 668 - 669, Update the error returns in the migration paths of the controller, including the reconcileLegacyMigratedStamp call and the corresponding path around the later migration operation, to wrap propagated errors with fmt.Errorf using a descriptive operation context and the %w verb. Ensure neither multi-operation path returns a bare err, and retain the existing return values and control flow.Source: Coding guidelines
740-758: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRoute pod listing through the DAL.
Use
r.dal.GetPodsfor this query and handle itsnilresult when no pods exist. No DAL or wrapper delete method exists, so retainr.Deletefor pod deletion.🤖 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/skyhook_controller.go` around lines 740 - 758, Replace the direct pod List call in the legacy-labeled pod cleanup flow with r.dal.GetPods, passing the namespace and legacyNameLabel selector. Treat a nil result as no pods when setting hadLegacy and iterating for pruning, while retaining r.Delete for pod deletion and its existing error handling.Source: Coding guidelines
740-757: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate legacy ownership before deleting or adopting resources.
The migration treats
legacyNameLabelandlegacyNodeMetaLabelas ownership proof. During prune, it deletes every matching Pod. For ConfigMaps,reparentToNodeWrightadopts every matching object without a controller owner and removes anyskyhook.nvidia.com/owner reference without checking its kind, name, or UID.If a user can create or relabel resources in
r.opts.Namespace, the operator can delete a user Pod or adopt a user ConfigMap. Require an exact legacy Skyhook owner reference, including its UID, or another non-forgeable operator-owned marker before mutation. Skip and report objects that fail the check.🤖 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/skyhook_controller.go` around lines 740 - 757, Validate legacy ownership before mutating resources in the prune Pod loop and reparentToNodeWright. Require an exact legacy Skyhook owner reference, including kind, name, and UID, or another non-forgeable operator-owned marker; do not treat legacyNameLabel or legacyNodeMetaLabel alone as proof. Skip and report Pods or ConfigMaps that fail validation, and only delete, adopt, or remove owner references from validated objects.Source: Coding guidelines
810-813: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPatch migrated ConfigMaps instead of using
Update.This migration changes only labels and owner references. Use
client.MergeFrom(cm.DeepCopy())withr.Patch(ctx, cm, patch)to avoid sending unrelated ConfigMap fields and reduce conflicts with concurrent writers.🤖 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/skyhook_controller.go` around lines 810 - 813, Replace the r.Update call in the ConfigMap migration flow with a MergeFrom patch based on cm.DeepCopy(), then apply it via r.Patch(ctx, cm, patch). Preserve the existing error wrapping, changed assignment, and label/owner-reference updates.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.
Outside diff comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 668-669: Update the error returns in the migration paths of the
controller, including the reconcileLegacyMigratedStamp call and the
corresponding path around the later migration operation, to wrap propagated
errors with fmt.Errorf using a descriptive operation context and the %w verb.
Ensure neither multi-operation path returns a bare err, and retain the existing
return values and control flow.
- Around line 740-758: Replace the direct pod List call in the legacy-labeled
pod cleanup flow with r.dal.GetPods, passing the namespace and legacyNameLabel
selector. Treat a nil result as no pods when setting hadLegacy and iterating for
pruning, while retaining r.Delete for pod deletion and its existing error
handling.
- Around line 740-757: Validate legacy ownership before mutating resources in
the prune Pod loop and reparentToNodeWright. Require an exact legacy Skyhook
owner reference, including kind, name, and UID, or another non-forgeable
operator-owned marker; do not treat legacyNameLabel or legacyNodeMetaLabel alone
as proof. Skip and report Pods or ConfigMaps that fail validation, and only
delete, adopt, or remove owner references from validated objects.
- Around line 810-813: Replace the r.Update call in the ConfigMap migration flow
with a MergeFrom patch based on cm.DeepCopy(), then apply it via r.Patch(ctx,
cm, patch). Preserve the existing error wrapping, changed assignment, and
label/owner-reference updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f08e2efc-28ab-4ee1-ada5-2615a43d3a3d
📒 Files selected for processing (2)
operator/internal/controller/skyhook_controller.gooperator/internal/wrapper/node.go
Coverage Report for CI Build 31653376698Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 78.92%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Brings the jobs branch up to main (15 commits behind, 55 ahead). Seven files conflicted; three needed a semantic resolution rather than a side pick, because main fixed code that this branch had moved or deleted. Ported forward, not dropped: - #450's semantic resource comparison. Main fixed podMatchesPackage to compare quantities with apiequality.Semantic instead of reflect.DeepEqual: apimachinery rewrites a quantity into canonical form on serialization ("4000m" -> "4"), so a pod read back is never byte-equal and reflect.DeepEqual reports a mismatch that isn't one, invalidating and recreating forever. That function moved to job_builder.go on this branch and still had the pre-fix reflect.DeepEqual, so the Jobs path carried the same bug. Taking either side of the conflict would have shipped it; the fix is now in job_builder.go where the function lives, and the now-unused apiequality import is dropped from skyhook_controller.go. - #410's skyhook -> nodewright event text. Main renamed every "[skyhook:%s]" in controller sources; this branch had added three more in job_controller.go and pod_controller.go that main could not reach. Renamed to match. - Main's "Resource Comparison" spec block. It exercises podMatchesPackage, which survives here, so it tests exactly the fix above and is kept alongside this branch's "cluster state compartments" block rather than replaced by it. Side picks, with reasons: - pod_controller.go / skyhook_controller.go ValidateRunningPackages: main edited raw-pod code this branch replaced with the Jobs path. Ours, plus the rename. - chart/templates/manager-rbac.yaml: main's #371 removed the kube-rbac-proxy Role; that deletion auto-merged elsewhere in the file and the conflicting hunk is this branch's new least-privilege namespaced Role. Ours. - strict-order chainsaw: main's script block lacks the "set -e" guard this branch added. Ours. - Both RELEASE_NOTES.md: additive bullets under the same heading describing different shipped changes. Both kept. Also repairs a textual auto-merge artifact: main added six workload_migration specs calling the 4-argument NewSkyhookReconciler, which this branch widened to six parameters (uncached reader + clientset). Updated to match. make unit-tests passes (8 suites, 333 controller specs), golangci-lint 0 issues. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
… text (#476) * fix(operator): rename skyhook -> nodewright in fmt.Errorf text Part 2/3 of #403 -- error text surfaced to callers via wrapped errors. Event/condition text landed in PR1, log messages/keys are PR3. Refs #403 Signed-off-by: Mohit Yadav <ymohit799057@gmail.com> * fix(operator): rename skyhook -> nodewright in operator log text Third and last part of #403, after the event text (#410) and the fmt.Errorf text in the preceding commit. Covers the log messages, the "skyhook" structured-log key, the ready-condition logger name, and the event source component. Two of these are observability breaks rather than cosmetics, so they are called out rather than swept in silently: a log query matching skyhook=<name> now needs nodewright=<name>, and --field-selector source=skyhook-controller now needs source=nodewright-controller. Deliberately unchanged: Go identifiers, the agent contract surfaces (SKYHOOK_* env vars, /skyhook-package mounts), the legacy migration shims and the legacy Skyhook webhook, whose text genuinely refers to skyhook.nvidia.com resources, the deprecated skyhook_* metrics, and the CLI. Closes #403 Signed-off-by: Brian Lockwood <lockwobr@gmail.com> --------- Signed-off-by: Mohit Yadav <ymohit799057@gmail.com> Signed-off-by: Brian Lockwood <lockwobr@gmail.com> Co-authored-by: Mohit Yadav <ymohit799057@gmail.com>
4 participants
Part 1/3 of #403 -- event messages and Condition messages users see via kubectl describe. Errorf text and log messages/keys follow in separate PRs.
Refs #403