Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Commit

Permalink
Move volumeMode check to canProvision and call it after name check
Browse files Browse the repository at this point in the history 
Move volumeMode check to canProvision and call it after name check.
Unit tests for volumeMode check are also modified.
  • Loading branch information
@mkimuram
mkimuram committed Jun 28, 2018
1 parent c2d3a68 commit 7ad86c3
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 85 deletions.
28 changes: 18 additions & 10 deletions lib/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -730,16 +730,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 {
Expand Down Expand Up @@ -804,6 +794,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
Expand Down Expand Up @@ -975,6 +975,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)
Expand Down
162 changes: 87 additions & 75 deletions lib/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,81 +405,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)
Expand Down Expand Up @@ -567,6 +492,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 TestIsOnlyRecordUpdate(t *testing.T) {
tests := []struct {
name string
Expand Down

0 comments on commit 7ad86c3

Please sign in to comment.