proposal: Improve static PVC handling code

[satoru-takeuchi]

It's a proposal of improving static PVC handling code to improve readbility and maintainability.

Static PVC needs special handling in many places. Although current code seems to work well, some gRPC handlers looks a bit confising. For instance, ControllerUnpublishVolume() returns successfulyl in error hander for older PVs with the following warning.

W0514 07:30:29.740472       1 controllerserver.go:1728] ID: 273184 Req-ID: <volume name> controller publish secret not found: failed to decode volume ID (mantle-verify-69cac52e-5bb5-4f0e-8b4a-35731a767e51): failed to decode CSI identifier, string underflow 

It works but is not intuitive. In this case, I believe this function should explicitly check whether the PVC is a static and exits if the result is true. Although I didn't checked all gRPC handlers, some other handlers seems to have similar problems.

I'd like to change the above mentioned code if possible. What do you think?

[Madhu-1]

@satoru-takeuchi if i remember correct it was not only for static volumes but also for volumes which are created by earlier CSI driver. @iPraveenParihar PTAL

[iPraveenParihar]

We considered early-exiting ControllerUnpublishVolume() for static volumes, but ControllerUnpublishVolumeRequest only provides VolumeId, NodeId, and Secrets there's no VolumeContext to check the staticVolume flag. The only way to detect a static volume here is DecomposeCSIID failure, which is already what GetControllerPublishSecretRef() does internally. So the current approach of catching the error and returning success is effectively the early-exit.

@satoru-takeuchi Do you have something in your mind to handle static volume early exit?

[satoru-takeuchi]

We considered early-exiting ControllerUnpublishVolume() for static volumes, but ControllerUnpublishVolumeRequest only provides VolumeId, NodeId, and Secrets there's no VolumeContext to check the staticVolume flag.

Oops, I forgot it.

The only way to detect a static volume here is DecomposeCSIID failure,

I see.

How about the following changes?

• Returns a dedicated error on GetControllerPublishSecretRef() on DecomposeCSIID() failure and use this error for early return.
  • This change can handle the failure of GetControllerPublishSecretRef by other reasons.
• Clarify that users need not to specify ControllerPublishSecretRef for static PVs in the spec.

[satoru-takeuchi]

We can discuss based on a draft PR if necessary.