Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sanity: capture newly created volume IDs #195

Merged
merged 1 commit into from
May 8, 2019
Merged

sanity: capture newly created volume IDs #195

merged 1 commit into from
May 8, 2019

Conversation

avalluri
Copy link
Contributor

In one of ListVolumes tests, newly created volume ids were not captured. Though the test has needed cleaning up code, it was affected because of the empty newVolIDs array. This leads to leaking of volumes.

/kind bug

@k8s-ci-robot
Copy link
Contributor

@avalluri: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 26, 2019
@k8s-ci-robot k8s-ci-robot requested review from jsafrane and lpabon April 26, 2019 13:18
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 26, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @avalluri. Thanks for your PR.

I'm waiting for a kubernetes-csi or kubernetes 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 26, 2019
@pohly
Copy link
Contributor

pohly commented Apr 29, 2019

/ok-to-test

@k8s-ci-robot 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 Apr 29, 2019
@pohly
Copy link
Contributor

pohly commented Apr 29, 2019

Thanks for highlighting this issue. However, the issue is even more severe: if one of the asserts fails after creating the volume, the cleanup code won't get executed. I missed that when merging #80.

Can you change the test such that it uses https://github.com/kubernetes-csi/csi-test/blob/master/pkg/sanity/cleanup.go?

@avalluri
sanity: capture newly created volume IDs
77fc8ea 
In one of ListVolumes tests, newly created volume ids were not captured
properly. Because of this, those volumes are not getting deleted even though the
test has needed cleanup code. This leads to leaking of volumes.

This change also handles the test assertion case by using Cleanup interface.

Signed-off-by: Amarnath Valluri <[email protected]>
@avalluri avalluri force-pushed the fix-listvolumes branch from e34aae8 to 77fc8ea Compare May 8, 2019 01:38
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 8, 2019
@avalluri
Copy link
Contributor Author

avalluri commented May 8, 2019

Thanks for highlighting this issue. However, the issue is even more severe: if one of the asserts fails after creating the volume, the cleanup code won't get executed. I missed that when merging #80.
Can you change the test such that it uses https://github.com/kubernetes-csi/csi-test/blob/master/pkg/sanity/cleanup.go?

@pohly, I updated the PR to use Cleanup interface as you suggested.

pohly
pohly approved these changes May 8, 2019
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks.

@pohly
Copy link
Contributor

pohly commented May 8, 2019

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avalluri, pohly

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2019
@pohly
Copy link
Contributor

pohly commented May 8, 2019

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 8, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3678ec3 into kubernetes-csi:master May 8, 2019
suneeth51 pushed a commit to suneeth51/csi-test that referenced this pull request Sep 11, 2019
@k8s-ci-robot
Merge pull request kubernetes-csi#195 from avalluri/fix-listvolumes
bd0bf72 
sanity: capture newly created volume IDs
xing-yang added a commit to xing-yang/csi-test that referenced this pull request Aug 23, 2022
@xing-yang
Squashed 'release-tools/' changes from e4dab7f..d24254f
e56dd6c 
d24254f Merge pull request kubernetes-csi#202 from xing-yang/kind_0.14.0
0faa3fc Update to Kind v0.14.0 images
ef4e1b2 Merge pull request kubernetes-csi#201 from xing-yang/add_1.24_image
4ddce25 Add 1.24 Kind image
7fe5149 Merge pull request kubernetes-csi#200 from pohly/bump-kubernetes-version
70915a8 prow.sh: update snapshotter version
31a3f38 Merge pull request kubernetes-csi#199 from pohly/bump-kubernetes-version
7577454 prow.sh: bump Kubernetes to v1.22.0
d29a2e7 Merge pull request kubernetes-csi#198 from pohly/csi-test-5.0.0
41cb70d prow.sh: sanity testing with csi-test v5.0.0
c85a63f Merge pull request kubernetes-csi#197 from pohly/fix-alpha-testing
b86d8e9 support Kubernetes 1.25 + Ginkgo v2
ab0b0a3 Merge pull request kubernetes-csi#192 from andyzhangx/patch-1
7bbab24 Merge pull request kubernetes-csi#196 from humblec/non-alpha
e51ff2c introduce control variable for non alpha feature gate configuration
ca19ef5 Merge pull request kubernetes-csi#195 from pohly/fix-alpha-testing
3948331 fix testing with latest Kubernetes
9a0260c fix boilerplate header

git-subtree-dir: release-tools
git-subtree-split: d24254f
@xing-yang xing-yang mentioned this pull request Aug 23, 2022
Merged
stmcginnis pushed a commit to stmcginnis/csi-test that referenced this pull request Oct 9, 2024
@k8s-ci-robot
Merge pull request kubernetes-csi#195 from pohly/fix-alpha-testing
ca19ef5 
fix testing with latest Kubernetes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly approved these changes

@jsafrane jsafrane Awaiting requested review from jsafrane

@lpabon lpabon Awaiting requested review from lpabon

Assignees

@pohly pohly

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@avalluri @k8s-ci-robot @pohly