From 5a770d723d702c45dbe1da150295db824b0b704b Mon Sep 17 00:00:00 2001
From: Xie Zheng <xie.zheng@broadcom.com>
Date: Mon, 30 Dec 2024 14:24:54 +0800
Subject: [PATCH] Fix bug which should delete node from store when there is
 node not found error

When node is deleted, the phase node.ObjectMeta.DeletionTimestamp being not zero is intermittent,
and it is more possible the node is not found, we should delete the node from store.
---
 pkg/controllers/node/node_controller.go      | 10 ++++--
 pkg/controllers/node/node_controller_test.go | 34 +++++++++++++++++---
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/pkg/controllers/node/node_controller.go b/pkg/controllers/node/node_controller.go
index c0197cb74..220175943 100644
--- a/pkg/controllers/node/node_controller.go
+++ b/pkg/controllers/node/node_controller.go
@@ -45,6 +45,10 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		if errors.IsNotFound(err) {
 			log.Info("node not found", "req", req.NamespacedName)
 			deleted = true
+			if err := r.Service.SyncNodeStore(req.NamespacedName.Name, deleted); err != nil {
+				log.Error(err, "failed to sync node store", "req", req.NamespacedName)
+				return common.ResultNormal, err
+			}
 		} else {
 			log.Error(err, "unable to fetch node", "req", req.NamespacedName)
 		}
@@ -53,7 +57,7 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 	if common.NodeIsMaster(node) {
 		// For WCP supervisor cluster, the master node isn't a transport node.
 		log.Info("skipping handling master node", "node", req.NamespacedName)
-		return ctrl.Result{}, nil
+		return common.ResultNormal, nil
 	}
 	if !node.ObjectMeta.DeletionTimestamp.IsZero() {
 		log.Info("node is being deleted", "node", req.NamespacedName)
@@ -62,9 +66,9 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	if err := r.Service.SyncNodeStore(node.Name, deleted); err != nil {
 		log.Error(err, "failed to sync node store", "req", req.NamespacedName)
-		return ctrl.Result{}, err
+		return common.ResultNormal, err
 	}
-	return ctrl.Result{}, nil
+	return common.ResultNormal, nil
 }
 
 // SetupWithManager sets up the controller with the Manager.
diff --git a/pkg/controllers/node/node_controller_test.go b/pkg/controllers/node/node_controller_test.go
index 49d03dde7..5145d6ce6 100644
--- a/pkg/controllers/node/node_controller_test.go
+++ b/pkg/controllers/node/node_controller_test.go
@@ -2,6 +2,7 @@ package node
 
 import (
 	"context"
+	"errors"
 	"testing"
 
 	"github.com/agiledragon/gomonkey/v2"
@@ -16,6 +17,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/event"
 
 	"github.com/vmware-tanzu/nsx-operator/pkg/config"
+	"github.com/vmware-tanzu/nsx-operator/pkg/controllers/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/metrics"
 	servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/node"
@@ -59,7 +61,7 @@ func TestNodeReconciler_Reconcile(t *testing.T) {
 		{
 			name:           "Node not found",
 			existingNode:   nil,
-			expectedResult: ctrl.Result{},
+			expectedResult: common.ResultNormal,
 			expectedErr:    false,
 		},
 		{
@@ -72,7 +74,7 @@ func TestNodeReconciler_Reconcile(t *testing.T) {
 					},
 				},
 			},
-			expectedResult: ctrl.Result{},
+			expectedResult: common.ResultNormal,
 			expectedErr:    false,
 		},
 		{
@@ -82,26 +84,50 @@ func TestNodeReconciler_Reconcile(t *testing.T) {
 					Name: "worker-node",
 				},
 			},
-			expectedResult: ctrl.Result{},
+			expectedResult: common.ResultNormal,
 			expectedErr:    false,
 		},
+		{
+			name:           "Sync node error",
+			existingNode:   nil,
+			expectedResult: common.ResultNormal,
+			expectedErr:    true,
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ctx := context.Background()
+			nodeName := "test-node"
 
 			if tt.existingNode != nil {
+				nodeName = tt.existingNode.Name
 				err := reconciler.Client.Create(ctx, tt.existingNode)
 				assert.NoError(t, err)
 			}
 
 			req := ctrl.Request{
 				NamespacedName: types.NamespacedName{
-					Name: "test-node",
+					Name: nodeName,
 				},
 			}
 
+			if tt.name != "Master Node" && tt.name != "Sync node error" {
+				patchesSyncNodeStore := gomonkey.ApplyFunc((*node.NodeService).SyncNodeStore,
+					func(n *node.NodeService, nodeName string, deleted bool) error {
+						return nil
+					})
+				defer patchesSyncNodeStore.Reset()
+			}
+
+			if tt.name == "Sync node error" {
+				patchesSyncNodeStore := gomonkey.ApplyFunc((*node.NodeService).SyncNodeStore,
+					func(n *node.NodeService, nodeName string, deleted bool) error {
+						return errors.New("Sync node error")
+					})
+				defer patchesSyncNodeStore.Reset()
+			}
+
 			result, err := reconciler.Reconcile(ctx, req)
 
 			if tt.expectedErr {