From 3899e0b4b52c03e9a9f190e5aa5b994ffcf6dc75 Mon Sep 17 00:00:00 2001 From: Yuriy Nesenenko Date: Mon, 4 Jan 2016 15:14:15 +0200 Subject: [PATCH] Fix service-list filter Currently cinder service list --host= will not show any results for the volume services. This is because the volume-services append an internal pool name to the hostname and as a result there won't be a match for the specific filter. This patch fixes the incorrect work of service-list with '--host' argument. Also it filters data on the DB side to improve performance. APIImpact Closes-Bug: 1530144 Change-Id: I21775106693176ca128dbfd9db0d43cfc58de00a Depends-On: I4044ab15078ecf54447e1c6e67c27fc8d7c9d6f7 --- cinder/api/contrib/hosts.py | 3 ++- cinder/api/contrib/services.py | 23 +++++++------------ cinder/db/api.py | 4 ++-- cinder/db/sqlalchemy/api.py | 17 +++++++++++--- cinder/tests/unit/api/contrib/test_hosts.py | 2 +- .../tests/unit/api/contrib/test_services.py | 8 ++++++- cinder/tests/unit/objects/test_service.py | 5 ++-- cinder/tests/unit/test_db_api.py | 19 ++++++++++----- 8 files changed, 50 insertions(+), 31 deletions(-) diff --git a/cinder/api/contrib/hosts.py b/cinder/api/contrib/hosts.py index 00ce08ae5d3..a09eff62464 100644 --- a/cinder/api/contrib/hosts.py +++ b/cinder/api/contrib/hosts.py @@ -99,7 +99,8 @@ def _list_hosts(req, service=None): """Returns a summary list of hosts.""" curr_time = timeutils.utcnow(with_timezone=True) context = req.environ['cinder.context'] - services = objects.ServiceList.get_all(context, False) + filters = {'disabled': False} + services = objects.ServiceList.get_all(context, filters) zone = '' if 'zone' in req.GET: zone = req.GET['zone'] diff --git a/cinder/api/contrib/services.py b/cinder/api/contrib/services.py index ccb75f5ef10..1adbbfcdeec 100644 --- a/cinder/api/contrib/services.py +++ b/cinder/api/contrib/services.py @@ -82,27 +82,20 @@ class ServiceController(wsgi.Controller): authorize(context, action='index') detailed = self.ext_mgr.is_loaded('os-extended-services') now = timeutils.utcnow(with_timezone=True) - services = objects.ServiceList.get_all(context) - host = '' + filters = {} + if 'host' in req.GET: - host = req.GET['host'] - service = '' - if 'service' in req.GET: - service = req.GET['service'] + filters['host'] = req.GET['host'] + if 'binary' in req.GET: + filters['binary'] = req.GET['binary'] + elif 'service' in req.GET: + filters['binary'] = req.GET['service'] versionutils.report_deprecated_feature(LOG, _( "Query by service parameter is deprecated. " "Please use binary parameter instead.")) - binary = '' - if 'binary' in req.GET: - binary = req.GET['binary'] - if host: - services = [s for s in services if s.host == host] - # NOTE(uni): deprecating service request key, binary takes precedence - binary_key = binary or service - if binary_key: - services = [s for s in services if s.binary == binary_key] + services = objects.ServiceList.get_all(context, filters) svcs = [] for svc in services: diff --git a/cinder/db/api.py b/cinder/db/api.py index 89fceac006f..70877735b13 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -106,9 +106,9 @@ def service_get_by_host_and_topic(context, host, topic): return IMPL.service_get_by_host_and_topic(context, host, topic) -def service_get_all(context, disabled=None): +def service_get_all(context, filters=None): """Get all services.""" - return IMPL.service_get_all(context, disabled) + return IMPL.service_get_all(context, filters) def service_get_all_by_topic(context, topic, disabled=None): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index e82edd89000..293ea1c94d1 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -372,11 +372,22 @@ def service_get(context, service_id): @require_admin_context -def service_get_all(context, disabled=None): +def service_get_all(context, filters=None): + if filters and not is_valid_model_filters(models.Service, filters): + return [] + query = model_query(context, models.Service) - if disabled is not None: - query = query.filter_by(disabled=disabled) + try: + host = filters.pop('host') + host_attr = models.Service.host + conditions = or_(host_attr == host, host_attr.op('LIKE')(host + '@%')) + query = query.filter(conditions) + except KeyError: + pass + + if filters: + query = query.filter_by(**filters) return query.all() diff --git a/cinder/tests/unit/api/contrib/test_hosts.py b/cinder/tests/unit/api/contrib/test_hosts.py index 4c91351257e..f1e54d1a868 100644 --- a/cinder/tests/unit/api/contrib/test_hosts.py +++ b/cinder/tests/unit/api/contrib/test_hosts.py @@ -70,7 +70,7 @@ def stub_utcnow(with_timezone=False): return datetime.datetime(2013, 7, 3, 0, 0, 2, tzinfo=tzinfo) -def stub_service_get_all(self, req): +def stub_service_get_all(context, filters=None): return SERVICE_LIST diff --git a/cinder/tests/unit/api/contrib/test_services.py b/cinder/tests/unit/api/contrib/test_services.py index 5cd9c1ebb6e..0af69a09d4a 100644 --- a/cinder/tests/unit/api/contrib/test_services.py +++ b/cinder/tests/unit/api/contrib/test_services.py @@ -132,7 +132,13 @@ class FakeRequestWithHostBinary(object): def fake_service_get_all(context, filters=None): - return fake_services_list + filters = filters or {} + host = filters.get('host') + binary = filters.get('binary') + return [s for s in fake_services_list + if (not host or s['host'] == host or + s['host'].startswith(host + '@')) + and (not binary or s['binary'] == binary)] def fake_service_get_by_host_binary(context, host, binary): diff --git a/cinder/tests/unit/objects/test_service.py b/cinder/tests/unit/objects/test_service.py index 923cffc2173..df24e8b19f0 100644 --- a/cinder/tests/unit/objects/test_service.py +++ b/cinder/tests/unit/objects/test_service.py @@ -137,8 +137,9 @@ class TestServiceList(test_objects.BaseObjectsTestCase): db_service = fake_service.fake_db_service() service_get_all.return_value = [db_service] - services = objects.ServiceList.get_all(self.context, 'foo') - service_get_all.assert_called_once_with(self.context, 'foo') + filters = {'host': 'host', 'binary': 'foo', 'disabled': False} + services = objects.ServiceList.get_all(self.context, filters) + service_get_all.assert_called_once_with(self.context, filters) self.assertEqual(1, len(services)) TestService._compare(self, db_service, services[0]) diff --git a/cinder/tests/unit/test_db_api.py b/cinder/tests/unit/test_db_api.py index ed321537856..f0204da23dd 100644 --- a/cinder/tests/unit/test_db_api.py +++ b/cinder/tests/unit/test_db_api.py @@ -181,18 +181,25 @@ class DBAPIServiceTestCase(BaseTest): def test_service_get_all(self): values = [ - {'host': 'host1', 'topic': 'topic1'}, - {'host': 'host2', 'topic': 'topic2'}, + {'host': 'host1', 'binary': 'b1'}, + {'host': 'host1@ceph', 'binary': 'b2'}, + {'host': 'host2', 'binary': 'b2'}, {'disabled': True} ] services = [self._create_service(vals) for vals in values] + disabled_services = [services[-1]] non_disabled_services = services[:-1] - + expected = services[:2] + expected_bin = services[1:3] compares = [ - (services, db.service_get_all(self.ctxt)), - (disabled_services, db.service_get_all(self.ctxt, True)), - (non_disabled_services, db.service_get_all(self.ctxt, False)) + (services, db.service_get_all(self.ctxt, {})), + (expected, db.service_get_all(self.ctxt, {'host': 'host1'})), + (expected_bin, db.service_get_all(self.ctxt, {'binary': 'b2'})), + (disabled_services, db.service_get_all(self.ctxt, + {'disabled': True})), + (non_disabled_services, db.service_get_all(self.ctxt, + {'disabled': False})), ] for comp in compares: self._assertEqualListsOfObjects(*comp)