Merge pull request #79696 from yittg/fix-pv-controller-affinity

fix pv-controller sync check node affinity for scheduled claim

pkg/controller/volume/persistentvolume/binder_test.go

@@ -248,6 +248,17 @@ func TestSync(t *testing.T) {
 			},
 			noevents, noerrors, testSyncClaim,
 		},
+		{
+			// syncClaim that scheduled to a selected node
+			"1-18 - successful pre-bound PV to PVC provisioning",
+			newVolumeArray("volume1-18", "1Gi", "uid1-18", "claim1-18", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classWait),
+			newVolumeArray("volume1-18", "1Gi", "uid1-18", "claim1-18", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classWait),
+			claimWithAnnotation(pvutil.AnnSelectedNode, "node1",
+				newClaimArray("claim1-18", "uid1-18", "1Gi", "", v1.ClaimPending, &classWait)),
+			claimWithAnnotation(pvutil.AnnSelectedNode, "node1",
+				newClaimArray("claim1-18", "uid1-18", "1Gi", "volume1-18", v1.ClaimBound, &classWait, pvutil.AnnBoundByController, pvutil.AnnBindCompleted)),
+			noevents, noerrors, testSyncClaim,
+		},
 
 		// [Unit test set 2] User asked for a specific PV.
 		// Test the binding when pv.ClaimRef is already set by controller or

pkg/controller/volume/persistentvolume/framework_test.go

@@ -471,9 +471,11 @@ const operationRecycle = "Recycle"
 var (
 	classGold                    string = "gold"
 	classSilver                  string = "silver"
+	classCopper                  string = "copper"
 	classEmpty                   string = ""
 	classNonExisting             string = "non-existing"
 	classExternal                string = "external"
+	classExternalWait            string = "external-wait"
 	classUnknownInternal         string = "unknown-internal"
 	classUnsupportedMountOptions string = "unsupported-mountoptions"
 	classLarge                   string = "large"

pkg/controller/volume/persistentvolume/provision_test.go

@@ -24,6 +24,8 @@ import (
 	storage "k8s.io/api/storage/v1"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	corelisters "k8s.io/client-go/listers/core/v1"
+	"k8s.io/client-go/tools/cache"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	pvtesting "k8s.io/kubernetes/pkg/controller/volume/persistentvolume/testing"
 	pvutil "k8s.io/kubernetes/pkg/controller/volume/persistentvolume/util"
@@ -64,6 +66,18 @@ var storageClasses = []*storage.StorageClass{
 		ReclaimPolicy:     &deleteReclaimPolicy,
 		VolumeBindingMode: &modeImmediate,
 	},
+	{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "StorageClass",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "copper",
+		},
+		Provisioner:       mockPluginName,
+		Parameters:        class1Parameters,
+		ReclaimPolicy:     &deleteReclaimPolicy,
+		VolumeBindingMode: &modeWait,
+	},
 	{
 		TypeMeta: metav1.TypeMeta{
 			Kind: "StorageClass",
@@ -76,6 +90,18 @@ var storageClasses = []*storage.StorageClass{
 		ReclaimPolicy:     &deleteReclaimPolicy,
 		VolumeBindingMode: &modeImmediate,
 	},
+	{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "StorageClass",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "external-wait",
+		},
+		Provisioner:       "vendor.com/my-volume-wait",
+		Parameters:        class1Parameters,
+		ReclaimPolicy:     &deleteReclaimPolicy,
+		VolumeBindingMode: &modeWait,
+	},
 	{
 		TypeMeta: metav1.TypeMeta{
 			Kind: "StorageClass",
@@ -443,6 +469,41 @@ func TestProvisionSync(t *testing.T) {
 			noevents,
 			noerrors, wrapTestWithProvisionCalls([]provisionCall{}, testSyncClaim),
 		},
+		{
+			// volume provision for PVC scheduled
+			"11-23 - skip finding PV and provision for PVC annotated with AnnSelectedNode",
+			newVolumeArray("volume11-23", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classCopper),
+			[]*v1.PersistentVolume{
+				newVolume("volume11-23", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classCopper),
+				newVolume("pvc-uid11-23", "1Gi", "uid11-23", "claim11-23", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classCopper, pvutil.AnnDynamicallyProvisioned, pvutil.AnnBoundByController),
+			},
+			claimWithAnnotation(pvutil.AnnSelectedNode, "node1",
+				newClaimArray("claim11-23", "uid11-23", "1Gi", "", v1.ClaimPending, &classCopper)),
+			claimWithAnnotation(pvutil.AnnSelectedNode, "node1",
+				newClaimArray("claim11-23", "uid11-23", "1Gi", "", v1.ClaimPending, &classCopper, pvutil.AnnStorageProvisioner)),
+			[]string{"Normal ProvisioningSucceeded"},
+			noerrors,
+			wrapTestWithInjectedOperation(wrapTestWithProvisionCalls([]provisionCall{provision1Success}, testSyncClaim),
+				func(ctrl *PersistentVolumeController, reactor *pvtesting.VolumeReactor) {
+					nodesIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+					node := &v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}}
+					nodesIndexer.Add(node)
+					ctrl.NodeLister = corelisters.NewNodeLister(nodesIndexer)
+				}),
+		},
+		{
+			// volume provision for PVC that scheduled
+			"11-24 - skip finding PV and wait external provisioner for PVC annotated with AnnSelectedNode",
+			newVolumeArray("volume11-24", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classExternalWait),
+			newVolumeArray("volume11-24", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classExternalWait),
+			claimWithAnnotation(pvutil.AnnSelectedNode, "node1",
+				newClaimArray("claim11-24", "uid11-24", "1Gi", "", v1.ClaimPending, &classExternalWait)),
+			claimWithAnnotation(pvutil.AnnStorageProvisioner, "vendor.com/my-volume-wait",
+				claimWithAnnotation(pvutil.AnnSelectedNode, "node1",
+					newClaimArray("claim11-24", "uid11-24", "1Gi", "", v1.ClaimPending, &classExternalWait))),
+			[]string{"Normal ExternalProvisioning"},
+			noerrors, testSyncClaim,
+		},
 	}
 	runSyncTests(t, tests, storageClasses, []*v1.Pod{})
 }

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -281,27 +281,6 @@ func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVo
 	return nil
 }
 
-func (ctrl *PersistentVolumeController) isDelayBindingProvisioning(claim *v1.PersistentVolumeClaim) bool {
-	// When feature VolumeScheduling enabled,
-	// Scheduler signal to the PV controller to start dynamic
-	// provisioning by setting the "AnnSelectedNode" annotation
-	// in the PVC
-	_, ok := claim.Annotations[pvutil.AnnSelectedNode]
-	return ok
-}
-
-// shouldDelayBinding returns true if binding of claim should be delayed, false otherwise.
-// If binding of claim should be delayed, only claims pbound by scheduler
-func (ctrl *PersistentVolumeController) shouldDelayBinding(claim *v1.PersistentVolumeClaim) (bool, error) {
-	// If claim has already been assigned a node by scheduler for dynamic provisioning.
-	if ctrl.isDelayBindingProvisioning(claim) {
-		return false, nil
-	}
-
-	// If claim is in delay binding mode.
-	return pvutil.IsDelayBindingMode(claim, ctrl.classLister)
-}
-
 // syncUnboundClaim is the main controller method to decide what to do with an
 // unbound claim.
 func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *v1.PersistentVolumeClaim) error {
@@ -309,7 +288,7 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *v1.PersistentVol
 	// OBSERVATION: pvc is "Pending"
 	if claim.Spec.VolumeName == "" {
 		// User did not care which PV they get.
-		delayBinding, err := ctrl.shouldDelayBinding(claim)
+		delayBinding, err := pvutil.IsDelayBindingMode(claim, ctrl.classLister)
 		if err != nil {
 			return err
 		}
@@ -325,7 +304,7 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *v1.PersistentVol
 			// No PV could be found
 			// OBSERVATION: pvc is "Pending", will retry
 			switch {
-			case delayBinding:
+			case delayBinding && !pvutil.IsDelayBindingProvisioning(claim):
 				ctrl.eventRecorder.Event(claim, v1.EventTypeNormal, events.WaitForFirstConsumer, "waiting for first consumer to be created before binding")
 			case v1helper.GetPersistentVolumeClaimClass(claim) != "":
 				if err = ctrl.provisionClaim(claim); err != nil {

pkg/controller/volume/persistentvolume/pv_controller_test.go

@@ -374,7 +374,7 @@ func TestControllerCacheParsingError(t *testing.T) {
 	}
 }
 
-func makePVCClass(scName *string, hasSelectNodeAnno bool) *v1.PersistentVolumeClaim {
+func makePVCClass(scName *string) *v1.PersistentVolumeClaim {
 	claim := &v1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: map[string]string{},
@@ -384,10 +384,6 @@ func makePVCClass(scName *string, hasSelectNodeAnno bool) *v1.PersistentVolumeCl
 		},
 	}
 
-	if hasSelectNodeAnno {
-		claim.Annotations[pvutil.AnnSelectedNode] = "node-name"
-	}
-
 	return claim
 }
 
@@ -400,37 +396,33 @@ func makeStorageClass(scName string, mode *storagev1.VolumeBindingMode) *storage
 	}
 }
 
-func TestDelayBinding(t *testing.T) {
+func TestDelayBindingMode(t *testing.T) {
 	tests := map[string]struct {
 		pvc         *v1.PersistentVolumeClaim
 		shouldDelay bool
 		shouldFail  bool
 	}{
 		"nil-class": {
-			pvc:         makePVCClass(nil, false),
+			pvc:         makePVCClass(nil),
 			shouldDelay: false,
 		},
 		"class-not-found": {
-			pvc:         makePVCClass(&classNotHere, false),
+			pvc:         makePVCClass(&classNotHere),
 			shouldDelay: false,
 		},
 		"no-mode-class": {
-			pvc:         makePVCClass(&classNoMode, false),
+			pvc:         makePVCClass(&classNoMode),
 			shouldDelay: false,
 			shouldFail:  true,
 		},
 		"immediate-mode-class": {
-			pvc:         makePVCClass(&classImmediateMode, false),
+			pvc:         makePVCClass(&classImmediateMode),
 			shouldDelay: false,
 		},
 		"wait-mode-class": {
-			pvc:         makePVCClass(&classWaitMode, false),
+			pvc:         makePVCClass(&classWaitMode),
 			shouldDelay: true,
 		},
-		"wait-mode-class-with-selectedNode": {
-			pvc:         makePVCClass(&classWaitMode, true),
-			shouldDelay: false,
-		},
 	}
 
 	classes := []*storagev1.StorageClass{
@@ -453,7 +445,7 @@ func TestDelayBinding(t *testing.T) {
 	}
 
 	for name, test := range tests {
-		shouldDelay, err := ctrl.shouldDelayBinding(test.pvc)
+		shouldDelay, err := pvutil.IsDelayBindingMode(test.pvc, ctrl.classLister)
 		if err != nil && !test.shouldFail {
 			t.Errorf("Test %q returned error: %v", name, err)
 		}

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

@@ -69,6 +69,16 @@ const (
 	AnnStorageProvisioner = "volume.beta.kubernetes.io/storage-provisioner"
 )
 
+// IsDelayBindingProvisioning checks if claim provisioning with selected-node annotation
+func IsDelayBindingProvisioning(claim *v1.PersistentVolumeClaim) bool {
+	// When feature VolumeScheduling enabled,
+	// Scheduler signal to the PV controller to start dynamic
+	// provisioning by setting the "AnnSelectedNode" annotation
+	// in the PVC
+	_, ok := claim.Annotations[AnnSelectedNode]
+	return ok
+}
+
 // IsDelayBindingMode checks if claim is in delay binding mode.
 func IsDelayBindingMode(claim *v1.PersistentVolumeClaim, classLister storagelisters.StorageClassLister) (bool, error) {
 	className := v1helper.GetPersistentVolumeClaimClass(claim)