diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index bf8b713ba0..2dc37d13b2 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -599,23 +599,62 @@ class NetAppCmodeFileStorageLibrary(object): msg_args = {'vserver': vserver, 'proto': share['share_proto']} raise exception.NetAppException(msg % msg_args) - interfaces = self._sort_lifs_by_aggregate_locality(share, interfaces) + # Get LIF addresses with metadata + export_addresses = self._get_export_addresses_with_metadata(share, + interfaces) + # Create the share and get a callback for generating export locations + callback = helper.create_share(share, share_name) + + # Generate export locations using addresses, metadata and callback + export_locations = [ + { + 'path': callback(export_address), + 'is_admin_only': metadata.pop('is_admin_only', False), + 'metadata': metadata, + } + for export_address, metadata + in copy.deepcopy(export_addresses).items() + ] + + # Sort the export locations to report preferred paths first + export_locations = self._sort_export_locations_by_preferred_paths( + export_locations) - export_addresses = [interface['address'] for interface in interfaces] - export_locations = helper.create_share( - share, share_name, export_addresses) return export_locations @na_utils.trace - def _sort_lifs_by_aggregate_locality(self, share, interfaces): - """Sort LIFs by placing aggregate-local LIFs first.""" + def _get_export_addresses_with_metadata(self, share, interfaces): + """Return interface addresses with locality and other metadata.""" + aggregate_name = share_utils.extract_host(share['host'], level='pool') home_node = self._get_aggregate_node(aggregate_name) - if not home_node: - return interfaces - return sorted(interfaces, - key=lambda intf: intf.get('home-node') != home_node) + addresses = {} + for interface in interfaces: + + address = interface['address'] + is_admin_only = False + + if home_node: + preferred = interface.get('home-node') == home_node + else: + preferred = None + + addresses[address] = { + 'is_admin_only': is_admin_only, + 'preferred': preferred, + } + + return addresses + + @na_utils.trace + def _sort_export_locations_by_preferred_paths(self, export_locations): + """Sort the export locations to report preferred paths first.""" + + sort_key = lambda location: location.get( + 'metadata', {}).get('preferred') is not True + + return sorted(export_locations, key=sort_key) @na_utils.trace def _remove_export(self, share, vserver_client): diff --git a/manila/share/drivers/netapp/dataontap/protocols/base.py b/manila/share/drivers/netapp/dataontap/protocols/base.py index 8089b05551..f6b708c38e 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/base.py +++ b/manila/share/drivers/netapp/dataontap/protocols/base.py @@ -53,7 +53,7 @@ class NetAppBaseHelper(object): return access_level == constants.ACCESS_LEVEL_RO @abc.abstractmethod - def create_share(self, share, share_name, export_addresses): + def create_share(self, share, share_name): """Creates NAS share.""" @abc.abstractmethod diff --git a/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py b/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py index 234c7bfab4..23e4844b11 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py +++ b/manila/share/drivers/netapp/dataontap/protocols/cifs_cmode.py @@ -33,12 +33,15 @@ class NetAppCmodeCIFSHelper(base.NetAppBaseHelper): """NetApp cDOT CIFS protocol helper class.""" @na_utils.trace - def create_share(self, share, share_name, export_addresses): + def create_share(self, share, share_name): """Creates CIFS share on Data ONTAP Vserver.""" self._client.create_cifs_share(share_name) self._client.remove_cifs_share_access(share_name, 'Everyone') - return [r'\\%s\%s' % (export_address, share_name) - for export_address in export_addresses] + + # Return a callback that may be used for generating export paths + # for this share. + return (lambda export_address, share_name=share_name: + r'\\%s\%s' % (export_address, share_name)) @na_utils.trace def delete_share(self, share, share_name): diff --git a/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py b/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py index bd939b21a1..b8785d7753 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py +++ b/manila/share/drivers/netapp/dataontap/protocols/nfs_cmode.py @@ -35,13 +35,16 @@ class NetAppCmodeNFSHelper(base.NetAppBaseHelper): """NetApp cDOT NFS protocol helper class.""" @na_utils.trace - def create_share(self, share, share_name, export_addresses): + def create_share(self, share, share_name): """Creates NFS share.""" self._client.clear_nfs_export_policy_for_volume(share_name) self._ensure_export_policy(share, share_name) export_path = self._client.get_volume_junction_path(share_name) - return [':'.join([export_address, export_path]) - for export_address in export_addresses] + + # Return a callback that may be used for generating export paths + # for this share. + return (lambda export_address, export_path=export_path: + ':'.join([export_address, export_path])) @na_utils.trace @base.access_rules_synchronized diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index 945b01e1f7..692270e4e3 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -902,25 +902,29 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): def test_create_export(self): protocol_helper = mock.Mock() - protocol_helper.create_share.return_value = fake.NFS_EXPORTS + callback = (lambda export_address, export_path='fake_export_path': + ':'.join([export_address, export_path])) + protocol_helper.create_share.return_value = callback self.mock_object(self.library, '_get_helper', mock.Mock(return_value=protocol_helper)) vserver_client = mock.Mock() vserver_client.get_network_interfaces.return_value = fake.LIFS - mock_sort_lifs_by_aggregate_locality = self.mock_object( - self.library, '_sort_lifs_by_aggregate_locality', - mock.Mock(return_value=fake.LIFS)) + fake_interface_addresses_with_metadata = copy.deepcopy( + fake.INTERFACE_ADDRESSES_WITH_METADATA) + mock_get_export_addresses_with_metadata = self.mock_object( + self.library, '_get_export_addresses_with_metadata', + mock.Mock(return_value=fake_interface_addresses_with_metadata)) result = self.library._create_export(fake.SHARE, fake.VSERVER1, vserver_client) self.assertEqual(fake.NFS_EXPORTS, result) - protocol_helper.create_share.assert_called_once_with( - fake.SHARE, fake.SHARE_NAME, fake.LIF_ADDRESSES) - mock_sort_lifs_by_aggregate_locality.assert_called_once_with( + mock_get_export_addresses_with_metadata.assert_called_once_with( fake.SHARE, fake.LIFS) + protocol_helper.create_share.assert_called_once_with( + fake.SHARE, fake.SHARE_NAME) def test_create_export_lifs_not_found(self): @@ -934,36 +938,45 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): fake.VSERVER1, vserver_client) - @ddt.data(fake.CLUSTER_NODES[0], fake.CLUSTER_NODES[1]) - def test_sort_lifs_by_aggregate_locality(self, node): + def test_get_export_addresses_with_metadata(self): mock_get_aggregate_node = self.mock_object( - self.library, '_get_aggregate_node', mock.Mock(return_value=node)) + self.library, '_get_aggregate_node', + mock.Mock(return_value=fake.CLUSTER_NODES[0])) - fake_share = copy.deepcopy(fake.SHARE) - fake_share['host'] = 'fake_host@fake_backend#fake_pool' + result = self.library._get_export_addresses_with_metadata( + fake.SHARE, fake.LIFS) - result = self.library._sort_lifs_by_aggregate_locality(fake.SHARE, - fake.LIFS) + self.assertEqual(fake.INTERFACE_ADDRESSES_WITH_METADATA, result) + mock_get_aggregate_node.assert_called_once_with(fake.POOL_NAME) - mock_get_aggregate_node.assert_called_once_with('fake_pool') - self.assertEqual(2, len(result)) - self.assertEqual(node, result[0]['home-node']) - - def test_sort_lifs_by_aggregate_locality_node_unknown(self): + def test_get_export_addresses_with_metadata_node_unknown(self): mock_get_aggregate_node = self.mock_object( - self.library, '_get_aggregate_node', mock.Mock(return_value=None)) + self.library, '_get_aggregate_node', + mock.Mock(return_value=None)) - fake_share = copy.deepcopy(fake.SHARE) - fake_share['host'] = 'fake_host@fake_backend#fake_pool' + result = self.library._get_export_addresses_with_metadata( + fake.SHARE, fake.LIFS) - result = self.library._sort_lifs_by_aggregate_locality(fake.SHARE, - fake.LIFS) + expected = copy.deepcopy(fake.INTERFACE_ADDRESSES_WITH_METADATA) + for key, value in expected.items(): + value['preferred'] = None - mock_get_aggregate_node.assert_called_once_with('fake_pool') - self.assertEqual(2, len(result)) - self.assertEqual(fake.LIFS, result) + self.assertEqual(expected, result) + mock_get_aggregate_node.assert_called_once_with(fake.POOL_NAME) + + @ddt.data(True, False) + def test_sort_export_locations_by_preferred_paths(self, reverse): + + export_locations = copy.copy(fake.NFS_EXPORTS) + if reverse: + export_locations.reverse() + + result = self.library._sort_export_locations_by_preferred_paths( + export_locations) + + self.assertEqual(fake.NFS_EXPORTS, result) def test_remove_export(self): diff --git a/manila/tests/share/drivers/netapp/dataontap/fakes.py b/manila/tests/share/drivers/netapp/dataontap/fakes.py index b6f89c5fc7..86fe5cc5cc 100644 --- a/manila/tests/share/drivers/netapp/dataontap/fakes.py +++ b/manila/tests/share/drivers/netapp/dataontap/fakes.py @@ -380,8 +380,33 @@ LIFS = ( }, ) -NFS_EXPORTS = [':'.join([LIF_ADDRESSES[0], 'fake_export_path']), - ':'.join([LIF_ADDRESSES[1], 'fake_export_path'])] +INTERFACE_ADDRESSES_WITH_METADATA = { + LIF_ADDRESSES[0]: { + 'is_admin_only': False, + 'preferred': True, + }, + LIF_ADDRESSES[1]: { + 'is_admin_only': False, + 'preferred': False, + }, +} + +NFS_EXPORTS = [ + { + 'path': ':'.join([LIF_ADDRESSES[0], 'fake_export_path']), + 'is_admin_only': False, + 'metadata': { + 'preferred': True, + }, + }, + { + 'path': ':'.join([LIF_ADDRESSES[1], 'fake_export_path']), + 'is_admin_only': False, + 'metadata': { + 'preferred': False, + }, + }, +] SHARE_ACCESS = { 'access_type': 'user', diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py b/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py index c6e0fab161..7d1b1f0ace 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/test_cifs_cmode.py @@ -49,27 +49,15 @@ class NetAppClusteredCIFSHelperTestCase(test.TestCase): def test_create_share(self): - result = self.helper.create_share(fake.CIFS_SHARE, - fake.SHARE_NAME, - [fake.SHARE_ADDRESS_1]) + result = self.helper.create_share(fake.CIFS_SHARE, fake.SHARE_NAME) - expected = [r'\\%s\%s' % (fake.SHARE_ADDRESS_1, fake.SHARE_NAME)] - self.assertEqual(expected, result) - self.mock_client.create_cifs_share.assert_called_once_with( - fake.SHARE_NAME) - self.mock_client.remove_cifs_share_access.assert_called_once_with( - fake.SHARE_NAME, 'Everyone') - - def test_create_share_multiple(self): - - result = self.helper.create_share(fake.CIFS_SHARE, - fake.SHARE_NAME, - [fake.SHARE_ADDRESS_1, - fake.SHARE_ADDRESS_2]) - - expected = [r'\\%s\%s' % (fake.SHARE_ADDRESS_1, fake.SHARE_NAME), - r'\\%s\%s' % (fake.SHARE_ADDRESS_2, fake.SHARE_NAME)] - self.assertEqual(expected, result) + export_addresses = [fake.SHARE_ADDRESS_1, fake.SHARE_ADDRESS_2] + export_paths = [result(address) for address in export_addresses] + expected_paths = [ + r'\\%s\%s' % (fake.SHARE_ADDRESS_1, fake.SHARE_NAME), + r'\\%s\%s' % (fake.SHARE_ADDRESS_2, fake.SHARE_NAME), + ] + self.assertEqual(expected_paths, export_paths) self.mock_client.create_cifs_share.assert_called_once_with( fake.SHARE_NAME) self.mock_client.remove_cifs_share_access.assert_called_once_with( diff --git a/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py b/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py index 477bfbc3ab..0c12b5bd5c 100644 --- a/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py +++ b/manila/tests/share/drivers/netapp/dataontap/protocols/test_nfs_cmode.py @@ -47,35 +47,19 @@ class NetAppClusteredNFSHelperTestCase(test.TestCase): self.mock_client.get_volume_junction_path.return_value = ( fake.NFS_SHARE_PATH) - result = self.helper.create_share(fake.NFS_SHARE, - fake.SHARE_NAME, - [fake.SHARE_ADDRESS_1]) + result = self.helper.create_share(fake.NFS_SHARE, fake.SHARE_NAME) - expected = [':'.join([fake.SHARE_ADDRESS_1, fake.NFS_SHARE_PATH])] - self.assertEqual(expected, result) + export_addresses = [fake.SHARE_ADDRESS_1, fake.SHARE_ADDRESS_2] + export_paths = [result(address) for address in export_addresses] + expected_paths = [ + fake.SHARE_ADDRESS_1 + ":" + fake.NFS_SHARE_PATH, + fake.SHARE_ADDRESS_2 + ":" + fake.NFS_SHARE_PATH, + ] + self.assertEqual(expected_paths, export_paths) (self.mock_client.clear_nfs_export_policy_for_volume. assert_called_once_with(fake.SHARE_NAME)) self.assertTrue(mock_ensure_export_policy.called) - def test_create_share_multiple(self): - - mock_ensure_export_policy = self.mock_object(self.helper, - '_ensure_export_policy') - self.mock_client.get_volume_junction_path.return_value = ( - fake.NFS_SHARE_PATH) - - result = self.helper.create_share(fake.NFS_SHARE, - fake.SHARE_NAME, - [fake.SHARE_ADDRESS_1, - fake.SHARE_ADDRESS_2]) - - expected = [':'.join([fake.SHARE_ADDRESS_1, fake.NFS_SHARE_PATH]), - ':'.join([fake.SHARE_ADDRESS_2, fake.NFS_SHARE_PATH])] - self.assertEqual(expected, result) - self.mock_client.clear_nfs_export_policy_for_volume.\ - assert_called_once_with(fake.SHARE_NAME) - self.assertTrue(mock_ensure_export_policy.called) - def test_delete_share(self): self.helper.delete_share(fake.NFS_SHARE, fake.SHARE_NAME)