Merge "Disallow scheduling multiple replicas on a given pool"

manila/scheduler/filters/share_replication.py

@@ -37,6 +37,8 @@ class ShareReplicationFilter(base_host.BaseHostFilter):
         """
         active_replica_host = filter_properties.get('request_spec', {}).get(
             'active_replica_host')
+        existing_replica_hosts = filter_properties.get('request_spec', {}).get(
+            'all_replica_hosts', '').split(',')
         replication_type = filter_properties.get('resource_type', {}).get(
             'extra_specs', {}).get('replication_type')
         active_replica_replication_domain = filter_properties.get(
@@ -45,8 +47,7 @@ class ShareReplicationFilter(base_host.BaseHostFilter):
 
         if replication_type is None:
             # NOTE(gouthamr): You're probably not creating a replicated
-            # share or a replica, then this host obviously passes. Also,
+            # share or a replica, then this host obviously passes.
-            # avoid creating a replica on the same host.
             return True
         elif host_replication_domain is None:
             msg = "Replication is not enabled on host %s."
@@ -72,4 +73,11 @@ class ShareReplicationFilter(base_host.BaseHostFilter):
             LOG.debug(msg, kwargs)
             return False
 
+        # Check host string for already created replicas
+        if host_state.host in existing_replica_hosts:
+            msg = ("Skipping host %s since it already hosts a replica for "
+                   "this share.")
+            LOG.debug(msg, host_state.host)
+            return False
+
         return True

manila/scheduler/manager.py

@@ -207,20 +207,31 @@ class SchedulerManager(manager.Manager):
                 self._set_cg_error_state('create_consistency_group',
                                          context, ex, request_spec)
 
+    def _set_share_replica_error_state(self, context, method, exc,
+                                       request_spec):
+
+        LOG.warning(_LW("Failed to schedule_%(method)s: %(exc)s"),
+                    {'method': method, 'exc': exc})
+        status_updates = {
+            'status': constants.STATUS_ERROR,
+            'replica_state': constants.STATUS_ERROR,
+        }
+        share_replica_id = request_spec.get(
+            'share_instance_properties').get('id')
+
+        db.share_replica_update(context, share_replica_id, status_updates)
+
     def create_share_replica(self, context, request_spec=None,
                              filter_properties=None):
         try:
             self.driver.schedule_create_replica(context, request_spec,
                                                 filter_properties)
-        except Exception as ex:
+
+        except exception.NoValidHost as exc:
+            self._set_share_replica_error_state(
+                context, 'create_share_replica', exc, request_spec)
+
+        except Exception as exc:
             with excutils.save_and_reraise_exception():
-
+                self._set_share_replica_error_state(
-                msg = _LW("Failed to schedule the new share replica: %s")
+                    context, 'create_share_replica', exc, request_spec)
-
-                LOG.warning(msg % ex)
-
-                db.share_replica_update(
-                    context,
-                    request_spec.get('share_instance_properties').get('id'),
-                    {'status': constants.STATUS_ERROR,
-                     'replica_state': constants.STATUS_ERROR})

manila/share/api.py

@@ -389,7 +389,12 @@ class API(base.Base):
                 context, share, availability_zone=availability_zone,
                 share_network_id=share_network_id))
 
+        all_replicas = self.db.share_replicas_get_all_by_share(
+            context, share['id'])
+        all_hosts = [r['host'] for r in all_replicas]
+
         request_spec['active_replica_host'] = active_replica['host']
+        request_spec['all_replica_hosts'] = ','.join(all_hosts)
 
         self.db.share_replica_update(
             context, share_replica['id'],

manila/tests/fake_share.py

@@ -167,16 +167,19 @@ def fake_replica(id=None, as_primitive=True, for_manager=False, **kwargs):
 
 
 def fake_replica_request_spec(as_primitive=True, **kwargs):
+    replica = fake_replica(id='9c0db763-a109-4862-b010-10f2bd395295')
+    all_replica_hosts = ','.join(['fake_active_replica_host', replica['host']])
     request_spec = {
         'share_properties': fake_share(
             id='f0e4bb5e-65f0-11e5-9d70-feff819cdc9f'),
-        'share_instance_properties': fake_replica(
+        'share_instance_properties': replica,
-            id='9c0db763-a109-4862-b010-10f2bd395295'),
         'share_proto': 'nfs',
         'share_id': 'f0e4bb5e-65f0-11e5-9d70-feff819cdc9f',
         'snapshot_id': None,
         'share_type': 'fake_share_type',
         'consistency_group': None,
+        'active_replica_host': 'fake_active_replica_host',
+        'all_replica_hosts': all_replica_hosts,
     }
     request_spec.update(kwargs)
     if as_primitive:

manila/tests/scheduler/filters/test_share_replication.py

@@ -37,12 +37,14 @@ class ShareReplicationFilterTestCase(test.TestCase):
     def _create_replica_request(replication_domain='kashyyyk',
                                 replication_type='dr',
                                 active_replica_host=fakes.FAKE_HOST_STRING_1,
+                                all_replica_hosts=fakes.FAKE_HOST_STRING_1,
                                 is_admin=False):
         ctxt = context.RequestContext('fake', 'fake', is_admin=is_admin)
         return {
             'context': ctxt,
             'request_spec': {
                 'active_replica_host': active_replica_host,
+                'all_replica_hosts': all_replica_hosts,
             },
             'resource_type': {
                 'extra_specs': {
@@ -75,6 +77,18 @@ class ShareReplicationFilterTestCase(test.TestCase):
         self.assertFalse(self.filter.host_passes(host, request))
         self.assertTrue(self.debug_log.called)
 
+    def test_share_replication_filter_fails_host_has_replicas(self):
+        all_replica_hosts = ','.join(['host1', fakes.FAKE_HOST_STRING_1])
+        request = self._create_replica_request(
+            all_replica_hosts=all_replica_hosts)
+
+        host = fakes.FakeHostState('host1',
+                                   {
+                                       'replication_domain': 'kashyyyk',
+                                   })
+        self.assertFalse(self.filter.host_passes(host, request))
+        self.assertTrue(self.debug_log.called)
+
     def test_share_replication_filter_passes_no_replication_type(self):
         request = self._create_replica_request(replication_type=None)
 
@@ -96,9 +110,11 @@ class ShareReplicationFilterTestCase(test.TestCase):
         self.assertTrue(self.filter.host_passes(host, request))
 
     def test_share_replication_filter_passes_happy_day(self):
-        request = self._create_replica_request()
+        all_replica_hosts = ','.join(['host1', fakes.FAKE_HOST_STRING_1])
+        request = self._create_replica_request(
+            all_replica_hosts=all_replica_hosts)
 
-        host = fakes.FakeHostState('host1',
+        host = fakes.FakeHostState('host2',
                                    {
                                        'replication_domain': 'kashyyyk',
                                    })

manila/tests/scheduler/test_manager.py

@@ -21,6 +21,7 @@ import ddt
 import mock
 from oslo_config import cfg
 
+from manila.common import constants
 from manila import context
 from manila import db
 from manila import exception
@@ -30,6 +31,7 @@ from manila.scheduler import manager
 from manila.share import rpcapi as share_rpcapi
 from manila import test
 from manila.tests import db_utils
+from manila.tests import fake_share as fakes
 
 CONF = cfg.CONF
 
@@ -51,6 +53,9 @@ class SchedulerManagerTestCase(test.TestCase):
         self.fake_args = (1, 2, 3)
         self.fake_kwargs = {'cat': 'meow', 'dog': 'woof'}
 
+    def raise_no_valid_host(*args, **kwargs):
+            raise exception.NoValidHost(reason="")
+
     def test_1_correct_init(self):
         # Correct scheduler driver
         manager = self.manager
@@ -116,15 +121,12 @@ class SchedulerManagerTestCase(test.TestCase):
 
         Puts the share in 'error' state and eats the exception.
         """
-        def raise_no_valid_host(*args, **kwargs):
-            raise exception.NoValidHost(reason="")
-
         fake_share_id = 1
 
         request_spec = {'share_id': fake_share_id}
-        with mock.patch.object(self.manager.driver,
+        with mock.patch.object(
-                               'schedule_create_share',
+                self.manager.driver, 'schedule_create_share',
-                               mock.Mock(side_effect=raise_no_valid_host)):
+                mock.Mock(side_effect=self.raise_no_valid_host)):
             self.mock_object(manager.LOG, 'error')
 
             self.manager.create_share_instance(
@@ -179,15 +181,13 @@ class SchedulerManagerTestCase(test.TestCase):
 
         Puts the share in 'error' state and eats the exception.
         """
-        def raise_no_valid_host(*args, **kwargs):
-            raise exception.NoValidHost(reason="")
 
         fake_cg_id = 1
         cg_id = fake_cg_id
         request_spec = {"consistency_group_id": cg_id}
-        with mock.patch.object(self.manager.driver,
+        with mock.patch.object(
-                               'schedule_create_consistency_group',
+                self.manager.driver, 'schedule_create_consistency_group',
-                               mock.Mock(side_effect=raise_no_valid_host)):
+                mock.Mock(side_effect=self.raise_no_valid_host)):
             self.manager.create_consistency_group(self.context,
                                                   fake_cg_id,
                                                   request_spec=request_spec,
@@ -242,3 +242,58 @@ class SchedulerManagerTestCase(test.TestCase):
         self.assertRaises(
             exception.NoValidHost, self.manager.migrate_share_to_host,
             self.context, share['id'], host, False, True, {}, None)
+
+    def test_create_share_replica_exception_path(self):
+        """Test 'raisable' exceptions for create_share_replica."""
+        db_update = self.mock_object(db, 'share_replica_update')
+        request_spec = fakes.fake_replica_request_spec()
+        replica_id = request_spec.get('share_instance_properties').get('id')
+        expected_updates = {
+            'status': constants.STATUS_ERROR,
+            'replica_state': constants.STATUS_ERROR,
+        }
+        with mock.patch.object(self.manager.driver, 'schedule_create_replica',
+                               mock.Mock(side_effect=exception.NotFound)):
+
+            self.assertRaises(exception.NotFound,
+                              self.manager.create_share_replica,
+                              self.context,
+                              request_spec=request_spec,
+                              filter_properties={})
+            db_update.assert_called_once_with(
+                self.context, replica_id, expected_updates)
+
+    def test_create_share_replica_no_valid_host(self):
+        """Test the NoValidHost exception for create_share_replica."""
+        db_update = self.mock_object(db, 'share_replica_update')
+        request_spec = fakes.fake_replica_request_spec()
+        replica_id = request_spec.get('share_instance_properties').get('id')
+        expected_updates = {
+            'status': constants.STATUS_ERROR,
+            'replica_state': constants.STATUS_ERROR,
+        }
+        with mock.patch.object(
+                self.manager.driver, 'schedule_create_replica',
+                mock.Mock(side_effect=self.raise_no_valid_host)):
+
+            retval = self.manager.create_share_replica(
+                self.context, request_spec=request_spec, filter_properties={})
+
+            self.assertIsNone(retval)
+            db_update.assert_called_once_with(
+                self.context, replica_id, expected_updates)
+
+    def test_create_share_replica(self):
+        """Test happy path for create_share_replica."""
+        db_update = self.mock_object(db, 'share_replica_update')
+        mock_scheduler_driver_call = self.mock_object(
+            self.manager.driver, 'schedule_create_replica')
+        request_spec = fakes.fake_replica_request_spec()
+
+        retval = self.manager.create_share_replica(
+            self.context, request_spec=request_spec, filter_properties={})
+
+        mock_scheduler_driver_call.assert_called_once_with(
+            self.context, request_spec, {})
+        self.assertFalse(db_update.called)
+        self.assertIsNone(retval)