Remove missing volume test case for NodeUnpublishVolume

[timoreimann]

What type of PR is this?

/kind bug

What this PR does / why we need it:

The "should fail when the volume is missing" test for NodeUnpublishVolume verifies that a NotFound error occurs when the endpoint is invoked for a missing volume. However, this expectation really only
holds for volume types that are locally attached, such as local disks or iscsi. Specifically, it would not hold for network-attached storage where it is fine to return Ok if the mount point does not exist. See also the related discussion on Slack, and in particular @msau42's comments:

Hm well it's strange.... If the mount point doesn't exist, then you return ok

But if the device isn't even there, then mount also doesn't exist and you should still return ok...

The not found may be more relevant to volume types that need to connect to the device locally, like local disks, iscsi, fc

This change introduces a flag to gate execution of the test, defaulting it not running since that was the behavior we have had before. I thought that might be a good way to deal with the situation, but it's definitely more of a proposal. I'm happy to adjust if you think we should approach this differently. (PR discussion led to the conclusion that it's better to delete the test.)

container-storage-interface/spec#433 was filed to improve the spec in this regard. For now we remove the test as it seems to block more people than it helps.

Relates to #242

The test in question was introduced in #242 by myself and is now affecting our own tests in the DigitalOcean CSI driver due to a misunderstanding I had about when an error for a missing volume should (not) be produced. Apologies for the extra churn.

Does this PR introduce a user-facing change?:

Remove missing volume test case for NodeUnpublishVolume

[k8s-ci-robot]

Hi @timoreimann. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

[pohly]

However, this expectation really only holds for volume types that are locally attached, such as local disks or iscsi.

Why should a driver return an error in that case?

[timoreimann]

@pohly

However, this expectation really only holds for volume types that are locally attached, such as local disks or iscsi.

Why should a driver return an error in that case?

This is me trying to paraphrase what @msau42 said on the Slack discussion I referenced and quoted in the PR description, in conjunction with the spec's mandate to return an error if the "Volume does not exist". My understanding is that some drivers with physical access to the underlying storage system need to be able to return an error if the volume cannot be found.

This scenario is clearly out of my zone of comfort, so I'll let Michelle comment on that. From the perspective of the driver that I maintain (DigitalOcean's), we would just keep the switch this PR introduces off. For what it's worth, we could do entirely without the affected test since it's a no-op for our driver if the target mount point does not exist.

[maennchen]

This is also a problem to get the GCS driver passing the tests. To make it work, the driver would have to introduce a memory leak on purpose. ofek/csi-gcs#9 (comment)

[pohly]

the spec's mandate to return an error if the "Volume does not exist"

So the spec is asking for something that cannot be implemented by real CSI drivers? That sounds like a problem with the spec and should be brought up there.

We can still merge this PR, but then the flag should be clearly marked as a workaround for an incomplete CSI implementation, with a link to some place where that is getting discussed.

[maennchen]

@pohly If the function would be provided with a secret, it could be checked. But since there's no secret and it has to be idempotent I see no way how to implement this.

(In some issue on the spec discussing why there was no secrets someone said that the driver should store the required secrets internally. But combining that with idempotency introduces memory leaks since the state can never be cleaned up.)

[timoreimann]

@pohly

So the spec is asking for something that cannot be implemented by real CSI drivers? That sounds like a problem with the spec and should be brought up there.

My understanding is that the spec allows certain drivers to return an error if they need to do so, but it's less clear on the fact that (other) drivers don't have to. If that's correct, then I agree it sounds like something which can be improved on.

I'm happy to file an issue on the spec repo to drive the improvement, but maybe we should wait for @msau42 to chime in here first as I was mostly trying to reflect in this issue what we talked about on Slack. I think this could help us make sure we are aligned on what the right path forward would look like (where I'm open for anything between removing the test in question from csi-test to modifying it to fixing the spec to any combo of the aforementioned).

[msau42]

I was mostly thinking about the update we made to the spec regarding ControllerUnpublish, where we clarified that if the volume not found actually indicates that it's unpublished, then it's fine for a driver to return OK. We did not make the spec change to NodeUnpublish, so I guess we technically shouldn't change the test here, but we should consider updating the spec in a similar way. cc @jsafrane @saad-ali

[jsafrane]

I was mostly thinking about the update we made to the spec regarding ControllerUnpublish, where we clarified that if the volume not found actually indicates that it's unpublished, then it's fine for a driver to return OK.

I agree.

CO has a record of a volume being NodePublished. It calls NodeUnpublish and the volume does not exist. Now what?

• CO can either discard the record when there is nothing to Unpublish - there is nothing mounted, no directory/device to be removed. Driver should return OK in this case.
• Or CO can retry - the storage backend has a temporary hiccup and it might find the volume later. Driver should return NOT_FOUND. I am not sure if NOT_FOUND is the correct error here, as the driver has probably some issues that would warrant INTERNAL error.

We need to fix the spec.

[timoreimann]

Thanks for the feedback everyone. I'll file an issue in the spec repo soon to kick off a discussion.

[pohly]

Oh the irony, indeed. PMEM-CSI also doesn't pass the new test... It returns OK because as far as it can tell, all the unpublish work has been done and thus there is no failure.

@timoreimann: have you filed a bug against the spec? If so, please add the link here (comment in the code where the test gets disabled might be best) and we can merge this.

[timoreimann]

@pohly I just filed container-storage-interface/spec#433 and updated the PR to reference the spec ticket.

[pohly]

This looks okay if we want to keep the test.

However, it occurred to me that skipping the test only happens after some potentially expensive per-test setup operation (like deploying the CSI driver) has been done. There is no good way around that. We could make the It() call itself conditional, but in some other context that was frowned upon (skipped tests no longer show up at all) and it deviates how the other TestConfig entries are used.

Perhaps we should simply remove the test? I doubt that many CSI driver developers will remember to turn it on.

[timoreimann]

@pohly I'm okay with removing it. It probably does more harm than good at this point.

I updated the PR and the PR description accordingly.

[pohly]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [pohly]

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