Skip to content

fix(operator): scope package pod events to the operator namespace - #646

Merged
lockwobr merged 3 commits into
NVIDIA:mainfrom
MaxFreedomPollard:fix/scope-package-pod-watch
Sep 23, 2026
Merged

fix(operator): scope package pod events to the operator namespace#646
lockwobr merged 3 commits into
NVIDIA:mainfrom
MaxFreedomPollard:fix/scope-package-pod-watch

Conversation

@MaxFreedomPollard

@MaxFreedomPollard MaxFreedomPollard commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #477.

The Pod cache is cluster-wide because drain must see workload pods in every namespace. The package-pod watch currently admits any pod carrying the name label, even outside the operator namespace.

Pass the configured namespace into PodReconciler and require both nodewright.nvidia.com/name and nodewright.nvidia.com/package at the watch predicate. This hardens the scope described in the issue while leaving the shared cache and drain behavior unchanged. The documentation now distinguishes cache scope from workqueue admission.

Tests cover all four predicate event types, copied labels outside the namespace, missing labels, and legacy/custom install namespaces. The copied-label and name-only cases fail on the base implementation.

Validation: make fmt, make unit-tests (15 suites, including envtest), and make lint pass with Go 1.27.1 and Kubernetes 1.37.0 envtest binaries. Cluster e2e suites were not run locally. The CLI contract is unchanged.

Checklist

  • I am familiar with the Contributing Guidelines.
  • My commits are signed off per the DCO and cryptographically signed.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@MaxFreedomPollard
MaxFreedomPollard requested a review from a team September 20, 2026 23:44
@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Welcome to NodeWright, @MaxFreedomPollard! Thanks for your first pull request.

Before review, please ensure:

  • All commits are signed off and signed: git commit -s -S (see CONTRIBUTING.md)
  • Commits follow Conventional Commits
  • CI checks pass (tests, lint, security scan)
  • The PR description explains the why behind your changes

A maintainer will review this soon.

@github-actions github-actions Bot added doc Documentation change (PR path label; doc issues use the Documentation type) component/operator Skyhook operator (controller-manager) labels Sep 20, 2026
@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: NVIDIA/nodewright/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 53fa1fad-11a4-4a65-825d-4693499d842c

📥 Commits

Reviewing files that changed from the base of the PR and between bc999e5 and 4ba06a0.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/nodewright/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 6e251922-92c2-48e7-99d7-8b95a194debf

📥 Commits

Reviewing files that changed from the base of the PR and between cdc1824 and 3e9ec35.

📒 Files selected for processing (5)
  • docs/operations/resources-at-scale.md
  • operator/cmd/manager/main.go
  • operator/internal/controller/pod_controller.go
  • operator/internal/controller/skyhook_controller_test.go
  • operator/internal/controller/swap_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

The Pod reconciler now receives and stores the configured operator namespace. Its pod predicate requires that pods belong to this namespace and have both node name and package labels. Pods in other namespaces remain available for draining but do not enter the package-pod workqueue. Tests cover event types, namespace variants, and incomplete labels. Documentation reflects the updated scope.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description check ✅ Passed The description directly explains the namespace and label filtering changes, testing, documentation updates, and validation results.
Linked Issues check ✅ Passed The description references issue #477, and the changes address the stated namespace and label-scoping objectives.
Out of Scope Changes check ✅ Passed The changes remain within the stated objective: scoping package-pod event admission while preserving cluster-wide cache and drain behavior.
Title check ✅ Passed The title clearly and concisely describes scoping package-pod events to the operator namespace.
Linked Issues check ✅ Passed The changes satisfy issue #477. NewPodReconciler receives options.Namespace. The predicate requires the configured namespace and both nodewright.nvidia.com/name and `nodewright.nvidia.com/packag…
Out of Scope Changes check ✅ Passed The changes stay within issue #477. The manager wiring, reconciler changes, predicate tests, and documentation directly support namespace-scoped workqueue admission, label requirements, or preservatio…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

🌿 Preview your docs: https://nvidia-preview-fix-scope-package-pod-watch.docs.buildwithfern.com/nodewright

@ayuskauskas ayuskauskas mentioned this pull request Sep 21, 2026
Closed
@coveralls

coveralls commented Sep 23, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 35904305825

Coverage increased (+0.3%) to 82.506%

Details

  • Coverage increased (+0.3%) from the base build.
  • Patch coverage: 10 of 10 lines across 2 files are fully covered (100%).
  • 2 coverage regressions across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
operator/internal/controller/skyhook_controller.go 2 80.18%

Coverage Stats

Coverage Status
Relevant Lines: 11358
Covered Lines: 9371
Line Coverage: 82.51%
Coverage Strength: 7.62 hits per line
💛 - Coveralls

@lockwobr
lockwobr merged commit ddcebbb into NVIDIA:main Sep 23, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ayuskauskas ayuskauskas ayuskauskas approved these changes
@lockwobr lockwobr Awaiting requested review from lockwobr lockwobr is a code owner
@rice-riley rice-riley Awaiting requested review from rice-riley rice-riley is a code owner

Assignees

@MaxFreedomPollard MaxFreedomPollard

Labels

component/operator Skyhook operator (controller-manager) doc Documentation change (PR path label; doc issues use the Documentation type)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Scope the pod controller's ownedPod predicate to the operator namespace

4 participants

@MaxFreedomPollard @coveralls @ayuskauskas @lockwobr