-
Notifications
You must be signed in to change notification settings - Fork 146
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.
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
sanity: Disable ListVolumes pagination test #226
Conversation
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 Once the patch is verified, the new status will be reflected by the 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. |
As discussed in intel/pmem-csi#424 (comment), the I propose to disable the test in addition to fixing the volume leak. If someone has a good idea for improving the test, then they can do that later. |
Yes, @davidz627 opened up #223 outlining various problems with the ListVolumes tests. I am ok with disabling the test because the issue in #223 I think is serious enough. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avalluri please disable "pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted" by replacing It
with XIt
, with a comment that links to #223 and intel/pmem-csi#424 (comment).
mock/service/controller.go
Outdated
@@ -73,7 +73,7 @@ func (s *service) CreateVolume( | |||
case *csi.VolumeContentSource_Volume: | |||
vid := req.GetVolumeContentSource().GetVolume().GetVolumeId() | |||
// Check if the source volume exists. | |||
if volID, _ := s.findVolNoLock("id", vid); volID > 0 { | |||
if volID, _ := s.findVolNoLock("id", vid); volID >= 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unrelated change. Can you move it to a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avalluri please disable "pagination should detect volumes added between pages and accept tokens when the last volume from a page is deleted" by replacing
It
withXIt
, with a comment that links to #223 and intel/pmem-csi#424 (comment).
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unrelated change. Can you move it to a separate PR?
Done: #227
As discussed here: kubernetes-csi#226 (review) This change fixes volume leakage in ListVolume pagination testcase and disable this case till we improve the test.
5d4a5d6
to
46ef6af
Compare
/lgtm |
/approve |
[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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Volume created by testcase is not adding to cleanup list, hence it results in
volume leaking. This change fixes this issue.
The side effect of this testcase fix discovered a minor issue in mock driver implementation.
Which was hidden due to the leftover volume created by above testcase.
Does this PR introduce a user-facing change?: