PWX-37959: [Cherry-pick] honour force flag over node status (#2479)

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

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 (

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)

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())
+}