From 0616fcb8fe4ef2734db4b0107a220068170e8683 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Mon, 7 Mar 2016 20:06:50 -0500 Subject: [PATCH] Disallow scheduling multiple replicas on a given pool Currently, while scheduling a new replica, the ShareReplicationFilter passes any host that shares the replication_domain of the active replica's host. On successive runs on the CI, it was observed that the same host was chosen multiple times. In a real deployment, there is no practicality in allowing replication within the same pool. Pass more information to assist the filter in disqualifying hosts that already host replicas for the share. Also treat NoValidHost exception differently from other scheduler exceptions to be consistent with other scheduling failures. Change-Id: Iedc7b752c7fc477d6257d100e2277378d59f1fde Closes-Bug: #1554282 --- manila/scheduler/filters/share_replication.py | 12 ++- manila/scheduler/manager.py | 33 +++++--- manila/share/api.py | 5 ++ manila/tests/fake_share.py | 7 +- .../filters/test_share_replication.py | 20 ++++- manila/tests/scheduler/test_manager.py | 77 ++++++++++++++++--- 6 files changed, 126 insertions(+), 28 deletions(-) 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)