From 35f831c07d061d4bb9552a06e0f016e70d855e58 Mon Sep 17 00:00:00 2001 From: Kris Hicks Date: Thu, 14 Jan 2021 12:44:50 -0800 Subject: [PATCH] csi: Return error when deleting node (#9803) In this change we'll properly return the error in the CSIPluginTypeMonolith case (which is the type given in DeleteNode()), and also return the error when the given ID is not found. This was found via errcheck. --- nomad/structs/csi.go | 30 ++++--- nomad/structs/csi_test.go | 184 +++++++++++++++++++++++++++++++++++++- 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/nomad/structs/csi.go b/nomad/structs/csi.go index ade9181fb..71a61ef39 100644 --- a/nomad/structs/csi.go +++ b/nomad/structs/csi.go @@ -5,6 +5,7 @@ import ( "strings" "time" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/helper" ) @@ -853,32 +854,41 @@ func (p *CSIPlugin) DeleteNode(nodeID string) error { func (p *CSIPlugin) DeleteNodeForType(nodeID string, pluginType CSIPluginType) error { switch pluginType { case CSIPluginTypeController: - prev, ok := p.Controllers[nodeID] - if ok { + if prev, ok := p.Controllers[nodeID]; ok { if prev == nil { return fmt.Errorf("plugin missing controller: %s", nodeID) } if prev.Healthy { - p.ControllersHealthy -= 1 + p.ControllersHealthy-- } + delete(p.Controllers, nodeID) } - delete(p.Controllers, nodeID) case CSIPluginTypeNode: - prev, ok := p.Nodes[nodeID] - if ok { + if prev, ok := p.Nodes[nodeID]; ok { if prev == nil { return fmt.Errorf("plugin missing node: %s", nodeID) } if prev.Healthy { - p.NodesHealthy -= 1 + p.NodesHealthy-- } + delete(p.Nodes, nodeID) } - delete(p.Nodes, nodeID) case CSIPluginTypeMonolith: - p.DeleteNodeForType(nodeID, CSIPluginTypeController) - p.DeleteNodeForType(nodeID, CSIPluginTypeNode) + var result error + + err := p.DeleteNodeForType(nodeID, CSIPluginTypeController) + if err != nil { + result = multierror.Append(result, err) + } + + err = p.DeleteNodeForType(nodeID, CSIPluginTypeNode) + if err != nil { + result = multierror.Append(result, err) + } + + return result } return nil diff --git a/nomad/structs/csi_test.go b/nomad/structs/csi_test.go index b1c2ceda5..35baf3cb0 100644 --- a/nomad/structs/csi_test.go +++ b/nomad/structs/csi_test.go @@ -204,10 +204,192 @@ func TestCSIPluginCleanup(t *testing.T) { require.Equal(t, 1, plug.ControllersHealthy) require.Equal(t, 1, plug.NodesHealthy) - plug.DeleteNode("n0") + err := plug.DeleteNode("n0") + require.NoError(t, err) + require.Equal(t, 0, plug.ControllersHealthy) require.Equal(t, 0, plug.NodesHealthy) require.Equal(t, 0, len(plug.Controllers)) require.Equal(t, 0, len(plug.Nodes)) } + +func TestDeleteNodeForType_Controller(t *testing.T) { + info := &CSIInfo{ + PluginID: "foo", + AllocID: "a0", + Healthy: true, + Provider: "foo-provider", + RequiresControllerPlugin: true, + RequiresTopologies: false, + ControllerInfo: &CSIControllerInfo{}, + } + + plug := NewCSIPlugin("foo", 1000) + + plug.Controllers["n0"] = info + plug.ControllersHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeController) + require.NoError(t, err) + + require.Equal(t, 0, plug.ControllersHealthy) + require.Equal(t, 0, len(plug.Controllers)) +} + +func TestDeleteNodeForType_NilController(t *testing.T) { + plug := NewCSIPlugin("foo", 1000) + + plug.Controllers["n0"] = nil + plug.ControllersHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeController) + require.Error(t, err) + require.Equal(t, 1, len(plug.Controllers)) + + _, ok := plug.Controllers["foo"] + require.False(t, ok) +} + +func TestDeleteNodeForType_Node(t *testing.T) { + info := &CSIInfo{ + PluginID: "foo", + AllocID: "a0", + Healthy: true, + Provider: "foo-provider", + RequiresControllerPlugin: true, + RequiresTopologies: false, + NodeInfo: &CSINodeInfo{}, + } + + plug := NewCSIPlugin("foo", 1000) + + plug.Nodes["n0"] = info + plug.NodesHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeNode) + require.NoError(t, err) + + require.Equal(t, 0, plug.NodesHealthy) + require.Equal(t, 0, len(plug.Nodes)) +} + +func TestDeleteNodeForType_NilNode(t *testing.T) { + plug := NewCSIPlugin("foo", 1000) + + plug.Nodes["n0"] = nil + plug.NodesHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeNode) + require.Error(t, err) + require.Equal(t, 1, len(plug.Nodes)) + + _, ok := plug.Nodes["foo"] + require.False(t, ok) +} + +func TestDeleteNodeForType_Monolith(t *testing.T) { + controllerInfo := &CSIInfo{ + PluginID: "foo", + AllocID: "a0", + Healthy: true, + Provider: "foo-provider", + RequiresControllerPlugin: true, + RequiresTopologies: false, + ControllerInfo: &CSIControllerInfo{}, + } + + nodeInfo := &CSIInfo{ + PluginID: "foo", + AllocID: "a0", + Healthy: true, + Provider: "foo-provider", + RequiresControllerPlugin: true, + RequiresTopologies: false, + NodeInfo: &CSINodeInfo{}, + } + + plug := NewCSIPlugin("foo", 1000) + + plug.Controllers["n0"] = controllerInfo + plug.ControllersHealthy = 1 + + plug.Nodes["n0"] = nodeInfo + plug.NodesHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeMonolith) + require.NoError(t, err) + + require.Equal(t, 0, len(plug.Controllers)) + require.Equal(t, 0, len(plug.Nodes)) + + _, ok := plug.Nodes["foo"] + require.False(t, ok) + + _, ok = plug.Controllers["foo"] + require.False(t, ok) +} + +func TestDeleteNodeForType_Monolith_NilController(t *testing.T) { + plug := NewCSIPlugin("foo", 1000) + + plug.Controllers["n0"] = nil + plug.ControllersHealthy = 1 + + nodeInfo := &CSIInfo{ + PluginID: "foo", + AllocID: "a0", + Healthy: true, + Provider: "foo-provider", + RequiresControllerPlugin: true, + RequiresTopologies: false, + NodeInfo: &CSINodeInfo{}, + } + + plug.Nodes["n0"] = nodeInfo + plug.NodesHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeMonolith) + require.Error(t, err) + + require.Equal(t, 1, len(plug.Controllers)) + require.Equal(t, 0, len(plug.Nodes)) + + _, ok := plug.Nodes["foo"] + require.False(t, ok) + + _, ok = plug.Controllers["foo"] + require.False(t, ok) +} + +func TestDeleteNodeForType_Monolith_NilNode(t *testing.T) { + plug := NewCSIPlugin("foo", 1000) + + plug.Nodes["n0"] = nil + plug.NodesHealthy = 1 + + controllerInfo := &CSIInfo{ + PluginID: "foo", + AllocID: "a0", + Healthy: true, + Provider: "foo-provider", + RequiresControllerPlugin: true, + RequiresTopologies: false, + ControllerInfo: &CSIControllerInfo{}, + } + + plug.Controllers["n0"] = controllerInfo + plug.ControllersHealthy = 1 + + err := plug.DeleteNodeForType("n0", CSIPluginTypeMonolith) + require.Error(t, err) + + require.Equal(t, 0, len(plug.Controllers)) + require.Equal(t, 1, len(plug.Nodes)) + + _, ok := plug.Nodes["foo"] + require.False(t, ok) + + _, ok = plug.Controllers["foo"] + require.False(t, ok) +}