Generic filter support for volume queries

DB functions exist to get all volumes, to get all volumes in a particular
project, to get all volumes in a particular group, and to get all volumes
hosted on a particular host. See the following functions in the DB API:

* volume_get_all
* volume_get_all_by_project
* volume_get_all_by_group
* volume_get_all_by_host

Only the queries that get all volumes and that get all volumes by project
support additional filtering.

The purpose of this patch set is to make the filtering support consistent
across these APIs, adding it to the volume_get_all_by_group and the
volume_get_all_by_host APIs.

Change-Id: I6af9b4de9e70ec442e7e61c6b0baa9b02798a06d
Implements: blueprint db-volume-filtering

cinder/db/api.py

@@ -186,14 +186,14 @@ def volume_get_all(context, marker, limit, sort_key, sort_dir,
                                filters=filters)
 
 
-def volume_get_all_by_host(context, host):
+def volume_get_all_by_host(context, host, filters=None):
     """Get all volumes belonging to a host."""
-    return IMPL.volume_get_all_by_host(context, host)
+    return IMPL.volume_get_all_by_host(context, host, filters=filters)
 
 
-def volume_get_all_by_group(context, group_id):
+def volume_get_all_by_group(context, group_id, filters=None):
     """Get all volumes belonging to a consistency group."""
-    return IMPL.volume_get_all_by_group(context, group_id)
+    return IMPL.volume_get_all_by_group(context, group_id, filters=filters)
 
 
 def volume_get_all_by_project(context, project_id, marker, limit, sort_key,

cinder/db/sqlalchemy/api.py

@@ -1194,10 +1194,10 @@ def volume_get_all(context, marker, limit, sort_key, sort_dir,
     :param limit: maximum number of items to return
     :param sort_key: single attributes by which results should be sorted
     :param sort_dir: direction in which results should be sorted (asc, desc)
-    :param filters: Filters for the query. A filter key/value of
+    :param filters: dictionary of filters; values that are in lists, tuples,
-                    'no_migration_targets'=True causes volumes with either
+                    or sets cause an 'IN' operation, while exact matching
-                    a NULL 'migration_status' or a 'migration_status' that
+                    is used for other values, see _process_volume_filters
-                    does not start with 'target:' to be retrieved.
+                    function for more information
     :returns: list of matching volumes
     """
     session = get_session()
@@ -1212,8 +1212,17 @@ def volume_get_all(context, marker, limit, sort_key, sort_dir,
 
 
 @require_admin_context
-def volume_get_all_by_host(context, host):
+def volume_get_all_by_host(context, host, filters=None):
-    """Retrieves all volumes hosted on a host."""
+    """Retrieves all volumes hosted on a host.
+
+    :param context: context to query under
+    :param host: host for all volumes being retrieved
+    :param filters: dictionary of filters; values that are in lists, tuples,
+                    or sets cause an 'IN' operation, while exact matching
+                    is used for other values, see _process_volume_filters
+                    function for more information
+    :returns: list of matching volumes
+    """
     # As a side effect of the introduction of pool-aware scheduler,
     # newly created volumes will have pool information appended to
     # 'host' field of a volume record. So a volume record in DB can
@@ -1226,16 +1235,36 @@ def volume_get_all_by_host(context, host):
             host_attr = getattr(models.Volume, 'host')
             conditions = [host_attr == host,
                           host_attr.op('LIKE')(host + '#%')]
-            result = _volume_get_query(context).filter(or_(*conditions)).all()
+            query = _volume_get_query(context).filter(or_(*conditions))
-            return result
+            if filters:
+                query = _process_volume_filters(query, filters)
+                # No volumes would match, return empty list
+                if query is None:
+                    return []
+            return query.all()
     elif not host:
         return []
 
 
 @require_admin_context
-def volume_get_all_by_group(context, group_id):
+def volume_get_all_by_group(context, group_id, filters=None):
-    return _volume_get_query(context).filter_by(consistencygroup_id=group_id).\
+    """Retrieves all volumes associated with the group_id.
-        all()
+
+    :param context: context to query under
+    :param group_id: group ID for all volumes being retrieved
+    :param filters: dictionary of filters; values that are in lists, tuples,
+                    or sets cause an 'IN' operation, while exact matching
+                    is used for other values, see _process_volume_filters
+                    function for more information
+    :returns: list of matching volumes
+    """
+    query = _volume_get_query(context).filter_by(consistencygroup_id=group_id)
+    if filters:
+        query = _process_volume_filters(query, filters)
+        # No volumes would match, return empty list
+        if query is None:
+            return []
+    return query.all()
 
 
 @require_context
@@ -1250,10 +1279,10 @@ def volume_get_all_by_project(context, project_id, marker, limit, sort_key,
     :param limit: maximum number of items to return
     :param sort_key: single attributes by which results should be sorted
     :param sort_dir: direction in which results should be sorted (asc, desc)
-    :param filters: Filters for the query. A filter key/value of
+    :param filters: dictionary of filters; values that are in lists, tuples,
-                    'no_migration_targets'=True causes volumes with either
+                    or sets cause an 'IN' operation, while exact matching
-                    a NULL 'migration_status' or a 'migration_status' that
+                    is used for other values, see _process_volume_filters
-                    does not start with 'target:' to be retrieved.
+                    function for more information
     :returns: list of matching volumes
     """
     session = get_session()
@@ -1285,21 +1314,51 @@ def _generate_paginate_query(context, session, marker, limit, sort_key,
     :param limit: maximum number of items to return
     :param sort_key: single attributes by which results should be sorted
     :param sort_dir: direction in which results should be sorted (asc, desc)
-    :param filters: dictionary of filters; values that are lists,
+    :param filters: dictionary of filters; values that are in lists, tuples,
-                    tuples, sets, or frozensets cause an 'IN' test to
+                    or sets cause an 'IN' operation, while exact matching
-                    be performed, while exact matching ('==' operator)
+                    is used for other values, see _process_volume_filters
-                    is used for other values
+                    function for more information
     :returns: updated query or None
     """
     query = _volume_get_query(context, session=session)
 
     if filters:
+        query = _process_volume_filters(query, filters)
+        if query is None:
+            return None
+
+    marker_volume = None
+    if marker is not None:
+        marker_volume = _volume_get(context, marker, session)
+
+    return sqlalchemyutils.paginate_query(query, models.Volume, limit,
+                                          [sort_key, 'created_at', 'id'],
+                                          marker=marker_volume,
+                                          sort_dir=sort_dir)
+
+
+def _process_volume_filters(query, filters):
+    """Common filter processing for Volume queries.
+
+    Filter values that are in lists, tuples, or sets cause an 'IN' operator
+    to be used, while exact matching ('==' operator) is used for other values.
+
+    A filter key/value of 'no_migration_targets'=True causes volumes with
+    either a NULL 'migration_status' or a 'migration_status' that does not
+    start with 'target:' to be retrieved.
+
+    A 'metadata' filter key must correspond to a dictionary value of metadata
+    key-value pairs.
+
+    :param query: Model query to use
+    :param filters: dictionary of filters
+    :returns: updated query or None
+    """
     filters = filters.copy()
 
     # 'no_migration_targets' is unique, must be either NULL or
     # not start with 'target:'
-        if ('no_migration_targets' in filters and
+    if filters.get('no_migration_targets', False):
-                filters['no_migration_targets'] is True):
         filters.pop('no_migration_targets')
         try:
             column_attr = getattr(models.Volume, 'migration_status')
@@ -1307,8 +1366,7 @@ def _generate_paginate_query(context, session, marker, limit, sort_key,
                           column_attr.op('NOT LIKE')('target:%')]
             query = query.filter(or_(*conditions))
         except AttributeError:
-                log_msg = _("'migration_status' column could not be found.")
+            LOG.debug("'migration_status' column could not be found.")
-                LOG.debug(log_msg)
             return None
 
     # Apply exact match filters for everything else, ensure that the
@@ -1317,8 +1375,7 @@ def _generate_paginate_query(context, session, marker, limit, sort_key,
         # metadata is unique, must be a dict
         if key == 'metadata':
             if not isinstance(filters[key], dict):
-                    log_msg = _("'metadata' filter value is not valid.")
+                LOG.debug("'metadata' filter value is not valid.")
-                    LOG.debug(log_msg)
                 return None
             continue
         try:
@@ -1327,19 +1384,17 @@ def _generate_paginate_query(context, session, marker, limit, sort_key,
             # schema specific knowledge
             prop = getattr(column_attr, 'property')
             if isinstance(prop, RelationshipProperty):
-                    log_msg = (_("'%s' filter key is not valid, "
+                LOG.debug(("'%s' filter key is not valid, "
-                                 "it maps to a relationship.")) % key
+                           "it maps to a relationship."), key)
-                    LOG.debug(log_msg)
                 return None
         except AttributeError:
-                log_msg = _("'%s' filter key is not valid.") % key
+            LOG.debug("'%s' filter key is not valid.", key)
-                LOG.debug(log_msg)
             return None
 
     # Holds the simple exact matches
     filter_dict = {}
 
-        # Iterate over all filters, special case the filter is necessary
+    # Iterate over all filters, special case the filter if necessary
     for key, value in filters.iteritems():
         if key == 'metadata':
             # model.VolumeMetadata defines the backref to Volumes as
@@ -1361,15 +1416,7 @@ def _generate_paginate_query(context, session, marker, limit, sort_key,
     # Apply simple exact matches
     if filter_dict:
         query = query.filter_by(**filter_dict)
-
+    return query
-    marker_volume = None
-    if marker is not None:
-        marker_volume = _volume_get(context, marker, session)
-
-    return sqlalchemyutils.paginate_query(query, models.Volume, limit,
-                                          [sort_key, 'created_at', 'id'],
-                                          marker=marker_volume,
-                                          sort_dir=sort_dir)
 
 
 @require_admin_context

cinder/tests/test_db_api.py

@@ -352,6 +352,77 @@ class DBAPIVolumeTestCase(BaseTest):
                                         db.volume_get_all_by_host(
                                         self.ctxt, 'foo'))
 
+    def test_volume_get_all_by_host_with_filters(self):
+        v1 = db.volume_create(self.ctxt, {'host': 'h1', 'display_name': 'v1',
+                                          'status': 'available'})
+        v2 = db.volume_create(self.ctxt, {'host': 'h1', 'display_name': 'v2',
+                                          'status': 'available'})
+        v3 = db.volume_create(self.ctxt, {'host': 'h2', 'display_name': 'v1',
+                                          'status': 'available'})
+        self._assertEqualListsOfObjects(
+            [v1],
+            db.volume_get_all_by_host(self.ctxt, 'h1',
+                                      filters={'display_name': 'v1'}))
+        self._assertEqualListsOfObjects(
+            [v1, v2],
+            db.volume_get_all_by_host(
+                self.ctxt, 'h1',
+                filters={'display_name': ['v1', 'v2', 'foo']}))
+        self._assertEqualListsOfObjects(
+            [v1, v2],
+            db.volume_get_all_by_host(self.ctxt, 'h1',
+                                      filters={'status': 'available'}))
+        self._assertEqualListsOfObjects(
+            [v3],
+            db.volume_get_all_by_host(self.ctxt, 'h2',
+                                      filters={'display_name': 'v1'}))
+        # No match
+        vols = db.volume_get_all_by_host(self.ctxt, 'h1',
+                                         filters={'status': 'foo'})
+        self.assertEqual([], vols)
+        # Bogus filter, should return empty list
+        vols = db.volume_get_all_by_host(self.ctxt, 'h1',
+                                         filters={'foo': 'bar'})
+        self.assertEqual([], vols)
+
+    def test_volume_get_all_by_group(self):
+        volumes = []
+        for i in xrange(3):
+            volumes.append([db.volume_create(self.ctxt, {
+                'consistencygroup_id': 'g%d' % i}) for j in xrange(3)])
+        for i in xrange(3):
+            self._assertEqualListsOfObjects(volumes[i],
+                                            db.volume_get_all_by_group(
+                                            self.ctxt, 'g%d' % i))
+
+    def test_volume_get_all_by_group_with_filters(self):
+        v1 = db.volume_create(self.ctxt, {'consistencygroup_id': 'g1',
+                                          'display_name': 'v1'})
+        v2 = db.volume_create(self.ctxt, {'consistencygroup_id': 'g1',
+                                          'display_name': 'v2'})
+        v3 = db.volume_create(self.ctxt, {'consistencygroup_id': 'g2',
+                                          'display_name': 'v1'})
+        self._assertEqualListsOfObjects(
+            [v1],
+            db.volume_get_all_by_group(self.ctxt, 'g1',
+                                       filters={'display_name': 'v1'}))
+        self._assertEqualListsOfObjects(
+            [v1, v2],
+            db.volume_get_all_by_group(self.ctxt, 'g1',
+                                       filters={'display_name': ['v1', 'v2']}))
+        self._assertEqualListsOfObjects(
+            [v3],
+            db.volume_get_all_by_group(self.ctxt, 'g2',
+                                       filters={'display_name': 'v1'}))
+        # No match
+        vols = db.volume_get_all_by_group(self.ctxt, 'g1',
+                                          filters={'display_name': 'foo'})
+        self.assertEqual([], vols)
+        # Bogus filter, should return empty list
+        vols = db.volume_get_all_by_group(self.ctxt, 'g1',
+                                          filters={'foo': 'bar'})
+        self.assertEqual([], vols)
+
     def test_volume_get_all_by_project(self):
         volumes = []
         for i in xrange(3):