[WIP] Add block volume support check to external provisioners

[mkimuram]

Adding the similar check logic to below PR for external provisioners which don't support block volume.

• Add block volume support to internal provisioners.
  Add block volume support to internal provisioners. kubernetes/kubernetes#64447

There could be many other external provisioners that are not in this git repository, however it would be better adding check to the ones in this git repository.

Not all external provisioner are tested currently, however, comments for this PR is welcomed.

Please review the code with below in mind:

• From external provisioner, features.BlockVolume is not acessible, so there is no check for this feature flag,
• Check logic are implemented with assuming below block volume support status,
• Flex volume and snapshot will be a special case, so they need further discussion,
• CSI is out of scope for this PR, because its in a different git repository, however, it needs similar consideration to flex volume.

[Unknown/Needdiscussion]

• flex/pkg/volume/provision.go
• snapshot/cmd/snapshot-pv-provisioner/snapshot-pv-provisioner.go

[Support block volume]

• ceph/rbd/pkg/provision/provision.go
• gluster/block/cmd/glusterblock-provisioner/glusterblock-provisioner.go (iSCSI backend?)
• iscsi/targetd/provisioner/iscsi-provisioner.go

[Not Support block volume]

• ceph/cephfs/cephfs-provisioner.go
• openebs/openebs-provisioner.go
• openstack/standalone-cinder/pkg/provisioner/provisioner.go
• gluster/file/cmd/glusterfile-provisioner/glusterfile-provisioner.go
• gluster/glusterfs/pkg/volume/provision.go
• digitalocean/pkg/volume/provision.go
• docs/demo/hostpath-provisioner/hostpath-provisioner.go (document)
• docs/demo/hostpath-provisioner/README.md (document)
• nfs/pkg/volume/provision.go
• nfs-client/cmd/nfs-client-provisioner/provisioner.go
• openstack-sharedfilesystems/cmd/manila-provisioner/main.go

[wongma7]

I think all of the provisioners in this repo should reject volumeMode: block, even if technically e.g. ceph rbd can do it, none of them know how to handle it yet.

Since every single provisioner needs this check, this raises a question...would it be better to add a method like func SupportsBlockVolume() bool to the provisioner interface that defaults false? While we're there, we should also have func GetAccessModes() []v1.PersistentVolumeAccessMode, this is something I forgot about until now. Basically instead of copy/pasting these checks to every provisioner we should let the library solve the problem

[mkimuram]

@wongma7

Thank you for your comment.

I think all of the provisioners in this repo should reject volumeMode: block, even if technically
e.g. ceph rbd can do it, none of them know how to handle it yet.

IIUC, all the provisioners which can support volumeMode: block should set volume mode for PV just as iSCSI provisioner does in below code.
https://github.com/kubernetes-incubator/external-storage/blob/master/iscsi/targetd/provisioner/iscsi-provisioner.go#L147

Also, similar codes are added to internal provisioners.
kubernetes/kubernetes@08564f2#diff-f7811184a41099ebc39b015ff49f491bR454

Therefore, we should leave a room for some of the external provisioners to make volumeMode: block accept.

Since every single provisioner needs this check, this raises a question...would it be better to add a method like
func SupportsBlockVolume() bool to the provisioner interface that defaults false?

Please see below discussion for it.
kubernetes/kubernetes#64447 (comment)

I thinks that there are four options:

1. Each provisioner has method to return whether its attacher supports volumeMode: block, external provisioner library checks supportability by using it before calling each provisioner's provision() and return error if it doesn't support volumeMode: block.
   https://github.com/kubernetes-incubator/external-storage/blob/master/lib/controller/controller.go#L984
   (As you mention.)

2. Each provisioner checks volumeMode inside its provision() and return error if it doesn't support volumeMode: block.
   (Implemented in this PR.)

3. External provisioner library check supportability for the attacher's supportability of volumeMode: block and return error if it doesn't support volumeMode: block.
   (I still can't find a good way for a provisioner to find its corresponding attacher, however, if it can, the provisioner would be able to check supportability by checking whether the attacher implements mapper interface.)

4. External provisioner library or provisioner check configuration for whether it should turn on volumeMode: block, and return error if it isn't turned on by its configuration.
   (External provisioners have different interfaces to read configurations and external provisioner library doesn't seem to have a way to read configuration currently, so it's difficult to do it in library side and each provisioner needs to implement this logic.)

As it is implemented by option 2 in internal provisioner, I implemented in the similar way for external provisioner in this PR. I think it would be better to discuss again for internal provisioner if we choose option 1 for external provisioner.

Basically instead of copy/pasting these checks to every provisioner we should let the library solve the problem

I agree with you that doing it in library is better if there is a way that each provisioner doesn't need to do it, so I was investigating a little bit for #3 and #4, but I can't find a good way.

Note that as I commented, how flexvolume should handle this is not solved in this PR, because it could be supported or unsupported, depending on the backend storage.
(flexvolume was decided to keep as is, so we might be able to choose keep it as is or make it regard unsupported.)

On the other hand, I found that CSI seems to have a plan to prepare an interface to specify block in CreateVolume() call. VolumeCapability in CreateVolumeRequest has BlockVolume field. So, we will be able to check if it supports block, by checking CreateVolume() with BlockVolume fails. Therefore, for CSI, just regarding it as supported and calling provision() would be fine, if CreateVolume() properly handles BlockVolume.
https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolume
https://github.com/kubernetes-csi/external-provisioner/blob/master/pkg/controller/controller.go#L303

[wongma7]

You're correct, I misunderstood, I thought the provisioners would have to do something with VolumeMode but of course it's actually kube that does something (don't create an fs, etc.) and provisioners just need to set it. So not every one should be false.

Correct me if I'm wrong, but flexvolume doesn't have a 'Map' call in its interface so isn't block always unsupported for flex, i.e. it is not a BlockVolumePlugin? If so then we should set it false. It can already basically do anything it wants in Mount, I guess Map would be redundant? I am only speculating based on what I have seen recently, again please correct me if I'm wrong!

snapshot should be false because it's always provisioning restored volumes, it never needs to create block devices.

I agree about option 3 and 4, I can think of no way to do them nicely.

I still lean toward option 1 even if it means we differ from internal. For internal plugins, it is much easier to audit and ensure everybody is copying the volumeMode or rejecting it as appropriate. For external, it is impossible to ensure authors are dealing with new fields appropriately, unless we force them to deal with it when they update the library by adding functions to the interface. (Assuming they even update the library). Internal, the Provisioner interface is simply Provision and I sympathize that he does not want to introduce many ugly little SupportsX functions. External, the Provisioner interface is basically the entire library and is already more complicated than just Provision, so I have no qualms with adding more functions to it.

And yeah, about CSI, saying supported then calling CreateVolume is fine.

Thanks for your thoughts on this matter, I'm still not certain how to proceed but :)

(BTW again, your linked comment raised a good point about SupportsMountOption, we should have that here as well! I have missed so many of these things...)

[mkimuram]

Thank you for your comment. @wongma7

Correct me if I'm wrong, but flexvolume doesn't have a 'Map' call in its interface so isn't block always unsupported for flex,
i.e. it is not a BlockVolumePlugin? If so then we should set it false. It can already basically do anything it wants in Mount,
I guess Map would be redundant?

Oh, you're right. Current flex doesn't have block feature, so we can safely make it unsupported.
I seemed to adhere too much to its future possibility... Thank you for pointing it out.

snapshot should be false because it's always provisioning restored volumes, it never needs to create block devices.

I'm afraid that there is a possibility to create a snapshot for block devices and restore block volumes from it, in the future. It seems that most of plugins for snapshot don't rely on filesystem when creating/restoring a snapshot, so they should be able to do it.
(Block unsupported plugins like hostpath will rely on filesystem, though. If all block supported plugin supports snapshot/restore for block, this feature should work well.)

However, again, current snapshot doesn't support block volume, so I agree to make it false until it supports block volume.
https://github.com/kubernetes-incubator/external-storage/blob/master/snapshot/cmd/snapshot-pv-provisioner/snapshot-pv-provisioner.go#L134

(Snapshot will only support CSI and CSI will be able to handle provisioning block safely, as we discussed, so provision() for snapshot might just need to set block for its volumeMode there, however, it would be better to keep it false until CSI provisioner for block is properly implemented and we are 100% sure that it works well. )

I still lean toward option 1 even if it means we differ from internal. For internal plugins, it is much easier to audit and ensure everybody
is copying the volumeMode or rejecting it as appropriate. For external, it is impossible to ensure authors are dealing with new fields
appropriately, unless we force them to deal with it when they update the library by adding functions to the interface.

Thank you for your explanation on the difference between external and internal. It makes sense to me to make some interfaces to library in order to keep compatibility for external plugins.

Thanks for your thoughts on this matter, I'm still not certain how to proceed but :)

I will implement a prototype for option 1 and then let's review both codes. It would be easier to compare them with the actual codes.

[mkimuram]

@wongma7

I've implemented option 1 as below. PTAL
(To keep this discussion here, I put the codes on another branch in my git repo and doesn't make a pull request, until we make a decision on implementation policy.)

mkimuram@87c3ab4

This implementation looks much simpler and doesn't require any changes to existing plugins that doesn't support block volume.
(Only external provisioners that have already implemented block volume feature, like iscsi/targetd, will be affected.)

Please also see the test logs below for how it behave:

[Block supported case]

# kubectl describe pvc
Name:          pvc01-block
Namespace:     default
StorageClass:  iscsi
Status:        Bound
Volume:        pvc-aff1d47c-78c7-11e8-baef-525400669073
Labels:        <none>
Annotations:   control-plane.alpha.kubernetes.io/leader={"holderIdentity":"9e5348ca-78c7-11e8-8f83-525400669073","leaseDurationSeconds":15,"acquireTime":"2018-06-25T22:32:48Z","renewTime":"2018-06-25T22:32:50Z","lea...
               pv.kubernetes.io/bind-completed=yes
               pv.kubernetes.io/bound-by-controller=yes
               volume.beta.kubernetes.io/storage-class=iscsi
               volume.beta.kubernetes.io/storage-provisioner=iscsi-targetd
Finalizers:    [kubernetes.io/pvc-protection]
Capacity:      1Gi
Access Modes:  RWO
VolumeMode:    Block
Events:
  Type    Reason                 Age                From                                                                Message
  ----    ------                 ----               ----                                                                -------
  Normal  ExternalProvisioning   17s (x2 over 17s)  persistentvolume-controller                                         waiting for a volume to be created, either by external provisioner "iscsi-targetd" or manually created by system administrator
  Normal  Provisioning           17s                iscsi-targetd k8sbuild-master 9e5348ca-78c7-11e8-8f83-525400669073  External provisioner is provisioning volume for claim "default/pvc01-block"
  Normal  ProvisioningSucceeded  17s                iscsi-targetd k8sbuild-master 9e5348ca-78c7-11e8-8f83-525400669073  Successfully provisioned volume pvc-aff1d47c-78c7-11e8-baef-525400669073

[Log for iscsi-targetd provisioner]
(No error message for provision failure.)

[Block unsupported case] (Tested by deleting SupportsBlock() or changing SupportsBlock() to return false in iscsi/targetd/provisioner/iscsi-provisioner.go.)

# kubectl describe pvc 
Name:          pvc01-block
Namespace:     default
StorageClass:  iscsi
Status:        Pending
Volume:        
Labels:        <none>
Annotations:   volume.beta.kubernetes.io/storage-class=iscsi
               volume.beta.kubernetes.io/storage-provisioner=iscsi-targetd
Finalizers:    [kubernetes.io/pvc-protection]
Capacity:      
Access Modes:  
VolumeMode:    Block
Events:
  Type     Reason                Age                From                                                                Message
  ----     ------                ----               ----                                                                -------
  Normal   ExternalProvisioning  11s (x2 over 13s)  persistentvolume-controller                                         waiting for a volume to be created, either by external provisioner "iscsi-targetd" or manually created by system administrator
  Warning  ProvisioningFailed    6s (x3 over 13s)   iscsi-targetd k8sbuild-master 7c00dcff-78c6-11e8-8f29-525400669073  iscsi-targetd does not support block volume provisioning

[Log for iscsi-targetd provisioner]
ERROR: logging before flag.Parse: E0625 22:24:19.886572       1 controller.go:739] Failed to provision volume for claim "default/pvc01-block" with StorageClass "iscsi": iscsi-targetd does not support block volume provisioning
ERROR: logging before flag.Parse: E0625 22:24:19.892790       1 controller.go:739] Failed to provision volume for claim "default/pvc01-block" with StorageClass "iscsi": iscsi-targetd does not support block volume provisioning
ERROR: logging before flag.Parse: E0625 22:24:26.420030       1 controller.go:739] Failed to provision volume for claim "default/pvc01-block" with StorageClass "iscsi": iscsi-targetd does not support block volume provisioning
ERROR: logging before flag.Parse: E0625 22:24:41.420288       1 controller.go:739] Failed to provision volume for claim "default/pvc01-block" with StorageClass "iscsi": iscsi-targetd does not support block volume provisioning
ERROR: logging before flag.Parse: E0625 22:24:56.420436       1 controller.go:739] Failed to provision volume for claim "default/pvc01-block" with StorageClass "iscsi": iscsi-targetd does not support block volume provisioning

[wongma7]

Yes I like that code quite a bit more, it lgtm. Behaviour is good. Since somebody already added the Qualifier interface there is some precedent for doing it this way too. And it kind of reminds me of upstream 'BlockVolumePlugin', 'XVolumePlugin', to say 'BlockProvisioner'.

[mkimuram]

@wongma7

Yes I like that code quite a bit more, it lgtm.

Thank you for your comment. I also add unit tests for it and make another PR #830. PTAL

[mkimuram]

Closing this PR for this issue is resolved by #830