fix(operator): rename skyhook -> nodewright in operator error and log text - #476
Conversation
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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe operator now uses Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This PR completes a localized rename of operator-facing errors and logs; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage Report for CI Build 31846374805Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.02%) to 78.917%Details
Uncovered Changes
Coverage Regressions8 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Closes #403. Supersedes #414.
Finishes the operator's user-visible text rename. #410 landed part 1 (event and condition text); this PR carries part 2 (
fmt.Errorftext) and part 3 (log messages and keys).Credit
The part 2 commit is @mohityadav8's, authored by them and preserved as such. I re-applied their original commit (
f88c4079, from before the merge-ups) onto currentmainrather than patching their merged-up branch, because that is what makes the defect below go away by construction. Part 3 is mine.What #414 hit, and why this branch is not affected
@ayuskauskas found in #414 (comment) that #414's
reconcileLegacyLabeledWorkloadswas a partial revert of #380, not a rename. Thef49c695amerge ofmain@84df6448resolved a conflict in favour of the branch's pre-#380 side and dropped three things: the two-key legacy ConfigMap sweep, thereparentToNodeWrightcall, andchanged = trueafter a successfulUpdate.Cherry-picking the pre-merge commit means that resolution never enters this history. The one conflict that did arise (the same function) was resolved by keeping
main's code and re-applying only the string edit on top, sogit diff origin/main -- operator/internal/controller/skyhook_controller.gois text-only. All three dropped pieces are present.Two observability breaks, deliberate
Renaming the structured-log key and the event source component is not cosmetic, so calling them out rather than letting them surprise someone:
skyhook=<name>needs to becomenodewright=<name>.kubectl get events --field-selector source=skyhook-controllerneedssource=nodewright-controller.Nothing else about the event surface changed; #410 already renamed the messages themselves.
Out of scope, unchanged on purpose
Go identifiers (
SkyhookReconciler,wrapper.SkyhookNode, filenames), the agent contract surfaces (SKYHOOK_RESOURCE_ID,/skyhook-package/*,/var/lib/skyhook), the legacy migration shims and the legacy Skyhook webhook, whose text genuinely refers toskyhook.nvidia.comresources, the deprecatedskyhook_*metrics (owned by #462), and the CLI.A sweep over
operator/confirms every survivingskyhookliteral falls into one of those buckets.Testing
make unit-tests: 13 suites, 851 specs, 0 failures.make manifests generateproduces no drift. No test or chainsaw file asserts on any renamed string.Note for whoever merges
#459 touches 5 of the 7 files here. Whichever lands second needs a re-resolve, and the resolution is the exact trap that broke #414, so it is worth taking
main's side and re-applying the string edits rather than accepting either side wholesale.