OCPBUGS-92062: reduce memory usage in MCC and MCD - #6259
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-92062, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR adds scoped MachineConfig informers to controller and daemon startup, updates node reconciliation and status handling, and changes ChangesNode controller runtime and status flow
Scoped informer integration
Layered node state
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/controller/common/layered_node_state.go`:
- Around line 183-186: The LayeredNodeState.Node method is currently documented
and used as write-safe, but it returns the underlying cached node directly and
bypasses ensureSafeForMutation, so update the API contract to make Node()
read-only and introduce a separate mutation-safe accessor or guard writes before
exposing a writable pointer. Adjust the Node() documentation and any callers
relying on lns.Node() for mutations so they use the safe mutation path in
LayeredNodeState instead of mutating informer-backed state directly.
In `@pkg/controller/node/status.go`:
- Line 34: syncStatusOnly now reads from ctrl.mcnLister.Get, but Run() only
waits on the other cache syncs, so the MachineConfigNode lister can still be
empty at startup and cause false “not found” results. Update the controller’s
WaitForCacheSync in Run() to include ctrl.mcnListerSynced alongside the existing
cache sync checks, keeping the syncStatusOnly path and mcnLister lookup
consistent during startup.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fed91d30-eac8-47e3-ac95-6d12303c4510
📒 Files selected for processing (5)
cmd/machine-config-controller/start.gocmd/machine-config-daemon/start.gopkg/controller/common/controller_context.gopkg/controller/common/layered_node_state.gopkg/controller/node/status.go
a3901bc to
081cd3d
Compare
|
/test e2e-aws-ovn e2e-gcp-op-part1 e2e-gcp-op-part2 |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-ovn-serial-2of2 This should cover any MCN tests |
|
@isabella-janssen: trigger 5 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/12185cd0-7586-11f1-87c5-ce5461a95323-0 |
isabella-janssen
left a comment
There was a problem hiding this comment.
/lgtm
Change looks reasonable to me & MCN tests are still passing as expected.
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/jira refresh |
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-92062, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by payload jobs |
|
@cheesesashimi: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Assisted-By: Claude Sonnet 4.5 Previously, the podLister was looking at all pods in all namespaces. This consumes a lot of unnecessary CPU and memory since the podLister must not only synchronize its internal state against the API server, it must also keep a copy of those objects in memory. The reason why this is unnecessary is because node controller is only looking for the node that the MCO pod is running on, which is a single pod.
Assisted-By: Claude Sonnet 4.5 Previously, the LayeredNodeState object would perform a DeepCopy() for every single mutation of the node object. This could cause significant memory consumption whenever multiple mutations are performed across many nodes. Instead, this new implementation will only perform a DeepCopy() whenever a mutation is actually made.
Assisted-By: Claude Sonnet 4.5 The backing cache for the MachineConfigNodeLister has all of the MachineConfigNode objects already in memory. Instead of constructing a potentially large list of MachineConfigNodes incrementally from the API server, they should be retrieved from the lister instead. This is more memory efficient because the lister will return references to the objects in its cache instead of a whole new copy of the object.
Assisted-By: Claude Sonnet 4.5 The MachineConfigNodeInformer was syncing all of the MachineConfigNode objects when it should only be syncing the MachineConfigNode for the node in which the Daemon is executing. By scoping the informer only to the current node, only the current MachineConfigNode will be cached, yielding memory savings as well as reducing API server load.
081cd3d to
5691c56
Compare
|
@cheesesashimi: This pull request references Jira Issue OCPBUGS-92062, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
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)
pkg/controller/node/status.go (1)
268-343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear
Updatingfor paused pools when non-build degradation is present.The paused-pool MOSB dispatch sets
Updating=TrueforIsBuildPrepared,IsBuilding, andIsInInitialStatewithout checkingMachineConfigPoolRenderDegraded,MachineConfigPoolNodeDegraded, orMachineConfigPoolPinnedImageSetsDegraded, while the unpaused branch gates those on!isNonBuildDegraded. Add the same guard here, and extract the shared MOSB state switch to keep the two branches in sync.🤖 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 `@pkg/controller/node/status.go` around lines 268 - 343, Update the paused-pool handling in the status reconciliation logic to compute and honor the existing non-build degradation conditions before setting Updating=True. Extract the duplicated mosbState dispatch used by both paused and unpaused paths into a shared helper or flow, ensuring RenderDegraded, NodeDegraded, and PinnedImageSetsDegraded clear Updating consistently while preserving paused-specific messages.
🧹 Nitpick comments (2)
pkg/controller/node/node_controller.go (2)
1477-1477: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
context.TODO()used despiteRun()now threading a realcontext.Context.
Run()was migrated to accept and propagatectxfor cancellation (Lines 321-354), butupdatePoolsstill creates a detachedcontext.TODO()for the taint-update calls, so those calls won't be cancelled on shutdown. As per path instructions, Go code should usecontext.Context for cancellation and timeouts.🤖 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 `@pkg/controller/node/node_controller.go` at line 1477, Replace the detached context.TODO() in updatePools with the context.Context received from Run and threaded through the call chain. Ensure the taint-update calls use this propagated ctx so cancellation and shutdown signals reach them, without changing unrelated behavior.Source: Path instructions
1382-1552: 📐 Maintainability & Code Quality | 🔵 TrivialMaster/arbiter sequencing depends on undocumented ordering of
pools.
masterUnavailableCountis only populated while processing a pool namedmaster(Lines 1517-1525) and only consumed while processing a pool namedarbiter(Lines 1505-1512). This is correct today only because the sole caller (syncMachineConfigPool) always buildspoolsToUpdateas[master, arbiter]. IfupdatePoolsis ever called with a different order (or a future caller changes the construction), the arbiter would silently skip the master-unavailability guard sincemasterUnavailableCountwould still be zero. Consider asserting/documenting the required ordering directly inupdatePools, or deriving the master state by name lookup instead of loop-order dependency.Separately,
getNodesForPoolis called for the arbiter pool twice per master-processing pass — once for the pre-check at Line 1455 and again for its own iteration later in the same call — a minor redundant listing.🤖 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 `@pkg/controller/node/node_controller.go` around lines 1382 - 1552, Update updatePools so master/arbiter sequencing does not rely on the caller’s pools order: explicitly process the master pool before the arbiter, or otherwise derive the master unavailable state by name lookup before applying the arbiter guard. Preserve the existing masterUnavailableCount behavior, and reuse cached arbiter nodes within the same updatePools invocation instead of calling getNodesForPool twice.
🤖 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 `@pkg/controller/node/status.go`:
- Around line 268-343: Update the paused-pool handling in the status
reconciliation logic to compute and honor the existing non-build degradation
conditions before setting Updating=True. Extract the duplicated mosbState
dispatch used by both paused and unpaused paths into a shared helper or flow,
ensuring RenderDegraded, NodeDegraded, and PinnedImageSetsDegraded clear
Updating consistently while preserving paused-specific messages.
---
Nitpick comments:
In `@pkg/controller/node/node_controller.go`:
- Line 1477: Replace the detached context.TODO() in updatePools with the
context.Context received from Run and threaded through the call chain. Ensure
the taint-update calls use this propagated ctx so cancellation and shutdown
signals reach them, without changing unrelated behavior.
- Around line 1382-1552: Update updatePools so master/arbiter sequencing does
not rely on the caller’s pools order: explicitly process the master pool before
the arbiter, or otherwise derive the master unavailable state by name lookup
before applying the arbiter guard. Preserve the existing masterUnavailableCount
behavior, and reuse cached arbiter nodes within the same updatePools invocation
instead of calling getNodesForPool twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ab490f7-10ff-416e-93c4-d79a0f85ebb0
📒 Files selected for processing (6)
cmd/machine-config-controller/start.gocmd/machine-config-daemon/start.gopkg/controller/common/controller_context.gopkg/controller/common/layered_node_state.gopkg/controller/node/node_controller.gopkg/controller/node/status.go
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, isabella-janssen, umohnani8 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-gcp-op-ocl-part2 e2e-gcp-op-part1 |
|
/test e2e-gcp-op-part1 |
|
/test tls-pqc-readiness |
|
/override ci/prow/e2e-gcp-op-part1 |
|
@isabella-janssen: Overrode contexts on behalf of isabella-janssen: ci/prow/e2e-gcp-op-part1 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/verified by CI test suites |
|
@cheesesashimi: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@cheesesashimi: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@cheesesashimi: Jira Issue Verification Checks: Jira Issue OCPBUGS-92062 Jira Issue OCPBUGS-92062 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in release 5.0.0-0.nightly-2026-08-07-030024 |
- What I did
In OCPBUGS-92062, numerous memory usage regressions were identified. The proximate cause was that numerous features were no longer kept behind a FeatureGate. However, the root cause was mostly due to widely-scoped informer configurations. To remedy this, I've done the following:
- How to verify it
Verifying this outside of the test scenario described in OCPBUGS-92062 is difficult because we do not (yet) have a good way to enable pprof analysis at runtime and capture it. Therefore, verification should ensure that the objects are being updated as expected. The current E2E test suite should be able to verify this.
- Description for the changelog
Reduce memory usage in both the MCC and MCD
Summary by CodeRabbit