diff --git a/pkg/controller/volume/persistentvolume/binder_test.go b/pkg/controller/volume/persistentvolume/binder_test.go index a91c897f702..b868fe62c5f 100644 --- a/pkg/controller/volume/persistentvolume/binder_test.go +++ b/pkg/controller/volume/persistentvolume/binder_test.go @@ -422,6 +422,86 @@ func TestSync(t *testing.T) { newClaimArray("claim4-8", "uid4-8", "10Gi", "volume4-8-x", api.ClaimBound), noevents, noerrors, testSyncVolume, }, + + // PVC with class + { + // syncVolume binds a claim to requested class even if there is a + // smaller PV available + "13-1 - binding to class", + []*api.PersistentVolume{ + newVolume("volume13-1-1", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain), + newVolume("volume13-1-2", "10Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain, annClass), + }, + []*api.PersistentVolume{ + newVolume("volume13-1-1", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain), + newVolume("volume13-1-2", "10Gi", "uid13-1", "claim13-1", api.VolumeBound, api.PersistentVolumeReclaimRetain, annBoundByController, annClass), + }, + newClaimArray("claim13-1", "uid13-1", "1Gi", "", api.ClaimPending, annClass), + withExpectedCapacity("10Gi", newClaimArray("claim13-1", "uid13-1", "1Gi", "volume13-1-2", api.ClaimBound, annBoundByController, annClass, annBindCompleted)), + noevents, noerrors, testSyncClaim, + }, + { + // syncVolume binds a claim without a class even if there is a + // smaller PV with a class available + "13-2 - binding without a class", + []*api.PersistentVolume{ + newVolume("volume13-2-1", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain, annClass), + newVolume("volume13-2-2", "10Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain), + }, + []*api.PersistentVolume{ + newVolume("volume13-2-1", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain, annClass), + newVolume("volume13-2-2", "10Gi", "uid13-2", "claim13-2", api.VolumeBound, api.PersistentVolumeReclaimRetain, annBoundByController), + }, + newClaimArray("claim13-2", "uid13-2", "1Gi", "", api.ClaimPending), + withExpectedCapacity("10Gi", newClaimArray("claim13-2", "uid13-2", "1Gi", "volume13-2-2", api.ClaimBound, annBoundByController, annBindCompleted)), + noevents, noerrors, testSyncClaim, + }, + { + // syncVolume binds a claim with given class even if there is a + // smaller PV with different class available + "13-3 - binding to specific a class", + volumeWithClass("silver", []*api.PersistentVolume{ + newVolume("volume13-3-1", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain), + newVolume("volume13-3-2", "10Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain, annClass), + }), + volumeWithClass("silver", []*api.PersistentVolume{ + newVolume("volume13-3-1", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain), + newVolume("volume13-3-2", "10Gi", "uid13-3", "claim13-3", api.VolumeBound, api.PersistentVolumeReclaimRetain, annBoundByController, annClass), + }), + newClaimArray("claim13-3", "uid13-3", "1Gi", "", api.ClaimPending, annClass), + withExpectedCapacity("10Gi", newClaimArray("claim13-3", "uid13-3", "1Gi", "volume13-3-2", api.ClaimBound, annBoundByController, annBindCompleted, annClass)), + noevents, noerrors, testSyncClaim, + }, + { + // syncVolume binds claim requesting class "" to claim to PV with + // class="" + "13-4 - empty class", + volumeWithClass("", newVolumeArray("volume13-4", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain)), + volumeWithClass("", newVolumeArray("volume13-4", "1Gi", "uid13-4", "claim13-4", api.VolumeBound, api.PersistentVolumeReclaimRetain, annBoundByController)), + claimWithClass("", newClaimArray("claim13-4", "uid13-4", "1Gi", "", api.ClaimPending)), + claimWithClass("", newClaimArray("claim13-4", "uid13-4", "1Gi", "volume13-4", api.ClaimBound, annBoundByController, annBindCompleted)), + noevents, noerrors, testSyncClaim, + }, + { + // syncVolume binds claim requesting class nil to claim to PV with + // class = "" + "13-5 - nil class", + volumeWithClass("", newVolumeArray("volume13-5", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain)), + volumeWithClass("", newVolumeArray("volume13-5", "1Gi", "uid13-5", "claim13-5", api.VolumeBound, api.PersistentVolumeReclaimRetain, annBoundByController)), + newClaimArray("claim13-5", "uid13-5", "1Gi", "", api.ClaimPending), + newClaimArray("claim13-5", "uid13-5", "1Gi", "volume13-5", api.ClaimBound, annBoundByController, annBindCompleted), + noevents, noerrors, testSyncClaim, + }, + { + // syncVolume binds claim requesting class "" to claim to PV with + // class=nil + "13-6 - nil class in PV, '' class in claim", + newVolumeArray("volume13-6", "1Gi", "", "", api.VolumePending, api.PersistentVolumeReclaimRetain), + newVolumeArray("volume13-6", "1Gi", "uid13-6", "claim13-6", api.VolumeBound, api.PersistentVolumeReclaimRetain, annBoundByController), + claimWithClass("", newClaimArray("claim13-6", "uid13-6", "1Gi", "", api.ClaimPending)), + claimWithClass("", newClaimArray("claim13-6", "uid13-6", "1Gi", "volume13-6", api.ClaimBound, annBoundByController, annBindCompleted)), + noevents, noerrors, testSyncClaim, + }, } runSyncTests(t, tests, []*extensions.StorageClass{}) } diff --git a/pkg/controller/volume/persistentvolume/controller.go b/pkg/controller/volume/persistentvolume/controller.go index f7991efea7f..34152258452 100644 --- a/pkg/controller/volume/persistentvolume/controller.go +++ b/pkg/controller/volume/persistentvolume/controller.go @@ -167,7 +167,6 @@ type PersistentVolumeController struct { volumePluginMgr vol.VolumePluginMgr enableDynamicProvisioning bool clusterName string - defaultStorageClass string // Cache of the last known version of volumes and claims. This cache is // thread safe as long as the volumes/claims there are not modified, they @@ -1176,7 +1175,8 @@ func (ctrl *PersistentVolumeController) doDeleteVolume(volume *api.PersistentVol return nil } -// provisionClaim starts new asynchronous operation to provision a claim if provisioning is enabled. +// provisionClaim starts new asynchronous operation to provision a claim if +// provisioning is enabled. func (ctrl *PersistentVolumeController) provisionClaim(claim *api.PersistentVolumeClaim) error { if !ctrl.enableDynamicProvisioning { return nil @@ -1198,7 +1198,8 @@ func (ctrl *PersistentVolumeController) provisionClaimOperation(claimObj interfa glog.Errorf("Cannot convert provisionClaimOperation argument to claim, got %#v", claimObj) return } - glog.V(4).Infof("provisionClaimOperation [%s] started", claimToClaimKey(claim)) + claimClass := getClaimClass(claim) + glog.V(4).Infof("provisionClaimOperation [%s] started, class: %q", claimToClaimKey(claim), claimClass) // A previous doProvisionClaim may just have finished while we were waiting for // the locks. Check that PV (with deterministic name) hasn't been provisioned @@ -1275,7 +1276,7 @@ func (ctrl *PersistentVolumeController) provisionClaimOperation(claimObj interfa // Add annBoundByController (used in deleting the volume) setAnnotation(&volume.ObjectMeta, annBoundByController, "yes") setAnnotation(&volume.ObjectMeta, annDynamicallyProvisioned, plugin.GetPluginName()) - setAnnotation(&volume.ObjectMeta, annClass, getClaimClass(claim)) + setAnnotation(&volume.ObjectMeta, annClass, claimClass) // Try to create the PV object several times for i := 0; i < ctrl.createProvisionedPVRetryCount; i++ { @@ -1356,38 +1357,25 @@ func (ctrl *PersistentVolumeController) scheduleOperation(operationName string, } func (ctrl *PersistentVolumeController) findProvisionablePlugin(claim *api.PersistentVolumeClaim) (vol.ProvisionableVolumePlugin, *extensions.StorageClass, error) { - storageClass, err := ctrl.findStorageClass(claim) + // provisionClaim() which leads here is never called with claimClass=="", we + // can save some checks. + claimClass := getClaimClass(claim) + classObj, found, err := ctrl.classes.GetByKey(claimClass) if err != nil { return nil, nil, err } - - // Find a plugin for the class - plugin, err := ctrl.volumePluginMgr.FindProvisionablePluginByName(storageClass.Provisioner) - if err != nil { - return nil, nil, err - } - return plugin, storageClass, nil -} - -func (ctrl *PersistentVolumeController) findStorageClass(claim *api.PersistentVolumeClaim) (*extensions.StorageClass, error) { - className := getClaimClass(claim) - if className == "" { - className = ctrl.defaultStorageClass - } - if className == "" { - return nil, fmt.Errorf("No default StorageClass configured") - } - - classObj, found, err := ctrl.classes.GetByKey(className) - if err != nil { - return nil, err - } if !found { - return nil, fmt.Errorf("StorageClass %q not found", className) + return nil, nil, fmt.Errorf("StorageClass %q not found", claimClass) } class, ok := classObj.(*extensions.StorageClass) if !ok { - return nil, fmt.Errorf("Cannot convert object to StorageClass: %+v", classObj) + return nil, nil, fmt.Errorf("Cannot convert object to StorageClass: %+v", classObj) } - return class, nil + + // Find a plugin for the class + plugin, err := ctrl.volumePluginMgr.FindProvisionablePluginByName(class.Provisioner) + if err != nil { + return nil, nil, err + } + return plugin, class, nil } diff --git a/pkg/controller/volume/persistentvolume/controller_base.go b/pkg/controller/volume/persistentvolume/controller_base.go index b92f65cb73b..5571fd5d5d4 100644 --- a/pkg/controller/volume/persistentvolume/controller_base.go +++ b/pkg/controller/volume/persistentvolume/controller_base.go @@ -609,16 +609,20 @@ func getVolumeClass(volume *api.PersistentVolume) string { if class, found := volume.Annotations[annClass]; found { return class } + + // 'nil' is interpreted as "", i.e. the volume does not belong to any class. return "" } -// getClaimClass returns value of annClass annotation or empty string in case -// the annotation does not exist. -// TODO: change to PersistentVolumeClaim.Spec.Class value when this attribute is -// introduced. +// getClaimClass returns name of class that is requested by given claim. +// Request for `nil` class is interpreted as request for class "", +// i.e. for a classless PV. func getClaimClass(claim *api.PersistentVolumeClaim) string { + // TODO: change to PersistentVolumeClaim.Spec.Class value when this + // attribute is introduced. if class, found := claim.Annotations[annClass]; found { return class } + return "" } diff --git a/pkg/controller/volume/persistentvolume/framework_test.go b/pkg/controller/volume/persistentvolume/framework_test.go index 879e7af0e19..1ece6f986f1 100644 --- a/pkg/controller/volume/persistentvolume/framework_test.go +++ b/pkg/controller/volume/persistentvolume/framework_test.go @@ -591,6 +591,9 @@ func newTestController(kubeClient clientset.Interface, volumeSource, claimSource if claimSource == nil { claimSource = framework.NewFakePVCControllerSource() } + if classSource == nil { + classSource = framework.NewFakeControllerSource() + } ctrl := NewPersistentVolumeController( kubeClient, 5*time.Second, // sync period diff --git a/pkg/controller/volume/persistentvolume/index.go b/pkg/controller/volume/persistentvolume/index.go index b63516ecf8c..8c47041b098 100644 --- a/pkg/controller/volume/persistentvolume/index.go +++ b/pkg/controller/volume/persistentvolume/index.go @@ -18,11 +18,12 @@ package persistentvolume import ( "fmt" + "sort" + "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/client/cache" "k8s.io/kubernetes/pkg/labels" - "sort" ) // persistentVolumeOrderedIndex is a cache.Store that keeps persistent volumes @@ -91,6 +92,7 @@ func (pvIndex *persistentVolumeOrderedIndex) findByClaim(claim *api.PersistentVo var smallestVolumeSize int64 requestedQty := claim.Spec.Resources.Requests[api.ResourceName(api.ResourceStorage)] requestedSize := requestedQty.Value() + requestedClass := getClaimClass(claim) var selector labels.Selector if claim.Spec.Selector != nil { @@ -131,8 +133,7 @@ func (pvIndex *persistentVolumeOrderedIndex) findByClaim(claim *api.PersistentVo } else if selector != nil && !selector.Matches(labels.Set(volume.Labels)) { continue } - claimClass := getClaimClass(claim) - if claimClass != "" && claimClass != getVolumeClass(volume) { + if getVolumeClass(volume) != requestedClass { continue } diff --git a/pkg/controller/volume/persistentvolume/index_test.go b/pkg/controller/volume/persistentvolume/index_test.go index c87d5a7e6e1..f233b7fe348 100644 --- a/pkg/controller/volume/persistentvolume/index_test.go +++ b/pkg/controller/volume/persistentvolume/index_test.go @@ -740,7 +740,7 @@ func TestFindingPreboundVolumes(t *testing.T) { // pretend the exact match is available but the largest volume is pre-bound to the claim. pv1.Spec.ClaimRef = nil pv8.Spec.ClaimRef = claimRef - volume, _ = index.findBestMatchForClaim(claim, "") + volume, _ = index.findBestMatchForClaim(claim) if volume.Name != pv8.Name { t.Errorf("Expected %s but got volume %s instead", pv8.Name, volume.Name) } diff --git a/pkg/controller/volume/persistentvolume/provision_test.go b/pkg/controller/volume/persistentvolume/provision_test.go index 8f317336212..f0d8fb5687b 100644 --- a/pkg/controller/volume/persistentvolume/provision_test.go +++ b/pkg/controller/volume/persistentvolume/provision_test.go @@ -81,7 +81,7 @@ var provision2Success = provisionCall{ func TestProvisionSync(t *testing.T) { tests := []controllerTest{ { - // Provision a volume (with the default class) + // Provision a volume (with a default class) "11-1 - successful provision with storage class 1", novolumes, newVolumeArray("pvc-uid11-1", "1Gi", "uid11-1", "claim11-1", api.VolumeBound, api.PersistentVolumeReclaimDelete, annBoundByController, annDynamicallyProvisioned, annClass), @@ -292,6 +292,24 @@ func TestProvisionSync(t *testing.T) { claimWithClass("non-existing", newClaimArray("claim11-14", "uid11-14", "1Gi", "", api.ClaimPending)), noevents, noerrors, wrapTestWithProvisionCalls([]provisionCall{}, testSyncClaim), }, + { + // No provisioning with class="" + "11-15 - no provisioning with class=''", + novolumes, + novolumes, + claimWithClass("", newClaimArray("claim11-15", "uid11-15", "1Gi", "", api.ClaimPending)), + claimWithClass("", newClaimArray("claim11-15", "uid11-15", "1Gi", "", api.ClaimPending)), + noevents, noerrors, wrapTestWithProvisionCalls([]provisionCall{}, testSyncClaim), + }, + { + // No provisioning with class=nil + "11-16 - no provisioning with class=nil", + novolumes, + novolumes, + newClaimArray("claim11-15", "uid11-15", "1Gi", "", api.ClaimPending), + newClaimArray("claim11-15", "uid11-15", "1Gi", "", api.ClaimPending), + noevents, noerrors, wrapTestWithProvisionCalls([]provisionCall{}, testSyncClaim), + }, } runSyncTests(t, tests, storageClasses) }