perf: Attempt to improve SimulateScheduling efficiency - #3240
Open
GnatorX wants to merge 7 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GnatorX The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
GnatorX
marked this pull request as ready for review
August 18, 2026 01:27
GnatorX
marked this pull request as draft
August 18, 2026 01:27
GnatorX
marked this pull request as ready for review
August 18, 2026 22:59
|
PR needs rebase. 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2972
Problem
We don't need to deep copy of all cluster nodes. From what I can tell we do deep copy for two reasons.
But deep copy is problematic because:
Memory Isolation
Looking at the code, we don't need to do the isolation because we only update state nodes during simulation for 2 fields,
HostPortUsageandVolumeUsage. We will make this part of statenode and avoid deep copying actual node object.We don't mutate underlying nodes too much (most happen against statenode) so attempting to move all nodes data that gets mutated to statenode so we can share cluster.nodes() data.
When controller.disrupt() happens we make a deep copy:
karpenter/pkg/controllers/disruption/helpers.go
Line 207 in d1aacae
Then each disruption method (consolidation and drift) runs
SimulateSchedulingkarpenter/pkg/controllers/disruption/helpers.go
Lines 53 to 57 in d1aacae
and makes another deep copy of nodes.
Only place here underlying node is mutated during disruption:
karpenter/pkg/controllers/provisioning/scheduling/existingnode.go
Lines 178 to 179 in d1aacae
This means that we likely can get away without copying this every time.
Consistent Snapshot
To address the second problem, we are going to move to taking snapshots of state of nodes instead of deep copy.
This was problematic because we have mutations that occurs during various events against the nodes:
field write still hit the live node).
still exists" branch did c.nodes[id].NodeClaim = nil / c.nodes[id].Node = nil straight on the map's pointer.
we found and fixed).
So each mutation action needs to create a new version of statenode now (not a deep copy so this is less memory intensive).
updateNodeUsageFromPod)n.updateForPod(...)mutated the live*StateNodesitting inc.nodes[id]directlyCopyForMutation()— clonespodRequests,podLimits,daemonSetRequests,daemonSetLimits,podDisruptionCosts,hostPortUsage,volumeUsage; sharesNode/NodeClaimupdateNodeUsageFromPodCompletion)n.cleanupForPod(...)mutated the live node directlyCopyForMutation()— same clone set as abovecleanupOldBindings)CopyForMutation()— same clone set as aboveNominateNodeForPod)n.Nominate(...)mutatednominatedUntildirectlyShallowCopy()— copies just the struct;nominatedUntilset on the copyShallowCopy(), but only for diffing —oldNode := n.ShallowCopy()was taken to compute the resource-pool delta, butn.markedForDeletionwas still flipped on the live node, not the copyShallowCopy(), and it's what gets published — flipsmarkedForDeletionon the copy, swaps it intoc.nodes[id]UpdateNodenewStateFromNodeconstructs a brand-new&StateNode{}(unchanged by this refactor)&StateNode{}; only change isc.generation++added after the swapUpdateNodeClaimnewStateFromNodeClaimconstructs a brand-new&StateNode{}(unchanged by this refactor)&StateNode{}; only change isc.generation++addedcleanupNodeClaim/cleanupNode, other side still exists)oldNode := c.nodes[id].ShallowCopy()was taken for diffing only;c.nodes[id].NodeClaim = nil(or.Node = nil) mutated the live node directlyShallowCopy(), and it's what gets published — nils the field on the copy, swaps it incleanupNodeClaim/cleanupNode, nothing left)delete(c.nodes, id)— no copy needed either waydelete(c.nodes, id), plusc.generation++Reset()c.nodes = map[string]*StateNode{}— no copy involved, no generation to invalidate (no cache existed)c.generation++andc.cachedSnap = nil(the bug we found: forgetting this leftSnapshot()serving the stale pre-Reset cache)DeepCopyNodes()/Snapshot())DeepCopy()on every node, every call — full clone ofNode,NodeClaim, all 5 maps (with per-Quantityclones), both usage trackerslo.Values(c.nodes), a pointer-slice copy, memoized by generation; cache-miss cost is O(n) pointer copies, not clonesOutcome
Replaces Cluster.DeepCopyNodes()'s full deep-clone-on-every-call with a copy-on-write model: all write paths (pod bind/unbind, mark-for-deletion, nomination, cleanup) now clone-then-swap only the specific
mutable fields they touch, instead of mutating live *StateNodes in place. Reads get a generation-counter-cached Snapshot() that returns a memoized pointer slice when nothing has changed since the last
call, and rebuilds a cheap pointer copy (not a deep clone) otherwise. ExistingNode now clones just its own host-port/volume usage trackers at construction, so SimulateScheduling no longer needs a
caller-provided deep copy at all.
Net effect: snapshot/candidate-construction cost drops from O(n) deep clone to near-O(1) cache hits, at the cost of a modest, bounded increase in per-mutation-event cost on the write path
(bind/unbind/nominate/mark).
New metrics
Added the per-candidate evaluation latency metric. CandidateEvaluationDurationSeconds
karpenter_voluntary_disruption_candidate_evaluation_duration_secondsnow wraps every SimulateScheduling call site: consolidation.go (single + multi-node), drift.go's per-candidate loop, and validation.go's final revalidation, labeled by reason, consolidation_type, and stage (evaluate vs validate)How was this change tested?
Perf test:
Snapshot/DeepCopyNodes(5000 nodes)Snapshot/DeepCopyNodes(400 nodes)ClusterDeepCopyNodes(5000, envtest-backed)CopyForMutationvsDeepCopy(100 pods/node)DeepCopy)CopyForMutation)SimulateSchedulingend-to-end (5000 nodes)SimulateSchedulingend-to-end (400 nodes)UpdatePodwrite path, high churn (1000 events/round)MarkForDeletion/NominateNodeForPodBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.