Reorder conditions in FindMatchingVolume to avoid checking NodeAffinity

in trivial cases.

Author: mborsz

pkg/controller/volume/persistentvolume/util/util.go

@@ -211,9 +211,14 @@ func FindMatchingVolume(
 			// Skip volumes in the excluded list
 			continue
 		}
+		if volume.Spec.ClaimRef != nil && !IsVolumeBoundToClaim(volume, claim) {
+			continue
+		}
 
 		volumeQty := volume.Spec.Capacity[v1.ResourceStorage]
-
+		if volumeQty.Cmp(requestedQty) < 0 {
+			continue
+		}
 		// filter out mismatching volumeModes
 		if CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) {
 			continue
@@ -230,20 +235,15 @@ func FindMatchingVolume(
 		if node != nil {
 			// Scheduler path, check that the PV NodeAffinity
 			// is satisfied by the node
+			// volumeutil.CheckNodeAffinity is the most expensive call in this loop.
+			// We should check cheaper conditions first or consider optimizing this function.
 			err := volumeutil.CheckNodeAffinity(volume, node.Labels)
 			if err != nil {
 				nodeAffinityValid = false
 			}
 		}
 
 		if IsVolumeBoundToClaim(volume, claim) {
-			// this claim and volume are pre-bound; return
-			// the volume if the size request is satisfied,
-			// otherwise continue searching for a match
-			if volumeQty.Cmp(requestedQty) < 0 {
-				continue
-			}
-
 			// If PV node affinity is invalid, return no match.
 			// This means the prebound PV (and therefore PVC)
 			// is not suitable for this node.
@@ -263,7 +263,6 @@ func FindMatchingVolume(
 
 		// filter out:
 		// - volumes in non-available phase
-		// - volumes bound to another claim
 		// - volumes whose labels don't match the claim's selector, if specified
 		// - volumes in Class that is not requested
 		// - volumes whose NodeAffinity does not match the node
@@ -273,8 +272,6 @@ func FindMatchingVolume(
 			// them now has high chance of encountering unnecessary failures
 			// due to API conflicts.
 			continue
-		} else if volume.Spec.ClaimRef != nil {
-			continue
 		} else if selector != nil && !selector.Matches(labels.Set(volume.Labels)) {
 			continue
 		}
@@ -293,11 +290,9 @@ func FindMatchingVolume(
 			}
 		}
 
-		if volumeQty.Cmp(requestedQty) >= 0 {
-			if smallestVolume == nil || smallestVolumeQty.Cmp(volumeQty) > 0 {
-				smallestVolume = volume
-				smallestVolumeQty = volumeQty
-			}
+		if smallestVolume == nil || smallestVolumeQty.Cmp(volumeQty) > 0 {
+			smallestVolume = volume
+			smallestVolumeQty = volumeQty
 		}
 	}
 

test/utils/runners.go

@@ -1385,12 +1385,11 @@ func CreatePodWithPersistentVolume(client clientset.Interface, namespace string,
 			createError = fmt.Errorf("error creating PV: %s", err)
 			return
 		}
-		// We need to update status separately, as creating persistentvolumes resets status to the default one
-		// (so with Status.Phase will be equal to PersistentVolumePhase).
+		// We need to update statuses separately, as creating pv/pvc resets status to the default one.
 		if _, err := client.CoreV1().PersistentVolumes().UpdateStatus(context.TODO(), pv, metav1.UpdateOptions{}); err != nil {
 			lock.Lock()
 			defer lock.Unlock()
-			createError = fmt.Errorf("error creating PV: %s", err)
+			createError = fmt.Errorf("error updating PV status: %s", err)
 			return
 		}
 
@@ -1400,6 +1399,12 @@ func CreatePodWithPersistentVolume(client clientset.Interface, namespace string,
 			createError = fmt.Errorf("error creating PVC: %s", err)
 			return
 		}
+		if _, err := client.CoreV1().PersistentVolumeClaims(namespace).UpdateStatus(context.TODO(), pvc, metav1.UpdateOptions{}); err != nil {
+			lock.Lock()
+			defer lock.Unlock()
+			createError = fmt.Errorf("error updating PVC status: %s", err)
+			return
+		}
 
 		// pod
 		pod := podTemplate.DeepCopy()