From: git-harry Date: Fri, 25 Apr 2014 16:03:13 +0000 (+0100) Subject: Add disabled kwarg to service_get_all_by_topic X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=2e15055eb572b1745e8c4a14b186e5defb7971f0;p=openstack-build%2Fcinder-build.git Add disabled kwarg to service_get_all_by_topic service_get_all_by_topic disabled kwarg allows results to include where disabled is True or False or both. Updated code where service_get_all_by_topic is used to make use of disabled kwarg instead of filtering afterwards. Removed logging of disabled services in host_manager. Closes-Bug: #1285673 Change-Id: I748461e64acd4e9563c4016781c5e1d4b116dac8 --- diff --git a/cinder/backup/api.py b/cinder/backup/api.py index 544cde723..37851e4bd 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -88,10 +88,12 @@ class API(base.Base): """Check if there is a backup service available.""" topic = CONF.backup_topic ctxt = context.get_admin_context() - services = self.db.service_get_all_by_topic(ctxt, topic) + services = self.db.service_get_all_by_topic(ctxt, + topic, + disabled=False) for srv in services: if (srv['availability_zone'] == volume['availability_zone'] and - srv['host'] == volume_host and not srv['disabled'] and + srv['host'] == volume_host and utils.service_is_up(srv)): return True return False diff --git a/cinder/db/api.py b/cinder/db/api.py index 6d05ba766..b81b04a38 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -99,9 +99,9 @@ def service_get_all(context, disabled=None): return IMPL.service_get_all(context, disabled) -def service_get_all_by_topic(context, topic): +def service_get_all_by_topic(context, topic, disabled=None): """Get all services for a given topic.""" - return IMPL.service_get_all_by_topic(context, topic) + return IMPL.service_get_all_by_topic(context, topic, disabled=disabled) def service_get_all_by_host(context, host): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 0add648bb..219740b1c 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -279,12 +279,15 @@ def service_get_all(context, disabled=None): @require_admin_context -def service_get_all_by_topic(context, topic): - return model_query( +def service_get_all_by_topic(context, topic, disabled=None): + query = model_query( context, models.Service, read_deleted="no").\ - filter_by(disabled=False).\ - filter_by(topic=topic).\ - all() + filter_by(topic=topic) + + if disabled is not None: + query = query.filter_by(disabled=disabled) + + return query.all() @require_admin_context diff --git a/cinder/scheduler/host_manager.py b/cinder/scheduler/host_manager.py index 5da8101fa..5c68fbd32 100644 --- a/cinder/scheduler/host_manager.py +++ b/cinder/scheduler/host_manager.py @@ -289,13 +289,14 @@ class HostManager(object): # Get resource usage across the available volume nodes: topic = CONF.volume_topic - volume_services = db.service_get_all_by_topic(context, topic) + volume_services = db.service_get_all_by_topic(context, + topic, + disabled=False) active_hosts = set() for service in volume_services: host = service['host'] - if not utils.service_is_up(service) or service['disabled']: - LOG.warn(_("volume service is down or disabled. " - "(host: %s)") % host) + if not utils.service_is_up(service): + LOG.warn(_("volume service is down. (host: %s)") % host) continue capabilities = self.service_states.get(host, None) host_state = self.host_state_map.get(host) diff --git a/cinder/tests/api/contrib/test_backups.py b/cinder/tests/api/contrib/test_backups.py index 432db8f0d..3cda44a48 100644 --- a/cinder/tests/api/contrib/test_backups.py +++ b/cinder/tests/api/contrib/test_backups.py @@ -512,10 +512,7 @@ class BackupsAPITestCase(test.TestCase): az_not_match = [{'availability_zone': "strange_az", 'host': test_host, 'disabled': 0, 'updated_at': timeutils.utcnow()}] #service disabled - disabled_service = [{'availability_zone': "fake_az", - 'host': test_host, - 'disabled': 1, - 'updated_at': timeutils.utcnow()}] + disabled_service = [] #dead service that last reported at 20th century dead_service = [{'availability_zone': "fake_az", 'host': alt_host, diff --git a/cinder/tests/scheduler/fakes.py b/cinder/tests/scheduler/fakes.py index ed6437d0b..61bafa761 100644 --- a/cinder/tests/scheduler/fakes.py +++ b/cinder/tests/scheduler/fakes.py @@ -66,7 +66,7 @@ class FakeHostState(host_manager.HostState): setattr(self, key, val) -def mock_host_manager_db_calls(mock_obj): +def mock_host_manager_db_calls(mock_obj, disabled=None): services = [ dict(id=1, host='host1', topic='volume', disabled=False, availability_zone='zone1', updated_at=timeutils.utcnow()), @@ -80,4 +80,8 @@ def mock_host_manager_db_calls(mock_obj): dict(id=5, host='host5', topic='volume', disabled=True, availability_zone='zone4', updated_at=timeutils.utcnow()), ] - mock_obj.return_value = services + if disabled is None: + mock_obj.return_value = services + else: + mock_obj.return_value = [service for service in services + if service['disabled'] == disabled] diff --git a/cinder/tests/scheduler/test_allocated_capacity_weigher.py b/cinder/tests/scheduler/test_allocated_capacity_weigher.py index 8a02ac92f..043a90069 100644 --- a/cinder/tests/scheduler/test_allocated_capacity_weigher.py +++ b/cinder/tests/scheduler/test_allocated_capacity_weigher.py @@ -42,12 +42,13 @@ class AllocatedCapacityWeigherTestCase(test.TestCase): weight_properties)[0] @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic') - def _get_all_hosts(self, _mock_service_get_all_by_topic): + def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False): ctxt = context.get_admin_context() - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic) + fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic, + disabled=disabled) host_states = self.host_manager.get_all_host_states(ctxt) _mock_service_get_all_by_topic.assert_called_once_with( - ctxt, CONF.volume_topic) + ctxt, CONF.volume_topic, disabled=disabled) return host_states def test_default_of_spreading_first(self): diff --git a/cinder/tests/scheduler/test_capacity_weigher.py b/cinder/tests/scheduler/test_capacity_weigher.py index 680dac117..28e266ef8 100644 --- a/cinder/tests/scheduler/test_capacity_weigher.py +++ b/cinder/tests/scheduler/test_capacity_weigher.py @@ -43,12 +43,13 @@ class CapacityWeigherTestCase(test.TestCase): weight_properties)[0] @mock.patch('cinder.db.sqlalchemy.api.service_get_all_by_topic') - def _get_all_hosts(self, _mock_service_get_all_by_topic): + def _get_all_hosts(self, _mock_service_get_all_by_topic, disabled=False): ctxt = context.get_admin_context() - fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic) + fakes.mock_host_manager_db_calls(_mock_service_get_all_by_topic, + disabled=disabled) host_states = self.host_manager.get_all_host_states(ctxt) _mock_service_get_all_by_topic.assert_called_once_with( - ctxt, CONF.volume_topic) + ctxt, CONF.volume_topic, disabled=disabled) return host_states def test_default_of_spreading_first(self): diff --git a/cinder/tests/scheduler/test_host_manager.py b/cinder/tests/scheduler/test_host_manager.py index 04ba8506d..7140fa1a7 100644 --- a/cinder/tests/scheduler/test_host_manager.py +++ b/cinder/tests/scheduler/test_host_manager.py @@ -133,9 +133,6 @@ class HostManagerTestCase(test.TestCase): availability_zone='zone2', updated_at=timeutils.utcnow()), dict(id=4, host='host4', topic='volume', disabled=False, availability_zone='zone3', updated_at=timeutils.utcnow()), - # service on host5 is disabled - dict(id=5, host='host5', topic='volume', disabled=True, - availability_zone='zone4', updated_at=timeutils.utcnow()), ] # First test: service_is_up is always True, host5 is disabled @@ -144,15 +141,15 @@ class HostManagerTestCase(test.TestCase): _mock_warning = mock.Mock() host_manager.LOG.warn = _mock_warning - # Get all states, make sure host5 is reported as down/disabled + # Get all states self.host_manager.get_all_host_states(context) - _mock_service_get_all_by_topic.assert_called_with(context, topic) + _mock_service_get_all_by_topic.assert_called_with(context, + topic, + disabled=False) expected = [] for service in services: expected.append(mock.call(service)) self.assertEqual(expected, _mock_service_is_up.call_args_list) - _mock_warning.assert_called_with("volume service is down or disabled. " - "(host: host5)") # Get host_state_map and make sure we have the first 4 hosts host_state_map = self.host_manager.host_state_map @@ -164,20 +161,22 @@ class HostManagerTestCase(test.TestCase): # Second test: Now service_is_up returns False for host4 _mock_service_is_up.reset_mock() - _mock_service_is_up.side_effect = [True, True, True, False, True] + _mock_service_is_up.side_effect = [True, True, True, False] _mock_service_get_all_by_topic.reset_mock() _mock_warning.reset_mock() - # Get all states, make sure hosts 4 and 5 is reported as down/disabled + # Get all states, make sure host 4 is reported as down self.host_manager.get_all_host_states(context) - _mock_service_get_all_by_topic.assert_called_with(context, topic) + _mock_service_get_all_by_topic.assert_called_with(context, + topic, + disabled=False) expected = [] for service in services: expected.append(mock.call(service)) self.assertEqual(expected, _mock_service_is_up.call_args_list) expected = [] - for num in ['4', '5']: - expected.append(mock.call("volume service is down or disabled. " + for num in ['4']: + expected.append(mock.call("volume service is down. " "(host: host" + num + ")")) self.assertEqual(expected, _mock_warning.call_args_list) diff --git a/cinder/tests/test_db_api.py b/cinder/tests/test_db_api.py index f11d9156b..532ba5a78 100644 --- a/cinder/tests/test_db_api.py +++ b/cinder/tests/test_db_api.py @@ -188,11 +188,11 @@ class DBAPIServiceTestCase(BaseTest): values = [ {'host': 'host1', 'topic': 't1'}, {'host': 'host2', 'topic': 't1'}, - {'disabled': True, 'topic': 't1'}, + {'host': 'host4', 'disabled': True, 'topic': 't1'}, {'host': 'host3', 'topic': 't2'} ] services = [self._create_service(vals) for vals in values] - expected = services[:2] + expected = services[:3] real = db.service_get_all_by_topic(self.ctxt, 't1') self._assertEqualListsOfObjects(expected, real) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 088f95c25..1ce4ecbb0 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -793,7 +793,9 @@ class API(base.Base): # Make sure the host is in the list of available hosts elevated = context.elevated() topic = CONF.volume_topic - services = self.db.service_get_all_by_topic(elevated, topic) + services = self.db.service_get_all_by_topic(elevated, + topic, + disabled=False) found = False for service in services: if utils.service_is_up(service) and service['host'] == host: