fix(kwok): prevent quitting when scaling down node group #6336
Conversation
k8s-ci-robot
added
kind/bug
qianlei90
force-pushed
the
fix-kwok-provider
branch
from
3a67a32
to
ca0bb24
Compare
| nodeGroup.targetSize += 1 | ||
| } | ||
|
|
||
| nodeGroup.targetSize = newSize | ||
|
|
There was a problem hiding this comment.
For a case in which some nodes are created successfully and some fail
There was a problem hiding this comment.
Makes sense. Should we add a test case around this (for both IncreaseSize and DeleteNodes)?
|
/assign @vadasambar |
|
Thank you for the PR! |
|
I can't reproduce the issue. I used the same commands and configmap you used. Here's what I did: kwok provider created 2 fake nodes. And then kwok provider scaled down the 2 fake nodes. Logs for reference: https://gist.github.com/vadasambar/56ac07f2eedbd97e5d8aaa1424df3481 |
|
Maybe you can share the error you saw? |
|
@vadasambar Sorry for the misunderstanding in the title. CA does not panic; it simply quits without any stack information. The latest line in your log shows the reason: |
qianlei90
changed the title
| no.GetName()) | ||
| } | ||
|
|
||
| ngName = fmt.Sprintf("%s-%v", ngName, time.Now().Unix()) |
There was a problem hiding this comment.
I think it might be better to keep the node group name unchanged, especially in cases where nodes still remain in the cluster and CA is restarted.
There was a problem hiding this comment.
Line 275 is clearly a bug. The original intention was to ensure targetSize of a nodegroup correctly reflects the number of nodes with matching annotation/label. Making sure nodegroup has a unique name on every pod restart achieves this.
If we already have nodes in the cluster with matching nodegroup annotations/labels and we remove time.Now().Unix() suffix, when creating nodegroups (imagine the CA pod got restarted) the target size won't accurately reflect the actual nodegroup size because there are more nodes matching nodegroup label in the cluster now.
One solution can be to implement Refresh and update the nodegroup target size based on the actual matching nodes in the cluster.
There was a problem hiding this comment.
The Kwok provider will calculate the target sizes during startup, and the Cluster Autoscaler will scale these nodegroups to the appropriate size. I think it's not necessary to calculate the target size in the Refresh function, just leave this work to CA may be a better choice.
There was a problem hiding this comment.
kwok provider will clean all the fake node when CA quit:
// Cleanup cleans up all resources before the cloud provider is removed
func (kwok *KwokCloudProvider) Cleanup() error {
for _, ng := range kwok.nodeGroups {
nodeNames, err := ng.getNodeNamesForNodeGroup()
if err != nil {
return fmt.Errorf("error cleaning up: %v", err)
}
for _, node := range nodeNames {
err := kwok.kubeClient.CoreV1().Nodes().Delete(context.Background(), node, v1.DeleteOptions{})
if err != nil {
klog.Errorf("error cleaning up kwok provider nodes '%v'", node)
}
}
}
return nil
}There was a problem hiding this comment.
True. I was considering a case when Cleanup doesn't clean all the nodes (it doesn't work correctly for whatever reason)
The Kwok provider will calculate the target sizes during startup, and the Cluster Autoscaler will scale these nodegroups to the appropriate size. I think it's not necessary to calculate the target size in the Refresh function, just leave this work to CA may be a better choice.
I think we might have to try this out to confirm if CA can handle such a situation. If you can try it out as a part of this PR, great. If not, we can take care of it in another issue.
There was a problem hiding this comment.
Hi @vadasambar, I have implemented the Refresh() function. Please take a look.
This is clearly a bug. Thank you for the explanation! |
Thanks for you advise, it will be done in a few days. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
qianlei90
force-pushed
the
fix-kwok-provider
branch
from
ca0bb24
to
3feb6ce
Compare
k8s-ci-robot
added
size/M
qianlei90
force-pushed
the
fix-kwok-provider
branch
from
3feb6ce
to
1b8f1cc
Compare
k8s-ci-robot
added
size/L
Done. /unhold |
| switch opts.CloudProviderName { | ||
| case cloudprovider.KwokProviderName: | ||
| return kwok.BuildKwokCloudProvider(opts, do, rl)(opts, do, rl) | ||
| return kwok.BuildKwok(opts, do, rl, informerFactory) |
|
@qianlei90 apologies for the delay (was out on vacation). I plan to review this PR this week. |
| // ngs = append(ngs, ng) | ||
| // } | ||
| for _, node := range allNodes { | ||
| ngName := getNGName(node, kwok.config) |
There was a problem hiding this comment.
This will lead to klog.Fatal for cases where the node doesn't have the nodegroup label or annotation.
You might have to use a filter function to filter only nodes which have the nodegroup label/annotation OR convert klog.Fatal into an non-fatal error log.
There was a problem hiding this comment.
ok. I converted klog.Fatal to klog.Warning and added comments to getNGName.
| } | ||
|
|
||
| for _, ng := range kwok.nodeGroups { | ||
| ng.targetSize = targetSizeInCluster[ng.Id()] |
There was a problem hiding this comment.
Maybe something for the future: I wonder if we should delete nodes which have ng that is not a part of kwok.nodeGroups.
| assert.NoError(t, err) | ||
| assert.NotNil(t, p) | ||
|
|
||
| err = p.Refresh() |
There was a problem hiding this comment.
kwokConfig.status is coming out nil here for some reason because of which node4 test case is not throwing an error
There was a problem hiding this comment.
Do you mean the p.config.status is nil? I ran this test case in debug mode and found that this field was not nil. Is this the expected value?
There was a problem hiding this comment.
Sorry, it was a misunderstanding on my end. I ran the test again. Looks good to me.
qianlei90
force-pushed
the
fix-kwok-provider
branch
from
1b8f1cc
to
e71a123
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
Thank you @qianlei90 . LGTM. @BigDarkClown can you please merge the PR 🙏 |
|
/assign @towca |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qianlei90, towca, vadasambar 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
What type of PR is this?
/kind bug
What this PR does / why we need it:
When using the Kwok provider, CA quits when scaling down a node group because the Kwok provider cannot retrieve the node group name from a fake node. This PR primarily aims to fix this issue.
Additionally, I have fixed the target size when scaling up and down the node group.
Which issue(s) this PR fixes:
kwok-provider-config
kwok-provider-templates
starting CA
POD_NAMESPACE=default KWOK_PROVIDER_MODE=local ./cluster-autoscaler-amd64 --cloud-provider=kwok \ --namespace=default \ --kubeconfig=<kubeconfig> \ --expander=random \ --scale-down-enabled=true \ --scale-down-utilization-threshold=0.5 \ --scale-down-gpu-utilization-threshold=0.5 \ --scale-down-delay-after-add=10s \ --scale-down-delay-after-failure=10s \ --scale-down-unneeded-time=0s \ --skip-nodes-with-system-pods=true \ --skip-nodes-with-local-storage=true \ --logtostderr=true \ --stderrthreshold=info \ --leader-elect=false \ --v=4 \ --scan-interval=3sscale this deployment to test scale up and down
CA log
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: