Cluster-autoscaler: remove nodes that have been unregistered for a long time #2292
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
This change is |
|
[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files: We suggest the following people: |
| glog.V(1).Infof("%d unregistered nodes present", len(unregisteredNodes)) | ||
| removedAny, err := removeOldUnregisteredNodes(unregisteredNodes, &autoscalingContext, time.Now()) | ||
| // There was a problem with removing unregistered nodes. Retry in the next loop. | ||
| if err != nil { |
There was a problem hiding this comment.
You are not handling correctly the case when some nodes were removed, but error was observed afterwards: "Some unregistered nodes were removed" should be printed before printing error.
There was a problem hiding this comment.
Do you mean the error itself message. Yeah - we can have 2 different here.
cluster-autoscaler/utils_test.go
Outdated
| assert.Equal(t, 1, len(unregisteredNodes)) | ||
|
|
||
| // Nothing should be removed. The unregistered node is not old enough. | ||
| removed, err := removeOldUnregisteredNodes(unregisteredNodes, context, time.Now().Add(-50*time.Minute)) |
There was a problem hiding this comment.
Nit: do not call Now() multiple times. Call it once and remember the value.
|
Just two minor comments, after applying them: LGTM |
mwielgus
force-pushed
the
remove-unregistered
branch
from
c025aa7 to
7f50928
Compare
|
Automatic merge from submit-queue |
…ve-unregistered Automatic merge from submit-queue Cluster-autoscaler: remove nodes that have been unregistered for a long time Ref: #2228 cc: @jszczepkowski @fgrzadkowski
…ve-unregistered Automatic merge from submit-queue Cluster-autoscaler: remove nodes that have been unregistered for a long time Ref: #2228 cc: @jszczepkowski @fgrzadkowski
5 participants
Ref: #2228
cc: @jszczepkowski @fgrzadkowski