Merge pull request #24 from Rakshith-R/BZ-2048370

Bug 2048370: CSI-Addons controller makes node reclaimspace request even when the PVC is not mounted to any pod

Author: openshift-merge-robot

controllers/reclaimspacejob_controller.go

@@ -173,6 +173,13 @@ type targetDetails struct {
 	nodeID     string
 }
 
+// canNodeReclaimSpace returns true if nodeID is not empty,
+// indicating volume is mounted to a pod and node reclaimspace
+// request can be sent.
+func (td *targetDetails) canNodeReclaimSpace() bool {
+	return td.nodeID != ""
+}
+
 // reconcile performs time based validation, fetches required details and makes
 // grpc request for controller and node reclaim space operation.
 func (r *ReclaimSpaceJobReconciler) reconcile(
@@ -211,15 +218,22 @@ func (r *ReclaimSpaceJobReconciler) reconcile(
 		return err
 	}
 
-	nodeFound, nodeReclaimedSpace, err := r.nodeReclaimSpace(ctx, logger, target)
-	if err != nil {
-		logger.Error(err, "Failed to make node request")
-		setFailedCondition(
-			&rsJob.Status.Conditions,
-			fmt.Sprintf("Failed to make node request: %v", util.GetErrorMessage(err)),
-			rsJob.Generation)
-
-		return err
+	var (
+		nodeFound          = false
+		nodeReclaimedSpace *int64
+	)
+	if target.canNodeReclaimSpace() {
+		nodeFound = true
+		nodeReclaimedSpace, err = r.nodeReclaimSpace(ctx, logger, target)
+		if err != nil {
+			logger.Error(err, "Failed to make node request")
+			setFailedCondition(
+				&rsJob.Status.Conditions,
+				fmt.Sprintf("Failed to make node request: %v", util.GetErrorMessage(err)),
+				rsJob.Generation)
+
+			return err
+		}
 	}
 
 	controllerFound, controllerReclaimedSpace, err := r.controllerReclaimSpace(ctx, logger, target)
@@ -366,23 +380,21 @@ func (r *ReclaimSpaceJobReconciler) controllerReclaimSpace(
 	return true, calculateReclaimedSpace(resp.PreUsage, resp.PostUsage), nil
 }
 
-// controllerReclaimSpace makes node reclaim space request if node client is found
+// nodeReclaimSpace makes node reclaim space request if node client is found
 // and returns amount of reclaimed space.
 // This function returns
-// - boolean to indicate client was found or not
 // - pointer to int64 indicating amount of reclaimed space, it is nil if not available
 // - error
 func (r *ReclaimSpaceJobReconciler) nodeReclaimSpace(
 	ctx context.Context,
 	logger *logr.Logger,
-	target *targetDetails) (bool, *int64, error) {
+	target *targetDetails) (*int64, error) {
 	clientName, nodeClient := r.getRSClientWithCap(
 		target.driverName,
 		target.nodeID,
 		identity.Capability_ReclaimSpace_ONLINE)
 	if nodeClient == nil {
-		logger.Info("Node Client not found")
-		return false, nil, nil
+		return nil, errors.New("node Client not found")
 	}
 	*logger = logger.WithValues("nodeClient", clientName)
 
@@ -394,10 +406,10 @@ func (r *ReclaimSpaceJobReconciler) nodeReclaimSpace(
 	defer cancel()
 	resp, err := nodeClient.NodeReclaimSpace(newCtx, req)
 	if err != nil {
-		return true, nil, err
+		return nil, err
 	}
 
-	return true, calculateReclaimedSpace(resp.PreUsage, resp.PostUsage), nil
+	return calculateReclaimedSpace(resp.PreUsage, resp.PostUsage), nil
 }
 
 // calculateReclaimedSpace returns amount of reclaimed space.

controllers/reclaimspacejob_controller_test.go

@@ -176,3 +176,36 @@ func TestCalculateReclaimedSpace(t *testing.T) {
 		})
 	}
 }
+
+func TestCanNodeReclaimSpace(t *testing.T) {
+	tests := []struct {
+		name string
+		td   targetDetails
+		want bool
+	}{
+		{
+			name: "empty nodeID",
+			td: targetDetails{
+				driverName: "csi.example.com",
+				pvName:     "pvc-a8a5c531-9f88-4fc8-b35d-564585fb42a8",
+				nodeID:     "",
+			},
+			want: false,
+		},
+		{
+			name: "non-empty nodeID",
+			td: targetDetails{
+				driverName: "csi.example.com",
+				pvName:     "pvc-a8a5c531-9f88-4fc8-b35d-564585fb42a8",
+				nodeID:     "worker-1",
+			},
+			want: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.td.canNodeReclaimSpace()
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}