Merge "Allow drivers to ask for additional share_servers"

manila/db/api.py

@@ -625,14 +625,12 @@ def share_server_get_by_host_and_share_net(context, host, share_net_id,
                                                        session=session)
 
 
-def share_server_get_by_host_and_share_net_valid(context, host,
-                                                 share_net_id,
-                                                 session=None):
+def share_server_get_all_by_host_and_share_net_valid(context, host,
+                                                     share_net_id,
+                                                     session=None):
     """Get share server DB records by host and share net not error."""
-    return IMPL.share_server_get_by_host_and_share_net_valid(context,
-                                                             host,
-                                                             share_net_id,
-                                                             session=session)
+    return IMPL.share_server_get_all_by_host_and_share_net_valid(
+        context, host, share_net_id, session=session)
 
 
 def share_server_get_all(context):

manila/db/sqlalchemy/api.py

@@ -2003,13 +2003,14 @@ def share_server_get(context, server_id, session=None):
 
 
 @require_context
-def share_server_get_by_host_and_share_net_valid(context, host, share_net_id,
-                                                 session=None):
+def share_server_get_all_by_host_and_share_net_valid(context, host,
+                                                     share_net_id,
+                                                     session=None):
     result = _server_get_query(context, session).filter_by(host=host)\
         .filter_by(share_network_id=share_net_id)\
         .filter(models.ShareServer.status.in_(
-            (constants.STATUS_CREATING, constants.STATUS_ACTIVE))).first()
-    if result is None:
+            (constants.STATUS_CREATING, constants.STATUS_ACTIVE))).all()
+    if not result:
         filters_description = ('share_network_id is "%(share_net_id)s",'
                                ' host is "%(host)s" and status in'
                                ' "%(status_cr)s" or "%(status_act)s"') % {

manila/exception.py

@@ -194,6 +194,10 @@ class ShareServerInUse(InUse):
     message = _("Share server %(share_server_id)s is in use.")
 
 
+class InvalidShareServer(Invalid):
+    message = _("Share server %(share_server_id)s is not valid.")
+
+
 class ShareServerNotCreated(ManilaException):
     message = _("Share server %(share_server_id)s failed on creation.")
 

manila/share/driver.py

@@ -327,6 +327,20 @@ class ShareDriver(object):
         if self.get_network_allocations_number():
             self.network_api.deallocate_network(context, share_server_id)
 
+    def choose_share_server_compatible_with_share(self, context, share_servers,
+                                                  share, snapshot=None):
+        """Method that allows driver to choose share server for provided share.
+
+        If compatible share-server is not found, method should return None.
+
+        :param context: Current context
+        :param share_servers: list with share-server models
+        :param share:  share model
+        :param snapshot: snapshot model
+        :returns: share-server or None
+        """
+        return share_servers[0] if share_servers else None
+
     def setup_server(self, *args, **kwargs):
         if self.driver_handles_share_servers:
             return self._setup_server(*args, **kwargs)

manila/share/manager.py

@@ -206,7 +206,7 @@ class ShareManager(manager.SchedulerDependentManager):
         self.publish_service_capabilities(ctxt)
 
     def _provide_share_server_for_share(self, context, share_network_id,
-                                        share_id):
+                                        share, snapshot=None):
         """Gets or creates share_server and updates share with its id.
 
         Active share_server can be deleted if there are no dependent shares
@@ -218,21 +218,87 @@ class ShareManager(manager.SchedulerDependentManager):
         For this purpose used shared lock between this method and the one
         with deletion of share_server.
 
+        :param context: Current context
+        :param share_network_id: Share network where existing share server
+                                 should be found or created. If
+                                 share_network_id is None method use
+                                 share_network_id from provided snapshot.
+        :param share: Share model
+        :param snapshot: Optional -- Snapshot model
+
         :returns: dict, dict -- first value is share_server, that
                   has been chosen for share schedule. Second value is
                   share updated with share_server_id.
         """
+        if not (share_network_id or snapshot):
+            msg = _("'share_network_id' parameter or 'snapshot'"
+                    " should be provided. ")
+            raise ValueError(msg)
+
+        parent_share_server = None
+
+        def error(msg, *args):
+            LOG.error(msg, *args)
+            self.db.share_update(context, share['id'],
+                                 {'status': constants.STATUS_ERROR})
+
+        if snapshot:
+            parent_share_server_id = snapshot['share']['share_server_id']
+            try:
+                parent_share_server = self.db.share_server_get(
+                    context, parent_share_server_id)
+            except exception.ShareServerNotFound:
+                with excutils.save_and_reraise_exception():
+                    error(_LE("Parent share server %s does not exist."),
+                          parent_share_server_id)
+
+            if parent_share_server['status'] != constants.STATUS_ACTIVE:
+                error_params = {
+                    'id': parent_share_server_id,
+                    'status': parent_share_server['status'],
+                }
+                error(_LE("Parent share server %(id)s has invalid status "
+                          "'%(status)s'."), error_params)
+                raise exception.InvalidShareServer(
+                    share_server_id=parent_share_server
+                )
+
+        if parent_share_server and not share_network_id:
+            share_network_id = parent_share_server['share_network_id']
+
+        def get_available_share_servers():
+            if parent_share_server:
+                return [parent_share_server]
+            else:
+                return (
+                    self.db.share_server_get_all_by_host_and_share_net_valid(
+                        context, self.host, share_network_id)
+                )
 
         @utils.synchronized("share_manager_%s" % share_network_id)
         def _provide_share_server_for_share():
-            exist = False
             try:
-                share_server = \
-                    self.db.share_server_get_by_host_and_share_net_valid(
-                        context, self.host, share_network_id)
-                exist = True
+                available_share_servers = get_available_share_servers()
             except exception.ShareServerNotFound:
-                share_server = self.db.share_server_create(
+                available_share_servers = None
+
+            compatible_share_server = None
+
+            if available_share_servers:
+                try:
+                    compatible_share_server = (
+                        self.driver.choose_share_server_compatible_with_share(
+                            context, available_share_servers, share,
+                            snapshot=snapshot
+                        )
+                    )
+                except Exception as e:
+                    with excutils.save_and_reraise_exception():
+                        error(_LE("Cannot choose compatible share-server: %s"),
+                              e)
+
+            if not compatible_share_server:
+                compatible_share_server = self.db.share_server_create(
                     context,
                     {
                         'host': self.host,
@@ -241,23 +307,28 @@ class ShareManager(manager.SchedulerDependentManager):
                     }
                 )
 
-            LOG.debug("Using share_server %s for share %s" % (
-                share_server['id'], share_id))
+            msg = "Using share_server %(share_server)s for share %(share_id)s"
+            LOG.debug(msg, {
+                'share_server': compatible_share_server['id'],
+                'share_id': share['id']
+            })
+
             share_ref = self.db.share_update(
                 context,
-                share_id,
-                {'share_server_id': share_server['id']},
+                share['id'],
+                {'share_server_id': compatible_share_server['id']},
             )
 
-            if not exist:
-                # Create share server on backend with data from db
-                share_server = self._setup_server(context, share_server)
+            if compatible_share_server['status'] == constants.STATUS_CREATING:
+                # Create share server on backend with data from db.
+                compatible_share_server = self._setup_server(
+                    context, compatible_share_server)
                 LOG.info(_LI("Share server created successfully."))
             else:
-                LOG.info(_LI("Used already existed share server "
+                LOG.info(_LI("Used preexisting share server "
                              "'%(share_server_id)s'"),
-                         {'share_server_id': share_server['id']})
-            return share_server, share_ref
+                         {'share_server_id': compatible_share_server['id']})
+            return compatible_share_server, share_ref
 
         return _provide_share_server_for_share()
 
@@ -292,24 +363,12 @@ class ShareManager(manager.SchedulerDependentManager):
             snapshot_ref = None
             parent_share_server_id = None
 
-        if parent_share_server_id:
-            try:
-                share_server = self.db.share_server_get(context,
-                                                        parent_share_server_id)
-                LOG.debug("Using share_server "
-                          "%s for share %s" % (share_server['id'], share_id))
-                share_ref = self.db.share_update(
-                    context, share_id, {'share_server_id': share_server['id']})
-            except exception.ShareServerNotFound:
-                with excutils.save_and_reraise_exception():
-                    LOG.error(_LE("Share server %s does not exist."),
-                              parent_share_server_id)
-                    self.db.share_update(context, share_id,
-                                         {'status': constants.STATUS_ERROR})
-        elif share_network_id:
+        if share_network_id or parent_share_server_id:
             try:
                 share_server, share_ref = self._provide_share_server_for_share(
-                    context, share_network_id, share_id)
+                    context, share_network_id, share_ref,
+                    snapshot=snapshot_ref
+                )
             except Exception:
                 with excutils.save_and_reraise_exception():
                     LOG.error(_LE("Failed to get share server"

manila/tests/share/drivers/test_generic.py

@@ -1384,6 +1384,21 @@ class GenericShareDriverTestCase(test.TestCase):
             fake_server_details, ['sudo', 'resize2fs', '/dev/fake'])
         self.assertEqual(2, self._driver._ssh_exec.call_count)
 
+    @ddt.data({'share_servers': [], 'result': None},
+              {'share_servers': None, 'result': None},
+              {'share_servers': ['fake'], 'result': 'fake'},
+              {'share_servers': ['fake', 'test'], 'result': 'fake'})
+    @ddt.unpack
+    def tests_choose_share_server_compatible_with_share(self, share_servers,
+                                                        result):
+        fake_share = "fake"
+
+        actual_result = self._driver.choose_share_server_compatible_with_share(
+            self._context, share_servers, fake_share
+        )
+
+        self.assertEqual(result, actual_result)
+
 
 @generic.ensure_server
 def fake(driver_instance, context, share_server=None):

manila/tests/share/test_manager.py

@@ -571,7 +571,10 @@ class ShareManagerTestCase(test.TestCase):
 
     def test_create_share_with_share_network_server_creation_failed(self):
         fake_share = {'id': 'fake_share_id', 'share_network_id': 'fake_sn_id'}
-        fake_server = {'id': 'fake_srv_id'}
+        fake_server = {
+            'id': 'fake_srv_id',
+            'status': constants.STATUS_CREATING,
+        }
         self.mock_object(db, 'share_server_create',
                          mock.Mock(return_value=fake_server))
         self.mock_object(db, 'share_update',
@@ -586,7 +589,8 @@ class ShareManagerTestCase(test.TestCase):
         def raise_manila_exception(*args, **kwargs):
             raise exception.ManilaException()
 
-        self.mock_object(db, 'share_server_get_by_host_and_share_net_valid',
+        self.mock_object(db,
+                         'share_server_get_all_by_host_and_share_net_valid',
                          mock.Mock(side_effect=raise_share_server_not_found))
         self.mock_object(self.share_manager, '_setup_server',
                          mock.Mock(side_effect=raise_manila_exception))
@@ -597,7 +601,7 @@ class ShareManagerTestCase(test.TestCase):
             self.context,
             fake_share['id'],
         )
-        db.share_server_get_by_host_and_share_net_valid.\
+        db.share_server_get_all_by_host_and_share_net_valid.\
             assert_called_once_with(
                 utils.IsAMatcher(context.RequestContext),
                 self.share_manager.host,
@@ -643,8 +647,12 @@ class ShareManagerTestCase(test.TestCase):
 
         share_id = share['id']
 
-        self.share_manager.driver = mock.Mock()
-        self.share_manager.driver.create_share.return_value = "fake_location"
+        driver_mock = mock.Mock()
+        driver_mock.create_share.return_value = "fake_location"
+        driver_mock.choose_share_server_compatible_with_share.return_value = (
+            share_srv
+        )
+        self.share_manager.driver = driver_mock
         self.share_manager.create_share(self.context, share_id)
         self.assertFalse(self.share_manager.driver.setup_network.called)
         self.assertEqual(share_id, db.share_get(context.get_admin_context(),
@@ -683,7 +691,10 @@ class ShareManagerTestCase(test.TestCase):
             share_network_id=share_net['id'], host=self.share_manager.host,
             state=constants.STATUS_ERROR)
         share_id = share['id']
-        fake_server = {'id': 'fake_srv_id'}
+        fake_server = {
+            'id': 'fake_srv_id',
+            'status': constants.STATUS_CREATING,
+        }
         self.mock_object(db, 'share_server_create',
                          mock.Mock(return_value=fake_server))
         self.mock_object(self.share_manager, '_setup_server',
@@ -737,6 +748,67 @@ class ShareManagerTestCase(test.TestCase):
             utils.IsAMatcher(models.Share),
             share_server=None)
 
+    def test_provide_share_server_for_share_incompatible_servers(self):
+        fake_exception = exception.ManilaException("fake")
+        fake_share_server = {'id': 'fake'}
+        share = self._create_share()
+
+        self.mock_object(db,
+                         'share_server_get_all_by_host_and_share_net_valid',
+                         mock.Mock(return_value=[fake_share_server]))
+        self.mock_object(
+            self.share_manager.driver,
+            "choose_share_server_compatible_with_share",
+            mock.Mock(side_effect=fake_exception)
+        )
+
+        self.assertRaises(exception.ManilaException,
+                          self.share_manager._provide_share_server_for_share,
+                          self.context, "fake_id", share)
+        driver_mock = self.share_manager.driver
+        driver_method_mock = (
+            driver_mock.choose_share_server_compatible_with_share
+        )
+        driver_method_mock.assert_called_once_with(
+            self.context, [fake_share_server], share, snapshot=None)
+
+    def test_provide_share_server_for_share_invalid_arguments(self):
+        self.assertRaises(ValueError,
+                          self.share_manager._provide_share_server_for_share,
+                          self.context, None, None)
+
+    def test_provide_share_server_for_share_parent_ss_not_found(self):
+        fake_parent_id = "fake_server_id"
+        fake_exception = exception.ShareServerNotFound("fake")
+        share = self._create_share()
+        fake_snapshot = {'share': {'share_server_id': fake_parent_id}}
+        self.mock_object(db, 'share_server_get',
+                         mock.Mock(side_effect=fake_exception))
+
+        self.assertRaises(exception.ShareServerNotFound,
+                          self.share_manager._provide_share_server_for_share,
+                          self.context, "fake_id", share,
+                          snapshot=fake_snapshot)
+
+        db.share_server_get.assert_called_once_with(
+            self.context, fake_parent_id)
+
+    def test_provide_share_server_for_share_parent_ss_invalid(self):
+        fake_parent_id = "fake_server_id"
+        share = self._create_share()
+        fake_snapshot = {'share': {'share_server_id': fake_parent_id}}
+        fake_parent_share_server = {'status': 'fake'}
+        self.mock_object(db, 'share_server_get',
+                         mock.Mock(return_value=fake_parent_share_server))
+
+        self.assertRaises(exception.InvalidShareServer,
+                          self.share_manager._provide_share_server_for_share,
+                          self.context, "fake_id", share,
+                          snapshot=fake_snapshot)
+
+        db.share_server_get.assert_called_once_with(
+            self.context, fake_parent_id)
+
     def test_manage_share_invalid_driver(self):
         self.mock_object(self.share_manager, 'driver', mock.Mock())
         self.share_manager.driver.driver_handles_share_servers = True

manila/tests/test_db.py

@@ -105,7 +105,7 @@ class ShareServerTableTestCase(test.TestCase):
                           db.share_server_update,
                           self.ctxt, fake_id, {})
 
-    def test_share_server_get_by_host_and_share_net_valid(self):
+    def test_share_server_get_all_by_host_and_share_net_valid(self):
         valid = {
             'share_network_id': '1',
             'host': 'host1',
@@ -125,15 +125,15 @@ class ShareServerTableTestCase(test.TestCase):
         self._create_share_server(invalid)
         self._create_share_server(other)
 
-        servers = db.share_server_get_by_host_and_share_net_valid(
+        servers = db.share_server_get_all_by_host_and_share_net_valid(
             self.ctxt,
             host='host1',
             share_net_id='1')
-        self.assertEqual(servers['id'], valid['id'])
+        self.assertEqual(servers[0]['id'], valid['id'])
 
-    def test_share_server_get_by_host_and_share_net_not_found(self):
+    def test_share_server_get_all_by_host_and_share_net_not_found(self):
         self.assertRaises(exception.ShareServerNotFound,
-                          db.share_server_get_by_host_and_share_net_valid,
+                          db.share_server_get_all_by_host_and_share_net_valid,
                           self.ctxt, host='fake', share_net_id='fake')
 
     def test_share_server_get_all(self):