volume controller: Speed up binding by not sorting volumes

The binder sorts all available volumes first, then it filters out volumes
that cannot be bound by processing each volume in a loop and then finds
the smallest matching volume by binary search.

So, if we process every available volume in a loop, we can also remember the
smallest matching one and save us potentially long sorting (and quick binary
search).

pkg/controller/persistentvolume/index.go

@@ -38,7 +38,7 @@ func accessModesIndexFunc(obj interface{}) ([]string, error) {
 	return []string{""}, fmt.Errorf("object is not a persistent volume: %v", obj)
 }
 
-// listByAccessModes returns all volumes with the given set of AccessModeTypes *in order* of their storage capacity (low to high)
+// listByAccessModes returns all volumes with the given set of AccessModeTypes. The list is unsorted!
 func (pvIndex *persistentVolumeOrderedIndex) listByAccessModes(modes []api.PersistentVolumeAccessMode) ([]*api.PersistentVolume, error) {
 	pv := &api.PersistentVolume{
 		Spec: api.PersistentVolumeSpec{
@@ -56,7 +56,6 @@ func (pvIndex *persistentVolumeOrderedIndex) listByAccessModes(modes []api.Persi
 		volumes[i] = obj.(*api.PersistentVolume)
 	}
 
-	sort.Sort(byCapacity{volumes})
 	return volumes, nil
 }
 
@@ -75,26 +74,43 @@ func (pvIndex *persistentVolumeOrderedIndex) findByClaim(claim *api.PersistentVo
 	// potential matches (the GCEPD example above).
 	allPossibleModes := pvIndex.allPossibleMatchingAccessModes(claim.Spec.AccessModes)
 
+	var smallestVolume *api.PersistentVolume
+	var smallestVolumeSize int64
+	requestedQty := claim.Spec.Resources.Requests[api.ResourceName(api.ResourceStorage)]
+	requestedSize := requestedQty.Value()
+
 	for _, modes := range allPossibleModes {
 		volumes, err := pvIndex.listByAccessModes(modes)
 		if err != nil {
 			return nil, err
 		}
 
-		// volumes are sorted by size but some may be bound or earmarked for a specific claim.
+		// Go through all available volumes with two goals:
-		// filter those volumes for easy binary search by size
+		// - find a volume that is either pre-bound by user or dynamically
-		// return the exact pre-binding match, if found
+		//   provisioned for this claim. Because of this we need to loop through
-		unboundVolumes := []*api.PersistentVolume{}
+		//   all volumes.
+		// - find the smallest matching one if there is no volume pre-bound to
+		//   the claim.
 		for _, volume := range volumes {
-			// volume isn't currently bound or pre-bound.
+			if isVolumeBoundToClaim(volume, claim) {
-			if volume.Spec.ClaimRef == nil {
+				// Exact match! No search required. This catches both volumes
-				unboundVolumes = append(unboundVolumes, volume)
+				// pre-bound by user and volumes dynamically provisioned by the
+				// controller.
+				return volume, nil
+			}
+
+			if volume.Spec.ClaimRef != nil {
+				// This volume waits for exact claim or is alredy bound.
 				continue
 			}
 
-			if isVolumeBoundToClaim(volume, claim) {
+			volumeQty := volume.Spec.Capacity[api.ResourceStorage]
-				// Exact match! No search required.
+			volumeSize := volumeQty.Value()
-				return volume, nil
+			if volumeSize >= requestedSize {
+				if smallestVolume == nil || smallestVolumeSize > volumeSize {
+					smallestVolume = volume
+					smallestVolumeSize = volumeSize
+				}
 			}
 		}
 
@@ -109,18 +125,9 @@ func (pvIndex *persistentVolumeOrderedIndex) findByClaim(claim *api.PersistentVo
 			return nil, nil
 		}
 
-		searchPV := &api.PersistentVolume{
+		if smallestVolume != nil {
-			Spec: api.PersistentVolumeSpec{
+			// Found a matching volume
-				AccessModes: claim.Spec.AccessModes,
+			return smallestVolume, nil
-				Capacity: api.ResourceList{
-					api.ResourceName(api.ResourceStorage): claim.Spec.Resources.Requests[api.ResourceName(api.ResourceStorage)],
-				},
-			},
-		}
-
-		i := sort.Search(len(unboundVolumes), func(i int) bool { return matchPredicate(searchPV, unboundVolumes[i]) })
-		if i < len(unboundVolumes) {
-			return unboundVolumes[i], nil
 		}
 	}
 	return nil, nil
@@ -131,23 +138,6 @@ func (pvIndex *persistentVolumeOrderedIndex) findBestMatchForClaim(claim *api.Pe
 	return pvIndex.findByClaim(claim, matchStorageCapacity)
 }
 
-// byCapacity is used to order volumes by ascending storage size
-type byCapacity struct {
-	volumes []*api.PersistentVolume
-}
-
-func (c byCapacity) Less(i, j int) bool {
-	return matchStorageCapacity(c.volumes[i], c.volumes[j])
-}
-
-func (c byCapacity) Swap(i, j int) {
-	c.volumes[i], c.volumes[j] = c.volumes[j], c.volumes[i]
-}
-
-func (c byCapacity) Len() int {
-	return len(c.volumes)
-}
-
 // matchStorageCapacity is a matchPredicate used to sort and find volumes
 func matchStorageCapacity(pvA, pvB *api.PersistentVolume) bool {
 	aQty := pvA.Spec.Capacity[api.ResourceStorage]

pkg/controller/persistentvolume/index_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package persistentvolume
 
 import (
+	"sort"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api"
@@ -193,7 +194,7 @@ func TestMatchingWithBoundVolumes(t *testing.T) {
 	}
 }
 
-func TestSort(t *testing.T) {
+func TestListByAccessModes(t *testing.T) {
 	volList := newPersistentVolumeOrderedIndex()
 	for _, pv := range createTestVolumes() {
 		volList.store.Add(pv)
@@ -203,6 +204,7 @@ func TestSort(t *testing.T) {
 	if err != nil {
 		t.Error("Unexpected error retrieving volumes by access modes:", err)
 	}
+	sort.Sort(byCapacity{volumes})
 
 	for i, expected := range []string{"gce-pd-1", "gce-pd-5", "gce-pd-10"} {
 		if string(volumes[i].UID) != expected {
@@ -214,6 +216,7 @@ func TestSort(t *testing.T) {
 	if err != nil {
 		t.Error("Unexpected error retrieving volumes by access modes:", err)
 	}
+	sort.Sort(byCapacity{volumes})
 
 	for i, expected := range []string{"nfs-1", "nfs-5", "nfs-10"} {
 		if string(volumes[i].UID) != expected {
@@ -552,3 +555,20 @@ func TestFindingPreboundVolumes(t *testing.T) {
 		t.Errorf("Expected %s but got volume %s instead", pv8.Name, volume.Name)
 	}
 }
+
+// byCapacity is used to order volumes by ascending storage size
+type byCapacity struct {
+	volumes []*api.PersistentVolume
+}
+
+func (c byCapacity) Less(i, j int) bool {
+	return matchStorageCapacity(c.volumes[i], c.volumes[j])
+}
+
+func (c byCapacity) Swap(i, j int) {
+	c.volumes[i], c.volumes[j] = c.volumes[j], c.volumes[i]
+}
+
+func (c byCapacity) Len() int {
+	return len(c.volumes)
+}