Merge "get volumes with limit and filters does not work"

cinder/db/api.py

@@ -205,9 +205,11 @@ def volume_get(context, volume_id):
     return IMPL.volume_get(context, volume_id)
 
 
-def volume_get_all(context, marker, limit, sort_key, sort_dir):
+def volume_get_all(context, marker, limit, sort_key, sort_dir,
+                   filters=None):
     """Get all volumes."""
-    return IMPL.volume_get_all(context, marker, limit, sort_key, sort_dir)
+    return IMPL.volume_get_all(context, marker, limit, sort_key, sort_dir,
+                               filters=filters)
 
 
 def volume_get_all_by_host(context, host):
@@ -221,10 +223,10 @@ def volume_get_all_by_instance_uuid(context, instance_uuid):
 
 
 def volume_get_all_by_project(context, project_id, marker, limit, sort_key,
-                              sort_dir):
+                              sort_dir, filters=None):
     """Get all volumes belonging to a project."""
     return IMPL.volume_get_all_by_project(context, project_id, marker, limit,
-                                          sort_key, sort_dir)
+                                          sort_key, sort_dir, filters=filters)
 
 
 def volume_get_iscsi_target_num(context, volume_id):

cinder/db/sqlalchemy/api.py

@@ -1,6 +1,7 @@
 # Copyright (c) 2011 X.commerce, a business unit of eBay Inc.
 # Copyright 2010 United States Government as represented by the
 # Administrator of the National Aeronautics and Space Administration.
+# Copyright 2014 IBM Corp.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -26,6 +27,7 @@ from oslo.config import cfg
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy import or_
 from sqlalchemy.orm import joinedload, joinedload_all
+from sqlalchemy.orm import RelationshipProperty
 from sqlalchemy.sql.expression import literal_column
 from sqlalchemy.sql import func
 
@@ -1153,20 +1155,30 @@ def volume_get(context, volume_id):
 
 
 @require_admin_context
-def volume_get_all(context, marker, limit, sort_key, sort_dir):
+def volume_get_all(context, marker, limit, sort_key, sort_dir,
+                   filters=None):
+    """Retrieves all volumes.
+
+    :param context: context to query under
+    :param marker: the last item of the previous page, used to determine the
+                   next page of results to return
+    :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.
+    :returns: list of matching volumes
+    """
     session = get_session()
     with session.begin():
-        query = _volume_get_query(context, session=session)
+        # Generate the query
-
+        query = _generate_paginate_query(context, session, marker, limit,
-        marker_volume = None
+                                         sort_key, sort_dir, filters)
-        if marker is not None:
+        # No volumes would match, return empty list
-            marker_volume = _volume_get(context, marker, session=session)
+        if query == None:
-
+            return []
-        query = sqlalchemyutils.paginate_query(query, models.Volume, limit,
-                                               [sort_key, 'created_at', 'id'],
-                                               marker=marker_volume,
-                                               sort_dir=sort_dir)
-
         return query.all()
 
 
@@ -1192,25 +1204,136 @@ def volume_get_all_by_instance_uuid(context, instance_uuid):
 
 @require_context
 def volume_get_all_by_project(context, project_id, marker, limit, sort_key,
-                              sort_dir):
+                              sort_dir, filters=None):
+    """"Retrieves all volumes in a project.
+
+    :param context: context to query under
+    :param project_id: project for all volumes being retrieved
+    :param marker: the last item of the previous page, used to determine the
+                   next page of results to return
+    :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.
+    :returns: list of matching volumes
+    """
     session = get_session()
     with session.begin():
         authorize_project_context(context, project_id)
-        query = _volume_get_query(context, session).\
+        # Add in the project filter without modifying the given filters
-            filter_by(project_id=project_id)
+        filters = filters.copy() if filters else {}
-
+        filters['project_id'] = project_id
-        marker_volume = None
+        # Generate the query
-        if marker is not None:
+        query = _generate_paginate_query(context, session, marker, limit,
-            marker_volume = _volume_get(context, marker, session)
+                                         sort_key, sort_dir, filters)
-
+        # No volumes would match, return empty list
-        query = sqlalchemyutils.paginate_query(query, models.Volume, limit,
+        if query == None:
-                                               [sort_key, 'created_at', 'id'],
+            return []
-                                               marker=marker_volume,
-                                               sort_dir=sort_dir)
-
         return query.all()
 
 
+def _generate_paginate_query(context, session, marker, limit, sort_key,
+                             sort_dir, filters):
+    """Generate the query to include the filters and the paginate options.
+
+    Returns a query with sorting / pagination criteria added or None
+    if the given filters will not yield any results.
+
+    :param context: context to query under
+    :param session: the session to use
+    :param marker: the last item of the previous page; we returns the next
+                    results after this value.
+    :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
+    :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'] == True):
+            filters.pop('no_migration_targets')
+            try:
+                column_attr = getattr(models.Volume, 'migration_status')
+                conditions = [column_attr == None,
+                              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', use that column attribute key
+                key = 'volume_metadata'
+                column_attr = getattr(models.Volume, key)
+                for k, v in value.iteritems():
+                    query = query.filter(column_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)
+
+    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
 def volume_get_iscsi_target_num(context, volume_id):
     result = model_query(context, models.IscsiTarget, read_deleted="yes").\

cinder/tests/api/v1/test_volumes.py

@@ -17,7 +17,6 @@ import datetime
 
 from lxml import etree
 from oslo.config import cfg
-import urllib
 import webob
 
 from cinder.api import extensions
@@ -517,130 +516,6 @@ class VolumeApiTest(test.TestCase):
                                  'size': 1}]}
         self.assertEqual(res_dict, expected)
 
-    def test_volume_list_by_name(self):
-        def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
-            return [
-                stubs.stub_volume(1, display_name='vol1'),
-                stubs.stub_volume(2, display_name='vol2'),
-                stubs.stub_volume(3, display_name='vol3'),
-            ]
-        self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db)
-        self.stubs.Set(db, 'volume_get_all_by_project',
-                       stub_volume_get_all_by_project)
-
-        # no display_name filter
-        req = fakes.HTTPRequest.blank('/v1/volumes')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 3)
-        # filter on display_name
-        req = fakes.HTTPRequest.blank('/v1/volumes?display_name=vol2')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['display_name'], 'vol2')
-        # filter no match
-        req = fakes.HTTPRequest.blank('/v1/volumes?display_name=vol4')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 0)
-
-    def test_volume_list_by_metadata(self):
-        def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
-            return [
-                stubs.stub_volume(1, display_name='vol1',
-                                  status='available',
-                                  volume_metadata=[{'key': 'key1',
-                                                    'value': 'value1'}]),
-                stubs.stub_volume(2, display_name='vol2',
-                                  status='available',
-                                  volume_metadata=[{'key': 'key1',
-                                                    'value': 'value2'}]),
-                stubs.stub_volume(3, display_name='vol3',
-                                  status='in-use',
-                                  volume_metadata=[{'key': 'key1',
-                                                    'value': 'value2'}]),
-            ]
-        self.stubs.Set(db, 'volume_get_all_by_project',
-                       stub_volume_get_all_by_project)
-
-        # no metadata filter
-        req = fakes.HTTPRequest.blank('/v1/volumes', use_admin_context=True)
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 3)
-
-        # single match
-        qparams = urllib.urlencode({'metadata': {'key1': 'value1'}})
-        req = fakes.HTTPRequest.blank('/v1/volumes?%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['display_name'], 'vol1')
-        self.assertEqual(resp['volumes'][0]['metadata']['key1'], 'value1')
-
-        # multiple matches
-        qparams = urllib.urlencode({'metadata': {'key1': 'value2'}})
-        req = fakes.HTTPRequest.blank('/v1/volumes?%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 2)
-        for volume in resp['volumes']:
-            self.assertEqual(volume['metadata']['key1'], 'value2')
-
-        # multiple filters
-        qparams = urllib.urlencode({'metadata': {'key1': 'value2'}})
-        req = fakes.HTTPRequest.blank('/v1/volumes?status=in-use&%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['display_name'], 'vol3')
-
-        # no match
-        qparams = urllib.urlencode({'metadata': {'key1': 'value3'}})
-        req = fakes.HTTPRequest.blank('/v1/volumes?%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 0)
-
-    def test_volume_list_by_status(self):
-        def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
-            return [
-                stubs.stub_volume(1, display_name='vol1', status='available'),
-                stubs.stub_volume(2, display_name='vol2', status='available'),
-                stubs.stub_volume(3, display_name='vol3', status='in-use'),
-            ]
-        self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db)
-        self.stubs.Set(db, 'volume_get_all_by_project',
-                       stub_volume_get_all_by_project)
-
-        # no status filter
-        req = fakes.HTTPRequest.blank('/v1/volumes')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 3)
-        # single match
-        req = fakes.HTTPRequest.blank('/v1/volumes?status=in-use')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['status'], 'in-use')
-        # multiple match
-        req = fakes.HTTPRequest.blank('/v1/volumes?status=available')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 2)
-        for volume in resp['volumes']:
-            self.assertEqual(volume['status'], 'available')
-        # multiple filters
-        req = fakes.HTTPRequest.blank('/v1/volumes?status=available&'
-                                      'display_name=vol1')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['display_name'], 'vol1')
-        self.assertEqual(resp['volumes'][0]['status'], 'available')
-        # no match
-        req = fakes.HTTPRequest.blank('/v1/volumes?status=in-use&'
-                                      'display_name=vol1')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 0)
-
     def test_volume_show(self):
         self.stubs.Set(db, 'volume_get', stubs.stub_volume_get_db)
 
@@ -740,7 +615,8 @@ class VolumeApiTest(test.TestCase):
     def test_volume_detail_limit_offset(self):
         def volume_detail_limit_offset(is_admin):
             def stub_volume_get_all_by_project(context, project_id, marker,
-                                               limit, sort_key, sort_dir):
+                                               limit, sort_key, sort_dir,
+                                               filters=None):
                 return [
                     stubs.stub_volume(1, display_name='vol1'),
                     stubs.stub_volume(2, display_name='vol2'),

cinder/tests/api/v2/stubs.py

@@ -109,7 +109,7 @@ def stub_volume_get_db(context, volume_id):
 
 
 def stub_volume_get_all(context, search_opts=None, marker=None, limit=None,
-                        sort_key='created_at', sort_dir='desc'):
+                        sort_key='created_at', sort_dir='desc', filters=None):
     return [stub_volume(100, project_id='fake'),
             stub_volume(101, project_id='superfake'),
             stub_volume(102, project_id='superduperfake')]

cinder/tests/api/v2/test_volumes.py

@@ -18,7 +18,6 @@ import datetime
 
 from lxml import etree
 from oslo.config import cfg
-import urllib
 import webob
 
 from cinder.api import extensions
@@ -584,7 +583,7 @@ class VolumeApiTest(test.TestCase):
 
     def test_volume_index_with_marker(self):
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
+                                           sort_key, sort_dir, filters=None):
             return [
                 stubs.stub_volume(1, display_name='vol1'),
                 stubs.stub_volume(2, display_name='vol2'),
@@ -635,7 +634,7 @@ class VolumeApiTest(test.TestCase):
 
     def test_volume_index_limit_offset(self):
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
+                                           sort_key, sort_dir, filters=None):
             return [
                 stubs.stub_volume(1, display_name='vol1'),
                 stubs.stub_volume(2, display_name='vol2'),
@@ -662,7 +661,7 @@ class VolumeApiTest(test.TestCase):
 
     def test_volume_detail_with_marker(self):
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
+                                           sort_key, sort_dir, filters=None):
             return [
                 stubs.stub_volume(1, display_name='vol1'),
                 stubs.stub_volume(2, display_name='vol2'),
@@ -713,7 +712,7 @@ class VolumeApiTest(test.TestCase):
 
     def test_volume_detail_limit_offset(self):
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
+                                           sort_key, sort_dir, filters=None):
             return [
                 stubs.stub_volume(1, display_name='vol1'),
                 stubs.stub_volume(2, display_name='vol2'),
@@ -760,7 +759,8 @@ class VolumeApiTest(test.TestCase):
 
         # Number of volumes equals the max, include next link
         def stub_volume_get_all(context, marker, limit,
-                                sort_key, sort_dir):
+                                sort_key, sort_dir,
+                                filters=None):
             vols = [stubs.stub_volume(i)
                     for i in xrange(CONF.osapi_max_limit)]
             if limit == None or limit >= len(vols):
@@ -777,7 +777,8 @@ class VolumeApiTest(test.TestCase):
 
         # Number of volumes less then max, do not include
         def stub_volume_get_all2(context, marker, limit,
-                                 sort_key, sort_dir):
+                                 sort_key, sort_dir,
+                                 filters=None):
             vols = [stubs.stub_volume(i)
                     for i in xrange(100)]
             if limit == None or limit >= len(vols):
@@ -793,7 +794,8 @@ class VolumeApiTest(test.TestCase):
 
         # Number of volumes more then the max, include next link
         def stub_volume_get_all3(context, marker, limit,
-                                 sort_key, sort_dir):
+                                 sort_key, sort_dir,
+                                 filters=None):
             vols = [stubs.stub_volume(i)
                     for i in xrange(CONF.osapi_max_limit + 100)]
             if limit == None or limit >= len(vols):
@@ -819,130 +821,78 @@ class VolumeApiTest(test.TestCase):
             volumes_links = res_dict['volumes_links']
             self.assertEqual(volumes_links[0]['rel'], 'next')
 
-    def test_volume_list_by_name(self):
+    def test_volume_list_default_filters(self):
+        """Tests that the default filters from volume.api.API.get_all are set.
+
+        1. 'no_migration_status'=True for non-admins and get_all_by_project is
+        invoked.
+        2. 'no_migration_status' is not included for admins.
+        3. When 'all_tenants' is not specified, then it is removed and
+        get_all_by_project is invoked for admins.
+        3. When 'all_tenants' is specified, then it is removed and get_all
+        is invoked for admins.
+        """
+        # Non-admin, project function should be called with no_migration_status
         def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
+                                           sort_key, sort_dir, filters=None):
-            return [
+            self.assertEqual(filters['no_migration_targets'], True)
-                stubs.stub_volume(1, display_name='vol1'),
+            self.assertFalse('all_tenants' in filters)
-                stubs.stub_volume(2, display_name='vol2'),
+            return [stubs.stub_volume(1, display_name='vol1')]
-                stubs.stub_volume(3, display_name='vol3'),
+
-            ]
+        def stub_volume_get_all(context, marker, limit,
+                                sort_key, sort_dir, filters=None):
+            return []
         self.stubs.Set(db, 'volume_get_all_by_project',
                        stub_volume_get_all_by_project)
-        self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get)
+        self.stubs.Set(db, 'volume_get_all', stub_volume_get_all)
 
-        # no name filter
+        # all_tenants does not matter for non-admin
-        req = fakes.HTTPRequest.blank('/v2/volumes')
+        for params in ['', '?all_tenants=1']:
-        resp = self.controller.index(req)
+            req = fakes.HTTPRequest.blank('/v2/volumes%s' % params)
-        self.assertEqual(len(resp['volumes']), 3)
+            resp = self.controller.index(req)
-        # filter on name
+            self.assertEqual(len(resp['volumes']), 1)
-        req = fakes.HTTPRequest.blank('/v2/volumes?name=vol2')
+            self.assertEqual(resp['volumes'][0]['name'], 'vol1')
+
+        # Admin, all_tenants is not set, project function should be called
+        # without no_migration_status
+        def stub_volume_get_all_by_project2(context, project_id, marker, limit,
+                                            sort_key, sort_dir, filters=None):
+            self.assertFalse('no_migration_targets' in filters)
+            return [stubs.stub_volume(1, display_name='vol2')]
+
+        def stub_volume_get_all2(context, marker, limit,
+                                 sort_key, sort_dir, filters=None):
+            return []
+        self.stubs.Set(db, 'volume_get_all_by_project',
+                       stub_volume_get_all_by_project2)
+        self.stubs.Set(db, 'volume_get_all', stub_volume_get_all2)
+
+        req = fakes.HTTPRequest.blank('/v2/volumes', use_admin_context=True)
         resp = self.controller.index(req)
         self.assertEqual(len(resp['volumes']), 1)
         self.assertEqual(resp['volumes'][0]['name'], 'vol2')
-        # filter no match
-        req = fakes.HTTPRequest.blank('/v2/volumes?name=vol4')
-        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 0)
 
-    def test_volume_list_by_metadata(self):
+        # Admin, all_tenants is set, get_all function should be called
-        def stub_volume_get_all_by_project(context, project_id, marker, limit,
+        # without no_migration_status
-                                           sort_key, sort_dir):
+        def stub_volume_get_all_by_project3(context, project_id, marker, limit,
-            return [
+                                            sort_key, sort_dir, filters=None):
-                stubs.stub_volume(1, display_name='vol1',
+            return []
-                                  status='available',
+
-                                  volume_metadata=[{'key': 'key1',
+        def stub_volume_get_all3(context, marker, limit,
-                                                    'value': 'value1'}]),
+                                 sort_key, sort_dir, filters=None):
-                stubs.stub_volume(2, display_name='vol2',
+            self.assertFalse('no_migration_targets' in filters)
-                                  status='available',
+            self.assertFalse('all_tenants' in filters)
-                                  volume_metadata=[{'key': 'key1',
+            return [stubs.stub_volume(1, display_name='vol3')]
-                                                    'value': 'value2'}]),
-                stubs.stub_volume(3, display_name='vol3',
-                                  status='in-use',
-                                  volume_metadata=[{'key': 'key1',
-                                                    'value': 'value2'}]),
-            ]
         self.stubs.Set(db, 'volume_get_all_by_project',
-                       stub_volume_get_all_by_project)
+                       stub_volume_get_all_by_project3)
+        self.stubs.Set(db, 'volume_get_all', stub_volume_get_all3)
 
-        # no metadata filter
+        req = fakes.HTTPRequest.blank('/v2/volumes?all_tenants=1',
-        req = fakes.HTTPRequest.blank('/v2/volumes', use_admin_context=True)
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 3)
-
-        # single match
-        qparams = urllib.urlencode({'metadata': {'key1': 'value1'}})
-        req = fakes.HTTPRequest.blank('/v2/volumes?%s' % qparams,
                                       use_admin_context=True)
-        resp = self.controller.detail(req)
+        resp = self.controller.index(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['name'], 'vol1')
-        self.assertEqual(resp['volumes'][0]['metadata']['key1'], 'value1')
-
-        # multiple matches
-        qparams = urllib.urlencode({'metadata': {'key1': 'value2'}})
-        req = fakes.HTTPRequest.blank('/v2/volumes?%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 2)
-        for volume in resp['volumes']:
-            self.assertEqual(volume['metadata']['key1'], 'value2')
-
-        # multiple filters
-        qparams = urllib.urlencode({'metadata': {'key1': 'value2'}})
-        req = fakes.HTTPRequest.blank('/v2/volumes?status=in-use&%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.detail(req)
         self.assertEqual(len(resp['volumes']), 1)
         self.assertEqual(resp['volumes'][0]['name'], 'vol3')
 
-        # no match
-        qparams = urllib.urlencode({'metadata': {'key1': 'value3'}})
-        req = fakes.HTTPRequest.blank('/v2/volumes?%s' % qparams,
-                                      use_admin_context=True)
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 0)
-
-    def test_volume_list_by_status(self):
-        def stub_volume_get_all_by_project(context, project_id, marker, limit,
-                                           sort_key, sort_dir):
-            return [
-                stubs.stub_volume(1, display_name='vol1', status='available'),
-                stubs.stub_volume(2, display_name='vol2', status='available'),
-                stubs.stub_volume(3, display_name='vol3', status='in-use'),
-            ]
-        self.stubs.Set(db, 'volume_get_all_by_project',
-                       stub_volume_get_all_by_project)
-        self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get)
-
-        # no status filter
-        req = fakes.HTTPRequest.blank('/v2/volumes/details')
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 3)
-        # single match
-        req = fakes.HTTPRequest.blank('/v2/volumes/details?status=in-use')
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['status'], 'in-use')
-        # multiple match
-        req = fakes.HTTPRequest.blank('/v2/volumes/details/?status=available')
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 2)
-        for volume in resp['volumes']:
-            self.assertEqual(volume['status'], 'available')
-        # multiple filters
-        req = fakes.HTTPRequest.blank('/v2/volumes/details/?status=available&'
-                                      'name=vol1')
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 1)
-        self.assertEqual(resp['volumes'][0]['name'], 'vol1')
-        self.assertEqual(resp['volumes'][0]['status'], 'available')
-        # no match
-        req = fakes.HTTPRequest.blank('/v2/volumes/details?status=in-use&'
-                                      'name=vol1')
-        resp = self.controller.detail(req)
-        self.assertEqual(len(resp['volumes']), 0)
-
     def test_volume_show(self):
         self.stubs.Set(volume_api.API, 'get', stubs.stub_volume_get)
 

cinder/tests/test_db_api.py

@@ -1,3 +1,4 @@
+#    Copyright 2014 IBM Corp.
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
 #    not use this file except in compliance with the License. You may obtain
 #    a copy of the License at
@@ -410,6 +411,257 @@ class DBAPIVolumeTestCase(BaseTest):
                                             self.ctxt, 'p%d' % i, None,
                                             None, 'host', None))
 
+    def test_volume_get_by_name(self):
+        db.volume_create(self.ctxt, {'display_name': 'vol1'})
+        db.volume_create(self.ctxt, {'display_name': 'vol2'})
+        db.volume_create(self.ctxt, {'display_name': 'vol3'})
+
+        # no name filter
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc')
+        self.assertEqual(len(volumes), 3)
+        # filter on name
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc', {'display_name': 'vol2'})
+        self.assertEqual(len(volumes), 1)
+        self.assertEqual(volumes[0]['display_name'], 'vol2')
+        # filter no match
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc', {'display_name': 'vol4'})
+        self.assertEqual(len(volumes), 0)
+
+    def test_volume_list_by_status(self):
+        db.volume_create(self.ctxt, {'display_name': 'vol1',
+                                     'status': 'available'})
+        db.volume_create(self.ctxt, {'display_name': 'vol2',
+                                     'status': 'available'})
+        db.volume_create(self.ctxt, {'display_name': 'vol3',
+                                     'status': 'in-use'})
+
+        # no status filter
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc')
+        self.assertEqual(len(volumes), 3)
+        # single match
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc', {'status': 'in-use'})
+        self.assertEqual(len(volumes), 1)
+        self.assertEqual(volumes[0]['status'], 'in-use')
+        # multiple match
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc', {'status': 'available'})
+        self.assertEqual(len(volumes), 2)
+        for volume in volumes:
+            self.assertEqual(volume['status'], 'available')
+        # multiple filters
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc', {'status': 'available',
+                                            'display_name': 'vol1'})
+        self.assertEqual(len(volumes), 1)
+        self.assertEqual(volumes[0]['display_name'], 'vol1')
+        self.assertEqual(volumes[0]['status'], 'available')
+        # no match
+        volumes = db.volume_get_all(self.ctxt, None, None, 'created_at',
+                                    'asc', {'status': 'in-use',
+                                            'display_name': 'vol1'})
+        self.assertEqual(len(volumes), 0)
+
+    def _assertEqualsVolumeOrderResult(self, correct_order, limit=None,
+                                       sort_key='created_at', sort_dir='asc',
+                                       filters=None, project_id=None,
+                                       match_keys=['id', 'display_name',
+                                                   'volume_metadata',
+                                                   'created_at']):
+        """"Verifies that volumes are returned in the correct order."""
+        if project_id:
+            result = db.volume_get_all_by_project(self.ctxt, project_id, None,
+                                                  limit, sort_key,
+                                                  sort_dir, filters=filters)
+        else:
+            result = db.volume_get_all(self.ctxt, None, limit, sort_key,
+                                       sort_dir, filters=filters)
+        self.assertEqual(len(correct_order), len(result))
+        self.assertEqual(len(result), len(correct_order))
+        for vol1, vol2 in zip(result, correct_order):
+            for key in match_keys:
+                val1 = vol1.get(key)
+                val2 = vol2.get(key)
+                # metadata is a list, compare the 'key' and 'value' of each
+                if key == 'volume_metadata':
+                    self.assertEqual(len(val1), len(val2))
+                    for m1, m2 in zip(val1, val2):
+                        self.assertEqual(m1.get('key'), m2.get('key'))
+                        self.assertEqual(m1.get('value'), m2.get('value'))
+                else:
+                    self.assertEqual(val1, val2)
+
+    def test_volume_get_by_filter(self):
+        """Verifies that all filtering is done at the DB layer."""
+        vols = []
+        vols.extend([db.volume_create(self.ctxt,
+                                      {'project_id': 'g1',
+                                       'display_name': 'name_%d' % i,
+                                       'size': 1})
+                     for i in xrange(2)])
+        vols.extend([db.volume_create(self.ctxt,
+                                      {'project_id': 'g1',
+                                       'display_name': 'name_%d' % i,
+                                       'size': 2})
+                     for i in xrange(2)])
+        vols.extend([db.volume_create(self.ctxt,
+                                      {'project_id': 'g1',
+                                       'display_name': 'name_%d' % i})
+                     for i in xrange(2)])
+        vols.extend([db.volume_create(self.ctxt,
+                                      {'project_id': 'g2',
+                                       'display_name': 'name_%d' % i,
+                                       'size': 1})
+                     for i in xrange(2)])
+
+        # By project, filter on size and name
+        filters = {'size': '1'}
+        correct_order = [vols[0], vols[1]]
+        self._assertEqualsVolumeOrderResult(correct_order, filters=filters,
+                                            project_id='g1')
+        filters = {'size': '1', 'display_name': 'name_1'}
+        correct_order = [vols[1]]
+        self._assertEqualsVolumeOrderResult(correct_order, filters=filters,
+                                            project_id='g1')
+
+        # Remove project scope
+        filters = {'size': '1'}
+        correct_order = [vols[0], vols[1], vols[6], vols[7]]
+        self._assertEqualsVolumeOrderResult(correct_order, filters=filters)
+        filters = {'size': '1', 'display_name': 'name_1'}
+        correct_order = [vols[1], vols[7]]
+        self._assertEqualsVolumeOrderResult(correct_order, filters=filters)
+
+        # Remove size constraint
+        filters = {'display_name': 'name_1'}
+        correct_order = [vols[1], vols[3], vols[5]]
+        self._assertEqualsVolumeOrderResult(correct_order, filters=filters,
+                                            project_id='g1')
+        correct_order = [vols[1], vols[3], vols[5], vols[7]]
+        self._assertEqualsVolumeOrderResult(correct_order, filters=filters)
+
+        # Verify bogus values return nothing
+        filters = {'display_name': 'name_1', 'bogus_value': 'foo'}
+        self._assertEqualsVolumeOrderResult([], filters=filters,
+                                            project_id='g1')
+        self._assertEqualsVolumeOrderResult([], project_id='bogus')
+        self._assertEqualsVolumeOrderResult([], filters=filters)
+        self._assertEqualsVolumeOrderResult([], filters={'metadata':
+                                                         'not valid'})
+        self._assertEqualsVolumeOrderResult([], filters={'metadata':
+                                                         ['not', 'valid']})
+
+        # Verify that relationship property keys return nothing, these
+        # exist on the Volumes model but are not columns
+        filters = {'volume_type': 'bogus_type'}
+        self._assertEqualsVolumeOrderResult([], filters=filters)
+
+    def test_volume_get_all_filters_limit(self):
+        vol1 = db.volume_create(self.ctxt, {'display_name': 'test1'})
+        vol2 = db.volume_create(self.ctxt, {'display_name': 'test2'})
+        vol3 = db.volume_create(self.ctxt, {'display_name': 'test2',
+                                            'metadata': {'key1': 'val1'}})
+        vol4 = db.volume_create(self.ctxt, {'display_name': 'test3',
+                                            'metadata': {'key1': 'val1',
+                                                         'key2': 'val2'}})
+        vol5 = db.volume_create(self.ctxt, {'display_name': 'test3',
+                                            'metadata': {'key2': 'val2',
+                                                         'key3': 'val3'},
+                                            'host': 'host5'})
+        vols = [vol1, vol2, vol3, vol4, vol5]
+
+        # Ensure we have 5 total instances
+        self._assertEqualsVolumeOrderResult(vols)
+
+        # No filters, test limit
+        self._assertEqualsVolumeOrderResult(vols[:1], limit=1)
+        self._assertEqualsVolumeOrderResult(vols[:4], limit=4)
+
+        # Just the test2 volumes
+        filters = {'display_name': 'test2'}
+        self._assertEqualsVolumeOrderResult([vol2, vol3], filters=filters)
+        self._assertEqualsVolumeOrderResult([vol2], limit=1,
+                                            filters=filters)
+        self._assertEqualsVolumeOrderResult([vol2, vol3], limit=2,
+                                            filters=filters)
+        self._assertEqualsVolumeOrderResult([vol2, vol3], limit=100,
+                                            filters=filters)
+
+        # metdata filters
+        filters = {'metadata': {'key1': 'val1'}}
+        self._assertEqualsVolumeOrderResult([vol3, vol4], filters=filters)
+        self._assertEqualsVolumeOrderResult([vol3], limit=1,
+                                            filters=filters)
+        self._assertEqualsVolumeOrderResult([vol3, vol4], limit=10,
+                                            filters=filters)
+
+        filters = {'metadata': {'key1': 'val1',
+                                'key2': 'val2'}}
+        self._assertEqualsVolumeOrderResult([vol4], filters=filters)
+        self._assertEqualsVolumeOrderResult([vol4], limit=1,
+                                            filters=filters)
+
+        # No match
+        filters = {'metadata': {'key1': 'val1',
+                                'key2': 'val2',
+                                'key3': 'val3'}}
+        self._assertEqualsVolumeOrderResult([], filters=filters)
+        filters = {'metadata': {'key1': 'val1',
+                                'key2': 'bogus'}}
+        self._assertEqualsVolumeOrderResult([], filters=filters)
+        filters = {'metadata': {'key1': 'val1',
+                                'key2': 'val1'}}
+        self._assertEqualsVolumeOrderResult([], filters=filters)
+
+        # Combination
+        filters = {'display_name': 'test2',
+                   'metadata': {'key1': 'val1'}}
+        self._assertEqualsVolumeOrderResult([vol3], filters=filters)
+        self._assertEqualsVolumeOrderResult([vol3], limit=1,
+                                            filters=filters)
+        self._assertEqualsVolumeOrderResult([vol3], limit=100,
+                                            filters=filters)
+        filters = {'display_name': 'test3',
+                   'metadata': {'key2': 'val2',
+                                'key3': 'val3'},
+                   'host': 'host5'}
+        self._assertEqualsVolumeOrderResult([vol5], filters=filters)
+        self._assertEqualsVolumeOrderResult([vol5], limit=1,
+                                            filters=filters)
+
+    def test_volume_get_no_migration_targets(self):
+        """Verifies the unique 'no_migration_targets'=True filter.
+
+        This filter returns volumes with either a NULL 'migration_status'
+        or a non-NULL value that does not start with 'target:'.
+        """
+        vol1 = db.volume_create(self.ctxt, {'display_name': 'test1'})
+        vol2 = db.volume_create(self.ctxt, {'display_name': 'test2',
+                                            'migration_status': 'bogus'})
+        vol3 = db.volume_create(self.ctxt, {'display_name': 'test3',
+                                            'migration_status': 'btarget:'})
+        vol4 = db.volume_create(self.ctxt, {'display_name': 'test4',
+                                            'migration_status': 'target:'})
+        vols = [vol1, vol2, vol3, vol4]
+
+        # Ensure we have 4 total instances
+        self._assertEqualsVolumeOrderResult(vols)
+
+        # Apply the unique filter
+        filters = {'no_migration_targets': True}
+        self._assertEqualsVolumeOrderResult([vol1, vol2, vol3],
+                                            filters=filters)
+        self._assertEqualsVolumeOrderResult([vol1, vol2], limit=2,
+                                            filters=filters)
+
+        filters = {'no_migration_targets': True,
+                   'display_name': 'test4'}
+        self._assertEqualsVolumeOrderResult([], filters=filters)
+
     def test_volume_get_iscsi_target_num(self):
         target = db.iscsi_target_create_safe(self.ctxt, {'volume_id': 42,
                                                          'target_num': 43})

cinder/volume/api.py

@@ -267,8 +267,10 @@ class API(base.Base):
         return volume
 
     def get_all(self, context, marker=None, limit=None, sort_key='created_at',
-                sort_dir='desc', filters={}):
+                sort_dir='desc', filters=None):
         check_policy(context, 'get_all')
+        if filters == None:
+            filters = {}
 
         try:
             if limit is not None:
@@ -280,60 +282,27 @@ class API(base.Base):
             msg = _('limit param must be an integer')
             raise exception.InvalidInput(reason=msg)
 
-        if (context.is_admin and 'all_tenants' in filters):
+        # Non-admin shouldn't see temporary target of a volume migration, add
-            # Need to remove all_tenants to pass the filtering below.
+        # unique filter data to reflect that only volumes with a NULL
-            del filters['all_tenants']
+        # 'migration_status' or a 'migration_status' that does not start with
-            volumes = self.db.volume_get_all(context, marker, limit, sort_key,
+        # 'target:' should be returned (processed in db/sqlalchemy/api.py)
-                                             sort_dir)
-        else:
-            volumes = self.db.volume_get_all_by_project(context,
-                                                        context.project_id,
-                                                        marker, limit,
-                                                        sort_key, sort_dir)
-
-        # Non-admin shouldn't see temporary target of a volume migration
         if not context.is_admin:
             filters['no_migration_targets'] = True
 
         if filters:
-            LOG.debug(_("Searching by: %s") % filters)
+            LOG.debug(_("Searching by: %s") % str(filters))
 
-            def _check_metadata_match(volume, searchdict):
+        if (context.is_admin and 'all_tenants' in filters):
-                volume_metadata = {}
+            # Need to remove all_tenants to pass the filtering below.
-                for i in volume.get('volume_metadata'):
+            del filters['all_tenants']
-                    volume_metadata[i['key']] = i['value']
+            volumes = self.db.volume_get_all(context, marker, limit, sort_key,
-
+                                             sort_dir, filters=filters)
-                for k, v in searchdict.iteritems():
+        else:
-                    if (k not in volume_metadata.keys() or
+            volumes = self.db.volume_get_all_by_project(context,
-                            volume_metadata[k] != v):
+                                                        context.project_id,
-                        return False
+                                                        marker, limit,
-                return True
+                                                        sort_key, sort_dir,
-
+                                                        filters=filters)
-            def _check_migration_target(volume, searchdict):
-                status = volume['migration_status']
-                if status and status.startswith('target:'):
-                    return False
-                return True
-
-            # search_option to filter_name mapping.
-            filter_mapping = {'metadata': _check_metadata_match,
-                              'no_migration_targets': _check_migration_target}
-
-            result = []
-            not_found = object()
-            for volume in volumes:
-                # go over all filters in the list
-                for opt, values in filters.iteritems():
-                    try:
-                        filter_func = filter_mapping[opt]
-                    except KeyError:
-                        def filter_func(volume, value):
-                            return volume.get(opt, not_found) == value
-                    if not filter_func(volume, values):
-                        break  # volume doesn't match this filter
-                else:  # did not break out loop
-                    result.append(volume)  # volume matches all filters
-            volumes = result
 
         return volumes