Use ScaleDownSetProcessor.GetNodesToRemove in scale down planner to filter NodesToDelete. #5330
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
the
size/M
| @@ -117,21 +117,20 @@ func (n *Nodes) Drop(node string) { | |||
| // RemovableAt returns all nodes that can be removed at a given time, divided | |||
| // into empty and non-empty node lists, as well as a list of nodes that were | |||
| // unneeded, but are not removable, annotated by reason. | |||
| func (n *Nodes) RemovableAt(context *context.AutoscalingContext, ts time.Time, resourcesLeft resource.Limits, resourcesWithLimits []string, as scaledown.ActuationStatus) (empty, needDrain []*apiv1.Node, unremovable []*simulator.UnremovableNode) { | |||
| func (n *Nodes) RemovableAt(context *context.AutoscalingContext, ts time.Time, resourcesLeft resource.Limits, resourcesWithLimits []string, as scaledown.ActuationStatus) (empty, needDrain []simulator.NodeToBeRemoved, unremovable []*simulator.UnremovableNode) { | |||
There was a problem hiding this comment.
Since you're already changing the function signature to return simulator.NodeToBeRemoved, I think it makes sense to just put empty & non-empty on a single list and let the caller worry about splitting them.
There was a problem hiding this comment.
I don't have strong opinion on this, but both callers of this method require distinction between empty and non-empty nodes to remove. Does it make sense then to merge the lists into one?
There was a problem hiding this comment.
I think this results in a slightly cleaner interface. Don't have a too strong opinion on this either though, so up to you.
There was a problem hiding this comment.
If there is no strong opinion here, I would leave it as is, given we need the distinction.
|
LGTM, putting on hold to give you a chance to consider my comment. Feel free to cancel hold if you disagree. /lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
| @@ -282,7 +282,10 @@ func (sd *ScaleDown) NodesToDelete(currentTime time.Time, pdbs []*policyv1.PodDi | |||
| } | |||
|
|
|||
| scaleDownResourcesLeft := sd.resourceLimitsFinder.LimitsLeft(sd.context, allNodes, resourceLimiter, currentTime) | |||
| empty, nonEmpty, unremovable := sd.unneededNodes.RemovableAt(sd.context, currentTime, scaleDownResourcesLeft, resourceLimiter.GetResources(), sd.nodeDeletionTracker) | |||
| emptyToRemove, nonEmpty, unremovable := sd.unneededNodes.RemovableAt(sd.context, currentTime, scaleDownResourcesLeft, resourceLimiter.GetResources(), sd.nodeDeletionTracker) | |||
| for _, emptyNode := range emptyToRemove { | |||
There was a problem hiding this comment.
Do we need the empty list? Isn't it only used to extract the name in the loop below? Couldn't we just do the same thing as with nonEmpty?
There was a problem hiding this comment.
Actually it is also returned, so we do need that.
There was a problem hiding this comment.
I mean we're setting the variable here, but empty isn't returned anywhere from this function, right? It's only used to create candidateNames. Or am I reading things wrong?
There was a problem hiding this comment.
It is named return value, see line 257 :)
There was a problem hiding this comment.
discussed offline, removed empty
filter NodesToDelete.
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olagacek, towca The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Which component this PR applies to?
cluster-autoscaler
What type of PR is this?
/kind feature
What this PR does / why we need it:
Allows filtering of scale down candidates in scale down planner.
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: