From de5b4dc5bcbaa870dd42565879d0bc3cb7e2019b Mon Sep 17 00:00:00 2001 From: mkimuram Date: Mon, 25 Jun 2018 18:41:18 -0400 Subject: [PATCH 1/3] [v2] Add block volume support check to external provisioners 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.) --- .../targetd/provisioner/iscsi-provisioner.go | 4 ++++ lib/controller/controller.go | 21 +++++++++++++++++++ lib/controller/volume.go | 8 +++++++ lib/util/util.go | 6 ++++++ 4 files changed, 39 insertions(+) diff --git a/iscsi/targetd/provisioner/iscsi-provisioner.go b/iscsi/targetd/provisioner/iscsi-provisioner.go index c6157c05709..150b4dcf8df 100644 --- a/iscsi/targetd/provisioner/iscsi-provisioner.go +++ b/iscsi/targetd/provisioner/iscsi-provisioner.go @@ -475,3 +475,7 @@ func (p *iscsiProvisioner) getConnection() (*jsonrpc2.Client, error) { log.Debugln("targetd client created") return client, nil } + +func (p *iscsiProvisioner) SupportsBlock() bool { + return true +} diff --git a/lib/controller/controller.go b/lib/controller/controller.go index f2ffdac83cc..c775ddc7b82 100644 --- a/lib/controller/controller.go +++ b/lib/controller/controller.go @@ -30,6 +30,7 @@ import ( "github.com/kubernetes-incubator/external-storage/lib/controller/metrics" "github.com/kubernetes-incubator/external-storage/lib/leaderelection" rl "github.com/kubernetes-incubator/external-storage/lib/leaderelection/resourcelock" + "github.com/kubernetes-incubator/external-storage/lib/util" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" "golang.org/x/time/rate" @@ -840,6 +841,16 @@ func (ctrl *ProvisionController) shouldProvision(claim *v1.PersistentVolumeClaim } } + // Check if this provisioner supports Block volume + if util.CheckPersistentVolumeClaimModeBlock(claim) && !ctrl.supportsBlock() { + claimClass := helper.GetPersistentVolumeClaimClass(claim) + strerr := fmt.Sprintf("%s does not support block volume provisioning", ctrl.provisionerName) + ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", strerr) + glog.Errorf("Failed to provision volume for claim %q with StorageClass %q: %v", + claimToClaimKey(claim), claimClass, strerr) + return false + } + // Kubernetes 1.5 provisioning with annStorageProvisioner if ctrl.kubeVersion.AtLeast(utilversion.MustParseSemantic("v1.5.0")) { if provisioner, found := claim.Annotations[annStorageProvisioner]; found { @@ -1361,3 +1372,13 @@ func (ctrl *ProvisionController) fetchReclaimPolicy(storageClassName string) (v1 return v1.PersistentVolumeReclaimDelete, fmt.Errorf("Cannot convert object to StorageClass: %+v", classObj) } + +// supportsBlock returns whether a provisioner supports block volume. +// Provisioners that implement BlockProvisioner interface and return true to SupportsBlock +// will be regarded as supported for block volume. +func (ctrl *ProvisionController) supportsBlock() bool { + if blockProvisioner, ok := ctrl.provisioner.(BlockProvisioner); ok { + return blockProvisioner.SupportsBlock() + } + return false +} diff --git a/lib/controller/volume.go b/lib/controller/volume.go index a714ce1ba57..944d73c6b6f 100644 --- a/lib/controller/volume.go +++ b/lib/controller/volume.go @@ -47,6 +47,14 @@ type Qualifier interface { ShouldProvision(*v1.PersistentVolumeClaim) bool } +// BlockProvisioner is an optional interface implemented by provisioners to determine +// whether it supports block volume. +type BlockProvisioner interface { + Provisioner + // SupportsBlock returns whether provisioner supports block volume. + SupportsBlock() bool +} + // IgnoredError is the value for Delete to return to indicate that the call has // been ignored and no action taken. In case multiple provisioners are serving // the same storage class, provisioners may ignore PVs they are not responsible diff --git a/lib/util/util.go b/lib/util/util.go index ec60bfaf5b9..fee69b55ec7 100644 --- a/lib/util/util.go +++ b/lib/util/util.go @@ -59,3 +59,9 @@ func AccessModesContainedInAll(indexedModes []v1.PersistentVolumeAccessMode, req } return true } + +// CheckPersistentVolumeClaimModeBlock checks VolumeMode. +// If the mode is Block, return true otherwise return false. +func CheckPersistentVolumeClaimModeBlock(pvc *v1.PersistentVolumeClaim) bool { + return pvc.Spec.VolumeMode != nil && *pvc.Spec.VolumeMode == v1.PersistentVolumeBlock +} From cf881a867e6a8705dfd7163c4180efb5bc18a462 Mon Sep 17 00:00:00 2001 From: mkimuram Date: Tue, 26 Jun 2018 17:43:06 -0400 Subject: [PATCH 2/3] Add test cases for checking block volume support 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 --- lib/controller/controller_test.go | 97 +++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/lib/controller/controller_test.go b/lib/controller/controller_test.go index 3ec4c297e9c..b774710107d 100644 --- a/lib/controller/controller_test.go +++ b/lib/controller/controller_test.go @@ -408,6 +408,81 @@ func TestShouldProvision(t *testing.T) { claim: newClaim("claim-1", "1-1", "class-1", "foo.bar/baz", "", nil), expectedShould: true, }, + // volumeMode tests for provisioner w/o BlockProvisoner I/F + { + name: "Undefined volumeMode PV request to provisioner w/o BlockProvisoner I/F", + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaim("claim-1", "volmode-1-1", "class-1", "foo.bar/baz", "", nil), + expectedShould: true, + }, + { + name: "FileSystem volumeMode PV request to provisioner w/o BlockProvisoner I/F", + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaimWithVolumeMode("claim-1", "volmode-1-2", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeFilesystem), + expectedShould: true, + }, + { + name: "Block volumeMode PV request to provisioner w/o BlockProvisoner I/F", + provisionerName: "foo.bar/baz", + provisioner: newTestProvisioner(), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaimWithVolumeMode("claim-1", "volmode-1-3", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeBlock), + expectedShould: false, + }, + // volumeMode tests for BlockProvisioner that returns false + { + name: "Undefined volumeMode PV request to BlockProvisoner that returns false", + provisionerName: "foo.bar/baz", + provisioner: newTestBlockProvisioner(false), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaim("claim-1", "volmode-2-1", "class-1", "foo.bar/baz", "", nil), + expectedShould: true, + }, + { + name: "FileSystem volumeMode PV request to BlockProvisoner that returns false", + provisionerName: "foo.bar/baz", + provisioner: newTestBlockProvisioner(false), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaimWithVolumeMode("claim-1", "volmode-2-2", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeFilesystem), + expectedShould: true, + }, + { + name: "Block volumeMode PV request to BlockProvisoner that returns false", + provisionerName: "foo.bar/baz", + provisioner: newTestBlockProvisioner(false), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaimWithVolumeMode("claim-1", "volmode-2-3", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeBlock), + expectedShould: false, + }, + // volumeMode tests for BlockProvisioner that returns true + { + name: "Undefined volumeMode PV request to BlockProvisoner that returns true", + provisionerName: "foo.bar/baz", + provisioner: newTestBlockProvisioner(true), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaim("claim-1", "volmode-3-1", "class-1", "foo.bar/baz", "", nil), + expectedShould: true, + }, + { + name: "FileSystem volumeMode PV request to BlockProvisoner that returns true", + provisionerName: "foo.bar/baz", + provisioner: newTestBlockProvisioner(true), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaimWithVolumeMode("claim-1", "volmode-3-2", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeFilesystem), + expectedShould: true, + }, + { + name: "Block volumeMode PV request to BlockProvisioner that returns true", + provisionerName: "foo.bar/baz", + provisioner: newTestBlockProvisioner(true), + class: newStorageClass("class-1", "foo.bar/baz"), + claim: newClaimWithVolumeMode("claim-1", "volmode-3-3", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeBlock), + expectedShould: true, + }, } for _, test := range tests { client := fake.NewSimpleClientset(test.claim) @@ -684,6 +759,12 @@ func newClaim(name, claimUID, class, provisioner, volumeName string, annotations return claim } +func newClaimWithVolumeMode(name, claimUID, class, provisioner, volumeName string, annotations map[string]string, volumeMode v1.PersistentVolumeMode) *v1.PersistentVolumeClaim { + claim := newClaim(name, claimUID, class, provisioner, volumeName, annotations) + claim.Spec.VolumeMode = &volumeMode + return claim +} + func newVolume(name string, phase v1.PersistentVolumePhase, policy v1.PersistentVolumeReclaimPolicy, annotations map[string]string) *v1.PersistentVolume { pv := &v1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{ @@ -786,6 +867,22 @@ func (p *testQualifiedProvisioner) ShouldProvision(claim *v1.PersistentVolumeCla return p.answer } +func newTestBlockProvisioner(answer bool) *testBlockProvisioner { + return &testBlockProvisioner{newTestProvisioner(), answer} +} + +type testBlockProvisioner struct { + *testProvisioner + answer bool +} + +var _ Provisioner = &testBlockProvisioner{} +var _ BlockProvisioner = &testBlockProvisioner{} + +func (p *testBlockProvisioner) SupportsBlock() bool { + return p.answer +} + func (p *testProvisioner) Provision(options VolumeOptions) (*v1.PersistentVolume, error) { p.provisionCalls <- true From 3fb3b7c033cd42722bf4bcc8be613d351b6ad3f0 Mon Sep 17 00:00:00 2001 From: mkimuram Date: Thu, 28 Jun 2018 16:39:41 -0400 Subject: [PATCH 3/3] Move volumeMode check to canProvision and call it after name check Move volumeMode check to canProvision and call it after name check. Unit tests for volumeMode check are also modified. --- lib/controller/controller.go | 28 ++++-- lib/controller/controller_test.go | 162 ++++++++++++++++-------------- 2 files changed, 105 insertions(+), 85 deletions(-) diff --git a/lib/controller/controller.go b/lib/controller/controller.go index c775ddc7b82..b68c60b6774 100644 --- a/lib/controller/controller.go +++ b/lib/controller/controller.go @@ -841,16 +841,6 @@ func (ctrl *ProvisionController) shouldProvision(claim *v1.PersistentVolumeClaim } } - // Check if this provisioner supports Block volume - if util.CheckPersistentVolumeClaimModeBlock(claim) && !ctrl.supportsBlock() { - claimClass := helper.GetPersistentVolumeClaimClass(claim) - strerr := fmt.Sprintf("%s does not support block volume provisioning", ctrl.provisionerName) - ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", strerr) - glog.Errorf("Failed to provision volume for claim %q with StorageClass %q: %v", - claimToClaimKey(claim), claimClass, strerr) - return false - } - // Kubernetes 1.5 provisioning with annStorageProvisioner if ctrl.kubeVersion.AtLeast(utilversion.MustParseSemantic("v1.5.0")) { if provisioner, found := claim.Annotations[annStorageProvisioner]; found { @@ -907,6 +897,16 @@ func (ctrl *ProvisionController) shouldDelete(volume *v1.PersistentVolume) bool return true } +// canProvision returns error if provisioner can't provision claim. +func (ctrl *ProvisionController) canProvision(claim *v1.PersistentVolumeClaim) error { + // Check if this provisioner supports Block volume + if util.CheckPersistentVolumeClaimModeBlock(claim) && !ctrl.supportsBlock() { + return fmt.Errorf("%s does not support block volume provisioning", ctrl.provisionerName) + } + + return nil +} + // lockProvisionClaimOperation wraps provisionClaimOperation. In case other // controllers are serving the same claims, to prevent them all from creating // volumes for a claim & racing to submit their PV, each controller creates a @@ -1032,6 +1032,14 @@ func (ctrl *ProvisionController) provisionClaimOperation(claim *v1.PersistentVol return nil } + // Check if this provisioner can provision this claim. + if err := ctrl.canProvision(claim); err != nil { + ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error()) + glog.Errorf("Failed to provision volume for claim %q with StorageClass %q: %v", + claimToClaimKey(claim), claimClass, err) + return nil + } + reclaimPolicy := v1.PersistentVolumeReclaimDelete if ctrl.kubeVersion.AtLeast(utilversion.MustParseSemantic("v1.8.0")) { reclaimPolicy, err = ctrl.fetchReclaimPolicy(claimClass) diff --git a/lib/controller/controller_test.go b/lib/controller/controller_test.go index b774710107d..0036a2e2663 100644 --- a/lib/controller/controller_test.go +++ b/lib/controller/controller_test.go @@ -408,81 +408,6 @@ func TestShouldProvision(t *testing.T) { claim: newClaim("claim-1", "1-1", "class-1", "foo.bar/baz", "", nil), expectedShould: true, }, - // volumeMode tests for provisioner w/o BlockProvisoner I/F - { - name: "Undefined volumeMode PV request to provisioner w/o BlockProvisoner I/F", - provisionerName: "foo.bar/baz", - provisioner: newTestProvisioner(), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaim("claim-1", "volmode-1-1", "class-1", "foo.bar/baz", "", nil), - expectedShould: true, - }, - { - name: "FileSystem volumeMode PV request to provisioner w/o BlockProvisoner I/F", - provisionerName: "foo.bar/baz", - provisioner: newTestProvisioner(), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaimWithVolumeMode("claim-1", "volmode-1-2", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeFilesystem), - expectedShould: true, - }, - { - name: "Block volumeMode PV request to provisioner w/o BlockProvisoner I/F", - provisionerName: "foo.bar/baz", - provisioner: newTestProvisioner(), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaimWithVolumeMode("claim-1", "volmode-1-3", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeBlock), - expectedShould: false, - }, - // volumeMode tests for BlockProvisioner that returns false - { - name: "Undefined volumeMode PV request to BlockProvisoner that returns false", - provisionerName: "foo.bar/baz", - provisioner: newTestBlockProvisioner(false), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaim("claim-1", "volmode-2-1", "class-1", "foo.bar/baz", "", nil), - expectedShould: true, - }, - { - name: "FileSystem volumeMode PV request to BlockProvisoner that returns false", - provisionerName: "foo.bar/baz", - provisioner: newTestBlockProvisioner(false), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaimWithVolumeMode("claim-1", "volmode-2-2", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeFilesystem), - expectedShould: true, - }, - { - name: "Block volumeMode PV request to BlockProvisoner that returns false", - provisionerName: "foo.bar/baz", - provisioner: newTestBlockProvisioner(false), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaimWithVolumeMode("claim-1", "volmode-2-3", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeBlock), - expectedShould: false, - }, - // volumeMode tests for BlockProvisioner that returns true - { - name: "Undefined volumeMode PV request to BlockProvisoner that returns true", - provisionerName: "foo.bar/baz", - provisioner: newTestBlockProvisioner(true), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaim("claim-1", "volmode-3-1", "class-1", "foo.bar/baz", "", nil), - expectedShould: true, - }, - { - name: "FileSystem volumeMode PV request to BlockProvisoner that returns true", - provisionerName: "foo.bar/baz", - provisioner: newTestBlockProvisioner(true), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaimWithVolumeMode("claim-1", "volmode-3-2", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeFilesystem), - expectedShould: true, - }, - { - name: "Block volumeMode PV request to BlockProvisioner that returns true", - provisionerName: "foo.bar/baz", - provisioner: newTestBlockProvisioner(true), - class: newStorageClass("class-1", "foo.bar/baz"), - claim: newClaimWithVolumeMode("claim-1", "volmode-3-3", "class-1", "foo.bar/baz", "", nil, v1.PersistentVolumeBlock), - expectedShould: true, - }, } for _, test := range tests { client := fake.NewSimpleClientset(test.claim) @@ -570,6 +495,93 @@ func TestShouldDelete(t *testing.T) { } } +func TestCanProvision(t *testing.T) { + const ( + provisionerName = "foo.bar/baz" + blockErrFormat = "%s does not support block volume provisioning" + ) + + tests := []struct { + name string + provisioner Provisioner + claim *v1.PersistentVolumeClaim + serverGitVersion string + expectedCan error + }{ + // volumeMode tests for provisioner w/o BlockProvisoner I/F + { + name: "Undefined volumeMode PV request to provisioner w/o BlockProvisoner I/F", + provisioner: newTestProvisioner(), + claim: newClaim("claim-1", "1-1", "class-1", provisionerName, "", nil), + expectedCan: nil, + }, + { + name: "FileSystem volumeMode PV request to provisioner w/o BlockProvisoner I/F", + provisioner: newTestProvisioner(), + claim: newClaimWithVolumeMode("claim-1", "1-1", "class-1", provisionerName, "", nil, v1.PersistentVolumeFilesystem), + expectedCan: nil, + }, + { + name: "Block volumeMode PV request to provisioner w/o BlockProvisoner I/F", + provisioner: newTestProvisioner(), + claim: newClaimWithVolumeMode("claim-1", "1-1", "class-1", provisionerName, "", nil, v1.PersistentVolumeBlock), + expectedCan: fmt.Errorf(blockErrFormat, provisionerName), + }, + // volumeMode tests for BlockProvisioner that returns false + { + name: "Undefined volumeMode PV request to BlockProvisoner that returns false", + provisioner: newTestBlockProvisioner(false), + claim: newClaim("claim-1", "1-1", "class-1", provisionerName, "", nil), + expectedCan: nil, + }, + { + name: "FileSystem volumeMode PV request to BlockProvisoner that returns false", + provisioner: newTestBlockProvisioner(false), + claim: newClaimWithVolumeMode("claim-1", "1-1", "class-1", provisionerName, "", nil, v1.PersistentVolumeFilesystem), + expectedCan: nil, + }, + { + name: "Block volumeMode PV request to BlockProvisoner that returns false", + provisioner: newTestBlockProvisioner(false), + claim: newClaimWithVolumeMode("claim-1", "1-1", "class-1", provisionerName, "", nil, v1.PersistentVolumeBlock), + expectedCan: fmt.Errorf(blockErrFormat, provisionerName), + }, + // volumeMode tests for BlockProvisioner that returns true + { + name: "Undefined volumeMode PV request to BlockProvisoner that returns true", + provisioner: newTestBlockProvisioner(true), + claim: newClaim("claim-1", "1-1", "class-1", provisionerName, "", nil), + expectedCan: nil, + }, + { + name: "FileSystem volumeMode PV request to BlockProvisoner that returns true", + provisioner: newTestBlockProvisioner(true), + claim: newClaimWithVolumeMode("claim-1", "1-1", "class-1", provisionerName, "", nil, v1.PersistentVolumeFilesystem), + expectedCan: nil, + }, + { + name: "Block volumeMode PV request to BlockProvisioner that returns true", + provisioner: newTestBlockProvisioner(true), + claim: newClaimWithVolumeMode("claim-1", "1-1", "class-1", provisionerName, "", nil, v1.PersistentVolumeBlock), + expectedCan: nil, + }, + } + for _, test := range tests { + client := fake.NewSimpleClientset(test.claim) + serverVersion := defaultServerVersion + if test.serverGitVersion != "" { + serverVersion = test.serverGitVersion + } + ctrl := newTestProvisionController(client, provisionerName, test.provisioner, serverVersion) + + can := ctrl.canProvision(test.claim) + if !reflect.DeepEqual(test.expectedCan, can) { + t.Logf("test case: %s", test.name) + t.Errorf("expected can provision %v but got %v\n", test.expectedCan, can) + } + } +} + func TestControllerSharedInformers(t *testing.T) { tests := []struct { name string