PWX-37595: honour force flag over node status (#2479)

* PWX-37959: honour force flag over node status Signed-off-by: sanjain <[email protected]> * minor changes Signed-off-by: sanjain <[email protected]> * minor changes Signed-off-by: sanjain <[email protected]> * added cluster listner mock Signed-off-by: sanjain <[email protected]> --------- Signed-off-by: sanjain <[email protected]>
libopenstorage · Sep 19, 2024 · e441df6 · e441df6
1 parent 464a4a2
commit e441df6
Show file tree

Hide file tree

Showing 5 changed files with 488 additions and 1 deletion.
diff --git a/Makefile b/Makefile
@@ -433,6 +433,7 @@ mockgen:
 	mockgen -destination=api/mock/mock_bucket.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageBucketServer,OpenStorageBucketClient
 	mockgen -destination=api/mock/mock_cloud_backup.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageCloudBackupServer,OpenStorageCloudBackupClient
 	mockgen -destination=cluster/mock/cluster.mock.go -package=mock github.com/libopenstorage/openstorage/cluster Cluster
+	mockgen -destination=cluster/mock/cluster_listener.mock.go -package=mock github.com/libopenstorage/openstorage/cluster ClusterListener
 	mockgen -destination=api/mock/mock_fstrim.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageFilesystemTrimServer,OpenStorageFilesystemTrimClient
 	mockgen -destination=api/mock/mock_fscheck.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageFilesystemCheckServer,OpenStorageFilesystemCheckClient
 	mockgen -destination=api/mock/mock_defrag.go -package=mock github.com/libopenstorage/openstorage/api OpenStorageFilesystemDefragServer,OpenStorageFilesystemDefragClient

diff --git a/cluster/cluster.go b/cluster/cluster.go
@@ -1,4 +1,5 @@
 //go:generate mockgen -package=mock -destination=mock/cluster.mock.go github.com/libopenstorage/openstorage/cluster Cluster
+//go:generate mockgen -destination=cluster/mock/cluster_listener.mock.go -package=mock github.com/libopenstorage/openstorage/cluster ClusterListener
 package cluster
 
 import (

diff --git a/cluster/manager/manager.go b/cluster/manager/manager.go
@@ -1830,7 +1830,8 @@ func (c *ClusterManager) Remove(nodes []api.Node, forceRemove bool) error {
 			// If node is not down, do not remove it
 			if nodeCacheStatus != api.Status_STATUS_OFFLINE &&
 				nodeCacheStatus != api.Status_STATUS_MAINTENANCE &&
-				nodeCacheStatus != api.Status_STATUS_DECOMMISSION {
+				nodeCacheStatus != api.Status_STATUS_DECOMMISSION &&
+				!forceRemove {
 
 				msg := fmt.Sprintf(decommissionErrMsg, nodes[i].Id)
 				logrus.Errorf(msg+", node status: %s", nodeCacheStatus)

diff --git a/cluster/manager/manager_test.go b/cluster/manager/manager_test.go
@@ -16,11 +16,16 @@ limitations under the License.
 package manager
 
 import (
+	"container/list"
+	"errors"
 	"fmt"
 	"testing"
 	"time"
 
+	"github.com/golang/mock/gomock"
+	"github.com/libopenstorage/openstorage/api"
 	"github.com/libopenstorage/openstorage/cluster"
+	"github.com/libopenstorage/openstorage/cluster/mock"
 	"github.com/libopenstorage/openstorage/config"
 	"github.com/libopenstorage/openstorage/pkg/auth"
 	"github.com/libopenstorage/openstorage/pkg/auth/systemtoken"
@@ -114,3 +119,39 @@ func TestUpdateSchedulerNodeName(t *testing.T) {
 
 	cleanup()
 }
+
+func TestRemoveOnlineNode(t *testing.T) {
+	const testNodeID = "test id"
+	mockErr := errors.New("mock err")
+	ctrl := gomock.NewController(t)
+	mockListener := mock.NewMockClusterListener(ctrl)
+	nodeToRemove := api.Node{
+		Id:     testNodeID,
+		Status: api.Status_STATUS_OK,
+	}
+	clusterListener := list.New()
+	clusterListener.PushBack(mockListener)
+	testManager := ClusterManager{
+		nodeCache: map[string]api.Node{
+			testNodeID: nodeToRemove,
+		},
+		listeners: clusterListener,
+	}
+
+	kv, err := kvdb.New(mem.Name, "test", []string{}, nil, kvdb.LogFatalErrorCB)
+	assert.NoError(t, err)
+	err = kvdb.SetInstance(kv)
+	assert.NoError(t, err)
+
+	// when force flag is false, node status check should take precedence
+	err = testManager.Remove([]api.Node{nodeToRemove}, false)
+	assert.Error(t, err)
+	assert.EqualError(t, err, fmt.Sprintf(decommissionErrMsg, testNodeID))
+
+	// when force flag is true, we shouldn't abort due to node status
+	mockListener.EXPECT().String().Return(testNodeID)
+	mockListener.EXPECT().MarkNodeDown(gomock.Any()).Return(mockErr)
+
+	err = testManager.Remove([]api.Node{nodeToRemove}, true)
+	assert.EqualError(t, err, mockErr.Error())
+}