Merge "Implement manage/unmanage support in generic driver"

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}

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'

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."),
 ]
 
 

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)
 

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]
+        return [res]

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

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,
+        LOG.debug("Manage: Found shares %s" % len(shares))
-                        constants.STATUS_UNMANAGED)
+
+        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')

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'))
+            raise exception.ManilaException(
-        return volume
+                _("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()

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,
+            # NOTE(vponomaryov): set size as 1 because design expects size
-                                 {'status': constants.STATUS_MANAGE_ERROR})
+            # 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

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

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

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}],
+    @ddt.data([{'id': 'fake', 'status': constants.STATUS_MANAGE_ERROR}])
-              [{'id': 'fake', 'status': constants.STATUS_UNMANAGED}],)
     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 = {

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'})

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,
+        fake_volume = {'fake': 'fake'}
-                          self.api.update, self.ctx, '', '')
+        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')

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)