Fix service-list filter

Currently cinder service list --host=<hostname> 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

diff --git a/cinder/api/contrib/hosts.py b/cinder/api/contrib/hosts.py
index 00ce08ae5d3d973e72c5c33340b50e6c32e747cf..a09eff624645022726c0be19e9c14c45fe73fca1 100644 (file)
--- 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 ccb75f5ef10dd18c23c1cd9fddae323ebaf55a18..1adbbfcdeecacdc8fff74504f123432f30392ad8 100644 (file)
--- 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 89fceac006f635865404ff072972e570edeb16e5..70877735b1389d22d14d8421493aa47a503aa561 100644 (file)
--- 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 e82edd8900066ae84e5e6d7234e2fd4c11d9e389..293ea1c94d1850bfe7dd9ac23fbe20209a80b6b9 100644 (file)
--- 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 4c91351257e84ca75728c400b65dc249d333d81c..f1e54d1a86800e84f2edf57d8fc9655bdabe0019 100644 (file)
--- 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 5cd9c1ebb6e03daf5df55532e006938f0027cec9..0af69a09d4a57252a401699aeaf538f2c0408e09 100644 (file)
--- 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 923cffc2173486cc8a897eaea76ddc0d26bf30f9..df24e8b19f0f15165060057b3136150c1463f946 100644 (file)
--- 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 ed321537856ac8f753d7c07163c73b0c392f3cf0..f0204da23dd8d16cb8a09ef1f21ab957984df70e 100644 (file)
--- 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)