diff --git a/contrib/ci/post_test_hook.sh b/contrib/ci/post_test_hook.sh index 18c283c716..897f3566c9 100755 --- a/contrib/ci/post_test_hook.sh +++ b/contrib/ci/post_test_hook.sh @@ -40,6 +40,9 @@ iniset $BASE/new/tempest/etc/tempest.conf share share_creation_retry_number 2 # Suppress errors in cleanup of resources iniset $BASE/new/tempest/etc/tempest.conf share suppress_errors_in_cleanup True +# Enable manage/unmanage tests +iniset $BASE/new/tempest/etc/tempest.conf share run_manage_unmanage_tests True + # Define whether share drivers handle share servers or not. # Requires defined config option 'driver_handles_share_servers'. MANILA_CONF=${MANILA_CONF:-/etc/manila/manila.conf} diff --git a/contrib/tempest/tempest/api/share/admin/test_share_manage.py b/contrib/tempest/tempest/api/share/admin/test_share_manage.py new file mode 100644 index 0000000000..8f99e9f4a3 --- /dev/null +++ b/contrib/tempest/tempest/api/share/admin/test_share_manage.py @@ -0,0 +1,145 @@ +# Copyright 2015 Mirantis Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from tempest_lib import exceptions as lib_exc # noqa +import testtools # noqa + +from tempest.api.share import base +from tempest.common.utils import data_utils +from tempest import config_share as config +from tempest import test + +CONF = config.CONF + + +class ManageNFSShareTest(base.BaseSharesAdminTest): + protocol = 'nfs' + + # NOTE(vponomaryov): be careful running these tests using generic driver + # because cinder volumes will stay attached to service Nova VM and + # won't be deleted. + + @classmethod + @testtools.skipIf( + CONF.share.multitenancy_enabled, + "Only for driver_handles_share_servers = False driver mode.") + @testtools.skipUnless( + CONF.share.run_manage_unmanage_tests, + "Manage/unmanage tests are disabled.") + def resource_setup(cls): + super(ManageNFSShareTest, cls).resource_setup() + if cls.protocol not in CONF.share.enable_protocols: + message = "%s tests are disabled" % cls.protocol + raise cls.skipException(message) + + # Create share types + cls.st_name = data_utils.rand_name("manage-st-name") + cls.st_name_invalid = data_utils.rand_name("manage-st-name-invalid") + cls.extra_specs = { + 'storage_protocol': CONF.share.storage_protocol, + 'driver_handles_share_servers': False + } + cls.extra_specs_invalid = { + 'storage_protocol': CONF.share.storage_protocol, + 'driver_handles_share_servers': True + } + + __, cls.st = cls.create_share_type( + name=cls.st_name, + cleanup_in_class=True, + extra_specs=cls.extra_specs) + + __, cls.st_invalid = cls.create_share_type( + name=cls.st_name_invalid, + cleanup_in_class=True, + extra_specs=cls.extra_specs_invalid) + + creation_data = {'kwargs': { + 'share_type_id': cls.st['share_type']['id'], + 'share_protocol': cls.protocol, + }} + # Create two shares in parallel + cls.shares = cls.create_shares([creation_data, creation_data]) + + # Load all share data (host, etc.) + __, cls.share1 = cls.shares_client.get_share(cls.shares[0]['id']) + __, cls.share2 = cls.shares_client.get_share(cls.shares[1]['id']) + + # Unmanage shares from manila + cls.shares_client.unmanage_share(cls.share1['id']) + cls.shares_client.wait_for_resource_deletion(share_id=cls.share1['id']) + cls.shares_client.unmanage_share(cls.share2['id']) + cls.shares_client.wait_for_resource_deletion(share_id=cls.share2['id']) + + @test.attr(type=["gate", "smoke"]) + def test_manage(self): + # manage share + resp, share = self.shares_client.manage_share( + service_host=self.share1['host'], + export_path=self.share1['export_locations'][0], + protocol=self.share1['share_proto'], + share_type_id=self.st['share_type']['id']) + self.assertIn(int(resp["status"]), self.HTTP_SUCCESS) + + # Add managed share to cleanup queue + self.method_resources.insert( + 0, {'type': 'share_type', 'id': share['id'], + 'client': self.shares_client}) + + # Wait for success + self.shares_client.wait_for_share_status(share['id'], 'available') + + # delete share + resp, __ = self.shares_client.delete_share(share['id']) + self.assertIn(int(resp["status"]), self.HTTP_SUCCESS) + self.shares_client.wait_for_resource_deletion(share_id=share['id']) + self.assertRaises(lib_exc.NotFound, + self.shares_client.get_share, + share['id']) + + @test.attr(type=["gate", "smoke"]) + def test_manage_retry(self): + # manage share with invalid parameters + share = None + parameters = [(self.st_invalid['share_type']['id'], 'MANAGE_ERROR'), + (self.st['share_type']['id'], 'available')] + + for share_type_id, status in parameters: + resp, share = self.shares_client.manage_share( + service_host=self.share2['host'], + export_path=self.share2['export_locations'][0], + protocol=self.share2['share_proto'], + share_type_id=share_type_id) + self.assertIn(int(resp["status"]), self.HTTP_SUCCESS) + + # Add managed share to cleanup queue + self.method_resources.insert( + 0, {'type': 'share_type', 'id': share['id'], + 'client': self.shares_client}) + + # Wait for success + self.shares_client.wait_for_share_status(share['id'], status) + + # delete share + resp, __ = self.shares_client.delete_share(share['id']) + self.assertIn(int(resp["status"]), self.HTTP_SUCCESS) + self.shares_client.wait_for_resource_deletion(share_id=share['id']) + self.assertRaises(lib_exc.NotFound, + self.shares_client.get_share, + share['id']) + + +class ManageCIFSShareTest(ManageNFSShareTest): + protocol = 'cifs' diff --git a/contrib/tempest/tempest/config_share.py b/contrib/tempest/tempest/config_share.py index b58bacfd5f..ce75769d60 100644 --- a/contrib/tempest/tempest/config_share.py +++ b/contrib/tempest/tempest/config_share.py @@ -108,6 +108,11 @@ ShareGroup = [ help="Whether to suppress errors with clean up operation " "or not. There are cases when we may want to skip " "such errors and catch only test errors."), + cfg.BoolOpt("run_manage_unmanage_tests", + default=False, + help="Defines whether to run manage/unmanage tests or not. " + "These test may leave orphaned resources, so be careful " + "enabling this opt."), ] diff --git a/contrib/tempest/tempest/services/share/json/shares_client.py b/contrib/tempest/tempest/services/share/json/shares_client.py index 95a3be62b2..7a9ee79d95 100644 --- a/contrib/tempest/tempest/services/share/json/shares_client.py +++ b/contrib/tempest/tempest/services/share/json/shares_client.py @@ -84,6 +84,23 @@ class SharesClient(service_client.ServiceClient): def delete_share(self, share_id): return self.delete("shares/%s" % share_id) + def manage_share(self, service_host, protocol, export_path, + share_type_id): + post_body = { + "share": { + "export_path": export_path, + "service_host": service_host, + "protocol": protocol, + "share_type": share_type_id, + } + } + body = json.dumps(post_body) + resp, body = self.post("os-share-manage", body) + return resp, self._parse_resp(body) + + def unmanage_share(self, share_id): + return self.post("os-share-unmanage/%s/unmanage" % share_id, None) + def list_shares(self, detailed=False, params=None): """Get list of shares w/o filters.""" uri = 'shares/detail' if detailed else 'shares' @@ -176,7 +193,7 @@ class SharesClient(service_client.ServiceClient): share_status = body['status'] if share_status == status: return - elif 'error' in share_status: + elif 'error' in share_status.lower(): raise share_exceptions.\ ShareBuildErrorException(share_id=share_id) diff --git a/manila/api/contrib/share_manage.py b/manila/api/contrib/share_manage.py index 66c6d42891..0cd92d8a45 100644 --- a/manila/api/contrib/share_manage.py +++ b/manila/api/contrib/share_manage.py @@ -13,11 +13,11 @@ # under the License. import six -import webob from webob import exc from manila.api import extensions from manila.api.openstack import wsgi +from manila.api.views import shares as share_views from manila import exception from manila.i18n import _ from manila import share @@ -29,6 +29,9 @@ authorize = extensions.extension_authorizer('share', 'manage') class ShareManageController(wsgi.Controller): + + _view_builder_class = share_views.ViewBuilder + def __init__(self, *args, **kwargs): super(ShareManageController, self).__init__(*args, **kwargs) self.share_api = share.API() @@ -41,7 +44,7 @@ class ShareManageController(wsgi.Controller): share = { 'host': share_data['service_host'], 'export_location': share_data['export_path'], - 'share_proto': share_data['protocol'], + 'share_proto': share_data['protocol'].upper(), 'share_type_id': share_data['share_type_id'], 'display_name': share_data.get('display_name', ''), 'display_description': share_data.get('display_description', ''), @@ -50,13 +53,13 @@ class ShareManageController(wsgi.Controller): driver_options = share_data.get('driver_options', {}) try: - self.share_api.manage(context, share, driver_options) + share_ref = self.share_api.manage(context, share, driver_options) except exception.PolicyNotAuthorized as e: raise exc.HTTPForbidden(explanation=six.text_type(e)) except exception.ManilaException as e: raise exc.HTTPConflict(explanation=six.text_type(e)) - return webob.Response(status_int=202) + return self._view_builder.detail(req, share_ref) def _validate_manage_parameters(self, context, body): if not (body and self.is_valid_body(body, 'share')): @@ -71,6 +74,9 @@ class ShareManageController(wsgi.Controller): if parameter not in data: msg = _("Required parameter %s not found") % parameter raise exc.HTTPUnprocessableEntity(explanation=msg) + if not data.get(parameter): + msg = _("Required parameter %s is empty") % parameter + raise exc.HTTPUnprocessableEntity(explanation=msg) if not share_utils.extract_host(data['service_host'], 'pool'): msg = _("service_host parameter should contain pool.") @@ -114,4 +120,4 @@ class Share_manage(extensions.ExtensionDescriptor): controller = ShareManageController() res = extensions.ResourceExtension(Share_manage.alias, controller) - return [res] \ No newline at end of file + return [res] diff --git a/manila/exception.py b/manila/exception.py index 7adcc0759e..319637ac71 100644 --- a/manila/exception.py +++ b/manila/exception.py @@ -163,6 +163,10 @@ class InvalidUUID(Invalid): message = _("Expected a uuid but received %(uuid)s.") +class InvalidDriverMode(Invalid): + message = _("Invalid driver mode: %(driver_mode)s.") + + class NotFound(ManilaException): message = _("Resource could not be found.") code = 404 diff --git a/manila/share/api.py b/manila/share/api.py index 14e0f9fbfd..88f12db811 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -230,11 +230,13 @@ class API(base.Base): 'project_id': context.project_id, 'status': constants.STATUS_MANAGING, 'scheduled_at': timeutils.utcnow(), - 'deleted': False, }) - retry_states = (constants.STATUS_MANAGE_ERROR, - constants.STATUS_UNMANAGED) + LOG.debug("Manage: Found shares %s" % len(shares)) + + retry_states = (constants.STATUS_MANAGE_ERROR,) + + export_location = share_data.pop('export_location') if len(shares) == 0: share = self.db.share_create(context, share_data) @@ -246,7 +248,11 @@ class API(base.Base): msg = _("Share already exists.") raise exception.ManilaException(msg) + self.db.share_export_locations_update(context, share['id'], + export_location) + self.share_rpcapi.manage_share(context, share, driver_options) + return self.db.share_get(context, share['id']) def unmanage(self, context, share): policy.check_policy(context, 'share', 'unmanage') diff --git a/manila/share/drivers/generic.py b/manila/share/drivers/generic.py index d75915f5c3..68037db24a 100644 --- a/manila/share/drivers/generic.py +++ b/manila/share/drivers/generic.py @@ -24,6 +24,7 @@ from oslo_config import cfg from oslo_log import log from oslo_utils import excutils from oslo_utils import importutils +from oslo_utils import strutils import six from manila.common import constants as const @@ -35,6 +36,7 @@ from manila.i18n import _LE from manila.i18n import _LW from manila.share import driver from manila.share.drivers import service_instance +from manila.share import share_types from manila import utils from manila import volume @@ -206,9 +208,8 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): volume['mountpoint']] self._ssh_exec(server_details, command) - def _is_device_mounted(self, share, server_details, volume=None): + def _is_device_mounted(self, mount_path, server_details, volume=None): """Checks whether volume already mounted or not.""" - mount_path = self._get_mount_path(share) log_data = { 'mount_path': mount_path, 'server_id': server_details['instance_id'], @@ -274,7 +275,8 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): 'server': server_details['instance_id'], } try: - if not self._is_device_mounted(share, server_details, volume): + if not self._is_device_mounted(mount_path, server_details, + volume): LOG.debug("Mounting '%(dev)s' to path '%(path)s' on " "server '%(server)s'.", log_data) mount_cmd = ['sudo mkdir -p', mount_path, '&&'] @@ -303,7 +305,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): 'path': mount_path, 'server': server_details['instance_id'], } - if self._is_device_mounted(share, server_details): + if self._is_device_mounted(mount_path, server_details): LOG.debug("Unmounting path '%(path)s' on server " "'%(server)s'.", log_data) unmount_cmd = ['sudo umount', mount_path, '&& sudo rmdir', @@ -354,24 +356,28 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): self.configuration.max_time_to_attach) return do_attach(volume) + def _get_volume_name(self, share_id): + return self.configuration.volume_name_template % share_id + def _get_volume(self, context, share_id): """Finds volume, associated to the specific share.""" - volume_name = self.configuration.volume_name_template % share_id + volume_name = self._get_volume_name(share_id) search_opts = {'name': volume_name} if context.is_admin: search_opts['all_tenants'] = True volumes_list = self.volume_api.get_all(context, search_opts) - volume = None if len(volumes_list) == 1: - volume = volumes_list[0] + return volumes_list[0] elif len(volumes_list) > 1: LOG.error( _LE("Expected only one volume in volume list with name " "'%(name)s', but got more than one in a result - " "'%(result)s'."), { 'name': volume_name, 'result': volumes_list}) - raise exception.ManilaException(_('Error. Ambiguous volumes')) - return volume + raise exception.ManilaException( + _("Error. Ambiguous volumes for name '%s'") % volume_name) + else: + raise exception.VolumeNotFound(volume_id=volume_name) def _get_volume_snapshot(self, context, snapshot_id): """Find volume snapshot associated to the specific share snapshot.""" @@ -633,6 +639,107 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver): self.service_instance_manager.delete_service_instance( self.admin_context, server_details) + def manage_existing(self, share, driver_options): + """Manage existing share to manila. + + Generic driver accepts only one driver_option 'volume_id'. + If an administrator provides this option, then appropriate Cinder + volume will be managed by Manila as well. + + :param share: share data + :param driver_options: Empty dict or dict with 'volume_id' option. + :return: dict with share size, example: {'size': 1} + """ + if self.driver_handles_share_servers: + msg = _('Operation "manage" for shares is supported only when ' + 'driver does not handle share servers.') + raise exception.InvalidDriverMode(msg) + + helper = self._get_helper(share) + driver_mode = share_types.get_share_type_extra_specs( + share['share_type_id'], + const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS) + + if strutils.bool_from_string(driver_mode): + msg = _("%(mode)s != False") % { + 'mode': const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS + } + raise exception.ManageExistingShareTypeMismatch(reason=msg) + + share_server = self.service_instance_manager.get_common_server() + server_details = share_server['backend_details'] + + mount_path = helper.get_share_path_by_export_location( + share_server['backend_details'], share['export_locations'][0]) + LOG.debug("Manage: mount path = %s", mount_path) + + mounted = self._is_device_mounted(mount_path, server_details) + LOG.debug("Manage: is share mounted = %s", mounted) + + if not mounted: + msg = _("Provided share %s is not mounted.") % share['id'] + raise exception.ManageInvalidShare(reason=msg) + + def get_volume(): + if 'volume_id' in driver_options: + try: + return self.volume_api.get( + self.admin_context, driver_options['volume_id']) + except exception.VolumeNotFound as e: + raise exception.ManageInvalidShare(reason=six.text_type(e)) + + # NOTE(vponomaryov): Manila can only combine volume name by itself, + # nowhere to get volume ID from. Return None since Cinder volume + # names are not unique or fixed, hence, they can not be used for + # sure. + return None + + share_volume = get_volume() + + if share_volume: + instance_volumes = self.compute_api.instance_volumes_list( + self.admin_context, server_details['instance_id']) + + attached_volumes = [vol.id for vol in instance_volumes] + LOG.debug('Manage: attached volumes = %s', + six.text_type(attached_volumes)) + + if share_volume['id'] not in attached_volumes: + msg = _("Provided volume %s is not attached " + "to service instance.") % share_volume['id'] + raise exception.ManageInvalidShare(reason=msg) + + linked_volume_name = self._get_volume_name(share['id']) + if share_volume['name'] != linked_volume_name: + LOG.debug('Manage: volume_id = %s' % share_volume['id']) + self.volume_api.update(self.admin_context, share_volume['id'], + {'name': linked_volume_name}) + + share_size = share_volume['size'] + else: + share_size = self._get_mounted_share_size( + mount_path, share_server['backend_details']) + + # TODO(vponomaryov): need update export_locations too using driver's + # information. + return {'size': share_size} + + def _get_mounted_share_size(self, mount_path, server_details): + share_size_cmd = ['df', '-PBG', mount_path] + output, __ = self._ssh_exec(server_details, share_size_cmd) + lines = output.split('\n') + + try: + size = int(lines[1].split()[1][:-1]) + except Exception as e: + msg = _("Cannot calculate size of share %(path)s : %(error)s") % { + 'path': mount_path, + 'error': six.text_type(e) + } + raise exception.ManageInvalidShare(reason=msg) + + return size + class NASHelperBase(object): """Interface to work with share.""" @@ -662,6 +769,10 @@ class NASHelperBase(object): """Deny access to the host.""" raise NotImplementedError() + def get_share_path_by_export_location(self, server, export_location): + """Returns share path by its export location.""" + raise NotImplementedError() + def nfs_synchronized(f): @@ -745,6 +856,9 @@ class NFSHelper(NASHelperBase): ] self._ssh_exec(server, sync_cmd) + def get_share_path_by_export_location(self, server, export_location): + return export_location.split(':')[-1] + class CIFSHelper(NASHelperBase): """Manage shares in samba server by net conf tool. @@ -855,3 +969,21 @@ class CIFSHelper(NASHelperBase): value = "\"" + ' '.join(hosts) + "\"" self._ssh_exec(server, ['sudo', 'net', 'conf', 'setparm', share_name, '\"hosts allow\"', value]) + + def get_share_path_by_export_location(self, server, export_location): + # Get name of group that contains share data on CIFS server + if export_location.startswith('\\\\'): + group_name = export_location.split('\\')[-1] + elif export_location.startswith('//'): + group_name = export_location.split('/')[-1] + else: + msg = _("Got incorrect CIFS export location " + "'%s'.") % export_location + raise exception.InvalidShare(reason=msg) + + # Get parameter 'path' from group that belongs to current share + (out, __) = self._ssh_exec( + server, ['sudo', 'net', 'conf', 'getparm', group_name, 'path']) + + # Remove special symbols from response and return path + return out.strip() diff --git a/manila/share/manager.py b/manila/share/manager.py index 0e169981b1..1db690320c 100644 --- a/manila/share/manager.py +++ b/manila/share/manager.py @@ -340,8 +340,7 @@ class ShareManager(manager.SchedulerDependentManager): raise exception.InvalidShare(reason=msg) share_update = ( - self.driver.manage_existing(share_ref, driver_options) or {} - ) + self.driver.manage_existing(share_ref, driver_options) or {}) if not share_update.get('size'): msg = _("Driver cannot calculate share size.") @@ -349,19 +348,22 @@ class ShareManager(manager.SchedulerDependentManager): self._update_quota_usages(context, project_id, { "shares": 1, - "gigabytes": share_update['size'] + "gigabytes": share_update['size'], }) share_update.update({ 'status': 'available', - 'launched_at': timeutils.utcnow() + 'launched_at': timeutils.utcnow(), }) self.db.share_update(context, share_id, share_update) - except Exception as e: LOG.error(_LW("Manage share failed: %s"), six.text_type(e)) - self.db.share_update(context, share_id, - {'status': constants.STATUS_MANAGE_ERROR}) + # NOTE(vponomaryov): set size as 1 because design expects size + # to be set, it also will allow us to handle delete/unmanage + # operations properly with this errored share according to quotas. + self.db.share_update( + context, share_id, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) def _update_quota_usages(self, context, project_id, usages): user_id = context.user_id diff --git a/manila/tests/api/contrib/test_share_manage.py b/manila/tests/api/contrib/test_share_manage.py index ecb856c8c4..0245bc9a7d 100644 --- a/manila/tests/api/contrib/test_share_manage.py +++ b/manila/tests/api/contrib/test_share_manage.py @@ -141,12 +141,14 @@ class ShareManageTest(test.TestCase): def test_share_manage(self): body = get_fake_manage_body() self._setup_manage_mocks() - self.mock_object(share_api.API, 'manage') + share = {'share_type_id': '', 'id': 'fake'} + self.mock_object(share_api.API, 'manage', + mock.Mock(return_value=share)) share = { 'host': body['share']['service_host'], 'export_location': body['share']['export_path'], - 'share_proto': body['share']['protocol'], + 'share_proto': body['share']['protocol'].upper(), 'share_type_id': 'fake', 'display_name': body['share']['display_name'], 'display_description': body['share']['display_description'], @@ -156,7 +158,7 @@ class ShareManageTest(test.TestCase): share_api.API.manage.assert_called_once_with( mock.ANY, share, {}) - self.assertEqual(202, actual_result.status_int) + self.assertIsNotNone(actual_result) def test_wrong_permissions(self): body = get_fake_manage_body() diff --git a/manila/tests/share/drivers/test_generic.py b/manila/tests/share/drivers/test_generic.py index a1ff9244cc..e640692263 100644 --- a/manila/tests/share/drivers/test_generic.py +++ b/manila/tests/share/drivers/test_generic.py @@ -29,6 +29,7 @@ from manila import context from manila import exception import manila.share.configuration from manila.share.drivers import generic +from manila.share import share_types from manila import test from manila.tests import fake_compute from manila.tests import fake_service_instance @@ -41,6 +42,16 @@ from manila import volume CONF = cfg.CONF +def get_fake_manage_share(): + return { + 'id': 'fake', + 'share_proto': 'NFS', + 'share_type_id': 'fake', + 'export_locations': [ + '10.0.0.1:/foo/fake/path', '11.0.0.1:/bar/fake/path'], + } + + @ddt.ddt class GenericShareDriverTestCase(test.TestCase): """Tests GenericShareDriver.""" @@ -185,21 +196,18 @@ class GenericShareDriverTestCase(test.TestCase): def test_mount_device_not_present(self): server = {'instance_id': 'fake_server_id'} - mount_path = '/fake/mount/path' + mount_path = self._driver._get_mount_path(self.share) volume = {'mountpoint': 'fake_mount_point'} self.mock_object(self._driver, '_is_device_mounted', mock.Mock(return_value=False)) self.mock_object(self._driver, '_sync_mount_temp_and_perm_files') - self.mock_object(self._driver, '_get_mount_path', - mock.Mock(return_value=mount_path)) self.mock_object(self._driver, '_ssh_exec', mock.Mock(return_value=('', ''))) self._driver._mount_device(self.share, server, volume) - self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._is_device_mounted.assert_called_once_with( - self.share, server, volume) + mount_path, server, volume) self._driver._sync_mount_temp_and_perm_files.assert_called_once_with( server) self._driver._ssh_exec.assert_called_once_with( @@ -222,13 +230,12 @@ class GenericShareDriverTestCase(test.TestCase): self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._is_device_mounted.assert_called_once_with( - self.share, self.server, volume) + mount_path, self.server, volume) generic.LOG.warning.assert_called_once_with(mock.ANY, mock.ANY) def test_mount_device_exception_raised(self): volume = {'mountpoint': 'fake_mount_point'} - self.mock_object(self._driver, '_get_mount_path', - mock.Mock(return_value='fake')) + self.mock_object( self._driver, '_is_device_mounted', mock.Mock(side_effect=exception.ProcessExecutionError)) @@ -240,9 +247,8 @@ class GenericShareDriverTestCase(test.TestCase): self.server, volume, ) - self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._is_device_mounted.assert_called_once_with( - self.share, self.server, volume) + self._driver._get_mount_path(self.share), self.server, volume) def test_unmount_device_present(self): mount_path = '/fake/mount/path' @@ -258,7 +264,7 @@ class GenericShareDriverTestCase(test.TestCase): self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._is_device_mounted.assert_called_once_with( - self.share, self.server) + mount_path, self.server) self._driver._sync_mount_temp_and_perm_files.assert_called_once_with( self.server) self._driver._ssh_exec.assert_called_once_with( @@ -278,7 +284,7 @@ class GenericShareDriverTestCase(test.TestCase): self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._is_device_mounted.assert_called_once_with( - self.share, self.server) + mount_path, self.server) generic.LOG.warning.assert_called_once_with(mock.ANY, mock.ANY) def test_is_device_mounted_true(self): @@ -288,13 +294,10 @@ class GenericShareDriverTestCase(test.TestCase): 'path': mount_path} self.mock_object(self._driver, '_ssh_exec', mock.Mock(return_value=(mounts, ''))) - self.mock_object(self._driver, '_get_mount_path', - mock.Mock(return_value=mount_path)) result = self._driver._is_device_mounted( - self.share, self.server, volume) + mount_path, self.server, volume) - self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._ssh_exec.assert_called_once_with( self.server, ['sudo', 'mount']) self.assertEqual(result, True) @@ -304,12 +307,9 @@ class GenericShareDriverTestCase(test.TestCase): mounts = "/fake/dev/path on %(path)s type fake" % {'path': mount_path} self.mock_object(self._driver, '_ssh_exec', mock.Mock(return_value=(mounts, ''))) - self.mock_object(self._driver, '_get_mount_path', - mock.Mock(return_value=mount_path)) - result = self._driver._is_device_mounted(self.share, self.server) + result = self._driver._is_device_mounted(mount_path, self.server) - self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._ssh_exec.assert_called_once_with( self.server, ['sudo', 'mount']) self.assertEqual(result, True) @@ -321,13 +321,10 @@ class GenericShareDriverTestCase(test.TestCase): 'path': mount_path} self.mock_object(self._driver, '_ssh_exec', mock.Mock(return_value=(mounts, ''))) - self.mock_object(self._driver, '_get_mount_path', - mock.Mock(return_value=mount_path)) result = self._driver._is_device_mounted( - self.share, self.server, volume) + mount_path, self.server, volume) - self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._ssh_exec.assert_called_once_with( self.server, ['sudo', 'mount']) self.assertEqual(result, False) @@ -340,9 +337,8 @@ class GenericShareDriverTestCase(test.TestCase): self.mock_object(self._driver, '_get_mount_path', mock.Mock(return_value=mount_path)) - result = self._driver._is_device_mounted(self.share, self.server) + result = self._driver._is_device_mounted(mount_path, self.server) - self._driver._get_mount_path.assert_called_once_with(self.share) self._driver._ssh_exec.assert_called_once_with( self.server, ['sudo', 'mount']) self.assertEqual(result, False) @@ -467,8 +463,12 @@ class GenericShareDriverTestCase(test.TestCase): self._driver.configuration.volume_name_template % self.share['id']) self.mock_object(self._driver.volume_api, 'get_all', mock.Mock(return_value=[])) - result = self._driver._get_volume(self._context, self.share['id']) - self.assertEqual(result, None) + + self.assertRaises( + exception.VolumeNotFound, + self._driver._get_volume, + self._context, self.share['id']) + self._driver.volume_api.get_all.assert_called_once_with( self._context, {'all_tenants': True, 'name': vol_name}) @@ -1011,6 +1011,183 @@ class GenericShareDriverTestCase(test.TestCase): self.assertEqual(True, result['driver_handles_share_servers']) self.assertEqual('Open Source', result['vendor_name']) + def test_manage_invalid_driver_mode(self): + CONF.set_default('driver_handles_share_servers', True) + + self.assertRaises(exception.InvalidDriverMode, + self._driver.manage_existing, 'fake', {}) + + def _setup_manage_mocks(self, + get_share_type_extra_specs='False', + is_device_mounted=True, + server_details=None): + CONF.set_default('driver_handles_share_servers', False) + + self.mock_object(share_types, 'get_share_type_extra_specs', + mock.Mock(return_value=get_share_type_extra_specs)) + + self.mock_object(self._driver, '_is_device_mounted', + mock.Mock(return_value=is_device_mounted)) + + self.mock_object(self._driver, 'service_instance_manager') + server = {'backend_details': server_details} + self.mock_object(self._driver.service_instance_manager, + 'get_common_server', + mock.Mock(return_value=server)) + + def test_manage_invalid_protocol(self): + share = {'share_proto': 'fake_proto'} + self._setup_manage_mocks() + + self.assertRaises(exception.InvalidShare, + self._driver.manage_existing, share, {}) + + def test_manage_share_type_mismatch(self): + share = {'share_proto': 'NFS', 'share_type_id': 'fake'} + self._setup_manage_mocks(get_share_type_extra_specs='True') + + self.assertRaises(exception.ManageExistingShareTypeMismatch, + self._driver.manage_existing, share, {}) + + share_types.get_share_type_extra_specs.assert_called_once_with( + share['share_type_id'], + const.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS + ) + + def test_manage_not_mounted_share(self): + share = get_fake_manage_share() + fake_path = '/foo/bar' + self._setup_manage_mocks(is_device_mounted=False) + self.mock_object( + self._driver._helpers[share['share_proto']], + 'get_share_path_by_export_location', + mock.Mock(return_value=fake_path)) + + self.assertRaises(exception.ManageInvalidShare, + self._driver.manage_existing, share, {}) + + self.assertEqual( + 1, + self._driver.service_instance_manager.get_common_server.call_count) + self._driver._is_device_mounted.assert_called_once_with( + fake_path, None) + self._driver._helpers[share['share_proto']].\ + get_share_path_by_export_location.assert_called_once_with( + None, share['export_locations'][0]) + + def test_manage_share_not_attached_to_cinder_volume_invalid_size(self): + share = get_fake_manage_share() + server_details = {} + fake_path = '/foo/bar' + self._setup_manage_mocks(server_details=server_details) + self.mock_object(self._driver, '_get_volume', + mock.Mock(return_value=None)) + error = exception.ManageInvalidShare(reason="fake") + self.mock_object( + self._driver, '_get_mounted_share_size', + mock.Mock(side_effect=error)) + self.mock_object( + self._driver._helpers[share['share_proto']], + 'get_share_path_by_export_location', + mock.Mock(return_value=fake_path)) + + self.assertRaises(exception.ManageInvalidShare, + self._driver.manage_existing, share, {}) + + self._driver._get_mounted_share_size.assert_called_once_with( + fake_path, server_details) + self._driver._helpers[share['share_proto']].\ + get_share_path_by_export_location.assert_called_once_with( + server_details, share['export_locations'][0]) + + def test_manage_share_not_attached_to_cinder_volume(self): + share = get_fake_manage_share() + share_size = "fake" + server_details = {} + self._setup_manage_mocks(server_details=server_details) + self.mock_object(self._driver, '_get_volume', + mock.Mock(return_value=None)) + self.mock_object(self._driver, '_get_mounted_share_size', + mock.Mock(return_value=share_size)) + + actual_result = self._driver.manage_existing(share, {}) + self.assertEqual({'size': share_size}, actual_result) + + def test_manage_share_attached_to_cinder_volume_not_found(self): + share = get_fake_manage_share() + server_details = {} + driver_options = {'volume_id': 'fake'} + self._setup_manage_mocks(server_details=server_details) + self.mock_object( + self._driver.volume_api, 'get', + mock.Mock(side_effect=exception.VolumeNotFound(volume_id="fake")) + ) + + self.assertRaises(exception.ManageInvalidShare, + self._driver.manage_existing, share, driver_options) + + self._driver.volume_api.get.assert_called_once_with( + mock.ANY, driver_options['volume_id']) + + def test_manage_share_attached_to_cinder_volume_not_mounted_to_srv(self): + share = get_fake_manage_share() + server_details = {'instance_id': 'fake'} + driver_options = {'volume_id': 'fake'} + volume = {'id': 'fake'} + self._setup_manage_mocks(server_details=server_details) + self.mock_object(self._driver.volume_api, 'get', + mock.Mock(return_value=volume)) + self.mock_object(self._driver.compute_api, 'instance_volumes_list', + mock.Mock(return_value=[])) + + self.assertRaises(exception.ManageInvalidShare, + self._driver.manage_existing, share, driver_options) + + self._driver.volume_api.get.assert_called_once_with( + mock.ANY, driver_options['volume_id']) + self._driver.compute_api.instance_volumes_list.assert_called_once_with( + mock.ANY, server_details['instance_id']) + + def test_manage_share_attached_to_cinder_volume(self): + share = get_fake_manage_share() + server_details = {'instance_id': 'fake'} + driver_options = {'volume_id': 'fake'} + volume = {'id': 'fake', 'name': 'fake_volume_1', 'size': 'fake'} + self._setup_manage_mocks(server_details=server_details) + self.mock_object(self._driver.volume_api, 'get', + mock.Mock(return_value=volume)) + self._driver.volume_api.update = mock.Mock() + fake_volume = mock.Mock() + fake_volume.id = 'fake' + self.mock_object(self._driver.compute_api, 'instance_volumes_list', + mock.Mock(return_value=[fake_volume])) + + actual_result = self._driver.manage_existing(share, driver_options) + + self.assertEqual({'size': 'fake'}, actual_result) + expected_volume_update = { + 'name': self._driver._get_volume_name(share['id']) + } + self._driver.volume_api.update.assert_called_once_with( + mock.ANY, volume['id'], expected_volume_update) + + def test_get_mounted_share_size(self): + output = ("Filesystem blocks Used Available Capacity Mounted on\n" + "/dev/fake 1G 1G 1G 4% /shares/share-fake") + self.mock_object(self._driver, '_ssh_exec', + mock.Mock(return_value=(output, ''))) + + actual_result = self._driver._get_mounted_share_size('/fake/path', {}) + self.assertEqual(1, actual_result) + + @ddt.data("fake\nfake\n", "fake", "fake\n") + def test_get_mounted_share_size_invalid_output(self, output): + self.mock_object(self._driver, '_ssh_exec', + mock.Mock(return_value=(output, ''))) + self.assertRaises(exception.ManageInvalidShare, + self._driver._get_mounted_share_size, + '/fake/path', {}) + @generic.ensure_server def fake(driver_instance, context, share_server=None): diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 704a807786..6eda46fca3 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -550,6 +550,9 @@ class ShareAPITestCase(test.TestCase): status='creating') self.mock_object(db_driver, 'share_create', mock.Mock(return_value=share)) + self.mock_object(db_driver, 'share_export_locations_update') + self.mock_object(db_driver, 'share_get', + mock.Mock(return_value=share)) self.mock_object(self.api, 'get_all', mock.Mock(return_value=[])) self.api.manage(self.context, @@ -561,17 +564,20 @@ class ShareAPITestCase(test.TestCase): 'project_id': self.context.project_id, 'status': constants.STATUS_MANAGING, 'scheduled_at': date, - 'deleted': False, }) + export_location = share_data.pop('export_location') self.api.get_all.assert_called_once_with(self.context, mock.ANY) db_driver.share_create.assert_called_once_with(self.context, share_data) + db_driver.share_get.assert_called_once_with(self.context, share['id']) + db_driver.share_export_locations_update.assert_called_once_with( + self.context, share['id'], export_location + ) self.share_rpcapi.manage_share.assert_called_once_with( self.context, share, driver_options) - @ddt.data([{'id': 'fake', 'status': constants.STATUS_MANAGE_ERROR}], - [{'id': 'fake', 'status': constants.STATUS_UNMANAGED}],) + @ddt.data([{'id': 'fake', 'status': constants.STATUS_MANAGE_ERROR}]) def test_manage_retry(self, shares): share_data = { 'host': 'fake', @@ -579,8 +585,12 @@ class ShareAPITestCase(test.TestCase): 'share_proto': 'fake', } driver_options = {} + share = fake_share('fakeid') self.mock_object(db_driver, 'share_update', - mock.Mock(return_value=fake_share('fakeid'))) + mock.Mock(return_value=share)) + self.mock_object(db_driver, 'share_get', + mock.Mock(return_value=share)) + self.mock_object(db_driver, 'share_export_locations_update') self.mock_object(self.api, 'get_all', mock.Mock(return_value=shares)) @@ -592,6 +602,9 @@ class ShareAPITestCase(test.TestCase): self.context, 'fake', mock.ANY) self.share_rpcapi.manage_share.assert_called_once_with( self.context, mock.ANY, driver_options) + db_driver.share_export_locations_update.assert_called_once_with( + self.context, share['id'], mock.ANY + ) def test_manage_duplicate(self): share_data = { diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py index ddd0fcf06b..b9254c8669 100644 --- a/manila/tests/share/test_manager.py +++ b/manila/tests/share/test_manager.py @@ -710,8 +710,8 @@ class ShareManagerTestCase(test.TestCase): self.share_manager.manage_share(self.context, share_id, {}) self.share_manager.db.share_update.assert_called_once_with( - mock.ANY, share_id, {'status': constants.STATUS_MANAGE_ERROR} - ) + mock.ANY, share_id, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) def test_manage_share_driver_exception(self): self.mock_object(self.share_manager, 'driver', mock.Mock()) @@ -729,8 +729,8 @@ class ShareManagerTestCase(test.TestCase): assert_called_once_with(mock.ANY, driver_options) self.share_manager.db.share_update.assert_called_once_with( - mock.ANY, share_id, {'status': constants.STATUS_MANAGE_ERROR} - ) + mock.ANY, share_id, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) def test_manage_share_invalid_size(self): self.mock_object(self.share_manager, 'driver') @@ -749,8 +749,8 @@ class ShareManagerTestCase(test.TestCase): assert_called_once_with(mock.ANY, driver_options) self.share_manager.db.share_update.assert_called_once_with( - mock.ANY, share_id, {'status': constants.STATUS_MANAGE_ERROR} - ) + mock.ANY, share_id, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) def test_manage_share_quota_error(self): self.mock_object(self.share_manager, 'driver') @@ -771,8 +771,8 @@ class ShareManagerTestCase(test.TestCase): assert_called_once_with(mock.ANY, driver_options) self.share_manager.db.share_update.assert_called_once_with( - mock.ANY, share_id, {'status': constants.STATUS_MANAGE_ERROR} - ) + mock.ANY, share_id, + {'status': constants.STATUS_MANAGE_ERROR, 'size': 1}) @ddt.data({'size': 1}, {'size': 1, 'name': 'fake'}) diff --git a/manila/tests/volume/test_cinder.py b/manila/tests/volume/test_cinder.py index 27a8d2d6bb..908fc3a230 100644 --- a/manila/tests/volume/test_cinder.py +++ b/manila/tests/volume/test_cinder.py @@ -129,8 +129,18 @@ class CinderApiTestCase(test.TestCase): self.assertIsNone(self.api.check_detach(self.ctx, volume)) def test_update(self): - self.assertRaises(NotImplementedError, - self.api.update, self.ctx, '', '') + fake_volume = {'fake': 'fake'} + self.mock_object(self.cinderclient.volumes, 'get', + mock.Mock(return_value=fake_volume)) + self.mock_object(self.cinderclient.volumes, 'update') + fake_volume_id = 'fake_volume' + fake_data = {'test': 'test'} + + self.api.update(self.ctx, fake_volume_id, fake_data) + + self.cinderclient.volumes.get.assert_called_once_with(fake_volume_id) + self.cinderclient.volumes.update.assert_called_once_with(fake_volume, + **fake_data) def test_reserve_volume(self): self.mock_object(self.cinderclient.volumes, 'reserve') diff --git a/manila/volume/cinder.py b/manila/volume/cinder.py index f9c671da0d..dafd429a8c 100644 --- a/manila/volume/cinder.py +++ b/manila/volume/cinder.py @@ -26,6 +26,7 @@ from cinderclient.v2 import client as cinder_client from oslo_config import cfg from oslo_log import log +import manila.context as ctxt from manila.db import base from manila import exception from manila.i18n import _ @@ -317,7 +318,12 @@ class API(base.Base): @translate_volume_exception def update(self, context, volume_id, fields): - raise NotImplementedError() + # Use Manila's context as far as Cinder's is restricted to update + # volumes. + manila_admin_context = ctxt.get_admin_context() + client = cinderclient(manila_admin_context) + item = client.volumes.get(volume_id) + client.volumes.update(item, **fields) def get_volume_encryption_metadata(self, context, volume_id): return cinderclient(context).volumes.get_encryption_metadata(volume_id)