Skip to content

Commit

Permalink
Fix bug which should delete node from store when there is node not fo…
Browse files Browse the repository at this point in the history
…und 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.
  • Loading branch information
zhengxiexie committed Jan 2, 2025
1 parent c729a64 commit 5a770d7
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 7 deletions.
10 changes: 7 additions & 3 deletions pkg/controllers/node/node_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
Expand All @@ -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.
Expand Down
34 changes: 30 additions & 4 deletions pkg/controllers/node/node_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package node

import (
"context"
"errors"
"testing"

"github.com/agiledragon/gomonkey/v2"
Expand All @@ -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"
Expand Down Expand Up @@ -59,7 +61,7 @@ func TestNodeReconciler_Reconcile(t *testing.T) {
{
name: "Node not found",
existingNode: nil,
expectedResult: ctrl.Result{},
expectedResult: common.ResultNormal,
expectedErr: false,
},
{
Expand All @@ -72,7 +74,7 @@ func TestNodeReconciler_Reconcile(t *testing.T) {
},
},
},
expectedResult: ctrl.Result{},
expectedResult: common.ResultNormal,
expectedErr: false,
},
{
Expand All @@ -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 {
Expand Down

0 comments on commit 5a770d7

Please sign in to comment.