You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#349 renamed the API group to nodewright.nvidia.com, but the operator's user-visible text still says "skyhook" in a lot of places. #402 fixed the one event string it added and deliberately left the rest, because sweeping them would collide with the other in-flight branches in this epic. This is the cleanup issue for that.
Important
Do this after the jobs epic merges, not during. Almost every site below sits in skyhook_controller.go, job_controller.go or pod_controller.go — the three files every open branch in #223 is editing. A sweep now guarantees conflicts for no functional gain.
Scope: strings a user reads, not identifiers
Counts from operator/ (excluding _test.go):
Surface
Count
Example
Event messages
6
"Package [%s:%s] state %s on [skyhook:%s]"
fmt.Errorf text
33
"error validating skyhook [%s]: %w"
Log messages / keys
11
logger.Error(err, "error getting skyhooks"), "skyhook", name
The 6 event strings are the sharpest, since they land on Node objects and show up in kubectl describe node:
pod_controller.go:177
job_controller.go:331
skyhook_controller.go:2175, :2205, :2296, :3039
(job_controller.go:543 is already [nodewright:%s] as of #402 — the odd one out until this lands.)
Log keys matter as much as messages: logger.Error(err, "error processing skyhook", "skyhook", name) puts skyhook= in structured output that operators may already be querying on. Renaming the key is a small observability break worth calling out in the changelog, so decide deliberately rather than sweeping it in silently.
Explicitly out of scope
Go identifiers — SkyhookReconciler, skyhookNode, SkyhookOperatorOptions, wrapper.SkyhookNode, EventsReasonSkyhook*, filenames like skyhook_controller.go. That's a mechanical but enormous refactor and belongs in its own issue, if at all. This issue is text only.
Agent contract surfaces — SKYHOOK_RESOURCE_ID, SKYHOOK_LOG_DIR, SKYHOOK_ROOT_DIR, /skyhook-package/* mount paths, COPY_DIR_ROOT / AGENT_LOG_ROOT defaults (/var/lib/skyhook, /var/log/skyhook). Renaming these breaks running packages and host state; per .claude/CLAUDE.md the agent hasn't migrated yet. Leave them.
The legacy migration shims — zz.migration.*.go, migration_hold.go. Their text says "Skyhook" because it is genuinely talking about the legacy skyhook.nvidia.com resources. Correct as-is.
The CLI (internal/cli/, cmd/cli/, 8 more Errorf sites). It ships and versions separately and kubectl skyhook is still the command name; renaming its text is a separate call tied to whenever the plugin is renamed.
Suggested split
The 6 event messages — smallest, highest visibility, easiest to review.
fmt.Errorf text.
Log messages, plus an explicit decision on the "skyhook" structured key.
Done when
No literal skyhook remains in operator event/error/log text outside the exclusions above.
The structured-log key decision is recorded in operator/CHANGELOG.md if it changes.
make unit-tests passes — several tests assert on message text (e.g. k8s-tests/chainsaw/** event assertions), so grep those too.
Part of #223. Follow-up from #402 (review comment).
#349 renamed the API group to
nodewright.nvidia.com, but the operator's user-visible text still says "skyhook" in a lot of places. #402 fixed the one event string it added and deliberately left the rest, because sweeping them would collide with the other in-flight branches in this epic. This is the cleanup issue for that.Important
Do this after the jobs epic merges, not during. Almost every site below sits in
skyhook_controller.go,job_controller.goorpod_controller.go— the three files every open branch in #223 is editing. A sweep now guarantees conflicts for no functional gain.Scope: strings a user reads, not identifiers
Counts from
operator/(excluding_test.go):"Package [%s:%s] state %s on [skyhook:%s]"fmt.Errorftext"error validating skyhook [%s]: %w"logger.Error(err, "error getting skyhooks"),"skyhook", nameThe 6 event strings are the sharpest, since they land on Node objects and show up in
kubectl describe node:pod_controller.go:177job_controller.go:331skyhook_controller.go:2175,:2205,:2296,:3039(
job_controller.go:543is already[nodewright:%s]as of #402 — the odd one out until this lands.)Log keys matter as much as messages:
logger.Error(err, "error processing skyhook", "skyhook", name)putsskyhook=in structured output that operators may already be querying on. Renaming the key is a small observability break worth calling out in the changelog, so decide deliberately rather than sweeping it in silently.Explicitly out of scope
SkyhookReconciler,skyhookNode,SkyhookOperatorOptions,wrapper.SkyhookNode,EventsReasonSkyhook*, filenames likeskyhook_controller.go. That's a mechanical but enormous refactor and belongs in its own issue, if at all. This issue is text only.SKYHOOK_RESOURCE_ID,SKYHOOK_LOG_DIR,SKYHOOK_ROOT_DIR,/skyhook-package/*mount paths,COPY_DIR_ROOT/AGENT_LOG_ROOTdefaults (/var/lib/skyhook,/var/log/skyhook). Renaming these breaks running packages and host state; per.claude/CLAUDE.mdthe agent hasn't migrated yet. Leave them.zz.migration.*.go,migration_hold.go. Their text says "Skyhook" because it is genuinely talking about the legacyskyhook.nvidia.comresources. Correct as-is.internal/cli/,cmd/cli/, 8 moreErrorfsites). It ships and versions separately andkubectl skyhookis still the command name; renaming its text is a separate call tied to whenever the plugin is renamed.Suggested split
fmt.Errorftext."skyhook"structured key.Done when
skyhookremains in operator event/error/log text outside the exclusions above.operator/CHANGELOG.mdif it changes.make unit-testspasses — several tests assert on message text (e.g.k8s-tests/chainsaw/**event assertions), so grep those too.