Idempotency: repeated Node Unpublish operations cause failure

[okartau]

Background: in pmem-csi driver we added more idempotency conformance.
To test, the repeated Node Volume operations were added in csi-test.
In process of upstreaming this change, the CI tests that use host-path-driver, fail:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_csi-test/229/pull-kubernetes-csi-csi-test/1185891170576764931

host-path driver currently does not pay attention to repeated requests possibility, there are no mutexes that protect against related race conditions.

[pohly]

We discussed this today in the Kubernetes-CSI standup and everyone agreed that a) this should be fixed in the hostpath driver and b) that having these idempotency tests in csi-test will be very useful.

[BenTheElder]

what is necessary to fix this? just limiting the concurrency of operations in the driver?

[BenTheElder]

xref: #72, this sounds like a superset of that

[okartau]

I created #139 to tolerate repeated NodeUnpublish requests.
My local testing shows that this allows hostpath driver to pass the added repeated csi-sanity testing currently coming in kubernetes-csi/csi-test#229, so right now there is no need to add mutex locking to satisfy those repeated tests, as I can't see failures.

[pohly]

so right now there is no need to add mutex locking to satisfy those repeated tests, as I can't see failures.

This is a dangerous argument: that you haven't been able to trigger a problem caused by missing locking does not mean that the problem doesn't exist or that we don't need to do anything about it.

In practice the problem should be exceedingly rare. It only happens when two gRPC calls reach the driver at the same time and get processed in parallel. I think the idempotency tests don't cover that, they just issue the same call multiple times sequentially.

So while we don't need mutex locking to make the NodeUnpublishVolume idempotent, we still need it to make the code more robust.

[okartau]

This is a dangerous argument: that you haven't been able to trigger a problem caused by missing locking does not mean that the problem doesn't exist or that we don't need to do anything about it.

I agree, I did not mean we don't need to deal with locking at all, just that currently to-be-added test cases do not expose problem that we can see fixed by added locking.
We should add locking, but the progress will be more assuring if we can have test case(s) improved by locking.