[InPlacePodVerticalScaling] refactor and increase coverage for pod resize tests #134283
Conversation
k8s-ci-robot
added
kind/cleanup
github-project-automation
bot
added this to
SIG Node: code and documentation PRs and SIG Node CI/Test Board
k8s-ci-robot
added
the
cncf-cla: yes
github-project-automation
bot
moved this to Triage
in SIG Node: code and documentation PRs
k8s-ci-robot
added
the
needs-triage
k8s-ci-robot
added
needs-priority
|
[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 |
|
/release-note-none |
k8s-ci-robot
added
release-note-none
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
natasha41575
changed the title
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
@natasha41575: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
Test failures look legit. |
+1 (but not in this PR) |
| containers: []podresize.ResizableContainerInfo{ | ||
| func doGuaranteedPodResizeTests(f *framework.Framework) { | ||
| ginkgo.Describe("guaranteed pods with a single regular container", func() { | ||
| ginkgo.DescribeTableSubtree("with various policies, init containers, and extended resources", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs, resizeInitCtrs, extendedResources bool) { |
There was a problem hiding this comment.
We're not resizing extended resources, just verifying that they're correctly copied to the status resources. Let's just add them on every test rather than making it a test dimension.
There was a problem hiding this comment.
I feel like we could do something similar with hasInitCtrs.
| ) | ||
| }) | ||
|
|
||
| ginkgo.DescribeTable("burstable pods - extended", func(ctx context.Context, originalContainers, expectedContainers []podresize.ResizableContainerInfo, doRollback bool) { |
There was a problem hiding this comment.
nit: can you give this a more descriptive name, or add a comment
| ginkgo.Describe("guaranteed pods with a single regular container", func() { | ||
| ginkgo.DescribeTableSubtree("with various policies, init containers, and extended resources", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs, resizeInitCtrs, extendedResources bool) { |
There was a problem hiding this comment.
nit: test case names are very verbose, e.g.
[sig-node] Pod InPlace Resize Container [FeatureGate:InPlacePodVerticalScaling] [Beta] guaranteed pods with a single regular container with various policies, init containers, and extended resources both cpu and mem restart policy, has initContainers, resize initContainers performing a resize operation increase cpu & decrease mem, then decrease cpu & increase mem
| ginkgo.Describe("guaranteed pods with a single regular container", func() { | |
| ginkgo.DescribeTableSubtree("with various policies, init containers, and extended resources", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs, resizeInitCtrs, extendedResources bool) { | |
| ginkgo.DescribeTableSubtree("guaranteed pods, single regular container", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs, resizeInitCtrs, extendedResources bool) { |
| ginkgo.Describe("burstable pods with a single regular container specifying all requests & limits", func() { | ||
| ginkgo.DescribeTableSubtree("with various policies", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs bool) { |
There was a problem hiding this comment.
I don't think you need the outer describe?
| ginkgo.Describe("burstable pods with a single regular container specifying all requests & limits", func() { | |
| ginkgo.DescribeTableSubtree("with various policies", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs bool) { | |
| ginkgo.DescribeTableSubtree("burstable pods, single regular container with all requests & limits", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs bool) { |
| func doGuaranteedPodResizeTests(f *framework.Framework) { | ||
| ginkgo.Describe("guaranteed pods with a single regular container", func() { | ||
| ginkgo.DescribeTableSubtree("with various policies, init containers, and extended resources", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs, resizeInitCtrs, extendedResources bool) { | ||
| ginkgo.DescribeTable("performing a resize operation", func(ctx context.Context, desiredCPU, desiredMem string) { |
There was a problem hiding this comment.
nit: for shorter test names
| ginkgo.DescribeTable("performing a resize operation", func(ctx context.Context, desiredCPU, desiredMem string) { | |
| ginkgo.DescribeTable("resize", func(ctx context.Context, desiredCPU, desiredMem string) { |
|
|
||
| func doBurstablePodResizeTests(f *framework.Framework) { | ||
| ginkgo.Describe("burstable pods with a single regular container specifying all requests & limits", func() { | ||
| ginkgo.DescribeTableSubtree("with various policies", func(cpuPolicy, memPolicy v1.ResourceResizeRestartPolicy, hasInitCtrs bool) { |
There was a problem hiding this comment.
Let's drop hasInitCtrs here too. I don't think we even need to add them to these tests - that's already covered by the guaranteed cases.
| ginkgo.By("creating and verifying pod") | ||
| newPod := createAndVerifyPod(ctx, f, podClient, originalContainers) | ||
|
|
||
| ginkgo.By("patching and verifying pod for resize") | ||
| patchAndVerify(ctx, f, podClient, newPod, originalContainers, expectedContainers, "resize") | ||
|
|
||
| // Resize has been actuated, test the reverse operation. | ||
| rollbackContainers := createRollbackContainers(originalContainers, expectedContainers) | ||
| ginkgo.By("patching and verifying pod for rollback") | ||
| patchAndVerify(ctx, f, podClient, newPod, expectedContainers, rollbackContainers, "rollback") | ||
|
|
||
| ginkgo.By("deleting pod") | ||
| podClient.DeleteSync(ctx, newPod.Name, metav1.DeleteOptions{}, f.Timeouts.PodDelete) |
There was a problem hiding this comment.
Is this logic common among the different tables? If so, extract it to a shared function?
What type of PR is this?
/kind cleanup
/area test
What this PR does / why we need it:
This PR is primarily a refactor of the pod resize e2e tests, largely for readability, but also increases coverage to cover a larger combination of things and more error cases.
I was going through them to make sure we don’t have significant gaps in coverage prior to GA, but I found it a little hard to systematically determine what cases we are covering, hence the refactor. I updated them to use nested ginkgo.Tables rather than go native subtests.
It is not practical to cover every possible combination of resize operations imaginable — the dimensions being qos, number of containers, number of init containers, operation (add, remove, increase, decrease), resource requirement (req or lim), and resource type (cpu or memory). This PR may have a slightly different subset of the possible combinations than the current tests have, but coverage of everything is still there and this PR definitely increases coverage. In fact, I think that some of the tests here are redundant and I’d be open to removing some of them for the sake of faster execution of the suite.
Special notes for your reviewer:
Some of the tests here (and some of the modifications I made to the test helpers) should be included in the v1.35 conformance suite, so this needs to merge early to allow for the 2 weeks of required soak time before we promote to conformance.
/sig node
/release-note-none