controller/reclaimspacejob: skip making node req if nodeID is empty #104

Rakshith-R · 2022-01-27T11:07:58Z

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 [email protected]

Madhu-1 · 2022-01-27T11:25:20Z

@Rakshith-R can you please provide more tails when the node ID will be empty?

nixpanic · 2022-01-27T11:25:50Z

controllers/reclaimspacejob_controller.go

@@ -376,6 +376,11 @@ func (r *ReclaimSpaceJobReconciler) nodeReclaimSpace(
 	ctx context.Context,
 	logger *logr.Logger,
 	target *targetDetails) (bool, *int64, error) {
+	if target.nodeID == "" {


it is cleaner to skip calling this function altogether. do the check where nodeReclaimSpace() is called?

is this means VA object is created but no NodeID is added to it? explaining in what case it can happen will be helpful.

maybe add a helper func (td *targetDetails) canNodeReclaimSpace() bool {} so that the check for target.nodeID does not need to be placed in the execution flow of reconcile(). You might be able to drop the 1st return value from nodeReclaimSpace() that way too?

is this means VA object is created but no NodeID is added to it? explaining in what case it can happen will be helpful.

It means no VA object exists for that pvc, I'll add comments

maybe add a helper func (td *targetDetails) canNodeReclaimSpace() bool {} so that the check for target.nodeID does not need to be placed in the execution flow of reconcile(). You might be able to drop the 1st return value from nodeReclaimSpace() that way too?

okay 👍

Rakshith-R · 2022-01-27T11:28:39Z

@Rakshith-R can you please provide more tails when the node ID will be empty?

When pvc is not mounted to any pod, no volumeattachments will be found, hence nodeID will be empty

Madhu-1 · 2022-01-27T12:42:55Z

controllers/reclaimspacejob_controller.go

+		err := fmt.Errorf("Node Client not found")
+		return nil, err


Suggested change

err := fmt.Errorf("Node Client not found")

return nil, err

return nil, errors.New("Node Client not found")

Madhu-1 · 2022-01-27T12:43:17Z

controllers/reclaimspacejob_controller_test.go

@@ -176,3 +176,42 @@ func TestCalculateReclaimedSpace(t *testing.T) {
 		})
 	}
 }
+
+func Test_targetDetails_canNodeReclaimSpace(t *testing.T) {


Suggested change

func Test_targetDetails_canNodeReclaimSpace(t *testing.T) {

TestcanNodeReclaimSpace

Madhu-1 · 2022-01-27T12:43:54Z

controllers/reclaimspacejob_controller_test.go

+func Test_targetDetails_canNodeReclaimSpace(t *testing.T) {
+	type fields struct {
+		driverName string
+		pvName     string


pv name and drivername are not required?

pv name and drivername are not required?

optional, added them now to keep it consistent in the tests.

Madhu-1 · 2022-01-27T13:09:45Z

controllers/reclaimspacejob_controller.go

 	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")


Error should not start with capital letters

Fixed, please take a look

Thanks

Pull request has been modified.

nixpanic

just a few nits for your consideration

nixpanic · 2022-01-27T14:27:36Z

controllers/reclaimspacejob_controller_test.go

+		{
+			name: "empty nodeID",
+			fields: fields{
+				driverName: "example.csi.com",


csi.com exists, use csi.example.com instead

csi.com exists, use csi.example.com instead

done

nixpanic · 2022-01-27T14:29:24Z

controllers/reclaimspacejob_controller.go

+	// PVC may not be mounted to a pod and volumeattachment for PVC
+	// may not be found due to which nodeID will be empty.
+	// Make node reclaimspace request only if
+	// nodeID is not empty.


the comment is not really needed to be this verbose. Only the comment for the function should explain about nodeID, the check may change in the future.

the comment is not really needed to be this verbose. Only the comment for the function should explain about nodeID, the check may change in the future.

I agree. The function comment should be sufficient.

nixpanic · 2022-01-27T14:31:09Z

controllers/reclaimspacejob_controller_test.go

@@ -176,3 +176,46 @@ func TestCalculateReclaimedSpace(t *testing.T) {
 		})
 	}
 }
+
+func TestCanNodeReclaimSpace(t *testing.T) {
+	type fields struct {


why introduce fields? just use targetDetails in the tests struct

why introduce fields? just use targetDetails in the tests struct

auto generated test, It uses targetDetails directly now.

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 <[email protected]>

Pull request has been modified.

yati1998

LGTM now.

Syncing latest changes from upstream main for kubernetes-csi-addons

mergify bot requested review from nixpanic, yati1998 and Yuggupta27 January 27, 2022 11:08

Rakshith-R requested a review from Madhu-1 January 27, 2022 11:12

nixpanic reviewed Jan 27, 2022

View reviewed changes

Rakshith-R force-pushed the fix-node-rs branch from c6f1071 to e2fd460 Compare January 27, 2022 12:38

Rakshith-R requested review from nixpanic and a team January 27, 2022 12:39

Madhu-1 reviewed Jan 27, 2022

View reviewed changes

Rakshith-R force-pushed the fix-node-rs branch from e2fd460 to 43f0056 Compare January 27, 2022 13:07

Rakshith-R requested a review from Madhu-1 January 27, 2022 13:08

Madhu-1 previously approved these changes Jan 27, 2022

View reviewed changes

Rakshith-R force-pushed the fix-node-rs branch from 43f0056 to 8818085 Compare January 27, 2022 13:13

Rakshith-R requested a review from Madhu-1 January 27, 2022 13:13

Madhu-1 previously approved these changes Jan 27, 2022

View reviewed changes

nixpanic reviewed Jan 27, 2022

View reviewed changes

controller/reclaimspacejob: skip making node req if nodeID is empty

13d04f7

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 <[email protected]>

Rakshith-R force-pushed the fix-node-rs branch from 8818085 to 13d04f7 Compare January 28, 2022 05:13

Rakshith-R requested review from a team, nixpanic and Madhu-1 January 28, 2022 05:14

Madhu-1 approved these changes Jan 28, 2022

View reviewed changes

yati1998 reviewed Jan 28, 2022

View reviewed changes

nixpanic approved these changes Jan 28, 2022

View reviewed changes

mergify bot merged commit e033960 into csi-addons:main Jan 28, 2022

Rakshith-R mentioned this pull request Jun 20, 2022

"error": "node Client not found" #166

Closed

nixpanic pushed a commit to nixpanic/kubernetes-csi-addons that referenced this pull request Jan 12, 2024

Merge pull request csi-addons#104 from red-hat-storage/sync_us--main

811b352

Syncing latest changes from upstream main for kubernetes-csi-addons

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller/reclaimspacejob: skip making node req if nodeID is empty #104

controller/reclaimspacejob: skip making node req if nodeID is empty #104

Rakshith-R commented Jan 27, 2022 •

edited

Loading

Madhu-1 commented Jan 27, 2022

nixpanic Jan 27, 2022

Madhu-1 Jan 27, 2022

nixpanic Jan 27, 2022

Rakshith-R Jan 27, 2022

Rakshith-R commented Jan 27, 2022

Madhu-1 Jan 27, 2022

Madhu-1 Jan 27, 2022

Madhu-1 Jan 27, 2022

Rakshith-R Jan 27, 2022

Madhu-1 Jan 27, 2022 •

edited by Rakshith-R

Loading

nixpanic left a comment

nixpanic Jan 27, 2022

Rakshith-R Jan 28, 2022

nixpanic Jan 27, 2022

Rakshith-R Jan 28, 2022

nixpanic Jan 27, 2022

Rakshith-R Jan 28, 2022

yati1998 left a comment

	err := fmt.Errorf("Node Client not found")
	return nil, err
	return nil, errors.New("Node Client not found")

	func Test_targetDetails_canNodeReclaimSpace(t *testing.T) {
	TestcanNodeReclaimSpace

controller/reclaimspacejob: skip making node req if nodeID is empty #104

controller/reclaimspacejob: skip making node req if nodeID is empty #104

Conversation

Rakshith-R commented Jan 27, 2022 • edited Loading

Madhu-1 commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 Jan 27, 2022 • edited by Rakshith-R Loading

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yati1998 left a comment

Choose a reason for hiding this comment

Rakshith-R commented Jan 27, 2022 •

edited

Loading

Madhu-1 Jan 27, 2022 •

edited by Rakshith-R

Loading