From c162c4540436f54af91b2f42a9138bb45bc88042 Mon Sep 17 00:00:00 2001 From: Valeriy Ponomaryov Date: Tue, 6 Dec 2016 16:22:35 +0300 Subject: [PATCH] [Generic driver] Fix generation of admin export location For managed and created from snapshot shares. Change-Id: I6e7eca54725d1aef994d617f253e73909a9d8303 Closes-Bug: #1646097 --- manila/share/drivers/generic.py | 92 ++++++---------------- manila/share/drivers/helpers.py | 80 ++++++++++++------- manila/share/drivers/lvm.py | 16 ++-- manila/tests/share/drivers/test_generic.py | 36 ++------- manila/tests/share/drivers/test_helpers.py | 78 +++++++++++++----- manila/tests/share/drivers/test_lvm.py | 17 ++-- 6 files changed, 157 insertions(+), 162 deletions(-) diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index 31ee58b7d7..e94ae6ab8e 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -220,46 +220,26 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): @ensure_server def create_share(self, context, share, share_server=None): """Creates share.""" + return self._create_share( + context, share, + snapshot=None, + share_server=share_server, + ) + + def _create_share(self, context, share, snapshot, share_server=None): helper = self._get_helper(share) server_details = share_server['backend_details'] - volume = self._allocate_container(self.admin_context, share) + volume = self._allocate_container( + self.admin_context, share, snapshot=snapshot) volume = self._attach_volume( - self.admin_context, - share, - server_details['instance_id'], - volume) - self._format_device(server_details, volume) + self.admin_context, share, server_details['instance_id'], volume) + if not snapshot: + self._format_device(server_details, volume) + self._mount_device(share, server_details, volume) - location = helper.create_export( - server_details, - share['name']) - export_list = [{ - "path": location, - "is_admin_only": False, - "metadata": { - # TODO(vponomaryov): remove this fake metadata when - # proper appears. - "export_location_metadata_example": "example", - }, - }] - # NOTE(vponomaryov): 'admin_ip' exists only in case of DHSS=True mode. - # 'ip' exists in case of DHSS=False mode. - # Use one of these for creation of export location for service needs. - service_address = server_details.get( - "admin_ip", server_details.get("ip")) - if service_address: - admin_location = location.replace( - server_details['public_address'], service_address) - export_list.append({ - "path": admin_location, - "is_admin_only": True, - "metadata": { - # TODO(vponomaryov): remove this fake metadata when - # proper appears. - "export_location_metadata_example": "example", - }, - }) - return export_list + export_locations = helper.create_exports( + server_details, share['name']) + return export_locations @utils.retry(exception.ProcessExecutionError, backoff_rate=1) def _is_device_file_available(self, server_details, volume): @@ -644,37 +624,11 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): def create_share_from_snapshot(self, context, share, snapshot, share_server=None): """Is called to create share from snapshot.""" - helper = self._get_helper(share) - server_details = share_server['backend_details'] - volume = self._allocate_container(self.admin_context, share, snapshot) - volume = self._attach_volume( - self.admin_context, share, - share_server['backend_details']['instance_id'], volume) - self._mount_device(share, share_server['backend_details'], volume) - location = helper.create_export(share_server['backend_details'], - share['name']) - export_list = [{ - "path": location, - "is_admin_only": False, - "metadata": { - # TODO(vponomaryov): remove this fake metadata when - # proper appears. - "export_location_metadata_example": "example", - }, - }] - if server_details.get('admin_ip'): - admin_location = location.replace( - server_details['public_address'], server_details['admin_ip']) - export_list.append({ - "path": admin_location, - "is_admin_only": True, - "metadata": { - # TODO(vponomaryov): remove this fake metadata when - # proper appears. - "export_location_metadata_example": "example", - }, - }) - return export_list + return self._create_share( + context, share, + snapshot=snapshot, + share_server=share_server, + ) @ensure_server def extend_share(self, share, new_size, share_server=None): @@ -783,7 +737,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): if not self.driver_handles_share_servers: share_server = self.service_instance_manager.get_common_server() if self._is_share_server_active(context, share_server): - helper.remove_export( + helper.remove_exports( share_server['backend_details'], share['name']) self._unmount_device(share, share_server['backend_details']) self._detach_volume(self.admin_context, share, @@ -869,7 +823,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): share_server['backend_details']['instance_id'], volume) self._mount_device(share, share_server['backend_details'], volume) - helper.create_export( + helper.create_exports( share_server['backend_details'], share['name'], recreate=True) @ensure_server diff --git a/manila/share/drivers/helpers.py b/manila/share/drivers/helpers.py index 068f990fbe..a134baf95f 100644 --- a/manila/share/drivers/helpers.py +++ b/manila/share/drivers/helpers.py @@ -37,12 +37,12 @@ class NASHelperBase(object): def init_helper(self, server): pass - def create_export(self, server, share_name, recreate=False): - """Create new export, delete old one if exists.""" + def create_exports(self, server, share_name, recreate=False): + """Create new exports, delete old ones if exist.""" raise NotImplementedError() - def remove_export(self, server, share_name): - """Remove export.""" + def remove_exports(self, server, share_name): + """Remove exports.""" raise NotImplementedError() def configure_access(self, server, share_name): @@ -80,10 +80,43 @@ class NASHelperBase(object): raise exception.ManilaException( _("Can not get 'public_address' for generation of export.")) - def get_exports_for_share(self, server, old_export_location): - """Returns list of exports based on server info.""" + def _get_export_location_template(self, export_location_or_path): + """Returns template of export location. + + Example for NFS: + %s:/path/to/share + Example for CIFS: + \\\\%s\\cifs_share_name + """ raise NotImplementedError() + def get_exports_for_share(self, server, export_location_or_path): + """Returns list of exports based on server info.""" + self._verify_server_has_public_address(server) + export_location_template = self._get_export_location_template( + export_location_or_path) + export_locations = [] + + # NOTE(vponomaryov): + # Generic driver case: 'admin_ip' exists only in case of DHSS=True + # mode and 'ip' exists in case of DHSS=False mode. + # Use one of these for creation of export location for service needs. + # LVM driver will have only single export location. + service_address = server.get("admin_ip", server.get("ip")) + for ip, is_admin in ((server['public_address'], False), + (service_address, True)): + if ip: + export_locations.append({ + "path": export_location_template % ip, + "is_admin_only": is_admin, + "metadata": { + # TODO(vponomaryov): remove this fake metadata when + # proper appears. + "export_location_metadata_example": "example", + }, + }) + return export_locations + def get_share_path_by_export_location(self, server, export_location): """Returns share path by its export location.""" raise NotImplementedError() @@ -137,11 +170,9 @@ def nfs_synchronized(f): class NFSHelper(NASHelperBase): """Interface to work with share.""" - def create_export(self, server, share_name, recreate=False): - """Create new export, delete old one if exists.""" - return ':'.join((server['public_address'], - os.path.join( - self.configuration.share_mount_path, share_name))) + def create_exports(self, server, share_name, recreate=False): + path = os.path.join(self.configuration.share_mount_path, share_name) + return self.get_exports_for_share(server, path) def init_helper(self, server): try: @@ -153,8 +184,8 @@ class NFSHelper(NASHelperBase): % server['instance_id']) LOG.error(e.stderr) - def remove_export(self, server, share_name): - """Remove export.""" + def remove_exports(self, server, share_name): + """Remove exports.""" def _get_parsed_access_to(self, access_to): netmask = utils.cidr_to_netmask(access_to) @@ -276,10 +307,9 @@ class NFSHelper(NASHelperBase): self._ssh_exec( server, ['sudo', 'service', 'nfs-kernel-server', 'restart']) - def get_exports_for_share(self, server, old_export_location): - self._verify_server_has_public_address(server) - path = old_export_location.split(':')[-1] - return [':'.join((server['public_address'], path))] + def _get_export_location_template(self, export_location_or_path): + path = export_location_or_path.split(':')[-1] + return '%s:' + path def get_share_path_by_export_location(self, server, export_location): return export_location.split(':')[-1] @@ -321,7 +351,6 @@ class CIFSHelperIPAccess(NASHelperBase): """ def __init__(self, *args): super(CIFSHelperIPAccess, self).__init__(*args) - self.export_format = '\\\\%s\\%s' self.parameters = { 'browseable': 'yes', 'create mask': '0755', @@ -334,7 +363,7 @@ class CIFSHelperIPAccess(NASHelperBase): # This is smoke check that we have required dependency self._ssh_exec(server, ['sudo', 'net', 'conf', 'list']) - def create_export(self, server, share_name, recreate=False): + def create_exports(self, server, share_name, recreate=False): """Create share at samba server.""" share_path = os.path.join(self.configuration.share_mount_path, share_name) @@ -374,9 +403,9 @@ class CIFSHelperIPAccess(NASHelperBase): self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, param, value]) - return self.export_format % (server['public_address'], share_name) + return self.get_exports_for_share(server, '\\\\%s\\' + share_name) - def remove_export(self, server, share_name): + def remove_exports(self, server, share_name): """Remove share definition from samba server.""" try: self._ssh_exec( @@ -426,12 +455,10 @@ class CIFSHelperIPAccess(NASHelperBase): msg = _("Got incorrect CIFS export location '%s'.") % export_location raise exception.InvalidShare(reason=msg) - def get_exports_for_share(self, server, old_export_location): - self._verify_server_has_public_address(server) + def _get_export_location_template(self, export_location_or_path): group_name = self._get_share_group_name_from_export_location( - old_export_location) - data = dict(ip=server['public_address'], share=group_name) - return ['\\\\%(ip)s\\%(share)s' % data] + export_location_or_path) + return ('\\\\%s' + ('\\%s' % group_name)) def get_share_path_by_export_location(self, server, export_location): # Get name of group that contains share data on CIFS server @@ -473,7 +500,6 @@ class CIFSHelperUserAccess(CIFSHelperIPAccess): """ def __init__(self, *args): super(CIFSHelperUserAccess, self).__init__(*args) - self.export_format = '//%s/%s' self.parameters = { 'browseable': 'yes', 'create mask': '0755', diff --git a/manila/share/drivers/lvm.py b/manila/share/drivers/lvm.py index b0b1035c79..cac19c99b6 100644 --- a/manila/share/drivers/lvm.py +++ b/manila/share/drivers/lvm.py @@ -210,8 +210,8 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): self._allocate_container(share) # create file system device_name = self._get_local_path(share) - location = self._get_helper(share).create_export(self.share_server, - share['name']) + location = self._get_helper(share).create_exports( + self.share_server, share['name']) self._mount_device(share, device_name) return location @@ -226,8 +226,8 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): ) self._copy_volume( snapshot_device_name, share_device_name, share['size']) - location = self._get_helper(share).create_export(self.share_server, - share['name']) + location = self._get_helper(share).create_exports( + self.share_server, share['name']) self._mount_device(share, share_device_name) return location @@ -258,14 +258,14 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver): """Ensure that storage are mounted and exported.""" device_name = self._get_local_path(share) self._mount_device(share, device_name) - self._get_helper(share).create_export(self.share_server, share['name'], - recreate=True) + self._get_helper(share).create_exports( + self.share_server, share['name'], recreate=True) def _delete_share(self, ctx, share): """Delete a share.""" try: - self._get_helper(share).remove_export(self.share_server, - share['name']) + self._get_helper(share).remove_exports( + self.share_server, share['name']) except exception.ProcessExecutionError: LOG.warning(_LW("Can't remove share %r"), share['id']) except exception.InvalidShare as exc: diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index 3b2849a20c..119fc7066c 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -317,35 +317,19 @@ class GenericShareDriverTestCase(test.TestCase): def test_create_share(self): volume = 'fake_volume' volume2 = 'fake_volume2' - location = ( - '%s:/fake/path' % self.server['backend_details']['public_address']) - self._helper_nfs.create_export.return_value = location self.mock_object(self._driver, '_allocate_container', mock.Mock(return_value=volume)) self.mock_object(self._driver, '_attach_volume', mock.Mock(return_value=volume2)) self.mock_object(self._driver, '_format_device') self.mock_object(self._driver, '_mount_device') - expected_el = [ - {'is_admin_only': False, - 'path': location, - 'metadata': {'export_location_metadata_example': 'example'}}, - {'is_admin_only': True, - 'path': location.replace( - self.server['backend_details']['public_address'], - self.server['backend_details']['ip']), - 'metadata': {'export_location_metadata_example': 'example'}}, - ] result = self._driver.create_share( self._context, self.share, share_server=self.server) - self.assertIsInstance(result, list) - self.assertEqual(2, len(result)) - for el in expected_el: - self.assertIn(el, result) + self.assertEqual(self._helper_nfs.create_exports.return_value, result) self._driver._allocate_container.assert_called_once_with( - self._driver.admin_context, self.share) + self._driver.admin_context, self.share, snapshot=None) self._driver._attach_volume.assert_called_once_with( self._driver.admin_context, self.share, self.server['backend_details']['instance_id'], @@ -990,12 +974,6 @@ class GenericShareDriverTestCase(test.TestCase): def test_create_share_from_snapshot(self): vol1 = 'fake_vol1' vol2 = 'fake_vol2' - self._helper_nfs.create_export.return_value = 'fakelocation' - expected_el = [{ - 'is_admin_only': False, - 'path': 'fakelocation', - 'metadata': {'export_location_metadata_example': 'example'}, - }] self.mock_object(self._driver, '_allocate_container', mock.Mock(return_value=vol1)) self.mock_object(self._driver, '_attach_volume', @@ -1008,15 +986,15 @@ class GenericShareDriverTestCase(test.TestCase): self.snapshot, share_server=self.server) - self.assertEqual(expected_el, result) + self.assertEqual(self._helper_nfs.create_exports.return_value, result) self._driver._allocate_container.assert_called_once_with( - self._driver.admin_context, self.share, self.snapshot) + self._driver.admin_context, self.share, snapshot=self.snapshot) self._driver._attach_volume.assert_called_once_with( self._driver.admin_context, self.share, self.server['backend_details']['instance_id'], vol1) self._driver._mount_device.assert_called_once_with( self.share, self.server['backend_details'], vol2) - self._helper_nfs.create_export.assert_called_once_with( + self._helper_nfs.create_exports.assert_called_once_with( self.server['backend_details'], self.share['name']) def test_create_share_from_snapshot_invalid_helper(self): @@ -1055,7 +1033,7 @@ class GenericShareDriverTestCase(test.TestCase): self._driver.delete_share( self._context, self.share, share_server=self.server) - self._helper_nfs.remove_export.assert_called_once_with( + self._helper_nfs.remove_exports.assert_called_once_with( self.server['backend_details'], self.share['name']) self._driver._unmount_device.assert_called_once_with( self.share, self.server['backend_details']) @@ -1214,7 +1192,7 @@ class GenericShareDriverTestCase(test.TestCase): self.server['backend_details']['instance_id'], vol1) self._driver._mount_device.assert_called_once_with( self.share, self.server['backend_details'], vol2) - self._helper_nfs.create_export.assert_called_once_with( + self._helper_nfs.create_exports.assert_called_once_with( self.server['backend_details'], self.share['name'], recreate=True) def test_ensure_share_volume_is_absent(self): diff --git a/manila/tests/share/drivers/test_helpers.py b/manila/tests/share/drivers/test_helpers.py index e0a4b0f94c..256242e650 100644 --- a/manila/tests/share/drivers/test_helpers.py +++ b/manila/tests/share/drivers/test_helpers.py @@ -80,12 +80,28 @@ class NFSHelperTestCase(test.TestCase): self._helper._ssh_exec.assert_called_once_with( self.server, ['sudo', 'exportfs']) - def test_create_export(self): - ret = self._helper.create_export(self.server, self.share_name) - expected_location = ':'.join([self.server['public_address'], - os.path.join(CONF.share_mount_path, - self.share_name)]) - self.assertEqual(expected_location, ret) + @ddt.data( + {"public_address": "1.2.3.4"}, + {"public_address": "1.2.3.4", "admin_ip": "5.6.7.8"}, + {"public_address": "1.2.3.4", "ip": "9.10.11.12"}, + ) + def test_create_exports(self, server): + result = self._helper.create_exports(server, self.share_name) + + expected_export_locations = [] + path = os.path.join(CONF.share_mount_path, self.share_name) + service_address = server.get("admin_ip", server.get("ip")) + for ip, is_admin in ((server['public_address'], False), + (service_address, True)): + if ip: + expected_export_locations.append({ + "path": "%s:%s" % (ip, path), + "is_admin_only": is_admin, + "metadata": { + "export_location_metadata_example": "example", + }, + }) + self.assertEqual(expected_export_locations, result) @ddt.data(const.ACCESS_LEVEL_RW, const.ACCESS_LEVEL_RO) def test_update_access(self, access_level): @@ -206,7 +222,12 @@ class NFSHelperTestCase(test.TestCase): result = self._helper.get_exports_for_share(server, export_location) path = export_location.split(':')[-1] - self.assertEqual([':'.join([server['public_address'], path])], result) + expected_export_locations = [ + {"is_admin_only": False, + "path": "%s:%s" % (server["public_address"], path), + "metadata": {"export_location_metadata_example": "example"}} + ] + self.assertEqual(expected_export_locations, result) @ddt.data( {'public_address_with_suffix': 'foo'}, @@ -307,9 +328,14 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self.mock_object(self._helper, '_ssh_exec', mock.Mock(side_effect=fake_ssh_exec)) - ret = self._helper.create_export(self.server_details, self.share_name) - expected_location = '\\\\%s\\%s' % ( - self.server_details['public_address'], self.share_name) + ret = self._helper.create_exports(self.server_details, self.share_name) + + expected_location = [{ + "is_admin_only": False, + "path": "\\\\%s\\%s" % ( + self.server_details['public_address'], self.share_name), + "metadata": {"export_location_metadata_example": "example"} + }] self.assertEqual(expected_location, ret) share_path = os.path.join( self._helper.configuration.share_mount_path, @@ -338,14 +364,19 @@ class CIFSHelperIPAccessTestCase(test.TestCase): )) self.assertRaises( - exception.ManilaException, self._helper.create_export, + exception.ManilaException, self._helper.create_exports, self.server_details, self.share_name) - def test_create_export_share_exist_recreate_true(self): - ret = self._helper.create_export(self.server_details, self.share_name, - recreate=True) - expected_location = '\\\\%s\\%s' % ( - self.server_details['public_address'], self.share_name) + def test_create_exports_share_exist_recreate_true(self): + ret = self._helper.create_exports( + self.server_details, self.share_name, recreate=True) + + expected_location = [{ + "is_admin_only": False, + "path": "\\\\%s\\%s" % ( + self.server_details['public_address'], self.share_name), + "metadata": {"export_location_metadata_example": "example"} + }] self.assertEqual(expected_location, ret) share_path = os.path.join( self._helper.configuration.share_mount_path, @@ -372,7 +403,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase): def test_create_export_share_exist_recreate_false(self): self.assertRaises( exception.ShareBackendException, - self._helper.create_export, + self._helper.create_exports, self.server_details, self.share_name, recreate=False, @@ -384,8 +415,9 @@ class CIFSHelperIPAccessTestCase(test.TestCase): ), ]) - def test_remove_export(self): - self._helper.remove_export(self.server_details, self.share_name) + def test_remove_exports(self): + self._helper.remove_exports(self.server_details, self.share_name) + self._helper._ssh_exec.assert_called_once_with( self.server_details, ['sudo', 'net', 'conf', 'delshare', self.share_name], @@ -403,7 +435,7 @@ class CIFSHelperIPAccessTestCase(test.TestCase): self.mock_object(self._helper, '_ssh_exec', mock.Mock(side_effect=fake_ssh_exec)) - self._helper.remove_export(self.server_details, self.share_name) + self._helper.remove_exports(self.server_details, self.share_name) self._helper._ssh_exec.assert_has_calls([ mock.call( @@ -484,7 +516,11 @@ class CIFSHelperIPAccessTestCase(test.TestCase): result = self._helper.get_exports_for_share(server, export_location) - expected_export_location = ['\\\\%s\\foo' % server['public_address']] + expected_export_location = [{ + "is_admin_only": False, + "path": "\\\\%s\\foo" % server['public_address'], + "metadata": {"export_location_metadata_example": "example"} + }] self.assertEqual(expected_export_location, result) self._helper._get_share_group_name_from_export_location.\ assert_called_once_with(export_location) diff --git a/manila/tests/share/drivers/test_lvm.py b/manila/tests/share/drivers/test_lvm.py index 538313eafe..e0b492d2f5 100644 --- a/manila/tests/share/drivers/test_lvm.py +++ b/manila/tests/share/drivers/test_lvm.py @@ -166,11 +166,12 @@ class LVMShareDriverTestCase(test.TestCase): self.assertEqual('/dev/mapper/fake--vg-fake--sharename', ret) def test_create_share(self): - self._helper_nfs.create_export.return_value = 'fakelocation' + CONF.set_default('lvm_share_mirrors', 0) self._driver._mount_device = mock.Mock() + ret = self._driver.create_share(self._context, self.share, self.share_server) - CONF.set_default('lvm_share_mirrors', 0) + self._driver._mount_device.assert_called_with( self.share, '/dev/mapper/fakevg-fakename') expected_exec = [ @@ -178,7 +179,7 @@ class LVMShareDriverTestCase(test.TestCase): 'mkfs.ext4 /dev/mapper/fakevg-fakename', ] self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) - self.assertEqual('fakelocation', ret) + self.assertEqual(self._helper_nfs.create_exports.return_value, ret) def test_create_share_from_snapshot(self): CONF.set_default('lvm_share_mirrors', 0) @@ -209,13 +210,13 @@ class LVMShareDriverTestCase(test.TestCase): self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) def test_create_share_mirrors(self): - share = fake_share(size='2048') CONF.set_default('lvm_share_mirrors', 2) - self._helper_nfs.create_export.return_value = 'fakelocation' self._driver._mount_device = mock.Mock() + ret = self._driver.create_share(self._context, share, self.share_server) + self._driver._mount_device.assert_called_with( share, '/dev/mapper/fakevg-fakename') expected_exec = [ @@ -223,7 +224,7 @@ class LVMShareDriverTestCase(test.TestCase): 'mkfs.ext4 /dev/mapper/fakevg-fakename', ] self.assertEqual(expected_exec, fake_utils.fake_execute_get_log()) - self.assertEqual('fakelocation', ret) + self.assertEqual(self._helper_nfs.create_exports.return_value, ret) def test_deallocate_container(self): expected_exec = ['lvremove -f fakevg/fakename'] @@ -335,7 +336,7 @@ class LVMShareDriverTestCase(test.TestCase): self.share_server) self._driver._mount_device.assert_called_with(self.share, device_name) - self._helper_nfs.create_export.assert_called_once_with( + self._helper_nfs.create_exports.assert_called_once_with( self.server, self.share['name'], recreate=True) def test_delete_share(self): @@ -361,7 +362,7 @@ class LVMShareDriverTestCase(test.TestCase): mock.Mock(side_effect=exception.ProcessExecutionError)) self._driver._delete_share(self._context, self.share) - self._helper_nfs.remove_export.assert_called_once_with( + self._helper_nfs.remove_exports.assert_called_once_with( self.server, self.share['name'])