[FG:InPlacePodVerticalScaling] Add a more complex e2e test for deferred resizes #132152
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
61c5761 to
6a356fd
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: natasha41575 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Do we have a presubmit job to run e2e serial tests? Though |
| CPUReq: testPod2CPUQuantityResized.String(), | ||
| CPULim: testPod2CPUQuantityResized.String(), | ||
| // Convert to KiB because ExpectPodResized does a string comparison. | ||
| MemReq: fmt.Sprintf("%dKi", testPod2MemoryQuantityResized.Value()/1024), |
There was a problem hiding this comment.
If the value is rounded at division, the comparison may fail.
| delErr3 := e2epod.DeletePodWithWait(ctx, f.ClientSet, testPod3) | ||
| framework.ExpectNoError(delErr3, "failed to delete pod %s", testPod3.Name) | ||
|
|
||
| // Deferred resize E2E test case #2: |
There was a problem hiding this comment.
Do we need to put two cases under one It?
| delErr2 = e2epod.DeletePodWithWait(ctx, f.ClientSet, testPod2) | ||
| framework.ExpectNoError(delErr2, "failed to delete pod %s", testPod2.Name) | ||
| delErr3 = e2epod.DeletePodWithWait(ctx, f.ClientSet, testPod3) | ||
| framework.ExpectNoError(delErr3, "failed to delete pod %s", testPod3.Name) |
There was a problem hiding this comment.
Shouldn't we also delete testPod4?
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Creates a more complex e2e test for deferred resizes, where there are several pending resizes that each make room for the next one. Admittedly a slightly contrived scenario, but I still think it's important that this works correctly, especially in contexts where there may be a controller making frequent resize requests.
Motivation: The logic for evaluating pending resizes is becoming more complicated and error prone with #131612. The test ensures that even when there are multiple pending resizes that depend on each other, (a) re-evaluation of the pending resize queue is triggered appropriately and (b) there are no deadlocks, which I was slightly nervous about with the mutex updates.
Special notes for your reviewer:
I was kind of on the fence about whether I should just add this to #131612 or make a separate PR, but since #131612 has already gone through some reviews I figured I wouldn't extend its scope. Also I checked that the test passes with both HEAD today and #131612 which demonstrates that #131612 doesn't change the behavior.
Does this PR introduce a user-facing change?
/sig node
/priority important-soon
/assign @tallclair