Add expire time for nodeInfo cache items #4669
Conversation
k8s-ci-robot
added
kind/bug
yaroslava-serdiuk
force-pushed
the
node_info_cache
branch
from
e80f889 to
b2943ad
Compare
|
/assign @x13n |
cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor.go
Outdated
Show resolved
Hide resolved
cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor.go
Show resolved
Hide resolved
yaroslava-serdiuk
force-pushed
the
node_info_cache
branch
2 times, most recently
from
fd7c94b to
336301e
Compare
cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor_test.go
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| func (p *MixedTemplateNodeInfoProvider) isCacheItemExpired(added time.Time) bool { | ||
| if p.ttl == nil { |
There was a problem hiding this comment.
nit: Why do you use a pointer at all? If ttl was a regular value you wouldn't have to do this.
There was a problem hiding this comment.
I left pointer for test that mocks MixedTemplateNodeInfoProvider, so there is no need to pass a specific parameter there. However it should be handled during provider creation.
There was a problem hiding this comment.
So you have different state for tests and a different one for actual execution. If you used a const like veryLongTime = 123456 * time.Hour in the tests, you wouldn't need special casing nil.
There was a problem hiding this comment.
I wouldn't say that state for test is different, a nil pointer correspond to default value — the behavior is identical as if --node-info-cache-expire-time flag is not set. Anyway we need to handle a nil pointer at same place, and I think this is the best place: https://github.com/kubernetes/autoscaler/pull/4669/files#diff-ad9d6ea8aa127e0dc3b271f9b5761df7ba18f9fd63ffd0a7f69a6886c86ef317R54
I think that if new test is introduced that doesn't test nodeInforCache directly but use it, then the writer shouldn't come up with a new veryLongTime variable.
Please, explain if my line of thought is wrong.
(Also, after your argument I used nil pointer for initialising in DefaultProcessor(), which is used not only for test.)
yaroslava-serdiuk
force-pushed
the
node_info_cache
branch
from
336301e to
c25d8e8
Compare
yaroslava-serdiuk
force-pushed
the
node_info_cache
branch
from
c25d8e8 to
a9a7d98
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @MaciekPytel |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: towca, yaroslava-serdiuk 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 bug
What this PR does / why we need it:
Adding expire time for nodeInfo cache items. Expire time could be set via flag
--node-info-cache-expire-time, by default there is no expire time (old behavior).Currently, item in the node info cache is updated only if there are any nodes in the node group. If node group is scaled down to 0, node info cache will store last node and use it in scale up algorithm. Thus if the last node is a wrong representative of the node group (for example it has custom taints), then Cluster Autoscaler may struggle to scale up this node group.