From 13d04f718dce7cfc95c54aa755197e4841f65410 Mon Sep 17 00:00:00 2001
From: Rakshith R <rar@redhat.com>
Date: Thu, 27 Jan 2022 16:34:57 +0530
Subject: [PATCH] controller/reclaimspacejob: skip making node req if nodeID is
 empty

Node reclaimspace request should not be made
when nodeID is empty indicating the pvc is
not mounted to any pod and no volumeattachment
object for that PVC was found.

Signed-off-by: Rakshith R <rar@redhat.com>
---
 controllers/reclaimspacejob_controller.go     | 44 ++++++++++++-------
 .../reclaimspacejob_controller_test.go        | 33 ++++++++++++++
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/controllers/reclaimspacejob_controller.go b/controllers/reclaimspacejob_controller.go
index 86fc23fbc..439dcb687 100644
--- a/controllers/reclaimspacejob_controller.go
+++ b/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.
diff --git a/controllers/reclaimspacejob_controller_test.go b/controllers/reclaimspacejob_controller_test.go
index 2a231a9f6..00e12e954 100644
--- a/controllers/reclaimspacejob_controller_test.go
+++ b/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)
+		})
+	}
+}