Normalize volume objects

Cinder v1 uses "display_name" and "display_description". Cinder v2
users "name" and "description". Normalize the volume objects so that
both values work as both input and output. Having done this, the
name_key param to _filter_list is no longer needed, since it was there
to work around cinder wanting display name. In the future, if we have
another such thing (like heat) we should likely just normalize the dict
to include a name param rather than making _filter_list carry the
weight - since humans will want to get the name of the objects they work
with.

Co-Authored-By: David Shrewsbury <shrewsbury.dave@gmail.com>

Change-Id: Ia43815fd3d25ff7308e91489645bd3c459072359

shade/_utils.py

@@ -57,14 +57,13 @@ def _iterate_timeout(timeout, message, wait=2):
     raise exc.OpenStackCloudTimeout(message)
 
 
-def _filter_list(data, name_or_id, filters, name_key='name'):
+def _filter_list(data, name_or_id, filters):
     """Filter a list by name/ID and arbitrary meta data.
 
     :param list data:
         The list of dictionary data to filter. It is expected that
         each dictionary contains an 'id' and 'name'
-        key if a value for name_or_id is given. The 'name' key can be
+        key if a value for name_or_id is given.
-        overridden with the name_key parameter.
     :param string name_or_id:
         The name or ID of the entity being filtered.
     :param dict filters:
@@ -77,15 +76,12 @@ def _filter_list(data, name_or_id, filters, name_key='name'):
                   'gender': 'Female'
               }
             }
-    :param string name_key:
-        The name of the name key. Cinder wants display_name. Heat wants
-        stack_name. Defaults to 'name'
     """
     if name_or_id:
         identifier_matches = []
         for e in data:
             e_id = str(e.get('id', None))
-            e_name = e.get(name_key, None)
+            e_name = e.get('name', None)
             if str(name_or_id) in (e_id, e_name):
                 identifier_matches.append(e)
         data = identifier_matches
@@ -314,6 +310,27 @@ def normalize_users(users):
     return meta.obj_list_to_dict(ret)
 
 
+def normalize_volumes(volumes):
+    ret = []
+    for vol in volumes:
+        new_vol = vol.copy()
+        name = vol.get('name', vol.get('display_name'))
+        description = vol.get('description', vol.get('display_description'))
+        new_vol['name'] = name
+        new_vol['display_name'] = name
+        new_vol['description'] = description
+        new_vol['display_description'] = description
+        # For some reason, cinder uses strings for bools for these fields
+        for field in ('bootable', 'multiattach'):
+            if field in new_vol and new_vol[field] is not None:
+                if new_vol[field].lower() == 'true':
+                    new_vol[field] = True
+                elif new_vol[field].lower() == 'false':
+                    new_vol[field] = False
+        ret.append(new_vol)
+    return meta.obj_list_to_dict(ret)
+
+
 def normalize_domains(domains):
     ret = [
         dict(

shade/openstackcloud.py

@@ -899,12 +899,12 @@ class OpenStackCloud(object):
     def search_volumes(self, name_or_id=None, filters=None):
         volumes = self.list_volumes()
         return _utils._filter_list(
-            volumes, name_or_id, filters, name_key='display_name')
+            volumes, name_or_id, filters)
 
     def search_volume_snapshots(self, name_or_id=None, filters=None):
         volumesnapshots = self.list_volume_snapshots()
         return _utils._filter_list(
-            volumesnapshots, name_or_id, filters, name_key='display_name')
+            volumesnapshots, name_or_id, filters)
 
     def search_flavors(self, name_or_id=None, filters=None):
         flavors = self.list_flavors()
@@ -1027,7 +1027,8 @@ class OpenStackCloud(object):
             warnings.warn('cache argument to list_volumes is deprecated. Use '
                           'invalidate instead.')
         with _utils.shade_exceptions("Error fetching volume list"):
-            return self.manager.submitTask(_tasks.VolumeList())
+            return _utils.normalize_volumes(
+                self.manager.submitTask(_tasks.VolumeList()))
 
     @_utils.cache_on_arguments()
     def list_flavors(self):
@@ -2095,12 +2096,19 @@ class OpenStackCloud(object):
         self.list_images.invalidate(self)
         return True
 
-    def create_volume(self, wait=True, timeout=None, **kwargs):
+    def create_volume(
+            self, size,
+            wait=True, timeout=None, image=None, **kwargs):
         """Create a volume.
 
+        :param size: Size, in GB of the volume to create.
+        :param name: (optional) Name for the volume.
+        :param description: (optional) Name for the volume.
         :param wait: If true, waits for volume to be created.
         :param timeout: Seconds to wait for volume creation. None is forever.
-        :param volkwargs: Keyword arguments as expected for cinder client.
+        :param image: (optional) Image name, id or object from which to create
+                      the volume
+        :param kwargs: Keyword arguments as expected for cinder client.
 
         :returns: The created volume object.
 
@@ -2108,8 +2116,18 @@ class OpenStackCloud(object):
         :raises: OpenStackCloudException on operation error.
         """
 
+        if image:
+            image_obj = self.get_image(image)
+            if not image_obj:
+                raise OpenStackCloudException(
+                    "Image {image} was requested as the basis for a new"
+                    " volume, but was not found on the cloud".format(
+                        image=image))
+            kwargs['imageRef'] = image_obj['id']
+        kwargs = self._get_volume_kwargs(kwargs)
         with _utils.shade_exceptions("Error in creating volume"):
-            volume = self.manager.submitTask(_tasks.VolumeCreate(**kwargs))
+            volume = self.manager.submitTask(_tasks.VolumeCreate(
+                size=size, **kwargs))
         self.list_volumes.invalidate(self)
 
         if volume['status'] == 'error':
@@ -2126,13 +2144,13 @@ class OpenStackCloud(object):
                     continue
 
                 if volume['status'] == 'available':
-                    break
+                    return volume
 
                 if volume['status'] == 'error':
                     raise OpenStackCloudException(
                         "Error in creating volume, please check logs")
 
-        return volume
+        return _utils.normalize_volumes([volume])[0]
 
     def delete_volume(self, name_or_id=None, wait=True, timeout=None):
         """Delete a volume.
@@ -2316,18 +2334,35 @@ class OpenStackCloud(object):
                     )
         return vol
 
+    def _get_volume_kwargs(self, kwargs):
+        name = kwargs.pop('name', kwargs.pop('display_name', None))
+        description = kwargs.pop('description',
+                                 kwargs.pop('display_description', None))
+        if name:
+            if self.cloud_config.get_api_version('volume').startswith('2'):
+                kwargs['name'] = name
+            else:
+                kwargs['display_name'] = name
+        if description:
+            if self.cloud_config.get_api_version('volume').startswith('2'):
+                kwargs['description'] = description
+            else:
+                kwargs['display_description'] = description
+        return kwargs
+
+    @_utils.valid_kwargs('name', 'display_name',
+                         'description', 'display_description')
     def create_volume_snapshot(self, volume_id, force=False,
-                               display_name=None, display_description=None,
+                               wait=True, timeout=None, **kwargs):
-                               wait=True, timeout=None):
         """Create a volume.
 
         :param volume_id: the id of the volume to snapshot.
         :param force: If set to True the snapshot will be created even if the
                       volume is attached to an instance, if False it will not
-        :param display_name: name of the snapshot, one will be generated if
+        :param name: name of the snapshot, one will be generated if one is
-                             one is not provided
+                     not provided
-        :param display_description: description of the snapshot, one will be
+        :param description: description of the snapshot, one will be generated
-                                    one is not provided
+                            if one is not provided
         :param wait: If true, waits for volume snapshot to be created.
         :param timeout: Seconds to wait for volume snapshot creation. None is
                         forever.
@@ -2337,15 +2372,15 @@ class OpenStackCloud(object):
         :raises: OpenStackCloudTimeout if wait time exceeded.
         :raises: OpenStackCloudException on operation error.
         """
+
+        kwargs = self._get_volume_kwargs(kwargs)
         with _utils.shade_exceptions(
                 "Error creating snapshot of volume {volume_id}".format(
                     volume_id=volume_id)):
             snapshot = self.manager.submitTask(
                 _tasks.VolumeSnapshotCreate(
                     volume_id=volume_id, force=force,
-                    display_name=display_name,
+                    **kwargs))
-                    display_description=display_description)
-                )
 
         if wait:
             snapshot_id = snapshot['id']
@@ -2360,9 +2395,9 @@ class OpenStackCloud(object):
 
                 if snapshot['status'] == 'error':
                     raise OpenStackCloudException(
-                        "Error in creating volume, please check logs")
+                        "Error in creating volume snapshot, please check logs")
 
-        return snapshot
+        return _utils.normalize_volumes([snapshot])[0]
 
     def get_volume_snapshot_by_id(self, snapshot_id):
         """Takes a snapshot_id and gets a dict of the snapshot
@@ -2382,7 +2417,7 @@ class OpenStackCloud(object):
                 )
             )
 
-        return snapshot
+        return _utils.normalize_volumes([snapshot])[0]
 
     def get_volume_snapshot(self, name_or_id, filters=None):
         """Get a volume by name or ID.
@@ -2413,10 +2448,10 @@ class OpenStackCloud(object):
 
         """
         with _utils.shade_exceptions("Error getting a list of snapshots"):
-            return self.manager.submitTask(
+            return _utils.normalize_volumes(
-                _tasks.VolumeSnapshotList(detailed=detailed,
+                self.manager.submitTask(
-                                          search_opts=search_opts)
+                    _tasks.VolumeSnapshotList(
-            )
+                        detailed=detailed, search_opts=search_opts)))
 
     def delete_volume_snapshot(self, name_or_id=None, wait=False,
                                timeout=None):

shade/tests/fakes.py

@@ -95,19 +95,34 @@ class FakeUser(object):
 
 
 class FakeVolume(object):
-    def __init__(self, id, status, display_name, attachments=[]):
+    def __init__(
+            self, id, status, name, attachments=[],
+            size=75):
         self.id = id
         self.status = status
-        self.display_name = display_name
+        self.name = name
         self.attachments = attachments
+        self.size = size
+        self.snapshot_id = 'id:snapshot'
+        self.description = 'description'
+        self.volume_type = 'type:volume'
+        self.availability_zone = 'az1'
+        self.created_at = '1900-01-01 12:34:56'
+        self.source_volid = '12345'
+        self.metadata = {}
 
 
 class FakeVolumeSnapshot(object):
-    def __init__(self, id, status, display_name, display_description):
+    def __init__(
+            self, id, status, name, description, size=75):
         self.id = id
         self.status = status
-        self.display_name = display_name
+        self.name = name
-        self.display_description = display_description
+        self.description = description
+        self.size = size
+        self.created_at = '1900-01-01 12:34:56'
+        self.volume_id = '12345'
+        self.metadata = {}
 
 
 class FakeMachine(object):

shade/tests/unit/test_caching.py

@@ -18,8 +18,8 @@ import os_client_config as occ
 import testtools
 import yaml
 
-import shade
 import shade.openstackcloud
+from shade import _utils
 from shade import exc
 from shade import meta
 from shade.tests import fakes
@@ -108,12 +108,14 @@ class TestMemoryCache(base.TestCase):
     def test_list_volumes(self, cinder_mock):
         fake_volume = fakes.FakeVolume('volume1', 'available',
                                        'Volume 1 Display Name')
-        fake_volume_dict = meta.obj_to_dict(fake_volume)
+        fake_volume_dict = _utils.normalize_volumes(
+            [meta.obj_to_dict(fake_volume)])[0]
         cinder_mock.volumes.list.return_value = [fake_volume]
         self.assertEqual([fake_volume_dict], self.cloud.list_volumes())
         fake_volume2 = fakes.FakeVolume('volume2', 'available',
                                         'Volume 2 Display Name')
-        fake_volume2_dict = meta.obj_to_dict(fake_volume2)
+        fake_volume2_dict = _utils.normalize_volumes(
+            [meta.obj_to_dict(fake_volume2)])[0]
         cinder_mock.volumes.list.return_value = [fake_volume, fake_volume2]
         self.assertEqual([fake_volume_dict], self.cloud.list_volumes())
         self.cloud.list_volumes.invalidate(self.cloud)
@@ -124,12 +126,14 @@ class TestMemoryCache(base.TestCase):
     def test_list_volumes_creating_invalidates(self, cinder_mock):
         fake_volume = fakes.FakeVolume('volume1', 'creating',
                                        'Volume 1 Display Name')
-        fake_volume_dict = meta.obj_to_dict(fake_volume)
+        fake_volume_dict = _utils.normalize_volumes(
+            [meta.obj_to_dict(fake_volume)])[0]
         cinder_mock.volumes.list.return_value = [fake_volume]
         self.assertEqual([fake_volume_dict], self.cloud.list_volumes())
         fake_volume2 = fakes.FakeVolume('volume2', 'available',
                                         'Volume 2 Display Name')
-        fake_volume2_dict = meta.obj_to_dict(fake_volume2)
+        fake_volume2_dict = _utils.normalize_volumes(
+            [meta.obj_to_dict(fake_volume2)])[0]
         cinder_mock.volumes.list.return_value = [fake_volume, fake_volume2]
         self.assertEqual([fake_volume_dict, fake_volume2_dict],
                          self.cloud.list_volumes())
@@ -138,7 +142,8 @@ class TestMemoryCache(base.TestCase):
     def test_create_volume_invalidates(self, cinder_mock):
         fake_volb4 = fakes.FakeVolume('volume1', 'available',
                                       'Volume 1 Display Name')
-        fake_volb4_dict = meta.obj_to_dict(fake_volb4)
+        fake_volb4_dict = _utils.normalize_volumes(
+            [meta.obj_to_dict(fake_volb4)])[0]
         cinder_mock.volumes.list.return_value = [fake_volb4]
         self.assertEqual([fake_volb4_dict], self.cloud.list_volumes())
         volume = dict(display_name='junk_vol',
@@ -146,6 +151,8 @@ class TestMemoryCache(base.TestCase):
                       display_description='test junk volume')
         fake_vol = fakes.FakeVolume('12345', 'creating', '')
         fake_vol_dict = meta.obj_to_dict(fake_vol)
+        fake_vol_dict = _utils.normalize_volumes(
+            [meta.obj_to_dict(fake_vol)])[0]
         cinder_mock.volumes.create.return_value = fake_vol
         cinder_mock.volumes.list.return_value = [fake_volb4, fake_vol]
 

shade/tests/unit/test_create_volume_snapshot.py

@@ -21,6 +21,7 @@ Tests for the `create_volume_snapshot` command.
 
 from mock import patch
 import os_client_config
+from shade import _utils
 from shade import meta
 from shade import OpenStackCloud
 from shade.tests import base, fakes
@@ -52,14 +53,14 @@ class TestCreateVolumeSnapshot(base.TestCase):
             build_snapshot, fake_snapshot]
 
         self.assertEqual(
-            meta.obj_to_dict(fake_snapshot),
+            _utils.normalize_volumes(
+                [meta.obj_to_dict(fake_snapshot)])[0],
             self.client.create_volume_snapshot(volume_id='1234',
                                                wait=True)
         )
 
         mock_cinder.volume_snapshots.create.assert_called_with(
-            display_description=None, display_name=None, force=False,
+            force=False, volume_id='1234'
-            volume_id='1234'
         )
         mock_cinder.volume_snapshots.get.assert_called_with(
             snapshot_id=meta.obj_to_dict(build_snapshot)['id']
@@ -84,8 +85,7 @@ class TestCreateVolumeSnapshot(base.TestCase):
             wait=True, timeout=1)
 
         mock_cinder.volume_snapshots.create.assert_called_with(
-            display_description=None, display_name=None, force=False,
+            force=False, volume_id='1234'
-            volume_id='1234'
         )
         mock_cinder.volume_snapshots.get.assert_called_with(
             snapshot_id=meta.obj_to_dict(build_snapshot)['id']
@@ -112,8 +112,7 @@ class TestCreateVolumeSnapshot(base.TestCase):
             wait=True, timeout=5)
 
         mock_cinder.volume_snapshots.create.assert_called_with(
-            display_description=None, display_name=None, force=False,
+            force=False, volume_id='1234'
-            volume_id='1234'
         )
         mock_cinder.volume_snapshots.get.assert_called_with(
             snapshot_id=meta.obj_to_dict(build_snapshot)['id']

shade/tests/unit/test_meta.py

@@ -427,7 +427,7 @@ class TestMeta(testtools.TestCase):
         fake_volume = fakes.FakeVolume(
             id='volume1',
             status='available',
-            display_name='Volume 1 Display Name',
+            name='Volume 1 Display Name',
             attachments=[{'device': '/dev/sda0'}])
         fake_volume_dict = meta.obj_to_dict(fake_volume)
         mock_cloud.get_volumes.return_value = [fake_volume_dict]