From 10dcf874ac16e410993d9d929a89bb07305f46da Mon Sep 17 00:00:00 2001 From: Steven Kaufer Date: Fri, 7 Mar 2014 23:01:51 +0000 Subject: [PATCH] get volumes with limit and filters does not work The /volumes and /volumes/detail REST APIs support filtering. Currently, all filtering is done after the volumes are retrieved from the database in the API.get_all function in /cinder/volume/api.py. Therefore, the usage combination of filters and limit will only work if all volumes matching the filters are in the page of data being retrieved from the database. For example, assume that all of the volumes with a name of "foo" would be retrieved from the database starting at index 100 and that you query for all volumes with a name of "foo" while specifying a limit of 50. In this case, the query would yield 0 results since the filter did not match any of the first 50 entries retrieved from the database. This patch removes all filtering from the volume API layer and moves it into the DB layer. Test cases were added to verify filtering at the DB level. Change-Id: Ia084e1f4cf59ea39bf8a0a36686146a315168cbb Closes-bug: 1287813 --- cinder/db/api.py | 10 +- cinder/db/sqlalchemy/api.py | 173 ++++++++++++++++--- cinder/tests/api/v1/test_volumes.py | 128 +------------- cinder/tests/api/v2/stubs.py | 2 +- cinder/tests/api/v2/test_volumes.py | 186 ++++++++------------ cinder/tests/test_db_api.py | 252 ++++++++++++++++++++++++++++ cinder/volume/api.py | 69 +++----- 7 files changed, 496 insertions(+), 324 deletions(-) diff --git a/cinder/db/api.py b/cinder/db/api.py index ccb40e7bf60..602a3084d08 100644 --- a/cinder/db/api.py +++ b/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): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index 41b94791338..e73b7324b0f 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/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) - - marker_volume = None - if marker is not None: - marker_volume = _volume_get(context, marker, session=session) - - query = sqlalchemyutils.paginate_query(query, models.Volume, limit, - [sort_key, 'created_at', 'id'], - marker=marker_volume, - sort_dir=sort_dir) - + # Generate the query + query = _generate_paginate_query(context, session, marker, limit, + sort_key, sort_dir, filters) + # No volumes would match, return empty list + if query == None: + return [] 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).\ - filter_by(project_id=project_id) - - marker_volume = None - if marker is not None: - marker_volume = _volume_get(context, marker, session) - - query = sqlalchemyutils.paginate_query(query, models.Volume, limit, - [sort_key, 'created_at', 'id'], - marker=marker_volume, - sort_dir=sort_dir) - + # Add in the project filter without modifying the given filters + filters = filters.copy() if filters else {} + filters['project_id'] = project_id + # Generate the query + query = _generate_paginate_query(context, session, marker, limit, + sort_key, sort_dir, filters) + # No volumes would match, return empty list + if query == None: + return [] 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").\ diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py index d072cd63ee2..1dcf653ecf9 100644 --- a/cinder/tests/api/v1/test_volumes.py +++ b/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'), diff --git a/cinder/tests/api/v2/stubs.py b/cinder/tests/api/v2/stubs.py index 49c2b80a197..dc269a67d99 100644 --- a/cinder/tests/api/v2/stubs.py +++ b/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')] diff --git a/cinder/tests/api/v2/test_volumes.py b/cinder/tests/api/v2/test_volumes.py index efdda4e87c0..fab7d98b6d1 100644 --- a/cinder/tests/api/v2/test_volumes.py +++ b/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): - return [ - stubs.stub_volume(1, display_name='vol1'), - stubs.stub_volume(2, display_name='vol2'), - stubs.stub_volume(3, display_name='vol3'), - ] + sort_key, sort_dir, filters=None): + self.assertEqual(filters['no_migration_targets'], True) + self.assertFalse('all_tenants' in filters) + return [stubs.stub_volume(1, display_name='vol1')] + + 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 - req = fakes.HTTPRequest.blank('/v2/volumes') - resp = self.controller.index(req) - self.assertEqual(len(resp['volumes']), 3) - # filter on name - req = fakes.HTTPRequest.blank('/v2/volumes?name=vol2') + # all_tenants does not matter for non-admin + for params in ['', '?all_tenants=1']: + req = fakes.HTTPRequest.blank('/v2/volumes%s' % params) + resp = self.controller.index(req) + self.assertEqual(len(resp['volumes']), 1) + 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): - 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'}]), - ] + # Admin, all_tenants is set, get_all function should be called + # without no_migration_status + def stub_volume_get_all_by_project3(context, project_id, marker, limit, + sort_key, sort_dir, filters=None): + return [] + + def stub_volume_get_all3(context, marker, limit, + sort_key, sort_dir, filters=None): + self.assertFalse('no_migration_targets' in filters) + self.assertFalse('all_tenants' in filters) + return [stubs.stub_volume(1, display_name='vol3')] 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', 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, + req = fakes.HTTPRequest.blank('/v2/volumes?all_tenants=1', use_admin_context=True) - resp = self.controller.detail(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) + resp = self.controller.index(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) diff --git a/cinder/tests/test_db_api.py b/cinder/tests/test_db_api.py index cb1cd7a5c92..0e9d49cb560 100644 --- a/cinder/tests/test_db_api.py +++ b/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}) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 37c3de30d8a..19cdade7cf7 100644 --- a/cinder/volume/api.py +++ b/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): - # Need to remove all_tenants to pass the filtering below. - del filters['all_tenants'] - volumes = self.db.volume_get_all(context, marker, limit, sort_key, - 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 + # Non-admin shouldn't see temporary target of a volume migration, add + # unique filter data to reflect that only volumes with a NULL + # 'migration_status' or a 'migration_status' that does not start with + # 'target:' should be returned (processed in db/sqlalchemy/api.py) 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): - volume_metadata = {} - for i in volume.get('volume_metadata'): - volume_metadata[i['key']] = i['value'] - - for k, v in searchdict.iteritems(): - if (k not in volume_metadata.keys() or - volume_metadata[k] != v): - return False - return True - - 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 + if (context.is_admin and 'all_tenants' in filters): + # Need to remove all_tenants to pass the filtering below. + del filters['all_tenants'] + volumes = self.db.volume_get_all(context, marker, limit, sort_key, + sort_dir, filters=filters) + else: + volumes = self.db.volume_get_all_by_project(context, + context.project_id, + marker, limit, + sort_key, sort_dir, + filters=filters) return volumes