Add Block volume support for CSI provisioner #135

mkimuram · 2018-09-13T21:06:51Z

Below three logics are implemented to add Block volume support to CSI provisioner:

Add SupportsBlock that properly returns whether Storage
Provider's plugin supports block (this is checked
by using ValidateVolumeCapabilities),
Pass BlockVolume instead of MountVolume to CreateVolume
if volumeMode is set to be Block on Provision,
Set volumeMode to PV returned by Provision.

Also, below 4 test cases for TestSupportsBlock and 2 test cases for TestProvision
are added.

TestSupportsBlock:

ValidateVolumeCapabilities return (true, nil) case: return true expected
ValidateVolumeCapabilities return (false, nil) case: return false expected
ValidateVolumeCapabilities return (true, err) case: return false expected
ValidateVolumeCapabilities return (false, err) case: return false expected

TestProvision:

volumeMode=Filesystem PVC case: return Filesystem PV expected
volumeMode=Block PVC case: return Block PV expected

Fixes #110

mkimuram · 2018-09-13T21:11:33Z

@wongma7 @vladimirvivien

This PR is to add Block volume support for CSI provisioner. PTAL

wongma7 · 2018-09-14T15:38:17Z

pkg/controller/controller.go

+	client := csi.NewControllerClient(p.grpcClient)
+	req := csi.ValidateVolumeCapabilitiesRequest{
+		VolumeCapabilities: []*csi.VolumeCapability{
+			volumeCapabilityMountReadWriteOnce,


we don't seem to use this function anywhere. also I think it should be volumeCapabilityBlockReadWriteOnce

@wongma7

also I think it should be volumeCapabilityBlockReadWriteOnce

Thank you for pointing it out. It's a typo. I fixed it and modified unit code to detect this error.

we don't seem to use this function anywhere.

SupportsBlock is called from library, as we added in the commit, a bit long ago.

Oh yeah, forgot already, my bad :p 👍

wongma7 · 2018-09-17T17:25:53Z

pkg/controller/controller_test.go

+
+		controllerServer.EXPECT().ValidateVolumeCapabilities(gomock.Any(), gomock.Any()).Return(&tc.volCapResp, tc.volCapErr).Times(1)
+
+		if csiBlockProvisioner, ok := csiProvisioner.(controller.BlockProvisioner); ok {


we can skip this if we add a line like
var _ controller.BlockProvisioner = &csiProvisioner{}
to controller.go, to catch the problem at compile time.
Keeping both is okay too

Thank you for your advice. I will add it and keep both of them.

mkimuram · 2018-09-17T18:34:28Z

@wongma7

I fixed as suggested. PTAL

vladimirvivien

Thanks for the great effort here. However, there are couple issues that needs addressing before this could work.

vladimirvivien · 2018-09-17T19:22:00Z

pkg/controller/controller.go

-				},
-				AccessType: accessType,
-			})
+		if util.CheckPersistentVolumeClaimModeBlock(options.PVC) {


It looks like the if-block have redundant statements as the else-block.

vladimirvivien · 2018-09-17T19:45:16Z

pkg/controller/controller.go

+		VolumeCapabilities: []*csi.VolumeCapability{
+			volumeCapabilityBlockReadWriteOnce,
+		},
+	}


The call to ValidateVolumeCapabilities requires a volume ID to specify the volume for which the capability query is being made. https://github.com/container-storage-interface/spec/blob/master/spec.md#validatevolumecapabilities

Thank you for pointing this out.

The intention of SupportsBlock is to check supportability of block volume feature for the driver before Provision is called so that provisioner can skip Provision, if the driver doesn't support block volume feature. (Drivers like Ceph, NFS, and hostpath that doesn't provide block volume would need this.) Therefore, if ValidateVolumeCapabilitiesRequest requires a volume ID, it won't be used here, because the existence of volume ID requires existence of volume.

So, the alternatives would be below three:

Just return true for SupportsBlock in all the driver and make Provision fail for block unsupported drivers.

Change CSI spec to allow ValidateVolumeCapabilitiesRequest not specifying volumeID and call ValidateVolumeCapabilitiesRequest here

Add another method in CSI spec to be used for this use case, and call the new method here.

Any thoughts?

I like alternative 1. The CSI spec doesn't let you query abstract capabilities, and 2 or 3 require spec changes which are unlikely to happen before the promotion to v1.

@bswartz

Thank you for your comment. I will rebase the implementation for alternative 1 and push here, if @vladimirvivien also agrees.

When you rebase, you'll have a conflict with the mount options work I did. I can help fix those conflicts if you like. I'm currently testing this PR in my lab to see if I can get it to work.

vladimirvivien · 2018-09-17T19:49:07Z

@mkimuram

Below three logics are implemented to add Block volume support to CSI provisioner:

Add SupportsBlock that properly returns whether Storage Provider's plugin supports block (this is checked
by using ValidateVolumeCapabilities)

Keep in mind that the ValidateVolumeCapabilities call is for a volume capabilities not the Driver capabilities. So that call must include a volume id.

mkimuram · 2018-09-18T21:44:23Z

@vladimirvivien

The call to ValidateVolumeCapabilities requires a volume ID to specify the volume for which the capability query is being made.
So, the alternatives would be below three:

Just return true for SupportsBlock in all the driver and make Provision fail for block unsupported drivers.

I implemented the alternative 1 above in another commit in a separate branch. PTAL

I think that AccessMode unsupported cases are in the same situation to this AccessType unsupported case, so this implementation should be still valid. If the commit makes sense, I will close this PR and make another PR for it.

mkimuram · 2018-11-02T20:56:16Z

@vladimirvivien @bswartz

I rebased alternative 1 and pushed to this PR. PTAL

bswartz · 2018-11-06T16:08:09Z

I downloaded this patch and tested it with my CSI driver, and it worked.

bswartz · 2018-11-06T16:09:43Z

/lgtm

k8s-ci-robot · 2018-11-06T16:09:51Z

@bswartz: changing LGTM is restricted to assignees, and only kubernetes-csi/external-provisioner repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

mkimuram · 2018-11-06T19:04:14Z

@bswartz

Thank you for your testing and sharing the results.

mkimuram · 2018-11-20T20:51:22Z

@bswartz @lpabon @saad-ali

I rebased the commit. PTAL

Now kubernetes/kubernetes#68635 is merged, so we can use k8s master branch for testing. On the other hand, as csi external-provisioner code is updated for csi v1.0.0 spec , csi driver code needs to be updated to meet csi v1.0.0 spec.

I tested with below combination and it is working fine:

k8s (master branch)
csi-external-provisioner (This PR)
csi-attacher and csi-registrar (Versions used in csi e2e tests)
csi rbd driver (Only csi rbd driver is updated with a quick hack for testing)

vladimirvivien · 2018-11-26T17:54:17Z

/lgtm
/approve

In CSI provisioner, below three logics need to be implemented, to add Block volume support to CSI provisioner: 1. Add SupportsBlock that always return true, 2. Pass BlockVolume instead of MountVolume to CreateVolume if volumeMode is set to be Block on Provision, 3. Set volumeMode and skip adding FsType to PV returned by Provision, if volumeMode is set to be Block on Provision. Also, below 2 test cases for TestProvision are added. 1. volumeMode=Filesystem PVC case: return Filesystem PV expected 2. volumeMode=Block PVC case: return Block PV expected Fixes kubernetes-csi#110

mkimuram · 2018-11-26T18:04:19Z

@vladimirvivien

Thank you for your review. I saw a merge conflict and just updated the commit.
PTAL , again.

Note that the conflict was just change in header and fixed like below:

- "github.com/kubernetes-incubator/external-storage/lib/controller"
- "github.com/kubernetes-incubator/external-storage/lib/util"
+ "github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/controller"
+ "github.com/kubernetes-sigs/sig-storage-lib-external-provisioner/util"

So, no logic change there.

vladimirvivien · 2018-11-26T19:05:58Z

/lgtm

vladimirvivien · 2018-11-26T19:11:15Z

@lpabon @saad-ali @jsafrane please take a look for approval.

saad-ali · 2018-11-26T20:02:12Z

/lgtm
/approve

k8s-ci-robot · 2018-11-26T20:02:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mkimuram, saad-ali, vladimirvivien

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~~ [saad-ali]

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

saad-ali · 2018-11-28T18:19:39Z

Cheery pick to release-1.0 branch so that it can be part of next 1.0.x release

Use leader election library from csi-lib-utils

k8s-ci-robot requested review from davidz627 and sbezverk September 13, 2018 21:06

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2018

mkimuram mentioned this pull request Sep 13, 2018

Add Block volume support for CSI provisioner #113

Closed

wongma7 reviewed Sep 14, 2018

View reviewed changes

wongma7 reviewed Sep 17, 2018

View reviewed changes

vladimirvivien reviewed Sep 17, 2018

View reviewed changes

mkimuram mentioned this pull request Nov 1, 2018

CSI block volume refactor to fix target path kubernetes/kubernetes#68635

Merged

mkimuram force-pushed the issue/110v3 branch from 05055d1 to fd4d55a Compare November 2, 2018 20:55

mkimuram force-pushed the issue/110v3 branch from fd4d55a to f4b5d54 Compare November 20, 2018 20:40

mkimuram mentioned this pull request Nov 20, 2018

[WIP] Add block supports to rbd driver ceph/ceph-csi#97

Closed

bertinatto mentioned this pull request Nov 23, 2018

Raw Block Volume Support kubernetes-sigs/aws-ebs-csi-driver#101

Closed

k8s-ci-robot assigned vladimirvivien Nov 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2018

mkimuram force-pushed the issue/110v3 branch from f4b5d54 to 72398be Compare November 26, 2018 18:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2018

k8s-ci-robot assigned saad-ali Nov 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2018

k8s-ci-robot merged commit 3827f80 into kubernetes-csi:master Nov 26, 2018

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#135 from andrewsykim/leaderelection

70a1411

Use leader election library from csi-lib-utils

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Block volume support for CSI provisioner #135

Add Block volume support for CSI provisioner #135

mkimuram commented Sep 13, 2018

mkimuram commented Sep 13, 2018

wongma7 Sep 14, 2018

mkimuram Sep 17, 2018

wongma7 Sep 17, 2018

wongma7 Sep 17, 2018

mkimuram Sep 17, 2018

mkimuram commented Sep 17, 2018

vladimirvivien left a comment

vladimirvivien Sep 17, 2018

vladimirvivien Sep 17, 2018

mkimuram Sep 17, 2018

bswartz Nov 1, 2018

mkimuram Nov 1, 2018

bswartz Nov 1, 2018

vladimirvivien commented Sep 17, 2018

mkimuram commented Sep 18, 2018

mkimuram commented Nov 2, 2018

bswartz commented Nov 6, 2018

bswartz commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

mkimuram commented Nov 6, 2018

mkimuram commented Nov 20, 2018

vladimirvivien commented Nov 26, 2018

mkimuram commented Nov 26, 2018 •

edited

Loading

vladimirvivien commented Nov 26, 2018

vladimirvivien commented Nov 26, 2018

saad-ali commented Nov 26, 2018

k8s-ci-robot commented Nov 26, 2018

saad-ali commented Nov 28, 2018


		controllerServer.EXPECT().ValidateVolumeCapabilities(gomock.Any(), gomock.Any()).Return(&tc.volCapResp, tc.volCapErr).Times(1)

		if csiBlockProvisioner, ok := csiProvisioner.(controller.BlockProvisioner); ok {

Add Block volume support for CSI provisioner #135

Add Block volume support for CSI provisioner #135

Conversation

mkimuram commented Sep 13, 2018

mkimuram commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkimuram commented Sep 17, 2018

vladimirvivien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimirvivien commented Sep 17, 2018

mkimuram commented Sep 18, 2018

mkimuram commented Nov 2, 2018

bswartz commented Nov 6, 2018

bswartz commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

mkimuram commented Nov 6, 2018

mkimuram commented Nov 20, 2018

vladimirvivien commented Nov 26, 2018

mkimuram commented Nov 26, 2018 • edited Loading

vladimirvivien commented Nov 26, 2018

vladimirvivien commented Nov 26, 2018

saad-ali commented Nov 26, 2018

k8s-ci-robot commented Nov 26, 2018

saad-ali commented Nov 28, 2018

mkimuram commented Nov 26, 2018 •

edited

Loading