Add fastpath binpacking algorithm #9145
Conversation
k8s-ci-robot
added
release-note
|
Welcome @yonizxz! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yonizxz 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 |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @yonizxz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
area/cluster-autoscaler
size/L
yonizxz
force-pushed
the
fastpath-binpacking
branch
from
fe60f6a to
8569f24
Compare
yonizxz
force-pushed
the
fastpath-binpacking
branch
from
8569f24 to
d6c0d7a
Compare
| if newNodesAvailable { | ||
| newNodesAvailable, err = e.tryToScheduleOnNewNodes(estimationState, nodeTemplate, remainingPods) | ||
| // Since fastpath binpacking adds just one node to the snapshot, it will cause inaccurate simulations on subsequent loops, therefore we only use it on the last group | ||
| if i == len(podsEquivalenceGroups)-1 && useFastpathOnLastPEG { |
There was a problem hiding this comment.
nit: instead of checking both i == len(podEquivalenceGroups) - 1 and useFastpathOnLastPEG you could have just i == fastpathPEGIndex (fastpathPEGIndex would be -1 by default).
There was a problem hiding this comment.
I think it's more readable this way, we have to use fastpath only on the last pod group, the comment explains this and the condition checks explicitly that we're on the last pod group.
It could work with i == fastpathPEGIndex and a longer explanation in the comment explaining why it is only a single index, and another comment in the place that sets it to len(pegs)-1 that explains why it is the final index specifically.
But I think it's just easier to read this way.
| return false | ||
| } | ||
|
|
||
| func shouldUseFastPath(pods []*apiv1.Pod) bool { |
There was a problem hiding this comment.
nit: Pass PodEquivalenceGroup in arg and use pod := peg.Exemplar() instead of pods[0] in this function?
|
|
||
| // Fastpath assumes that once a pod failed to schedule on a node, it will never be possible to schedule any more pods on this node. | ||
| // This assumption is not correct in cases of topology spread constraints. | ||
| // Fastpath also assumes that if it was able to schedule pods on a new node, it will be able to then schedule an equal amount of pods on an additional identical new node, |
There was a problem hiding this comment.
I don't believe this is true. Why would zonal/regional anti affinity affect # of pods that can schedule on subsequent nodes?
There was a problem hiding this comment.
According to - https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#an-example-of-a-pod-that-uses-pod-affinity
With zonal/regional pod anti affinity, a pod can schedule on a node only if it is in a zone/region that doesn't contain another pod with the same value.
So fastpath will create a new node and will be able to schedule the first pod, and then won't be able to schedule the second one (there's already a pod in the same zone/region, since it's on the same node), and will then assume it can schedule the remaining x pods by creating x more nodes. which is not true, because the node group is bound to a location.
|
|
||
| if peg.Exemplar().Spec.Affinity != nil && peg.Exemplar().Spec.Affinity.PodAntiAffinity != nil { | ||
| for _, affinityTerm := range peg.Exemplar().Spec.Affinity.PodAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution { | ||
| if affinityTerm.TopologyKey == apiv1.LabelHostname { |
There was a problem hiding this comment.
Note: this isn't strictly sufficient - even with hostname topology, the pod can have anti-affinity to other groups of pods, not to other pods in the group it belongs to. Consider:
- Deployment A has anti-affinity on hostname topology for pods in Deployment B
- Deployment B has anti-affinity on hostname topology for pods in Deployment A
In this setup, pods from Deployment A can binpack just fine, as long as no pods from Deployment B are present.
Practically speaking though, I wonder if a simple heuristic of picking biggest PEG in terms of # of pods (assuming it is fastpath compatible) wouldn't be sufficient.
| expectPodCount: 12, | ||
| }, | ||
| { | ||
| name: "fastpath - simple resource-based binpacking", |
There was a problem hiding this comment.
It might be useful to have tests covering multiple PEGs as well. Also, for a single PEG, fastpath logic should yield the same result as non-fastpath, which would be good to validate in tests.
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
An optimization of the binpacking algorithm.
The standard binpacking algorithm operates by simulating the scheduling of all pods.
If pods cannot be scheduled on existing nodes in the simulation, new nodes are added to the simulation.
The Fastpath logic estimates the required number of nodes using simple arithmetic, avoiding the full simulation. It works by:
EstimatedNodes = ceil(TotalPods / PodsFittingOnASingleNode)Current BenchmarkBinpackingEstimate results:
BenchmarkBinpackingEstimate with fastpath enabled:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Additional explanation:
Since the algorithm only adds 1 node to the snapshot, subsequent iterations will not take the rest of the upcoming nodes into account during
tryToScheduleOnExistingNodes, therefore it is used only in the final iteration.In order to maximize gains from the optimization, a heuristic determines the pod equivalence group that will benefit the most from the fastpath algorithm and preemptively places it at the end of the list.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: