diff --git a/manila/scheduler/filters/share_replication.py b/manila/scheduler/filters/share_replication.py index 039f8b1795..c912710d36 100644 --- a/manila/scheduler/filters/share_replication.py +++ b/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, - # avoid creating a replica on the same host. + # share or a replica, then this host obviously passes. 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 diff --git a/manila/scheduler/manager.py b/manila/scheduler/manager.py index 42c402169d..95c70bf946 100644 --- a/manila/scheduler/manager.py +++ b/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(): - - msg = _LW("Failed to schedule the new share replica: %s") - - 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}) + self._set_share_replica_error_state( + context, 'create_share_replica', exc, request_spec) diff --git a/manila/share/api.py b/manila/share/api.py index 439e94e8a2..a30c64698d 100644 --- a/manila/share/api.py +++ b/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'], diff --git a/manila/tests/fake_share.py b/manila/tests/fake_share.py index 329ec8bca2..03bde75616 100644 --- a/manila/tests/fake_share.py +++ b/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( - id='9c0db763-a109-4862-b010-10f2bd395295'), + 'share_instance_properties': replica, '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: diff --git a/manila/tests/scheduler/filters/test_share_replication.py b/manila/tests/scheduler/filters/test_share_replication.py index c25eef3c6e..a8e6aa6dd7 100644 --- a/manila/tests/scheduler/filters/test_share_replication.py +++ b/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', }) diff --git a/manila/tests/scheduler/test_manager.py b/manila/tests/scheduler/test_manager.py index 7aee08d0fa..c10173b270 100644 --- a/manila/tests/scheduler/test_manager.py +++ b/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, - 'schedule_create_share', - mock.Mock(side_effect=raise_no_valid_host)): + with mock.patch.object( + self.manager.driver, 'schedule_create_share', + 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, - 'schedule_create_consistency_group', - mock.Mock(side_effect=raise_no_valid_host)): + with mock.patch.object( + self.manager.driver, 'schedule_create_consistency_group', + 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)