Fix a problem with 'volume list' when 'all_tenants=0'

This change checks the value of 'all_tenants' and returns a list of volumes
for all tenants when 'all_tenants' is 1 (or True). A tenant-specific volume
list is returned when 'all_tenants' is 0 (or False).

An InvalidInput exception is thrown if the 'all_tenants' value is not 0, 1,
True, or False (case insensitive).

Provided new unit tests for the get_all api which check the all_tenants, and
limits parameters.

Fixed pep8 compliance test fails with code being improperly indented, and
changed calls using str() to use six.text_type()

Change-Id: If0b8c343ee7e4e05c9aec89241458dbdf2e4734b
Closes-Bug: #1342046

diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index ca57c7bbec863b9468f57474e9126af58709ed4f..d79ba6f2075fcc83d8c5f360dd5b14ae03e7b9dd 100644 (file)
--- a/cinder/tests/test_volume.py
+++ b/cinder/tests/test_volume.py
@@ -670,6 +670,68 @@ class VolumeTestCase(BaseVolumeTestCase):
 
         self.volume.delete_volume(self.context, volume_id)
 
+    def test_get_all_limit_bad_value(self):
+        """Test value of 'limit' is numeric and >= 0"""
+        volume_api = cinder.volume.api.API()
+
+        self.assertRaises(exception.InvalidInput,
+                          volume_api.get_all,
+                          self.context,
+                          limit="A")
+        self.assertRaises(exception.InvalidInput,
+                          volume_api.get_all,
+                          self.context,
+                          limit="-1")
+
+    def test_get_all_tenants_value(self):
+        """Validate allowable values for --all_tenants
+
+           Note: type of the value could be String, Boolean, or Int
+        """
+        api = cinder.volume.api.API()
+
+        self.assertTrue(api._get_all_tenants_value({'all_tenants': True}))
+        self.assertTrue(api._get_all_tenants_value({'all_tenants': 1}))
+        self.assertFalse(api._get_all_tenants_value({'all_tenants': 'False'}))
+        self.assertFalse(api._get_all_tenants_value({'all_tenants': '0'}))
+        self.assertRaises(exception.InvalidInput,
+                          api._get_all_tenants_value,
+                          {'all_tenants': 'No'})
+        self.assertRaises(exception.InvalidInput,
+                          api._get_all_tenants_value,
+                          {'all_tenants': -1})
+
+    def test_get_all_tenants_volume_list(self):
+        """Validate when the volume list for all tenants is returned"""
+        volume_api = cinder.volume.api.API()
+
+        with mock.patch.object(volume_api.db,
+                               'volume_get_all_by_project') as by_project:
+            with mock.patch.object(volume_api.db,
+                                   'volume_get_all') as get_all:
+                fake_volume = {'volume_type_id': 'fake_type_id',
+                               'name': 'fake_name',
+                               'host': 'fake_host',
+                               'id': 'fake_volume_id'}
+
+                fake_volume_list = []
+                fake_volume_list.append([fake_volume])
+                by_project.return_value = fake_volume_list
+                get_all.return_value = fake_volume_list
+
+                volume_api.get_all(self.context, filters={'all_tenants': '0'})
+                self.assertTrue(by_project.called)
+                by_project.called = False
+
+                self.context.is_admin = False
+                volume_api.get_all(self.context, filters={'all_tenants': '1'})
+                self.assertTrue(by_project.called)
+
+                # check for volume list of all tenants
+                self.context.is_admin = True
+                volume_api.get_all(self.context, filters={'all_tenants': '1'})
+                self.assertTrue(get_all.called)
+
     def test_delete_volume_in_error_extending(self):
         """Test volume can be deleted in error_extending stats."""
         # create a volume
@@ -2033,7 +2095,7 @@ class VolumeTestCase(BaseVolumeTestCase):
 
         self.volume.driver.delete_snapshot(
             mox.IgnoreArg()).AndRaise(
-                exception.SnapshotIsBusy(snapshot_name='fake'))
+            exception.SnapshotIsBusy(snapshot_name='fake'))
         self.mox.ReplayAll()
         self.volume.delete_snapshot(self.context, snapshot_id)
         snapshot_ref = db.snapshot_get(self.context, snapshot_id)
@@ -2063,7 +2125,7 @@ class VolumeTestCase(BaseVolumeTestCase):
 
         self.volume.driver.delete_snapshot(
             mox.IgnoreArg()).AndRaise(
-                exception.SnapshotIsBusy(snapshot_name='fake'))
+            exception.SnapshotIsBusy(snapshot_name='fake'))
         self.mox.ReplayAll()
         self.volume.delete_snapshot(self.context, snapshot_id)
         snapshot_ref = db.snapshot_get(self.context, snapshot_id)
@@ -2206,8 +2268,8 @@ class VolumeTestCase(BaseVolumeTestCase):
                         'container_format': 'bare',
                         'status': 'active'}
 
-        volume_api = cinder.volume.api.API(image_service=
-                                           _ModifiedFakeImageService())
+        volume_api = cinder.volume.api.API(
+            image_service=_ModifiedFakeImageService())
 
         self.assertRaises(exception.InvalidInput,
                           volume_api.create,
@@ -2224,8 +2286,8 @@ class VolumeTestCase(BaseVolumeTestCase):
                         'min_disk': 5,
                         'status': 'active'}
 
-        volume_api = cinder.volume.api.API(image_service=
-                                           _ModifiedFakeImageService())
+        volume_api = cinder.volume.api.API(
+            image_service=_ModifiedFakeImageService())
 
         self.assertRaises(exception.InvalidInput,
                           volume_api.create,
@@ -2242,8 +2304,8 @@ class VolumeTestCase(BaseVolumeTestCase):
                         'min_disk': 5,
                         'status': 'deleted'}
 
-        volume_api = cinder.volume.api.API(image_service=
-                                           _ModifiedFakeImageService())
+        volume_api = cinder.volume.api.API(
+            image_service=_ModifiedFakeImageService())
 
         self.assertRaises(exception.InvalidInput,
                           volume_api.create,
@@ -2476,12 +2538,12 @@ class VolumeTestCase(BaseVolumeTestCase):
         def fake_create_volume(*args, **kwargs):
             raise exception.CinderException('fake exception')
 
-        #create context for testing
+        # create context for testing
         ctxt = self.context.deepcopy()
         if 'admin' in ctxt.roles:
             ctxt.roles.remove('admin')
             ctxt.is_admin = False
-        #create one copy of context for future comparison
+        # create one copy of context for future comparison
         self.saved_ctxt = ctxt.deepcopy()
 
         self.stubs.Set(self.volume.driver, 'create_volume', fake_create_volume)
@@ -2500,10 +2562,10 @@ class VolumeTestCase(BaseVolumeTestCase):
         volume_src = tests_utils.create_volume(self.context,
                                                **self.volume_params)
         self.volume.create_volume(self.context, volume_src['id'])
-        volume_dst = tests_utils.create_volume(self.context,
-                                               source_replicaid=
-                                               volume_src['id'],
-                                               **self.volume_params)
+        volume_dst = tests_utils.create_volume(
+            self.context,
+            source_replicaid=volume_src['id'],
+            **self.volume_params)
         self.volume.create_volume(self.context, volume_dst['id'],
                                   source_replicaid=volume_src['id'])
         self.assertEqual('available',
@@ -3050,13 +3112,13 @@ class VolumeTestCase(BaseVolumeTestCase):
         group_id = group['id']
         volume = tests_utils.create_volume(
             self.context,
-            consistencygroup_id = group_id,
+            consistencygroup_id=group_id,
             **self.volume_params)
         volume_id = volume['id']
         self.volume.create_volume(self.context, volume_id)
         cgsnapshot = tests_utils.create_cgsnapshot(
             self.context,
-            consistencygroup_id = group_id)
+            consistencygroup_id=group_id)
         cgsnapshot_id = cgsnapshot['id']
         self.assertEqual(len(fake_notifier.NOTIFICATIONS), 2)
         cgsnapshot_returns = self._create_cgsnapshot(group_id, volume_id)

diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index 8bbe004aedf948fdb9d4166ec9f6e2ddc6045e60..bc302d1b6cb91b3637992f68befd8ac5c1887c1d 100644 (file)
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -24,6 +24,7 @@ import datetime
 import functools
 
 from oslo.config import cfg
+import six
 
 from cinder import context
 from cinder.db import base
@@ -313,12 +314,35 @@ class API(base.Base):
             raise exception.VolumeNotFound(volume_id=volume_id)
         return volume
 
+    def _get_all_tenants_value(self, filters):
+        """Returns a Boolean for the value of filters['all_tenants'].
+
+           False is returned if 'all_tenants' is not in the filters dictionary.
+           An InvalidInput exception is thrown for invalid values.
+        """
+
+        b = False
+        if 'all_tenants' in filters:
+            val = six.text_type(filters['all_tenants']).lower()
+            if val in ['true', '1']:
+                b = True
+            elif val in ['false', '0']:
+                b = False
+            else:
+                msg = _('all_tenants param must be 0 or 1')
+                raise exception.InvalidInput(reason=msg)
+
+        return b
+
     def get_all(self, context, marker=None, limit=None, sort_key='created_at',
                 sort_dir='desc', filters=None, viewable_admin_meta=False):
         check_policy(context, 'get_all')
+
         if filters is None:
             filters = {}
 
+        allTenants = self._get_all_tenants_value(filters)
+
         try:
             if limit is not None:
                 limit = int(limit)
@@ -337,9 +361,9 @@ class API(base.Base):
             filters['no_migration_targets'] = True
 
         if filters:
-            LOG.debug("Searching by: %s" % str(filters))
+            LOG.debug("Searching by: %s" % six.text_type(filters))
 
-        if (context.is_admin and 'all_tenants' in filters):
+        if context.is_admin and allTenants:
             # 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,
@@ -1079,7 +1103,7 @@ class API(base.Base):
             msg = _('Volume status must be available to update readonly flag.')
             raise exception.InvalidVolume(reason=msg)
         self.update_volume_admin_metadata(context.elevated(), volume,
-                                          {'readonly': str(flag)})
+                                          {'readonly': six.text_type(flag)})
 
     @wrap_check_policy
     def retype(self, context, volume, new_type, migration_policy=None):