openstack/manila
Code Issues Proposed changes

[Generic driver] Fix generation of admin export location

Browse Source 
For managed and created from snapshot shares.

Change-Id: I6e7eca54725d1aef994d617f253e73909a9d8303
Closes-Bug: #1646097
This commit is contained in:
Valeriy Ponomaryov 2016-12-06 16:22:35 +03:00
parent e8af5394b8
commit c162c45404
6 changed files with 157 additions and 162 deletions

92
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

80
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',

16
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:

36
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):

78
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)

17
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'])