CSI: Allow NodePublishVolume even when plugin does not support staging

[olljanat]

- What I did
I was packing NFS CSI for Swarm on https://github.com/olljanat/csi-plugins-for-docker-swarm/tree/master/csi-driver-nfs when noticed that it was able to create volume and folder to NFS share but mounting volume to didn't worked but instead of it ended up looping these messaged on Docker engine log:

INFO[2023-02-04T19:21:08.939610819+01:00] attempting to publish volume                  attempt=1 module=node/agent/csi volume.id=0f9wq7wvz41wxw81u4gfsiy55
DEBU[2023-02-04T19:21:08.939765667+01:00] staging volume succeeded, attempting to publish volume  attempt=1 module=node/agent/csi volume.id=0f9wq7wvz41wxw81u4gfsiy55
INFO[2023-02-04T19:21:08.939788029+01:00] publishing volume failed                      attempt=1 error="rpc error: code = FailedPrecondition desc = volume not staged" module=node/agent/csi volume.id=0f9wq7wvz41wxw81u4gfsiy55

- How I did it
Set stagingPath to empty string by default and continue logic instead of return error when plugin does not support staging.

- How to test it
Added another test case without staging. Manual testing can be done with linked NFS CSI plugin.

- Description for the changelog

[s4ke]

+1 on this.

The Hetzner CSI plugin I am working on does not support staging either:

func (s *NodeService) NodeStageVolume(ctx context.Context, req *proto.NodeStageVolumeRequest) (*proto.NodeStageVolumeResponse, error) {
	return nil, status.Error(codes.Unimplemented, "not supported")
}

func (s *NodeService) NodeUnstageVolume(ctx context.Context, req *proto.NodeUnstageVolumeRequest) (*proto.NodeUnstageVolumeResponse, error) {
	return nil, status.Error(codes.Unimplemented, "not supported")
}

[crazy-max]

We got an issue with CI and fix had been merged, can you rebase please?

[codecov-commenter]

Codecov Report

Merging #3116 (50ca5ad) into master (8892b33) will increase coverage by 61.78%.
The diff coverage is 75.00%.

❗ Current head 50ca5ad differs from pull request most recent head 1ab7f13. Consider uploading reports for the commit 1ab7f13 to get more accurate results

@@             Coverage Diff             @@
##           master    #3116       +/-   ##
===========================================
+ Coverage        0   61.78%   +61.78%     
===========================================
  Files           0      153      +153     
  Lines           0    31023    +31023     
===========================================
+ Hits            0    19167    +19167     
- Misses          0    10317    +10317     
- Partials        0     1539     +1539     

[olljanat]

@crazy-max yes, CI started working with rebase and found two issues which I needed to solve ( accessMode cannot be nil and there need to be separate test case with and without staging).

@dperny PTAL

[neersighted]

Some things we'd like to see changed after a peer session with @dperny.

[neersighted]

Let's factor this out into a common helper and use it in the Stage/Unstage methods as well (every method that calls makeCapability). I'd also like to see that change in a separate commit.

[olljanat]

@neersighted I was not sure what would be correct location for that helper and when started working with it I found that makeCapability function did exists twice so I introduced pkg/... structure for swarmkit too as it looks to be quite common way 😄

[neersighted]

I think the pkg pattern has been quite detrimental overall; if we need to share things across two packages I'd suggest we use internal/ instead (so otherwise exported fields are kept from being public).

[olljanat]

Now those are in internal/csi/capability and CI is green (looks to be flaky if you update PR when CI is not ready from earlier run).

[neersighted]

capability is really not a great home for these, but I think that's a bikeshed we can worry about later -- most of these helpers should probably go into a apihelpers or typeconversion or something package that has its own unit tests.

I think this LGTM to me overall, I'll sync up with @dperny again to go over it once we both have bandwidth.

[dperny]

Not a blocker on this PR, but we should move the volumequeue package from the top level into internal/csi/volumequeue. Not sure why I didn't think of that in the first place.