Merge pull request #141 from sunnylovestiramisu/v8.0.1

Cherry-pick fix indefinite stuck Pending pod on a deleted node

allocator/minmax.go

@@ -44,7 +44,7 @@ var (
 	ErrInternal = errors.New("internal error")
 )
 
-//MinMaxAllocator defines allocator struct.
+// MinMaxAllocator defines allocator struct.
 type MinMaxAllocator struct {
 	lock sync.Mutex
 	min  int
@@ -79,7 +79,7 @@ func NewMinMaxAllocator(min, max int) (*MinMaxAllocator, error) {
 	}, nil
 }
 
-//SetRange defines the range/pool with provided min and max values.
+// SetRange defines the range/pool with provided min and max values.
 func (a *MinMaxAllocator) SetRange(min, max int) error {
 	if min > max {
 		return ErrInvalidRange
@@ -108,7 +108,7 @@ func (a *MinMaxAllocator) SetRange(min, max int) error {
 	return nil
 }
 
-//Allocate allocates provided value in the allocator and mark it as used.
+// Allocate allocates provided value in the allocator and mark it as used.
 func (a *MinMaxAllocator) Allocate(i int) (bool, error) {
 	a.lock.Lock()
 	defer a.lock.Unlock()
@@ -127,7 +127,7 @@ func (a *MinMaxAllocator) Allocate(i int) (bool, error) {
 	return true, nil
 }
 
-//AllocateNext allocates next value from the allocator.
+// AllocateNext allocates next value from the allocator.
 func (a *MinMaxAllocator) AllocateNext() (int, bool, error) {
 	a.lock.Lock()
 	defer a.lock.Unlock()
@@ -150,7 +150,7 @@ func (a *MinMaxAllocator) AllocateNext() (int, bool, error) {
 	return 0, false, ErrInternal
 }
 
-//Release free/delete provided value from the allocator.
+// Release free/delete provided value from the allocator.
 func (a *MinMaxAllocator) Release(i int) error {
 	a.lock.Lock()
 	defer a.lock.Unlock()
@@ -173,15 +173,15 @@ func (a *MinMaxAllocator) has(i int) bool {
 	return ok
 }
 
-//Has check whether the provided value is used in the allocator
+// Has check whether the provided value is used in the allocator
 func (a *MinMaxAllocator) Has(i int) bool {
 	a.lock.Lock()
 	defer a.lock.Unlock()
 
 	return a.has(i)
 }
 
-//Free returns the number of free values in the allocator.
+// Free returns the number of free values in the allocator.
 func (a *MinMaxAllocator) Free() int {
 	a.lock.Lock()
 	defer a.lock.Unlock()

controller/controller.go

@@ -1386,6 +1386,10 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
 			selectedNode, err = ctrl.client.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) // TODO (verult) cache Nodes
 		}
 		if err != nil {
+			// if node does not exist, reschedule and remove volume.kubernetes.io/selected-node annotation
+			if apierrs.IsNotFound(err) {
+				return ctrl.provisionVolumeErrorHandling(ctx, ProvisioningReschedule, err, claim, operation)
+			}
 			err = fmt.Errorf("failed to get target node: %v", err)
 			ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error())
 			return ProvisioningNoChange, err
@@ -1408,35 +1412,9 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
 			klog.Info(logOperation(operation, "volume provision ignored: %v", ierr))
 			return ProvisioningFinished, errStopProvision
 		}
-		err = fmt.Errorf("failed to provision volume with StorageClass %q: %v", claimClass, err)
-		ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error())
-		if _, ok := claim.Annotations[annSelectedNode]; ok && result == ProvisioningReschedule {
-			// For dynamic PV provisioning with delayed binding, the provisioner may fail
-			// because the node is wrong (permanent error) or currently unusable (not enough
-			// capacity). If the provisioner wants to give up scheduling with the currently
-			// selected node, then it can ask for that by returning ProvisioningReschedule
-			// as state.
-			//
-			// `selectedNode` must be removed to notify scheduler to schedule again.
-			if errLabel := ctrl.rescheduleProvisioning(ctx, claim); errLabel != nil {
-				klog.Info(logOperation(operation, "volume rescheduling failed: %v", errLabel))
-				// If unsetting that label fails in ctrl.rescheduleProvisioning, we
-				// keep the volume in the work queue as if the provisioner had
-				// returned ProvisioningFinished and simply try again later.
-				return ProvisioningFinished, err
-			}
-			// Label was removed, stop working on the volume.
-			klog.Info(logOperation(operation, "volume rescheduled because: %v", err))
-			return ProvisioningFinished, errStopProvision
-		}
 
-		// ProvisioningReschedule shouldn't have been returned for volumes without selected node,
-		// but if we get it anyway, then treat it like ProvisioningFinished because we cannot
-		// reschedule.
-		if result == ProvisioningReschedule {
-			result = ProvisioningFinished
-		}
-		return result, err
+		err = fmt.Errorf("failed to provision volume with StorageClass %q: %v", claimClass, err)
+		return ctrl.provisionVolumeErrorHandling(ctx, result, err, claim, operation)
 	}
 
 	klog.Info(logOperation(operation, "volume %q provisioned", volume.Name))
@@ -1463,6 +1441,37 @@ func (ctrl *ProvisionController) provisionClaimOperation(ctx context.Context, cl
 	return ProvisioningFinished, nil
 }
 
+func (ctrl *ProvisionController) provisionVolumeErrorHandling(ctx context.Context, result ProvisioningState, err error, claim *v1.PersistentVolumeClaim, operation string) (ProvisioningState, error) {
+	ctrl.eventRecorder.Event(claim, v1.EventTypeWarning, "ProvisioningFailed", err.Error())
+	if _, ok := claim.Annotations[annSelectedNode]; ok && result == ProvisioningReschedule {
+		// For dynamic PV provisioning with delayed binding, the provisioner may fail
+		// because the node is wrong (permanent error) or currently unusable (not enough
+		// capacity). If the provisioner wants to give up scheduling with the currently
+		// selected node, then it can ask for that by returning ProvisioningReschedule
+		// as state.
+		//
+		// `selectedNode` must be removed to notify scheduler to schedule again.
+		if errLabel := ctrl.rescheduleProvisioning(ctx, claim); errLabel != nil {
+			klog.Info(logOperation(operation, "volume rescheduling failed: %v", errLabel))
+			// If unsetting that label fails in ctrl.rescheduleProvisioning, we
+			// keep the volume in the work queue as if the provisioner had
+			// returned ProvisioningFinished and simply try again later.
+			return ProvisioningFinished, err
+		}
+		// Label was removed, stop working on the volume.
+		klog.Info(logOperation(operation, "volume rescheduled because: %v", err))
+		return ProvisioningFinished, errStopProvision
+	}
+
+	// ProvisioningReschedule shouldn't have been returned for volumes without selected node,
+	// but if we get it anyway, then treat it like ProvisioningFinished because we cannot
+	// reschedule.
+	if result == ProvisioningReschedule {
+		result = ProvisioningFinished
+	}
+	return result, err
+}
+
 // deleteVolumeOperation attempts to delete the volume backing the given
 // volume. Returns error, which indicates whether deletion should be retried
 // (requeue the volume) or not

controller/controller_test.go

@@ -542,14 +542,33 @@ func TestController(t *testing.T) {
 		},
 		{
 			name: "do not remove selectedNode after final error, only the claim",
+			objs: []runtime.Object{
+				newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
+				newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
+				newNode("node-1"),
+			},
+			provisionerName: "foo.bar/baz",
+			provisioner:     newBadTestProvisioner(),
+			expectedClaims: []v1.PersistentVolumeClaim{
+				*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
+			},
+			expectedClaimsInProgress: nil, // not in progress anymore
+			expectedMetrics: testMetrics{
+				provisioned: counts{
+					"class-1": count{failed: 1},
+				},
+			},
+		},
+		{
+			name: "remove selectedNode if no node exists",
 			objs: []runtime.Object{
 				newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
 				newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
 			},
 			provisionerName: "foo.bar/baz",
 			provisioner:     newBadTestProvisioner(),
 			expectedClaims: []v1.PersistentVolumeClaim{
-				*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
+				*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz"}),
 			},
 			expectedClaimsInProgress: nil, // not in progress anymore
 			expectedMetrics: testMetrics{
@@ -560,14 +579,32 @@ func TestController(t *testing.T) {
 		},
 		{
 			name: "do not remove selectedNode if nothing changes",
+			objs: []runtime.Object{
+				newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
+				newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
+				newNode("node-1"),
+			},
+			provisionerName: "foo.bar/baz",
+			provisioner:     newNoChangeTestProvisioner(),
+			expectedClaims: []v1.PersistentVolumeClaim{
+				*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-1"}),
+			},
+			expectedMetrics: testMetrics{
+				provisioned: counts{
+					"class-1": count{failed: 1},
+				},
+			},
+		},
+		{
+			name: "remove selectedNode if nothing changes and no node exists",
 			objs: []runtime.Object{
 				newStorageClassWithVolumeBindingMode("class-1", "foo.bar/baz", &modeWait),
 				newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
 			},
 			provisionerName: "foo.bar/baz",
 			provisioner:     newNoChangeTestProvisioner(),
 			expectedClaims: []v1.PersistentVolumeClaim{
-				*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz", annSelectedNode: "node-wrong"}),
+				*newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annBetaStorageProvisioner: "foo.bar/baz"}),
 			},
 			expectedMetrics: testMetrics{
 				provisioned: counts{
@@ -876,7 +913,7 @@ func TestTopologyParams(t *testing.T) {
 				newStorageClass("class-1", "foo.bar/baz"),
 				newClaim("claim-1", "uid-1-1", "class-1", "foo.bar/baz", "", map[string]string{annSelectedNode: "node-1"}),
 			},
-			expectedParams: nil,
+			expectedParams: &provisionParams{},
 		},
 	}
 	for _, test := range tests {

gidallocator/gidallocator.go

@@ -113,12 +113,10 @@ func (a *Allocator) Release(volume *v1.PersistentVolume) error {
 	return nil
 }
 
-//
 // Return the gid table for a storage class.
-// - If this is the first time, fill it with all the gids
-//   used in PVs of this storage class by traversing the PVs.
-// - Adapt the range of the table to the current range of the SC.
-//
+//   - If this is the first time, fill it with all the gids
+//     used in PVs of this storage class by traversing the PVs.
+//   - Adapt the range of the table to the current range of the SC.
 func (a *Allocator) getGidTable(className string, min int, max int) (*allocator.MinMaxAllocator, error) {
 	var err error
 	a.gidTableLock.Lock()
@@ -174,7 +172,6 @@ func (a *Allocator) getGidTable(className string, min int, max int) (*allocator.
 
 // Traverse the PVs, fetching all the GIDs from those
 // in a given storage class, and mark them in the table.
-//
 func (a *Allocator) collectGids(className string, gidTable *allocator.MinMaxAllocator) error {
 	pvList, err := a.client.CoreV1().PersistentVolumes().List(context.Background(), metav1.ListOptions{})
 	if err != nil {

mount/mountinfo_linux.go

@@ -1,3 +1,4 @@
+//go:build linux
 // +build linux
 
 /*

mount/mountinfo_unsupported.go

@@ -1,3 +1,4 @@
+//go:build (!windows && !linux && !freebsd && !solaris) || (freebsd && !cgo) || (solaris && !cgo)
 // +build !windows,!linux,!freebsd,!solaris freebsd,!cgo solaris,!cgo
 
 /*