diff --git a/CHANGELOG.md b/CHANGELOG.md index 05d59c418..b39e095ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - Fixed issue where CSI storage class parameters weren't allowed. (Issue [#598](https://github.com/NetApp/trident/issues/598)) - Fixed duplicate key declaration in Trident CRD. (Issue [#671](https://github.com/NetApp/trident/issues/671)) - Fixed inaccurate CSI Snapshot logs. (Issue [#629](https://github.com/NetApp/trident/issues/629)) +- Fixed issue with unpublishing volumes on deleted nodes. (Issue [#691](https://github.com/NetApp/trident/issues/691)) **Enhancements** diff --git a/core/orchestrator_core.go b/core/orchestrator_core.go index 585eaa064..0b1329359 100644 --- a/core/orchestrator_core.go +++ b/core/orchestrator_core.go @@ -2174,8 +2174,9 @@ func (o *TridentOrchestrator) GetVolumeExternal( } // GetVolumeByInternalName returns a volume by the given internal name -func (o *TridentOrchestrator) GetVolumeByInternalName(volumeInternal string, ctx context.Context) (volume string, - err error) { +func (o *TridentOrchestrator) GetVolumeByInternalName( + volumeInternal string, _ context.Context, +) (volume string, err error) { defer recordTiming("volume_internal_get", &err)() @@ -3031,9 +3032,7 @@ func (o *TridentOrchestrator) PublishVolume( return nil } -func (o *TridentOrchestrator) UnpublishVolume( - ctx context.Context, volumeName string, publishInfo *utils.VolumePublishInfo, -) (err error) { +func (o *TridentOrchestrator) UnpublishVolume(ctx context.Context, volumeName, nodeName string) (err error) { if o.bootstrapError != nil { return o.bootstrapError @@ -3049,19 +3048,56 @@ func (o *TridentOrchestrator) UnpublishVolume( return utils.NotFoundError(fmt.Sprintf("volume %s not found", volumeName)) } - nodes := make([]*utils.Node, 0) - for _, node := range o.nodes { - nodes = append(nodes, node) + node, ok := o.nodes[nodeName] + if !ok { + return utils.NotFoundError(fmt.Sprintf("node %s not found", nodeName)) } - publishInfo.Nodes = nodes - publishInfo.BackendUUID = volume.BackendUUID + + // Build list of nodes to which the volume remains published + nodeMap := make(map[string]*utils.Node) + for _, pub := range o.listVolumePublicationsForVolume(ctx, volumeName) { + if pub.NodeName == nodeName { + continue + } + if n, found := o.nodes[pub.NodeName]; !found { + Logc(ctx).WithField("node", pub.NodeName).Warning("Node not found during volume unpublish.") + continue + } else { + nodeMap[pub.NodeName] = n + } + } + nodes := make([]*utils.Node, 0, len(nodeMap)) + for _, n := range nodeMap { + nodes = append(nodes, n) + } + backend, ok := o.backends[volume.BackendUUID] if !ok { // Not a not found error because this is not user input return fmt.Errorf("backend %s not found", volume.BackendUUID) } - return backend.UnpublishVolume(ctx, volume.Config, publishInfo) + // Unpublish the volume + if err = backend.UnpublishVolume(ctx, volume.Config, nodes); err != nil { + return err + } + + // Check for publications remaining on the node, not counting the one we just unpublished. + nodePubFound := false + for _, pub := range o.listVolumePublicationsForNode(ctx, nodeName) { + if pub.VolumeName == volumeName { + continue + } + nodePubFound = true + break + } + + // If the node is known to be gone, and if we just unpublished the last volume on that node, delete the node. + if !nodePubFound && node.Deleted { + return o.deleteNode(ctx, nodeName) + } + + return nil } // AttachVolume mounts a volume to the local host. This method is currently only used by Docker, @@ -4263,7 +4299,7 @@ func (o *TridentOrchestrator) ListNodes(context.Context) (nodes []*utils.Node, e return nodes, nil } -func (o *TridentOrchestrator) DeleteNode(ctx context.Context, nName string) (err error) { +func (o *TridentOrchestrator) DeleteNode(ctx context.Context, nodeName string) (err error) { if o.bootstrapError != nil { return o.bootstrapError } @@ -4274,14 +4310,50 @@ func (o *TridentOrchestrator) DeleteNode(ctx context.Context, nName string) (err defer o.mutex.Unlock() defer o.updateMetrics() - node, found := o.nodes[nName] + node, found := o.nodes[nodeName] if !found { - return utils.NotFoundError(fmt.Sprintf("node %s not found", nName)) + return utils.NotFoundError(fmt.Sprintf("node %s not found", nodeName)) + } + + publicationCount := len(o.listVolumePublicationsForNode(ctx, nodeName)) + logFields := log.Fields{ + "name": nodeName, + "publications": publicationCount, } + + if publicationCount == 0 { + + // No volumes are published to this node, so delete the CR and update backends that support node-level access. + Logc(ctx).WithFields(logFields).Debug("No volumes publications remain for node, removing node CR.") + + return o.deleteNode(ctx, nodeName) + + } else { + + // There are still volumes published to this node, so this is a sudden node removal. Preserve the node CR + // with a Deleted flag so we can handle the eventual unpublish calls for the affected volumes. + Logc(ctx).WithFields(logFields).Debug("Volume publications remain for node, marking node CR as deleted.") + + node.Deleted = true + if err = o.storeClient.AddOrUpdateNode(ctx, node); err != nil { + return err + } + } + + return nil +} + +func (o *TridentOrchestrator) deleteNode(ctx context.Context, nodeName string) (err error) { + + node, found := o.nodes[nodeName] + if !found { + return utils.NotFoundError(fmt.Sprintf("node %s not found", nodeName)) + } + if err = o.storeClient.DeleteNode(ctx, node); err != nil { return err } - delete(o.nodes, nName) + delete(o.nodes, nodeName) o.invalidateAllBackendNodeAccess() return o.reconcileNodeAccessOnAllBackends(ctx) } @@ -4365,11 +4437,19 @@ func (o *TridentOrchestrator) ListVolumePublicationsForVolume( o.mutex.Lock() defer o.mutex.Unlock() + publications = o.listVolumePublicationsForVolume(ctx, volumeName) + return +} + +func (o *TridentOrchestrator) listVolumePublicationsForVolume( + _ context.Context, volumeName string, +) (publications []*utils.VolumePublication) { + publications = []*utils.VolumePublication{} for _, pub := range o.volumePublications[volumeName] { publications = append(publications, pub) } - return publications, nil + return publications } // ListVolumePublicationsForNode returns a list of all volume publications for a given node @@ -4385,13 +4465,21 @@ func (o *TridentOrchestrator) ListVolumePublicationsForNode( o.mutex.Lock() defer o.mutex.Unlock() + publications = o.listVolumePublicationsForNode(ctx, nodeName) + return +} + +func (o *TridentOrchestrator) listVolumePublicationsForNode( + _ context.Context, nodeName string, +) (publications []*utils.VolumePublication) { + publications = []*utils.VolumePublication{} for _, pubs := range o.volumePublications { if pubs[nodeName] != nil { publications = append(publications, pubs[nodeName]) } } - return publications, nil + return } // DeleteVolumePublication deletes the record of the volume publication for a given volume/node pair diff --git a/core/orchestrator_core_test.go b/core/orchestrator_core_test.go index b170dbf88..91d66e6b7 100644 --- a/core/orchestrator_core_test.go +++ b/core/orchestrator_core_test.go @@ -3260,6 +3260,7 @@ func TestAddNode(t *testing.T) { IQN: "myIQN", IPs: []string{"1.1.1.1", "2.2.2.2"}, TopologyLabels: map[string]string{"topology.kubernetes.io/region": "Region1"}, + Deleted: false, } orchestrator := getOrchestrator(t) if err := orchestrator.AddNode(ctx(), node, nil); err != nil { @@ -3274,12 +3275,14 @@ func TestGetNode(t *testing.T) { IQN: "myIQN", IPs: []string{"1.1.1.1", "2.2.2.2"}, TopologyLabels: map[string]string{"topology.kubernetes.io/region": "Region1"}, + Deleted: false, } unexpectedNode := &utils.Node{ Name: "testNode2", IQN: "myOtherIQN", IPs: []string{"3.3.3.3", "4.4.4.4"}, TopologyLabels: map[string]string{"topology.kubernetes.io/region": "Region2"}, + Deleted: false, } initialNodes := map[string]*utils.Node{} initialNodes[expectedNode.Name] = expectedNode @@ -3299,14 +3302,16 @@ func TestGetNode(t *testing.T) { func TestListNodes(t *testing.T) { orchestrator := getOrchestrator(t) expectedNode1 := &utils.Node{ - Name: "testNode", - IQN: "myIQN", - IPs: []string{"1.1.1.1", "2.2.2.2"}, + Name: "testNode", + IQN: "myIQN", + IPs: []string{"1.1.1.1", "2.2.2.2"}, + Deleted: false, } expectedNode2 := &utils.Node{ - Name: "testNode2", - IQN: "myOtherIQN", - IPs: []string{"3.3.3.3", "4.4.4.4"}, + Name: "testNode2", + IQN: "myOtherIQN", + IPs: []string{"3.3.3.3", "4.4.4.4"}, + Deleted: false, } initialNodes := map[string]*utils.Node{} initialNodes[expectedNode1.Name] = expectedNode1 @@ -3350,9 +3355,10 @@ func unorderedNodeSlicesEqual(x, y []*utils.Node) bool { func TestDeleteNode(t *testing.T) { orchestrator := getOrchestrator(t) initialNode := &utils.Node{ - Name: "testNode", - IQN: "myIQN", - IPs: []string{"1.1.1.1", "2.2.2.2"}, + Name: "testNode", + IQN: "myIQN", + IPs: []string{"1.1.1.1", "2.2.2.2"}, + Deleted: false, } initialNodes := map[string]*utils.Node{} initialNodes[initialNode.Name] = initialNode diff --git a/core/types.go b/core/types.go index 5b7def9af..cbd9b81b4 100644 --- a/core/types.go +++ b/core/types.go @@ -55,7 +55,7 @@ type Orchestrator interface { ListVolumes(ctx context.Context) ([]*storage.VolumeExternal, error) ListVolumesByPlugin(ctx context.Context, pluginName string) ([]*storage.VolumeExternal, error) PublishVolume(ctx context.Context, volumeName string, publishInfo *utils.VolumePublishInfo) error - UnpublishVolume(ctx context.Context, volumeName string, publishInfo *utils.VolumePublishInfo) error + UnpublishVolume(ctx context.Context, volumeName, nodeName string) error ResizeVolume(ctx context.Context, volumeName, newSize string) error SetVolumeState(ctx context.Context, volumeName string, state storage.VolumeState) error diff --git a/frontend/csi/controller_server.go b/frontend/csi/controller_server.go index 42974aef3..c9701c583 100644 --- a/frontend/csi/controller_server.go +++ b/frontend/csi/controller_server.go @@ -480,48 +480,24 @@ func (p *Plugin) ControllerUnpublishVolume( return nil, status.Error(codes.InvalidArgument, "no node ID provided") } - // Check if volume exists. If not, return success. - volume, err := p.orchestrator.GetVolume(ctx, volumeID) - if err != nil { - if utils.IsNotFoundError(err) { - return &csi.ControllerUnpublishVolumeResponse{}, nil - } else { - return nil, p.getCSIErrorForOrchestratorError(err) - } - } + logFields := log.Fields{"volume": req.GetVolumeId(), "node": req.GetNodeId()} - // Get node attributes from the node ID - nodeInfo, err := p.orchestrator.GetNode(ctx, nodeID) - if err != nil { - Logc(ctx).WithField("node", nodeID).Error("Node info not found.") - return nil, status.Error(codes.NotFound, err.Error()) - } - - // Set up volume publish info with what we know about the node - volumePublishInfo := &utils.VolumePublishInfo{ - Localhost: false, - HostIQN: []string{nodeInfo.IQN}, - HostIP: nodeInfo.IPs, - HostName: nodeInfo.Name, - Unmanaged: volume.Config.ImportNotManaged, - } - - // Optionally update NFS export rules, remove node IQN from igroup, etc. - if err = p.orchestrator.UnpublishVolume(ctx, volume.Config.Name, volumePublishInfo); err != nil { - return nil, status.Error(codes.Internal, err.Error()) + // Unpublish the volume by updating NFS export rules, removing node IQN from igroup, etc. + if err := p.orchestrator.UnpublishVolume(ctx, volumeID, nodeID); err != nil { + if !utils.IsNotFoundError(err) { + Logc(ctx).WithFields(logFields).WithError(err).Error("Could not unpublish volume.") + return nil, status.Error(codes.Internal, err.Error()) + } + Logc(ctx).WithFields(logFields).WithError(err).Warning("Unpublish targets not found, continuing.") } - if err = p.orchestrator.DeleteVolumePublication(ctx, req.GetVolumeId(), req.GetNodeId()); err != nil { + // Remove the stateful publish tracking record. + if err := p.orchestrator.DeleteVolumePublication(ctx, req.GetVolumeId(), req.GetNodeId()); err != nil { if !utils.IsNotFoundError(err) { - msg := "error removing volume publication record" - Logc(ctx).WithError(err).Error(msg) - return nil, status.Error(codes.Internal, msg) - } else { - Logc(ctx).WithFields(log.Fields{ - "VolumeID": req.GetVolumeId(), - "NodeID": req.GetNodeId(), - }).Warn("Volume publication record not found.") + Logc(ctx).WithFields(logFields).WithError(err).Error("Could not remove volume publication record.") + return nil, status.Error(codes.Internal, err.Error()) } + Logc(ctx).WithFields(logFields).Warning("Volume publication record not found, returning success.") } return &csi.ControllerUnpublishVolumeResponse{}, nil diff --git a/frontend/csi/controller_server_test.go b/frontend/csi/controller_server_test.go index 1bd037654..b32d3950f 100644 --- a/frontend/csi/controller_server_test.go +++ b/frontend/csi/controller_server_test.go @@ -58,7 +58,8 @@ func generateFakeUnpublishVolumeRequest() *csi.ControllerUnpublishVolumeRequest func generateFakeNode(nodeID string) *utils.Node { fakeNode := &utils.Node{ - Name: nodeID, + Name: nodeID, + Deleted: false, } return fakeNode } @@ -231,11 +232,7 @@ func TestControllerUnpublishVolume(t *testing.T) { // Create fake objects for this test req := generateFakeUnpublishVolumeRequest() - fakeVolumeExternal := generateFakeVolumeExternal(req.VolumeId) - fakeNode := generateFakeNode(req.NodeId) - mockOrchestrator.EXPECT().GetVolume(ctx, req.VolumeId).Return(fakeVolumeExternal, nil) - mockOrchestrator.EXPECT().GetNode(ctx, req.NodeId).Return(fakeNode, nil) mockOrchestrator.EXPECT().UnpublishVolume(ctx, req.VolumeId, gomock.Any()).Return(nil) // Verify we remove the volume publication on a successful volume unpublish mockOrchestrator.EXPECT().DeleteVolumePublication(ctx, req.VolumeId, req.NodeId).Return(nil) @@ -244,7 +241,7 @@ func TestControllerUnpublishVolume(t *testing.T) { assert.Nil(t, err, "unexpected error unpublishing volume") } -func TestControllerUnpublishVolumeError(t *testing.T) { +func TestControllerUnpublishVolume_UnpublishError(t *testing.T) { mockCtrl := gomock.NewController(t) // Create a mocked orchestrator mockOrchestrator := mockcore.NewMockOrchestrator(mockCtrl) @@ -255,11 +252,7 @@ func TestControllerUnpublishVolumeError(t *testing.T) { // Create fake objects for this test req := generateFakeUnpublishVolumeRequest() - fakeVolumeExternal := generateFakeVolumeExternal(req.VolumeId) - fakeNode := generateFakeNode(req.NodeId) - mockOrchestrator.EXPECT().GetVolume(ctx, req.VolumeId).Return(fakeVolumeExternal, nil) - mockOrchestrator.EXPECT().GetNode(ctx, req.NodeId).Return(fakeNode, nil) // Simulate an error during volume unpublishing mockOrchestrator.EXPECT().UnpublishVolume(ctx, req.VolumeId, gomock.Any()).Return(fmt.Errorf("some error")) // Verify we do not remove the volume publication if unpublishing fails @@ -268,3 +261,45 @@ func TestControllerUnpublishVolumeError(t *testing.T) { _, err := controllerServer.ControllerUnpublishVolume(ctx, req) assert.NotNil(t, err, "unexpected success unpublishing volume") } + +func TestControllerUnpublishVolume_DeleteVolumePublicationError(t *testing.T) { + mockCtrl := gomock.NewController(t) + // Create a mocked orchestrator + mockOrchestrator := mockcore.NewMockOrchestrator(mockCtrl) + // Create a mocked helper + mockHelper := mockhelpers.NewMockHybridPlugin(mockCtrl) + // Create an instance of ControllerServer for this test + controllerServer := generateController(mockOrchestrator, mockHelper) + + // Create fake objects for this test + req := generateFakeUnpublishVolumeRequest() + + // Simulate an error during volume unpublishing + mockOrchestrator.EXPECT().UnpublishVolume(ctx, req.VolumeId, gomock.Any()).Return(nil).Times(1) + // Verify we do not remove the volume publication if unpublishing fails + mockOrchestrator.EXPECT().DeleteVolumePublication(ctx, gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed")).Times(1) + + _, err := controllerServer.ControllerUnpublishVolume(ctx, req) + assert.NotNil(t, err, "unexpected success unpublishing volume") +} + +func TestControllerUnpublishVolume_NotFoundErrors(t *testing.T) { + mockCtrl := gomock.NewController(t) + // Create a mocked orchestrator + mockOrchestrator := mockcore.NewMockOrchestrator(mockCtrl) + // Create a mocked helper + mockHelper := mockhelpers.NewMockHybridPlugin(mockCtrl) + // Create an instance of ControllerServer for this test + controllerServer := generateController(mockOrchestrator, mockHelper) + + // Create fake objects for this test + req := generateFakeUnpublishVolumeRequest() + + // Simulate an error during volume unpublishing + mockOrchestrator.EXPECT().UnpublishVolume(ctx, req.VolumeId, gomock.Any()).Return(utils.NotFoundError("not found")).Times(1) + // Verify we do not remove the volume publication if unpublishing fails + mockOrchestrator.EXPECT().DeleteVolumePublication(ctx, gomock.Any(), gomock.Any()).Return(utils.NotFoundError("not found")).Times(1) + + _, err := controllerServer.ControllerUnpublishVolume(ctx, req) + assert.Nil(t, err, "unexpected error unpublishing volume") +} diff --git a/frontend/csi/node_server.go b/frontend/csi/node_server.go index 204b55ed2..14757510a 100644 --- a/frontend/csi/node_server.go +++ b/frontend/csi/node_server.go @@ -468,6 +468,7 @@ func (p *Plugin) nodeGetInfo(ctx context.Context) *utils.Node { IPs: ips, NodePrep: p.nodePrep, HostInfo: p.hostInfo, + Deleted: false, } return node } diff --git a/mocks/mock_core/mock_core.go b/mocks/mock_core/mock_core.go index e5506f2b6..38814154e 100644 --- a/mocks/mock_core/mock_core.go +++ b/mocks/mock_core/mock_core.go @@ -914,7 +914,7 @@ func (mr *MockOrchestratorMockRecorder) SetVolumeState(arg0, arg1, arg2 interfac } // UnpublishVolume mocks base method. -func (m *MockOrchestrator) UnpublishVolume(arg0 context.Context, arg1 string, arg2 *utils.VolumePublishInfo) error { +func (m *MockOrchestrator) UnpublishVolume(arg0 context.Context, arg1, arg2 string) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "UnpublishVolume", arg0, arg1, arg2) ret0, _ := ret[0].(error) diff --git a/mocks/mock_storage/mock_storage.go b/mocks/mock_storage/mock_storage.go index 3a0b79bbc..35efc08ef 100644 --- a/mocks/mock_storage/mock_storage.go +++ b/mocks/mock_storage/mock_storage.go @@ -654,7 +654,7 @@ func (mr *MockBackendMockRecorder) Terminate(arg0 interface{}) *gomock.Call { } // UnpublishVolume mocks base method. -func (m *MockBackend) UnpublishVolume(arg0 context.Context, arg1 *storage.VolumeConfig, arg2 *utils.VolumePublishInfo) error { +func (m *MockBackend) UnpublishVolume(arg0 context.Context, arg1 *storage.VolumeConfig, arg2 []*utils.Node) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "UnpublishVolume", arg0, arg1, arg2) ret0, _ := ret[0].(error) diff --git a/persistent_store/crd/apis/netapp/v1/node.go b/persistent_store/crd/apis/netapp/v1/node.go index c3f03dc10..b0a4ed154 100644 --- a/persistent_store/crd/apis/netapp/v1/node.go +++ b/persistent_store/crd/apis/netapp/v1/node.go @@ -41,6 +41,7 @@ func (in *TridentNode) Apply(persistent *utils.Node) error { in.Name = persistent.Name in.IQN = persistent.IQN in.IPs = persistent.IPs + in.Deleted = persistent.Deleted nodePrep, err := json.Marshal(persistent.NodePrep) if err != nil { @@ -65,6 +66,7 @@ func (in *TridentNode) Persistent() (*utils.Node, error) { IPs: in.IPs, NodePrep: &utils.NodePrep{}, HostInfo: &utils.HostSystem{}, + Deleted: in.Deleted, } if string(in.NodePrep.Raw) != "" { diff --git a/persistent_store/crd/apis/netapp/v1/types.go b/persistent_store/crd/apis/netapp/v1/types.go index 0534dabd6..8a692ec6e 100644 --- a/persistent_store/crd/apis/netapp/v1/types.go +++ b/persistent_store/crd/apis/netapp/v1/types.go @@ -311,6 +311,8 @@ type TridentNode struct { NodePrep runtime.RawExtension `json:"nodePrep,omitempty"` // HostInfo contains information about the node's host machine HostInfo runtime.RawExtension `json:"hostInfo,omitempty"` + // Deleted indicates that Trident received an event that the node has been removed + Deleted bool `json:"deleted"` } // TridentNodeList is a list of TridentNode objects. diff --git a/persistent_store/crdv1_test.go b/persistent_store/crdv1_test.go index 1536a9ee1..14907bbde 100644 --- a/persistent_store/crdv1_test.go +++ b/persistent_store/crdv1_test.go @@ -882,6 +882,7 @@ func TestKubernetesAddOrUpdateNode(t *testing.T) { IPs: []string{ "192.168.0.1", }, + Deleted: false, } // should not exist diff --git a/persistent_store/passthrough_test.go b/persistent_store/passthrough_test.go index a2ed6c179..4eb8835d8 100644 --- a/persistent_store/passthrough_test.go +++ b/persistent_store/passthrough_test.go @@ -95,9 +95,10 @@ func getFakeStorageClassWithName(name string, iops int, snapshots bool, provType func getFakeNode() *utils.Node { return &utils.Node{ - Name: "testNode", - IQN: "myIQN", - IPs: []string{"1.1.1.1", "2.2.2.2"}, + Name: "testNode", + IQN: "myIQN", + IPs: []string{"1.1.1.1", "2.2.2.2"}, + Deleted: false, } } diff --git a/storage/backend.go b/storage/backend.go index e181b5fe1..c4a4377db 100644 --- a/storage/backend.go +++ b/storage/backend.go @@ -72,7 +72,7 @@ type Driver interface { } type Unpublisher interface { - Unpublish(ctx context.Context, volConfig *VolumeConfig, publishInfo *utils.VolumePublishInfo) error + Unpublish(ctx context.Context, volConfig *VolumeConfig, nodes []*utils.Node) error } // Mirrorer provides a common interface for backends that support mirror replication @@ -519,7 +519,7 @@ func (b *StorageBackend) PublishVolume( } func (b *StorageBackend) UnpublishVolume( - ctx context.Context, volConfig *VolumeConfig, publishInfo *utils.VolumePublishInfo, + ctx context.Context, volConfig *VolumeConfig, nodes []*utils.Node, ) error { Logc(ctx).WithFields(log.Fields{ @@ -538,7 +538,7 @@ func (b *StorageBackend) UnpublishVolume( if unpublisher, ok := b.driver.(Unpublisher); !ok { return nil } else { - return unpublisher.Unpublish(ctx, volConfig, publishInfo) + return unpublisher.Unpublish(ctx, volConfig, nodes) } } diff --git a/storage/types.go b/storage/types.go index 0de6ba43e..ffbf2441d 100644 --- a/storage/types.go +++ b/storage/types.go @@ -47,7 +47,7 @@ type Backend interface { PublishVolume( ctx context.Context, volConfig *VolumeConfig, publishInfo *utils.VolumePublishInfo, ) error - UnpublishVolume(ctx context.Context, volConfig *VolumeConfig, publishInfo *utils.VolumePublishInfo) error + UnpublishVolume(ctx context.Context, volConfig *VolumeConfig, nodes []*utils.Node) error GetVolumeExternal(ctx context.Context, volumeName string) (*VolumeExternal, error) ImportVolume(ctx context.Context, volConfig *VolumeConfig) (*Volume, error) ResizeVolume(ctx context.Context, volConfig *VolumeConfig, newSize string) error diff --git a/storage_drivers/astrads/astrads.go b/storage_drivers/astrads/astrads.go index 40bab052c..ca29ffa0a 100644 --- a/storage_drivers/astrads/astrads.go +++ b/storage_drivers/astrads/astrads.go @@ -1259,7 +1259,7 @@ func (d *StorageDriver) Publish( publishInfo.FilesystemType = "nfs" publishInfo.MountOptions = mountOptions - err = d.publishNFSShare(ctx, publishInfo, volume) + err = d.publishNFSShare(ctx, volConfig, publishInfo, volume) if err != nil { Logc(ctx).WithField("name", volume.Name).WithError(err).Error("Could not publish volume.") } else { @@ -1271,7 +1271,7 @@ func (d *StorageDriver) Publish( // publishNFSShare ensures that the volume has the correct export policy applied // along with the needed access rules. func (d *StorageDriver) publishNFSShare( - ctx context.Context, publishInfo *utils.VolumePublishInfo, volume *api.Volume, + ctx context.Context, volConfig *storage.VolumeConfig, publishInfo *utils.VolumePublishInfo, volume *api.Volume, ) error { name := volume.Name @@ -1287,7 +1287,7 @@ func (d *StorageDriver) publishNFSShare( defer Logc(ctx).WithFields(fields).Debug("<<<< publishNFSShare") } - if !d.Config.AutoExportPolicy || publishInfo.Unmanaged { + if !d.Config.AutoExportPolicy || volConfig.ImportNotManaged { // Nothing to do if we're not configuring export policies automatically or volume is not managed return nil } @@ -1353,7 +1353,7 @@ func (d *StorageDriver) grantNodeAccess( // where the volume will be mounted, so it should limit itself to updating access rules, initiator groups, etc. // that require some host identity (but not locality) as well as storage controller API access. func (d *StorageDriver) Unpublish( - ctx context.Context, volConfig *storage.VolumeConfig, publishInfo *utils.VolumePublishInfo, + ctx context.Context, volConfig *storage.VolumeConfig, nodes []*utils.Node, ) error { name := volConfig.InternalName @@ -1373,7 +1373,7 @@ func (d *StorageDriver) Unpublish( return fmt.Errorf("could not find volume %s; %v", name, err) } - err = d.unpublishNFSShare(ctx, publishInfo, volume) + err = d.unpublishNFSShare(ctx, volConfig, nodes, volume) if err != nil { Logc(ctx).WithField("name", volume.Name).WithError(err).Error("Could not unpublish volume.") } else { @@ -1382,9 +1382,10 @@ func (d *StorageDriver) Unpublish( return err } -// unpublishNFSShare ensures that the volume does not have access to the specified node. +// unpublishNFSShare ensures that the volume does not have access to a node to which is has been unpublished. +// The node list represents the set of nodes to which the volume should be published. func (d *StorageDriver) unpublishNFSShare( - ctx context.Context, publishInfo *utils.VolumePublishInfo, volume *api.Volume, + ctx context.Context, volConfig *storage.VolumeConfig, nodes []*utils.Node, volume *api.Volume, ) error { name := volume.Name @@ -1400,13 +1401,13 @@ func (d *StorageDriver) unpublishNFSShare( defer Logc(ctx).WithFields(fields).Debug("<<<< unpublishNFSShare") } - if !d.Config.AutoExportPolicy || publishInfo.Unmanaged { + if !d.Config.AutoExportPolicy || volConfig.ImportNotManaged { // Nothing to do if we're not configuring export policies automatically or volume is not managed return nil } // Ensure the export policy exists and has the correct rule set - if err := d.revokeNodeAccess(ctx, publishInfo, policyName); err != nil { + if err := d.setNodeAccess(ctx, nodes, policyName); err != nil { return err } @@ -1430,30 +1431,36 @@ func (d *StorageDriver) unpublishNFSShare( return nil } -// revokeNodeAccess checks to see if the export policy exists and if not it will create it. Then it ensures -// that the IPs in the publish info are not reflected as rules on the export policy. -func (d *StorageDriver) revokeNodeAccess( - ctx context.Context, publishInfo *utils.VolumePublishInfo, policyName string, +// setNodeAccess checks to see if the export policy exists and if not it will create it. Then it ensures +// that the IPs in the node list exactly match the rules on the export policy. +func (d *StorageDriver) setNodeAccess( + ctx context.Context, nodes []*utils.Node, policyName string, ) error { exportPolicy, err := d.API.EnsureExportPolicyExists(ctx, policyName) if err != nil { err = fmt.Errorf("unable to ensure export policy exists; %v", err) - Logc(ctx).WithField("exportPolicy", policyName).WithError(err).Error("Could not revoke node access.") + Logc(ctx).WithField("exportPolicy", policyName).WithError(err).Error("Could not set node access.") return err } - removedIPs, err := utils.FilterIPs(ctx, publishInfo.HostIP, d.Config.AutoExportCIDRs) - if err != nil { - err = fmt.Errorf("unable to determine undesired export policy rules; %v", err) - Logc(ctx).WithField("exportPolicy", policyName).WithError(err).Error("Could not revoke node access.") - return err + allIPs := make([]string, 0) + + for _, node := range nodes { + nodeIPs, ipErr := utils.FilterIPs(ctx, node.IPs, d.Config.AutoExportCIDRs) + if ipErr != nil { + err = fmt.Errorf("unable to determine undesired export policy rules; %v", err) + Logc(ctx).WithField("exportPolicy", policyName).WithError(err).Error("Could not set node access.") + return err + } + + allIPs = append(allIPs, nodeIPs...) } - err = d.reconcileExportPolicyRules(ctx, exportPolicy, nil, removedIPs) + err = d.setExportPolicyRules(ctx, exportPolicy, allIPs) if err != nil { - err = fmt.Errorf("unable to reconcile export policy rules; %v", err) - Logc(ctx).WithField("exportPolicy", policyName).WithError(err).Error("Could not revoke node access.") + err = fmt.Errorf("unable to set export policy rules; %v", err) + Logc(ctx).WithField("exportPolicy", policyName).WithError(err).Error("Could not set node access.") return err } @@ -1512,6 +1519,38 @@ func (d *StorageDriver) reconcileExportPolicyRules( return nil } +// setExportPolicyRules replaces a set of access rules on an export policy to the specified set. +func (d *StorageDriver) setExportPolicyRules(ctx context.Context, policy *api.ExportPolicy, IPs []string) error { + + // Replace the rules on the export policy + policy.Rules = make([]api.ExportPolicyRule, 0) + index := uint64(1) + + for _, ip := range IPs { + + policy.Rules = append(policy.Rules, api.ExportPolicyRule{ + Clients: []string{ip}, + Protocols: []string{"nfs4"}, + RuleIndex: index, + RoRules: []string{"any"}, + RwRules: []string{"any"}, + SuperUser: []string{"any"}, + AnonUser: 65534, + }) + + index++ + } + + if err := d.API.SetExportPolicyAttributes(ctx, policy, roaring.BitmapOf(api.UpdateFlagExportRules)); err != nil { + Logc(ctx).WithField("name", policy.Name).WithError(err).Error("Could not update export policy rules.") + return err + } + + Logc(ctx).WithField("name", policy.Name).Info("Updated export policy rules.") + + return nil +} + // CanSnapshot determines whether a snapshot as specified in the provided snapshot config may be taken. func (d *StorageDriver) CanSnapshot(_ context.Context, _ *storage.SnapshotConfig, _ *storage.VolumeConfig) error { return nil diff --git a/utils/types.go b/utils/types.go index d2f53c838..624072472 100644 --- a/utils/types.go +++ b/utils/types.go @@ -73,6 +73,7 @@ type Node struct { TopologyLabels map[string]string `json:"topologyLabels,omitempty"` NodePrep *NodePrep `json:"nodePrep"` HostInfo *HostSystem `json:"hostInfo,omitempty"` + Deleted bool `json:"deleted"` } type NodePrep struct {