Filter volumes and snapshots by query string

Fix query string search_opts filters in api.openstack.volume.volumes and
snapshots.  Fix get_all methods in volumes.api to adequately handle
parameter matching.

The current api does not properly apply query string filters by
display_name, status, or (in the case of snapshots) volume_id.  These
filters are needed both for the end user and operator to display logical
grouping of these resources.

Fixes: bug #1046353
Change-Id: Ia12ffe1bf8a27c8a78daa24c4b728b394932a2b0

diff --git a/cinder/api/openstack/volume/snapshots.py b/cinder/api/openstack/volume/snapshots.py
index 9fe84a7fd9e41baaf2a61a65ea8327b62ca08f5b..3254382b39e19cebbb75f564acebfa4d03ad1498 100644 (file)
--- a/cinder/api/openstack/volume/snapshots.py
+++ b/cinder/api/openstack/volume/snapshots.py
@@ -21,6 +21,7 @@ import webob
 from cinder.api.openstack import common
 from cinder.api.openstack import wsgi
 from cinder.api.openstack import xmlutil
+from cinder.api.openstack.volume import volumes
 from cinder import exception
 from cinder import flags
 from cinder.openstack.common import log as logging
@@ -131,6 +132,9 @@ class SnapshotsController(object):
 
         search_opts = {}
         search_opts.update(req.GET)
+        allowed_search_options = ('status', 'volume_id', 'display_name')
+        volumes.remove_invalid_options(context, search_opts,
+                                       allowed_search_options)
 
         snapshots = self.volume_api.get_all_snapshots(context,
                                                       search_opts=search_opts)

diff --git a/cinder/api/openstack/volume/volumes.py b/cinder/api/openstack/volume/volumes.py
index 4f43e60383af4aa1c23f5527b24286a0b38b5ede..1090ab6e1a6a50855efd8a9f022e5464f6a3cf0d 100644 (file)
--- a/cinder/api/openstack/volume/volumes.py
+++ b/cinder/api/openstack/volume/volumes.py
@@ -296,7 +296,7 @@ class VolumeController(object):
 
     def _get_volume_search_options(self):
         """Return volume search options allowed by non-admin."""
-        return ('name', 'status')
+        return ('display_name', 'status')
 
 
 def create_resource(ext_mgr):
@@ -315,4 +315,4 @@ def remove_invalid_options(context, search_options, allowed_search_options):
     log_msg = _("Removing options '%(bad_options)s' from query") % locals()
     LOG.debug(log_msg)
     for opt in unknown_options:
-        search_options.pop(opt, None)
+        del search_options[opt]

diff --git a/cinder/tests/api/openstack/volume/test_snapshots.py b/cinder/tests/api/openstack/volume/test_snapshots.py
index b146271f75215c3e67bbe91c2fc9a638246a4e55..07d57057b0ff08321b59156d6ee69fefc4ec3239 100644 (file)
--- a/cinder/tests/api/openstack/volume/test_snapshots.py
+++ b/cinder/tests/api/openstack/volume/test_snapshots.py
@@ -165,6 +165,92 @@ class SnapshotApiTest(test.TestCase):
         resp_snapshot = resp_snapshots.pop()
         self.assertEqual(resp_snapshot['id'], UUID)
 
+    def test_snapshot_list_by_status(self):
+        def stub_snapshot_get_all_by_project(context, project_id):
+            return [
+                fakes.stub_snapshot(1, display_name='backup1',
+                                    status='available'),
+                fakes.stub_snapshot(2, display_name='backup2',
+                                    status='available'),
+                fakes.stub_snapshot(3, display_name='backup3',
+                                    status='creating'),
+            ]
+        self.stubs.Set(db, 'snapshot_get_all_by_project',
+                       stub_snapshot_get_all_by_project)
+
+        # no status filter
+        req = fakes.HTTPRequest.blank('/v1/snapshots')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 3)
+        # single match
+        req = fakes.HTTPRequest.blank('/v1/snapshots?status=creating')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 1)
+        self.assertEqual(resp['snapshots'][0]['status'], 'creating')
+        # multiple match
+        req = fakes.HTTPRequest.blank('/v1/snapshots?status=available')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 2)
+        for snapshot in resp['snapshots']:
+            self.assertEquals(snapshot['status'], 'available')
+        # no match
+        req = fakes.HTTPRequest.blank('/v1/snapshots?status=error')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 0)
+
+    def test_snapshot_list_by_volume(self):
+        def stub_snapshot_get_all_by_project(context, project_id):
+            return [
+                fakes.stub_snapshot(1, volume_id='vol1', status='creating'),
+                fakes.stub_snapshot(2, volume_id='vol1', status='available'),
+                fakes.stub_snapshot(3, volume_id='vol2', status='available'),
+            ]
+        self.stubs.Set(db, 'snapshot_get_all_by_project',
+                       stub_snapshot_get_all_by_project)
+
+        # single match
+        req = fakes.HTTPRequest.blank('/v1/snapshots?volume_id=vol2')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 1)
+        self.assertEqual(resp['snapshots'][0]['volume_id'], 'vol2')
+        # multiple match
+        req = fakes.HTTPRequest.blank('/v1/snapshots?volume_id=vol1')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 2)
+        for snapshot in resp['snapshots']:
+            self.assertEqual(snapshot['volume_id'], 'vol1')
+        # multiple filters
+        req = fakes.HTTPRequest.blank('/v1/snapshots?volume_id=vol1'
+                                      '&status=available')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 1)
+        self.assertEqual(resp['snapshots'][0]['volume_id'], 'vol1')
+        self.assertEqual(resp['snapshots'][0]['status'], 'available')
+
+    def test_snapshot_list_by_name(self):
+        def stub_snapshot_get_all_by_project(context, project_id):
+            return [
+                fakes.stub_snapshot(1, display_name='backup1'),
+                fakes.stub_snapshot(2, display_name='backup2'),
+                fakes.stub_snapshot(3, display_name='backup3'),
+            ]
+        self.stubs.Set(db, 'snapshot_get_all_by_project',
+                       stub_snapshot_get_all_by_project)
+
+        # no display_name filter
+        req = fakes.HTTPRequest.blank('/v1/snapshots')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 3)
+        # filter by one name
+        req = fakes.HTTPRequest.blank('/v1/snapshots?display_name=backup2')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 1)
+        self.assertEquals(resp['snapshots'][0]['display_name'], 'backup2')
+        # filter no match
+        req = fakes.HTTPRequest.blank('/v1/snapshots?display_name=backup4')
+        resp = self.controller.index(req)
+        self.assertEqual(len(resp['snapshots']), 0)
+
     def test_admin_list_snapshots_limited_to_project(self):
         req = fakes.HTTPRequest.blank('/v1/fake/snapshots',
                                       use_admin_context=True)

diff --git a/cinder/tests/api/openstack/volume/test_volumes.py b/cinder/tests/api/openstack/volume/test_volumes.py
index 2c69ea55384d2c879195423ee6404ab37107f98d..410e5962780d407c240ed8964da417aebb5e1f86 100644 (file)
--- a/cinder/tests/api/openstack/volume/test_volumes.py
+++ b/cinder/tests/api/openstack/volume/test_volumes.py
@@ -232,6 +232,67 @@ 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):
+            return [
+                fakes.stub_volume(1, display_name='vol1'),
+                fakes.stub_volume(2, display_name='vol2'),
+                fakes.stub_volume(3, display_name='vol3'),
+            ]
+        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_status(self):
+        def stub_volume_get_all_by_project(context, project_id):
+            return [
+                fakes.stub_volume(1, display_name='vol1', status='available'),
+                fakes.stub_volume(2, display_name='vol2', status='available'),
+                fakes.stub_volume(3, display_name='vol3', status='in-use'),
+            ]
+        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):
         req = fakes.HTTPRequest.blank('/v1/volumes/1')
         res_dict = self.controller.show(req, '1')

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index e6a6d6d3f3d3e886a85551f14ada14afd564002a..6840c8058bb5938eaa0177ebf1f90124e960fa3c 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -268,18 +268,19 @@ class API(base.Base):
             filter_mapping = {'metadata': _check_metadata_match}
 
             result = []
+            not_found = object()
             for volume in volumes:
                 # go over all filters in the list
                 for opt, values in search_opts.iteritems():
                     try:
                         filter_func = filter_mapping[opt]
                     except KeyError:
-                        # no such filter - ignore it, go to next filter
-                        continue
-                    else:
-                        if filter_func(volume, values):
-                            result.append(volume)
-                            break
+                        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
 
@@ -296,10 +297,24 @@ class API(base.Base):
         if (context.is_admin and 'all_tenants' in search_opts):
             # Need to remove all_tenants to pass the filtering below.
             del search_opts['all_tenants']
-            return self.db.snapshot_get_all(context)
+            snapshots = self.db.snapshot_get_all(context)
         else:
-            return self.db.snapshot_get_all_by_project(context,
-                                                       context.project_id)
+            snapshots = self.db.snapshot_get_all_by_project(
+                context, context.project_id)
+
+        if search_opts:
+            LOG.debug(_("Searching by: %s") % str(search_opts))
+
+            results = []
+            not_found = object()
+            for snapshot in snapshots:
+                for opt, value in search_opts.iteritems():
+                    if snapshot.get(opt, not_found) != value:
+                        break
+                else:
+                    results.append(snapshot)
+            snapshots = results
+        return snapshots
 
     @wrap_check_policy
     def check_attach(self, context, volume):