[v2] Add block volume support check to external provisioners #830
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
I read through the codes again and come to think that current my code might output unnecessary error messages in multiple external provisioner case. Correct me if I'm wrong. If above is yes, block-unsupported external provisioners that are not used for provisioning a certain claim will output messages that they don't support block volume, because I put a block volume check code before the check. Therefore, block volume check should be moved to after the check. Also, I found below check inside provisionClaimOperation(). If similar issue might happen here, block volume check code should be moved to after this check. I would appreciate it if you could give me an advice on it. I will fix it if needed. |
|
You're right, we need to move the check to inside provisionClaimOperation, after that |
|
Thank you for your comment. I will re-implement my code to reflect your comment and will ask you for review. |
This patch add a block volume support check to external provisioners. This patch introduces BlockProvisioner interface and SupportsBlock() to check whether the provisioner supports block volume. They are used by the external provisioner library and if a provisioner doesn't supports block volume, the library output an error and doesn't try provisioning the block volume. With this change, external provisioner that supports block volume is required to implement SupportsBlock() and return true. (Also, it must set volumeMode to pv spec that it returns.)
Added below 9 test cases for TestShouldProvision().
BlockProvisioner I/F SupportsBlock volumeMode Expectation(ShouldProvision)
-- ---------------------- --------------- ------------ ------------------------------
1 Not implemented N/A Undefined true
2 Not implemented N/A FileSystem true
3 Not implemented N/A Block false
4 Implemented false Undefined true
5 Implemented false FileSystem true
6 Implemented false Block false
7 Implemented true Undefined true
8 Implemented true FileSystem true
9 Implemented true Block true
Move volumeMode check to canProvision and call it after name check. Unit tests for volumeMode check are also modified.
|
I've fixed the codes as we discussed. PTAL @wongma7 |
|
/lgtm thank you for the tests as well |
To update "github.com/kubernetes-incubator/external-storage" to use kubernetes-retired/external-storage#830, just dep-ensure it doesn't work well with below error. # dep ensure -update "github.com/kubernetes-incubator/external-storage" # make container mkdir -p bin CGO_ENABLED=0 GOOS=linux go build -a -ldflags '-X main.version=v0.2.0-24-g6f650608-dirty -extldflags "-static"' -o ./bin/csi-provisioner ./cmd/csi-provisioner # github.com/kubernetes-csi/external-provisioner/vendor/github.com/kubernetes-incubator/external-storage/lib/controller vendor/github.com/kubernetes-incubator/external-storage/lib/controller/controller.go:455:39: unknown field 'Limiter' in struct literal of type workqueue.BucketRateLimiter vendor/github.com/kubernetes-incubator/external-storage/lib/controller/controller.go:459:40: unknown field 'Limiter' in struct literal of type workqueue.BucketRateLimiter make: *** [Makefile:32: csi-provisioner] Error 2 Because kubernetes-incubator/external-storage was changed to access to Limiter in workqueue.BucketRateLimiter in (*1), and it is introduced to client-go in (*2). So, client-go needs to be updated to the version after this commit. (*1) Use rate limited work queues and up resync from 15s to 15m kubernetes-retired/external-storage@126c9ff (*2) Switch from juju/ratelimit to golang.org/x/time/rate kubernetes/client-go@acc5249 So, Gopkg.toml is updated to make "k8s.io/client-go" use "v7.0.0" instead of "v6.0.0", and also it is dep-ensure'd. # dep ensure -update "k8s.io/client-go"
This PR add a block volume support check to external provisioners. This PR introduces BlockProvisioner interface and SupportsBlock() to check whether the provisioner supports block volume. They are used by the external provisioner library and if a provisioner doesn't supports block volume, the library output an error and doesn't try provisioning the block volume.
With this change, external provisioner that supports block volume is required to implement SupportsBlock() and return true. (Also, it must set volumeMode to pv spec that it returns.)
Specification and implementation have been discussed in below PR.
#787