volume scheduler: move reason strings into volume code

The scheduler doesn't really need to know in detail which reasons
rendered a node unusable for a node. All it needs from the volume
binder is a list of reasons that it then can present to the user.

This seems a bit cleaner. But the main reason for the change is that
it simplifies the checking of CSI inline volumes and perhaps later
capacity checking. Both will lead to new failure reasons, which then
can be added without changing the interface.

pkg/controller/volume/scheduling/scheduler_binder_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"sort"
 	"testing"
 	"time"
 
@@ -735,6 +736,41 @@ func addProvisionAnn(pvc *v1.PersistentVolumeClaim) *v1.PersistentVolumeClaim {
 	return res
 }
 
+// reasonNames pretty-prints a list of reasons with variable names in
+// case of a test failure because that is easier to read than the full
+// strings.
+func reasonNames(reasons []string) string {
+	var varNames []string
+	for _, reason := range reasons {
+		switch reason {
+		case ErrReasonBindConflict:
+			varNames = append(varNames, "ErrReasonBindConflict")
+		case ErrReasonNodeConflict:
+			varNames = append(varNames, "ErrReasonNodeConflict")
+		default:
+			varNames = append(varNames, reason)
+		}
+	}
+	return fmt.Sprintf("%v", varNames)
+}
+
+func checkReasons(t *testing.T, actual, expected []string) {
+	equal := len(actual) == len(expected)
+	sort.Strings(actual)
+	sort.Strings(expected)
+	if equal {
+		for i, reason := range actual {
+			if reason != expected[i] {
+				equal = false
+				break
+			}
+		}
+	}
+	if !equal {
+		t.Errorf("expected failure reasons %s, got %s", reasonNames(expected), reasonNames(actual))
+	}
+}
+
 func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 	type scenarioType struct {
 		// Inputs
@@ -749,39 +785,28 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 		expectedBindings []*bindingInfo
 
 		// Expected return values
-		expectedUnbound bool
-		expectedBound   bool
-		shouldFail      bool
+		reasons    []string
+		shouldFail bool
 	}
 	scenarios := map[string]scenarioType{
 		"no-volumes": {
-			pod:             makePod(nil),
-			expectedUnbound: true,
-			expectedBound:   true,
+			pod: makePod(nil),
 		},
 		"no-pvcs": {
-			pod:             makePodWithoutPVC(),
-			expectedUnbound: true,
-			expectedBound:   true,
+			pod: makePodWithoutPVC(),
 		},
 		"pvc-not-found": {
-			cachePVCs:       []*v1.PersistentVolumeClaim{},
-			podPVCs:         []*v1.PersistentVolumeClaim{boundPVC},
-			expectedUnbound: false,
-			expectedBound:   false,
-			shouldFail:      true,
+			cachePVCs:  []*v1.PersistentVolumeClaim{},
+			podPVCs:    []*v1.PersistentVolumeClaim{boundPVC},
+			shouldFail: true,
 		},
 		"bound-pvc": {
-			podPVCs:         []*v1.PersistentVolumeClaim{boundPVC},
-			pvs:             []*v1.PersistentVolume{pvBound},
-			expectedUnbound: true,
-			expectedBound:   true,
+			podPVCs: []*v1.PersistentVolumeClaim{boundPVC},
+			pvs:     []*v1.PersistentVolume{pvBound},
 		},
 		"bound-pvc,pv-not-exists": {
-			podPVCs:         []*v1.PersistentVolumeClaim{boundPVC},
-			expectedUnbound: false,
-			expectedBound:   false,
-			shouldFail:      true,
+			podPVCs:    []*v1.PersistentVolumeClaim{boundPVC},
+			shouldFail: true,
 		},
 		"prebound-pvc": {
 			podPVCs:    []*v1.PersistentVolumeClaim{preboundPVC},
@@ -792,48 +817,37 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 			podPVCs:          []*v1.PersistentVolumeClaim{unboundPVC},
 			pvs:              []*v1.PersistentVolume{pvNode2, pvNode1a, pvNode1b},
 			expectedBindings: []*bindingInfo{makeBinding(unboundPVC, pvNode1a)},
-			expectedUnbound:  true,
-			expectedBound:    true,
 		},
 		"unbound-pvc,pv-different-node": {
-			podPVCs:         []*v1.PersistentVolumeClaim{unboundPVC},
-			pvs:             []*v1.PersistentVolume{pvNode2},
-			expectedUnbound: false,
-			expectedBound:   true,
+			podPVCs: []*v1.PersistentVolumeClaim{unboundPVC},
+			pvs:     []*v1.PersistentVolume{pvNode2},
+			reasons: []string{ErrReasonBindConflict},
 		},
 		"two-unbound-pvcs": {
 			podPVCs:          []*v1.PersistentVolumeClaim{unboundPVC, unboundPVC2},
 			pvs:              []*v1.PersistentVolume{pvNode1a, pvNode1b},
 			expectedBindings: []*bindingInfo{makeBinding(unboundPVC, pvNode1a), makeBinding(unboundPVC2, pvNode1b)},
-			expectedUnbound:  true,
-			expectedBound:    true,
 		},
 		"two-unbound-pvcs,order-by-size": {
 			podPVCs:          []*v1.PersistentVolumeClaim{unboundPVC2, unboundPVC},
 			pvs:              []*v1.PersistentVolume{pvNode1a, pvNode1b},
 			expectedBindings: []*bindingInfo{makeBinding(unboundPVC, pvNode1a), makeBinding(unboundPVC2, pvNode1b)},
-			expectedUnbound:  true,
-			expectedBound:    true,
 		},
 		"two-unbound-pvcs,partial-match": {
 			podPVCs:          []*v1.PersistentVolumeClaim{unboundPVC, unboundPVC2},
 			pvs:              []*v1.PersistentVolume{pvNode1a},
 			expectedBindings: []*bindingInfo{makeBinding(unboundPVC, pvNode1a)},
-			expectedUnbound:  false,
-			expectedBound:    true,
+			reasons:          []string{ErrReasonBindConflict},
 		},
 		"one-bound,one-unbound": {
 			podPVCs:          []*v1.PersistentVolumeClaim{unboundPVC, boundPVC},
 			pvs:              []*v1.PersistentVolume{pvBound, pvNode1a},
 			expectedBindings: []*bindingInfo{makeBinding(unboundPVC, pvNode1a)},
-			expectedUnbound:  true,
-			expectedBound:    true,
 		},
 		"one-bound,one-unbound,no-match": {
-			podPVCs:         []*v1.PersistentVolumeClaim{unboundPVC, boundPVC},
-			pvs:             []*v1.PersistentVolume{pvBound, pvNode2},
-			expectedUnbound: false,
-			expectedBound:   true,
+			podPVCs: []*v1.PersistentVolumeClaim{unboundPVC, boundPVC},
+			pvs:     []*v1.PersistentVolume{pvBound, pvNode2},
+			reasons: []string{ErrReasonBindConflict},
 		},
 		"one-prebound,one-unbound": {
 			podPVCs:    []*v1.PersistentVolumeClaim{unboundPVC, preboundPVC},
@@ -841,35 +855,26 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 			shouldFail: true,
 		},
 		"immediate-bound-pvc": {
-			podPVCs:         []*v1.PersistentVolumeClaim{immediateBoundPVC},
-			pvs:             []*v1.PersistentVolume{pvBoundImmediate},
-			expectedUnbound: true,
-			expectedBound:   true,
+			podPVCs: []*v1.PersistentVolumeClaim{immediateBoundPVC},
+			pvs:     []*v1.PersistentVolume{pvBoundImmediate},
 		},
 		"immediate-bound-pvc-wrong-node": {
-			podPVCs:         []*v1.PersistentVolumeClaim{immediateBoundPVC},
-			pvs:             []*v1.PersistentVolume{pvBoundImmediateNode2},
-			expectedUnbound: true,
-			expectedBound:   false,
+			podPVCs: []*v1.PersistentVolumeClaim{immediateBoundPVC},
+			pvs:     []*v1.PersistentVolume{pvBoundImmediateNode2},
+			reasons: []string{ErrReasonNodeConflict},
 		},
 		"immediate-unbound-pvc": {
-			podPVCs:         []*v1.PersistentVolumeClaim{immediateUnboundPVC},
-			expectedUnbound: false,
-			expectedBound:   false,
-			shouldFail:      true,
+			podPVCs:    []*v1.PersistentVolumeClaim{immediateUnboundPVC},
+			shouldFail: true,
 		},
 		"immediate-unbound-pvc,delayed-mode-bound": {
-			podPVCs:         []*v1.PersistentVolumeClaim{immediateUnboundPVC, boundPVC},
-			pvs:             []*v1.PersistentVolume{pvBound},
-			expectedUnbound: false,
-			expectedBound:   false,
-			shouldFail:      true,
+			podPVCs:    []*v1.PersistentVolumeClaim{immediateUnboundPVC, boundPVC},
+			pvs:        []*v1.PersistentVolume{pvBound},
+			shouldFail: true,
 		},
 		"immediate-unbound-pvc,delayed-mode-unbound": {
-			podPVCs:         []*v1.PersistentVolumeClaim{immediateUnboundPVC, unboundPVC},
-			expectedUnbound: false,
-			expectedBound:   false,
-			shouldFail:      true,
+			podPVCs:    []*v1.PersistentVolumeClaim{immediateUnboundPVC, unboundPVC},
+			shouldFail: true,
 		},
 	}
 
@@ -902,7 +907,7 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 		}
 
 		// Execute
-		unboundSatisfied, boundSatisfied, err := testEnv.binder.FindPodVolumes(scenario.pod, testNode)
+		reasons, err := testEnv.binder.FindPodVolumes(scenario.pod, testNode)
 
 		// Validate
 		if !scenario.shouldFail && err != nil {
@@ -911,12 +916,7 @@ func TestFindPodVolumesWithoutProvisioning(t *testing.T) {
 		if scenario.shouldFail && err == nil {
 			t.Error("returned success but expected error")
 		}
-		if boundSatisfied != scenario.expectedBound {
-			t.Errorf("expected boundSatisfied %v, got %v", scenario.expectedBound, boundSatisfied)
-		}
-		if unboundSatisfied != scenario.expectedUnbound {
-			t.Errorf("expected unboundSatisfied %v, got %v", scenario.expectedUnbound, unboundSatisfied)
-		}
+		checkReasons(t, reasons, scenario.reasons)
 		testEnv.validatePodCache(t, testNode.Name, scenario.pod, scenario.expectedBindings, nil)
 	}
 
@@ -940,61 +940,46 @@ func TestFindPodVolumesWithProvisioning(t *testing.T) {
 		expectedProvisions []*v1.PersistentVolumeClaim
 
 		// Expected return values
-		expectedUnbound bool
-		expectedBound   bool
-		shouldFail      bool
+		reasons    []string
+		shouldFail bool
 	}
 	scenarios := map[string]scenarioType{
 		"one-provisioned": {
 			podPVCs:            []*v1.PersistentVolumeClaim{provisionedPVC},
 			expectedProvisions: []*v1.PersistentVolumeClaim{provisionedPVC},
-			expectedUnbound:    true,
-			expectedBound:      true,
 		},
 		"two-unbound-pvcs,one-matched,one-provisioned": {
 			podPVCs:            []*v1.PersistentVolumeClaim{unboundPVC, provisionedPVC},
 			pvs:                []*v1.PersistentVolume{pvNode1a},
 			expectedBindings:   []*bindingInfo{makeBinding(unboundPVC, pvNode1a)},
 			expectedProvisions: []*v1.PersistentVolumeClaim{provisionedPVC},
-			expectedUnbound:    true,
-			expectedBound:      true,
 		},
 		"one-bound,one-provisioned": {
 			podPVCs:            []*v1.PersistentVolumeClaim{boundPVC, provisionedPVC},
 			pvs:                []*v1.PersistentVolume{pvBound},
 			expectedProvisions: []*v1.PersistentVolumeClaim{provisionedPVC},
-			expectedUnbound:    true,
-			expectedBound:      true,
 		},
 		"one-binding,one-selected-node": {
 			podPVCs:            []*v1.PersistentVolumeClaim{boundPVC, selectedNodePVC},
 			pvs:                []*v1.PersistentVolume{pvBound},
 			expectedProvisions: []*v1.PersistentVolumeClaim{selectedNodePVC},
-			expectedUnbound:    true,
-			expectedBound:      true,
 		},
 		"immediate-unbound-pvc": {
-			podPVCs:         []*v1.PersistentVolumeClaim{immediateUnboundPVC},
-			expectedUnbound: false,
-			expectedBound:   false,
-			shouldFail:      true,
+			podPVCs:    []*v1.PersistentVolumeClaim{immediateUnboundPVC},
+			shouldFail: true,
 		},
 		"one-immediate-bound,one-provisioned": {
 			podPVCs:            []*v1.PersistentVolumeClaim{immediateBoundPVC, provisionedPVC},
 			pvs:                []*v1.PersistentVolume{pvBoundImmediate},
 			expectedProvisions: []*v1.PersistentVolumeClaim{provisionedPVC},
-			expectedUnbound:    true,
-			expectedBound:      true,
 		},
 		"invalid-provisioner": {
-			podPVCs:         []*v1.PersistentVolumeClaim{noProvisionerPVC},
-			expectedUnbound: false,
-			expectedBound:   true,
+			podPVCs: []*v1.PersistentVolumeClaim{noProvisionerPVC},
+			reasons: []string{ErrReasonBindConflict},
 		},
 		"volume-topology-unsatisfied": {
-			podPVCs:         []*v1.PersistentVolumeClaim{topoMismatchPVC},
-			expectedUnbound: false,
-			expectedBound:   true,
+			podPVCs: []*v1.PersistentVolumeClaim{topoMismatchPVC},
+			reasons: []string{ErrReasonBindConflict},
 		},
 	}
 
@@ -1027,7 +1012,7 @@ func TestFindPodVolumesWithProvisioning(t *testing.T) {
 		}
 
 		// Execute
-		unboundSatisfied, boundSatisfied, err := testEnv.binder.FindPodVolumes(scenario.pod, testNode)
+		reasons, err := testEnv.binder.FindPodVolumes(scenario.pod, testNode)
 
 		// Validate
 		if !scenario.shouldFail && err != nil {
@@ -1036,12 +1021,7 @@ func TestFindPodVolumesWithProvisioning(t *testing.T) {
 		if scenario.shouldFail && err == nil {
 			t.Error("returned success but expected error")
 		}
-		if boundSatisfied != scenario.expectedBound {
-			t.Errorf("expected boundSatisfied %v, got %v", scenario.expectedBound, boundSatisfied)
-		}
-		if unboundSatisfied != scenario.expectedUnbound {
-			t.Errorf("expected unboundSatisfied %v, got %v", scenario.expectedUnbound, unboundSatisfied)
-		}
+		checkReasons(t, reasons, scenario.reasons)
 		testEnv.validatePodCache(t, testNode.Name, scenario.pod, scenario.expectedBindings, scenario.expectedProvisions)
 	}
 
@@ -1067,41 +1047,33 @@ func TestFindPodVolumesWithCSIMigration(t *testing.T) {
 		initCSINodes []*storagev1.CSINode
 
 		// Expected return values
-		expectedUnbound bool
-		expectedBound   bool
-		shouldFail      bool
+		reasons    []string
+		shouldFail bool
 	}
 	scenarios := map[string]scenarioType{
 		"pvc-bound": {
-			podPVCs:         []*v1.PersistentVolumeClaim{boundMigrationPVC},
-			pvs:             []*v1.PersistentVolume{migrationPVBound},
-			initNodes:       []*v1.Node{node1Zone1},
-			initCSINodes:    []*storagev1.CSINode{csiNode1Migrated},
-			expectedBound:   true,
-			expectedUnbound: true,
+			podPVCs:      []*v1.PersistentVolumeClaim{boundMigrationPVC},
+			pvs:          []*v1.PersistentVolume{migrationPVBound},
+			initNodes:    []*v1.Node{node1Zone1},
+			initCSINodes: []*storagev1.CSINode{csiNode1Migrated},
 		},
 		"pvc-bound,csinode-not-migrated": {
-			podPVCs:         []*v1.PersistentVolumeClaim{boundMigrationPVC},
-			pvs:             []*v1.PersistentVolume{migrationPVBound},
-			initNodes:       []*v1.Node{node1Zone1},
-			initCSINodes:    []*storagev1.CSINode{csiNode1NotMigrated},
-			expectedBound:   true,
-			expectedUnbound: true,
+			podPVCs:      []*v1.PersistentVolumeClaim{boundMigrationPVC},
+			pvs:          []*v1.PersistentVolume{migrationPVBound},
+			initNodes:    []*v1.Node{node1Zone1},
+			initCSINodes: []*storagev1.CSINode{csiNode1NotMigrated},
 		},
 		"pvc-bound,missing-csinode": {
-			podPVCs:         []*v1.PersistentVolumeClaim{boundMigrationPVC},
-			pvs:             []*v1.PersistentVolume{migrationPVBound},
-			initNodes:       []*v1.Node{node1Zone1},
-			expectedBound:   true,
-			expectedUnbound: true,
+			podPVCs:   []*v1.PersistentVolumeClaim{boundMigrationPVC},
+			pvs:       []*v1.PersistentVolume{migrationPVBound},
+			initNodes: []*v1.Node{node1Zone1},
 		},
 		"pvc-bound,node-different-zone": {
-			podPVCs:         []*v1.PersistentVolumeClaim{boundMigrationPVC},
-			pvs:             []*v1.PersistentVolume{migrationPVBound},
-			initNodes:       []*v1.Node{node1Zone2},
-			initCSINodes:    []*storagev1.CSINode{csiNode1Migrated},
-			expectedBound:   false,
-			expectedUnbound: true,
+			podPVCs:      []*v1.PersistentVolumeClaim{boundMigrationPVC},
+			pvs:          []*v1.PersistentVolume{migrationPVBound},
+			initNodes:    []*v1.Node{node1Zone2},
+			initCSINodes: []*storagev1.CSINode{csiNode1Migrated},
+			reasons:      []string{ErrReasonNodeConflict},
 		},
 	}
 
@@ -1140,7 +1112,7 @@ func TestFindPodVolumesWithCSIMigration(t *testing.T) {
 		}
 
 		// Execute
-		unboundSatisfied, boundSatisfied, err := testEnv.binder.FindPodVolumes(scenario.pod, node)
+		reasons, err := testEnv.binder.FindPodVolumes(scenario.pod, node)
 
 		// Validate
 		if !scenario.shouldFail && err != nil {
@@ -1149,12 +1121,7 @@ func TestFindPodVolumesWithCSIMigration(t *testing.T) {
 		if scenario.shouldFail && err == nil {
 			t.Error("returned success but expected error")
 		}
-		if boundSatisfied != scenario.expectedBound {
-			t.Errorf("expected boundSatisfied %v, got %v", scenario.expectedBound, boundSatisfied)
-		}
-		if unboundSatisfied != scenario.expectedUnbound {
-			t.Errorf("expected unboundSatisfied %v, got %v", scenario.expectedUnbound, unboundSatisfied)
-		}
+		checkReasons(t, reasons, scenario.reasons)
 	}
 
 	for name, scenario := range scenarios {
@@ -1966,12 +1933,12 @@ func TestFindAssumeVolumes(t *testing.T) {
 
 	// Execute
 	// 1. Find matching PVs
-	unboundSatisfied, _, err := testEnv.binder.FindPodVolumes(pod, testNode)
+	reasons, err := testEnv.binder.FindPodVolumes(pod, testNode)
 	if err != nil {
 		t.Errorf("Test failed: FindPodVolumes returned error: %v", err)
 	}
-	if !unboundSatisfied {
-		t.Errorf("Test failed: couldn't find PVs for all PVCs")
+	if len(reasons) > 0 {
+		t.Errorf("Test failed: couldn't find PVs for all PVCs: %v", reasons)
 	}
 	expectedBindings := testEnv.getPodBindings(t, testNode.Name, pod)
 
@@ -1992,12 +1959,12 @@ func TestFindAssumeVolumes(t *testing.T) {
 	// This should always return the original chosen pv
 	// Run this many times in case sorting returns different orders for the two PVs.
 	for i := 0; i < 50; i++ {
-		unboundSatisfied, _, err := testEnv.binder.FindPodVolumes(pod, testNode)
+		reasons, err := testEnv.binder.FindPodVolumes(pod, testNode)
 		if err != nil {
 			t.Errorf("Test failed: FindPodVolumes returned error: %v", err)
 		}
-		if !unboundSatisfied {
-			t.Errorf("Test failed: couldn't find PVs for all PVCs")
+		if len(reasons) > 0 {
+			t.Errorf("Test failed: couldn't find PVs for all PVCs: %v", reasons)
 		}
 		testEnv.validatePodCache(t, testNode.Name, pod, expectedBindings, nil)
 	}