Add additional test cases to scale down budget tests. #6055
Conversation
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
area/cluster-autoscaler
size/L
| @@ -298,6 +362,62 @@ func TestCropNodesToBudgets(t *testing.T) { | |||
| wantEmpty: generateNodeGroupViewList(testNg, 0, 4), | |||
| wantDrain: generateNodeGroupViewList(testNg, 0, 2), | |||
| }, | |||
| "empty&drain atomic nodes with deletions in progress, overall budget exceeded, both empty&drain nodes fit": { | |||
There was a problem hiding this comment.
both empty&drain nodes fit
nit: it looks like only empty nodes fit in this test case? Unless I'm misunderstanding something :)
There was a problem hiding this comment.
You are right, fixed it :)
| wantEmpty: generateNodeGroupViewList(atomic5, 0, 1), | ||
| wantDrain: generateNodeGroupViewList(atomic5, 1, 5), | ||
| }, | ||
| "empty&drain regular and atomic nodes with deletions in progress, overall budget exceeded, only atomic is drained": { |
There was a problem hiding this comment.
only atomic is drained
nit: it seems that in this test case nothing is drained, only the entirely empty atomic group is removed?
There was a problem hiding this comment.
Fixed the test case name
|
Wow, that's a lot of test cases! Thank you very much, @hbostan! Left some nits, in general I think the descriptions could be a bit more detailed to make it easier to understand what is the expected behavior. But I don't think that's blocking. @MaciekPytel I hope this addresses your concerns about unit test coverage? /lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
| }, | ||
| "empty&drain regular and multiple atomic nodes in same group exceeding drain limit, no deletions in progress": { | ||
| empty: append(append(generateNodeGroupViewList(testNg, 0, 5), generateNodeGroupViewList(atomic8, 0, 5)...), generateNodeGroupViewList(atomic11, 0, 8)...), | ||
| drain: append(generateNodeGroupViewList(atomic11, 8, 3), generateNodeGroupViewList(atomic8, 5, 8)...), |
There was a problem hiding this comment.
generateNodeGroupViewList(atomic11, 8, 3) - shouldn't from < to? Now this returns no nodes
There was a problem hiding this comment.
Nice catch! It should be correct now.
| wantDrain: generateNodeGroupViewList(atomic8, 5, 8), | ||
| }, | ||
| "empty&drain multiple atomic nodes in same group exceeding overall limit, no deletions in progress": { | ||
| empty: append(append(generateNodeGroupViewList(atomic3, 0, 3), generateNodeGroupViewList(atomic4, 0, 2)...), generateNodeGroupViewList(atomic11, 0, 8)...), |
There was a problem hiding this comment.
atomic11 has 8 nodes, I think it should have 11.
Tests were missing cases where the empty and drain nodes are from the same atomic group.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BigDarkClown, hbostan 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 cleanup
What this PR does / why we need it:
Tests were missing cases where the empty and drain nodes are from the same atomic group.
Which issue(s) this PR fixes:
Special notes for your reviewer:
This is a follow-up pr addressing this comment: https://github.com/kubernetes/autoscaler/pull/6034/files#r1291199619
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: