Fix scale up with no assigned node #467
Conversation
go/controller/cluster.go
Outdated
| func syncNodesIdleResource(podList *v1.PodList, nodesCPUIdleMilli map[string]int64, nodesMemoryIdleMega map[string]int64) (err error) { | ||
| for _, pod := range podList.Items { | ||
| nodeName := pod.Spec.NodeName | ||
| // if nodeName is empty, the pod is not assigned. |
There was a problem hiding this comment.
This comment seems not needed, the below code is clear enough.
| @@ -282,6 +309,11 @@ func scaleDryRun(r *ClusterResource, j job, curDiff int, maxLoadDesired float64, | |||
| return | |||
| } | |||
|
|
|||
| if !searchAssignableNodeByCPU(r, j) || !searchAssignableNodeByMem(r, j) { | |||
| // can not find assignable node, do not scale | |||
| additional = 0 | |||
There was a problem hiding this comment.
Need to return immediately from here, or else the code below may be executed.
go/autoscaler/autoscaler.go
Outdated
| assignable = false | ||
| for _, idle := range r.NodesCPUIdleMilli { | ||
| if j.TrainerCPURequestMilli() <= idle { | ||
| assignable = true |
There was a problem hiding this comment.
In go, if return value is declared with a var name, return will return the var name directly, this is useful when we need to declare the return value at the begining of the function, like var ret bool.
In your case, just define return type bool and return false or return true will do the job.
There was a problem hiding this comment.
Thanks, got it. Done.
go/controller/cluster.go
Outdated
|
|
||
| NodesCPUIdleMilli: nodesCPUIdleMilli, | ||
| NodesMemoryIdleMega: nodesMemoryIdleMega, |
There was a problem hiding this comment.
We say idle cpu and free memory, not idle memory.
| if nodeName == "" { | ||
| continue | ||
| } | ||
| for _, container := range pod.Spec.Containers { |
There was a problem hiding this comment.
Should we check only pending and running pods?
There was a problem hiding this comment.
Only Terminating and Running Pod will take up the resource.
There was a problem hiding this comment.
Oh ok, so maybe we need to check only "Terminating and Running" pod? :)
There was a problem hiding this comment.
I think so, follow the comment https://github.com/kubernetes/kubernetes/blob/v1.6.2/pkg/api/v1/types.go#L2331, if NodeName is non-empty, the Pod will be assigned to the Node, mean take up the resource on the Node.
There was a problem hiding this comment.
I see, so it's not necessary to check pod status, since as long as pod.Spec.NodeName is not empty, it's assigned to a node, and occupying resource.
|
LGTM after the test passes. |
| @@ -505,7 +498,9 @@ func (a *Autoscaler) Monitor() { | |||
| continue | |||
| } | |||
| j.TrainerJob = tj | |||
| a.jobs[key] = j | |||
There was a problem hiding this comment.
Need to write back to the map, j.TrainerJob = tj is not sufficient.
There was a problem hiding this comment.
Refine the code later is fine. Also, the scaling algorithm should be updated like:
ScaleDryRunsearch in order to find a sufficient node to scale up 1 pod, return the diff.- Cluster total resource is not used for the scale-up, use only per node resource.
- Find a way to ensure
max_load_desiredfor each node.
I'll add a design doc for details.
There was a problem hiding this comment.
Thanks @typhoonzero !
Btw, maybe max_load_desired is for the entire cluster? Otherwise if it's for each node on max_load_desired=0.97, each node will have 0.03*12=0.36 CPUs, probably many pod can not be scheduled on it anyway.
Fixed #465