Add ControllerModifyVolume E2E tests #1826

travisyx · 2024-09-17T17:57:38Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

This PR adds ControllerModifyVolume E2E tests. This allows us to directly test the various APIs to check for any breaking changes in the future with the clients used.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2024-09-17T17:57:47Z

Hi @travisyx. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

amacaskill · 2024-09-17T18:00:12Z

/ok-to-test

amacaskill · 2024-09-17T20:23:14Z

test/run-e2e.sh

@@ -5,4 +5,4 @@ set -x

 readonly PKGDIR=sigs.k8s.io/gcp-compute-persistent-disk-csi-driver

-go test --timeout 30m --v "${PKGDIR}/test/e2e/tests" --run-in-prow=true --delete-instances=true --logtostderr $@
+go test --timeout 45m --v "${PKGDIR}/test/e2e/tests" --run-in-prow=true --delete-instances=true --logtostderr $@


previously these tests were taking 27 min ish to run, and now they are taking 44 min. Why do these tests make the tests run so much longer?

The changes from ControllerModifyVolume take 5-6 minutes to be reflected. I added tests for both IOPS/throughput, just IOPS, and just throughput, causing the tests to take longer

Do these e2e tests not run in parallel?

No, they don't run in parallel by default

amacaskill · 2024-09-17T20:25:36Z

test/e2e/tests/single_zone_e2e_test.go

@@ -1549,6 +1560,84 @@ var _ = Describe("GCE PD CSI Driver", func() {
 		Entry("with missing multi-zone label", multiZoneTestConfig{diskType: standardDiskType, readOnly: true, hasMultiZoneLabel: false, wantErrSubstring: "points to disk that is missing label \"goog-gke-multi-zone\""}),
 		Entry("with unsupported disk-type pd-extreme", multiZoneTestConfig{diskType: extremeDiskType, readOnly: true, hasMultiZoneLabel: true, wantErrSubstring: "points to disk with unsupported disk type"}),
 	)
+
+	DescribeTable("Should update metadata when providing valid metadata",


If we cherry pick PRs back to previous release branches (before your change), is there a way to skip these test cases ? Otherwise I think these will fail against PRs merged to old release branches. In the test log, I see

Ran 43 of 51 Specs in 1471.037 seconds SUCCESS! -- 43 Passed | 0 Failed | 2 Pending | 6 Skipped

So I think there is a way to skip somewhere

I changed the table to be pending, so this should skip for older versions. Is this sufficient?

How does that make it skip for older versions? I see your tests now are pending even for this PR. I'm assuming that means they didn't run at all. What are the tests we are skipping now, and can we make those be skipped based on release branch?

The other tests that were skipped are the tests in multi_zone_e2e_test.go. They get skipped if an environment variable isn't set. It doesn't seem like doing the same for these tests makes sense as they just wouldn't run on the tests for this PR. I set the tests to read the latest tag information and base the test skip on if the version is before v1.15.0.

Actually, based on a past new feature / new e2e test (70148a9#diff-2547c5a5595d73ed4720c414c96d15d919e53abf7441f91f1b2c7250196a2606) I don't think this is an issue I guess?

So basing test skips off an environment variable is fine? I guess that would make sense especially given that the tests were verified to be passing from an earlier PR run

k8s-ci-robot · 2024-09-20T22:46:57Z

@travisyx: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gcp-compute-persistent-disk-csi-driver-verify	`a72a91e`	link	true	`/test pull-gcp-compute-persistent-disk-csi-driver-verify`

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.

amacaskill · 2024-09-22T00:38:20Z

/lgtm
/approve

k8s-ci-robot · 2024-09-22T00:38:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amacaskill, travisyx

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:

~~OWNERS~~ [amacaskill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

travisyx · 2024-11-05T18:28:41Z

/cherry-pick release-1.15

k8s-infra-cherrypick-robot · 2024-11-05T18:28:50Z

@travisyx: only kubernetes-sigs org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually.

In response to this:

/cherry-pick release-1.15

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.

…1836-#1838-#1841-upstream-release-1.15 Automated cherry pick of #1826: Add ControllerModifyVolume E2E tests #1836: Create documentation for ControllerModifyVolume and controller default #1838: Enable VolumeAttributesClass feature gate for CI runs #1841: Update logic to use SI for VACs

…ModifyVolume E2E tests kubernetes-sigs#1836: Create documentation for ControllerModifyVolume and controller default kubernetes-sigs#1838: Enable VolumeAttributesClass feature gate for CI runs kubernetes-sigs#1841: Update logic to use SI for VACs"

…ick-of-#1826-#1836-#1838-#1841-upstream-release-1.15 Revert "Automated cherry pick of #1826: Add ControllerModifyVolume E2E tests#1836: Create documentation for ControllerModifyVolume and controller default#1838: Enable VolumeAttributesClass feature gate for CI runs#1841: Update logic to use SI for VACs"

k8s-ci-robot requested review from pwschuurman and saikat-royc September 17, 2024 17:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 17, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 17, 2024

amacaskill reviewed Sep 17, 2024

View reviewed changes

Add ControllerModifyVolume e2e tests

a72a91e

travisyx force-pushed the master branch from 371dffc to a72a91e Compare September 20, 2024 22:14

k8s-ci-robot assigned amacaskill Sep 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2024

amacaskill merged commit feb8570 into kubernetes-sigs:master Sep 22, 2024
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ControllerModifyVolume E2E tests #1826

Add ControllerModifyVolume E2E tests #1826

travisyx commented Sep 17, 2024 •

edited

Loading

k8s-ci-robot commented Sep 17, 2024

amacaskill commented Sep 17, 2024

amacaskill Sep 17, 2024

travisyx Sep 17, 2024

amacaskill Sep 17, 2024

travisyx Sep 19, 2024

amacaskill Sep 17, 2024

travisyx Sep 17, 2024

amacaskill Sep 17, 2024

travisyx Sep 19, 2024

amacaskill Sep 19, 2024

travisyx Sep 19, 2024

k8s-ci-robot commented Sep 20, 2024

amacaskill commented Sep 22, 2024

k8s-ci-robot commented Sep 22, 2024

travisyx commented Nov 5, 2024

k8s-infra-cherrypick-robot commented Nov 5, 2024

Add ControllerModifyVolume E2E tests #1826

Add ControllerModifyVolume E2E tests #1826

Conversation

travisyx commented Sep 17, 2024 • edited Loading

k8s-ci-robot commented Sep 17, 2024

amacaskill commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 20, 2024

amacaskill commented Sep 22, 2024

k8s-ci-robot commented Sep 22, 2024

travisyx commented Nov 5, 2024

k8s-infra-cherrypick-robot commented Nov 5, 2024

travisyx commented Sep 17, 2024 •

edited

Loading