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

diff --git a/cinder/db/api.py b/cinder/db/api.py
index f8b37e816483764b102f56477c6ac1856fa3efba..9d36132dd9e732c9fe93f0696dc4356f2245d606 100644 (file)
--- a/cinder/db/api.py
+++ b/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,

diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py
index cbb6df13dc53a7333cf43dfa04833a28715a8eb8..f42c9c4cd46e3d5b374f971a9f2a5abfe8ebc656 100644 (file)
--- a/cinder/db/sqlalchemy/api.py
+++ b/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
-                    '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.
+    :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
     """
     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):
-    """Retrieves all volumes hosted on a host."""
+def volume_get_all_by_host(context, host, filters=None):
+    """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()
-            return result
+            query = _volume_get_query(context).filter(or_(*conditions))
+            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):
-    return _volume_get_query(context).filter_by(consistencygroup_id=group_id).\
-        all()
+def volume_get_all_by_group(context, group_id, filters=None):
+    """Retrieves all volumes associated with the group_id.
+
+    :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
-                    '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.
+    :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
     """
     session = get_session()
@@ -1285,82 +1314,18 @@ 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,
-                    tuples, sets, or frozensets cause an 'IN' test to
-                    be performed, while exact matching ('==' operator)
-                    is used for other values
+    :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: updated query or None
     """
     query = _volume_get_query(context, session=session)
 
     if filters:
-        filters = filters.copy()
-
-        # 'no_migration_targets' is unique, must be either NULL or
-        # not start with 'target:'
-        if ('no_migration_targets' in filters and
-                filters['no_migration_targets'] is True):
-            filters.pop('no_migration_targets')
-            try:
-                column_attr = getattr(models.Volume, 'migration_status')
-                conditions = [column_attr == None,  # noqa
-                              column_attr.op('NOT LIKE')('target:%')]
-                query = query.filter(or_(*conditions))
-            except AttributeError:
-                log_msg = _("'migration_status' column could not be found.")
-                LOG.debug(log_msg)
-                return None
-
-        # Apply exact match filters for everything else, ensure that the
-        # filter value exists on the model
-        for key in filters.keys():
-            # 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(log_msg)
-                    return None
-                continue
-            try:
-                column_attr = getattr(models.Volume, key)
-                # Do not allow relationship properties since those require
-                # schema specific knowledge
-                prop = getattr(column_attr, 'property')
-                if isinstance(prop, RelationshipProperty):
-                    log_msg = (_("'%s' filter key is not valid, "
-                                 "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(log_msg)
-                return None
-
-        # Holds the simple exact matches
-        filter_dict = {}
-
-        # Iterate over all filters, special case the filter is necessary
-        for key, value in filters.iteritems():
-            if key == 'metadata':
-                # model.VolumeMetadata defines the backref to Volumes as
-                # 'volume_metadata' or 'volume_admin_metadata', use those as
-                # column attribute keys
-                col_attr = getattr(models.Volume, 'volume_metadata')
-                col_ad_attr = getattr(models.Volume, 'volume_admin_metadata')
-                for k, v in value.iteritems():
-                    query = query.filter(or_(col_attr.any(key=k, value=v),
-                                             col_ad_attr.any(key=k, value=v)))
-            elif isinstance(value, (list, tuple, set, frozenset)):
-                # Looking for values in a list; apply to query directly
-                column_attr = getattr(models.Volume, key)
-                query = query.filter(column_attr.in_(value))
-            else:
-                # OK, simple exact match; save for later
-                filter_dict[key] = value
-
-        # Apply simple exact matches
-        if filter_dict:
-            query = query.filter_by(**filter_dict)
+        query = _process_volume_filters(query, filters)
+        if query is None:
+            return None
 
     marker_volume = None
     if marker is not None:
@@ -1372,6 +1337,88 @@ def _generate_paginate_query(context, session, marker, limit, sort_key,
                                           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 filters.get('no_migration_targets', False):
+        filters.pop('no_migration_targets')
+        try:
+            column_attr = getattr(models.Volume, 'migration_status')
+            conditions = [column_attr == None,  # noqa
+                          column_attr.op('NOT LIKE')('target:%')]
+            query = query.filter(or_(*conditions))
+        except AttributeError:
+            LOG.debug("'migration_status' column could not be found.")
+            return None
+
+    # Apply exact match filters for everything else, ensure that the
+    # filter value exists on the model
+    for key in filters.keys():
+        # metadata is unique, must be a dict
+        if key == 'metadata':
+            if not isinstance(filters[key], dict):
+                LOG.debug("'metadata' filter value is not valid.")
+                return None
+            continue
+        try:
+            column_attr = getattr(models.Volume, key)
+            # Do not allow relationship properties since those require
+            # schema specific knowledge
+            prop = getattr(column_attr, 'property')
+            if isinstance(prop, RelationshipProperty):
+                LOG.debug(("'%s' filter key is not valid, "
+                           "it maps to a relationship."), key)
+                return None
+        except AttributeError:
+            LOG.debug("'%s' filter key is not valid.", key)
+            return None
+
+    # Holds the simple exact matches
+    filter_dict = {}
+
+    # 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
+            # 'volume_metadata' or 'volume_admin_metadata', use those as
+            # column attribute keys
+            col_attr = getattr(models.Volume, 'volume_metadata')
+            col_ad_attr = getattr(models.Volume, 'volume_admin_metadata')
+            for k, v in value.iteritems():
+                query = query.filter(or_(col_attr.any(key=k, value=v),
+                                         col_ad_attr.any(key=k, value=v)))
+        elif isinstance(value, (list, tuple, set, frozenset)):
+            # Looking for values in a list; apply to query directly
+            column_attr = getattr(models.Volume, key)
+            query = query.filter(column_attr.in_(value))
+        else:
+            # OK, simple exact match; save for later
+            filter_dict[key] = value
+
+    # Apply simple exact matches
+    if filter_dict:
+        query = query.filter_by(**filter_dict)
+    return query
+
+
 @require_admin_context
 def volume_get_iscsi_target_num(context, volume_id):
     result = model_query(context, models.IscsiTarget, read_deleted="yes").\

diff --git a/cinder/tests/test_db_api.py b/cinder/tests/test_db_api.py
index aad3a352d6379b3d3b0a629e6b8f81a51b9d2225..ae4dfcffd48fb0d61c87d20e55019d2ca4acc9c5 100644 (file)
--- a/cinder/tests/test_db_api.py
+++ b/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):