From 55db0a565cebcc09c4277e44b356a2e2a7bf2fc1 Mon Sep 17 00:00:00 2001
From: David Shrewsbury <shrewsbury.dave@gmail.com>
Date: Fri, 18 Mar 2016 14:02:01 -0400
Subject: [PATCH] Bug fix: Make set/unset of flavor specs work again

The 1.2.0 release broke the API methods to set and unset
flavor extra specs because we need the raw object, as returned
from the nova client, to make method calls to change those
values. As a result, a new 'raw' parameter was added to the
submitTask() method of TaskManager to allow us to get these
raw objects back.

Additionally, we were never displaying the 'extra_specs' for
a flavor. This, too, requires a raw object method call.

Flavors are now normalized to remove client cruft and make sure
that 'extra_specs' is always an attribute.

As if that weren't enough, we now do functional tests for these
things! What more could one ask for???

Change-Id: Ie5c132317392cf26df2c8f43e9f07d040119eca0
---
 .../notes/flavor_fix-a53c6b326dc34a2c.yaml    |  7 +++
 shade/_utils.py                               | 12 +++++
 shade/openstackcloud.py                       |  9 +++-
 shade/operatorcloud.py                        |  4 +-
 shade/task_manager.py                         | 16 ++++--
 shade/tests/fakes.py                          |  6 ++-
 shade/tests/functional/test_flavor.py         | 43 +++++++++++++--
 shade/tests/unit/test_caching.py              |  6 ++-
 shade/tests/unit/test_flavors.py              |  4 +-
 shade/tests/unit/test_shade.py                | 52 ++++---------------
 10 files changed, 101 insertions(+), 58 deletions(-)
 create mode 100644 releasenotes/notes/flavor_fix-a53c6b326dc34a2c.yaml

diff --git a/releasenotes/notes/flavor_fix-a53c6b326dc34a2c.yaml b/releasenotes/notes/flavor_fix-a53c6b326dc34a2c.yaml
new file mode 100644
index 000000000..9a7ba7de1
--- /dev/null
+++ b/releasenotes/notes/flavor_fix-a53c6b326dc34a2c.yaml
@@ -0,0 +1,7 @@
+---
+features:
+  - Flavors will always contain an 'extra_specs' attribute. Client cruft,
+    such as 'links', 'HUMAN_ID', etc. has been removed.
+fixes:
+  - Setting and unsetting flavor extra specs now works. This had
+    been broken since the 1.2.0 release.
diff --git a/shade/_utils.py b/shade/_utils.py
index f0833a726..7c6eaaf02 100644
--- a/shade/_utils.py
+++ b/shade/_utils.py
@@ -468,6 +468,18 @@ def normalize_stacks(stacks):
     return stacks
 
 
+def normalize_flavors(flavors):
+    """ Normalize a list of flavor objects """
+    for flavor in flavors:
+        flavor.pop('links', None)
+        flavor.pop('NAME_ATTR', None)
+        flavor.pop('HUMAN_ID', None)
+        flavor.pop('human_id', None)
+        if 'extra_specs' not in flavor:
+            flavor['extra_specs'] = {}
+    return flavors
+
+
 def valid_kwargs(*valid_args):
     # This decorator checks if argument passed as **kwargs to a function are
     # present in valid_args.
diff --git a/shade/openstackcloud.py b/shade/openstackcloud.py
index 647c698cd..b89b8a816 100644
--- a/shade/openstackcloud.py
+++ b/shade/openstackcloud.py
@@ -1137,7 +1137,14 @@ class OpenStackCloud(object):
 
         """
         with _utils.shade_exceptions("Error fetching flavor list"):
-            return self.manager.submitTask(_tasks.FlavorList(is_public=None))
+            raw_flavors = self.manager.submitTask(
+                _tasks.FlavorList(is_public=None), raw=True)
+            for flavor in raw_flavors:
+                flavor.extra_specs = flavor.get_keys()
+
+            return _utils.normalize_flavors(
+                meta.obj_list_to_dict(raw_flavors)
+            )
 
     @_utils.cache_on_arguments(should_cache_fn=_no_pending_stacks)
     def list_stacks(self):
diff --git a/shade/operatorcloud.py b/shade/operatorcloud.py
index ea5342abc..66da61494 100644
--- a/shade/operatorcloud.py
+++ b/shade/operatorcloud.py
@@ -1429,7 +1429,7 @@ class OperatorCloud(openstackcloud.OpenStackCloud):
                                     is_public=is_public)
             )
 
-        return flavor
+        return _utils.normalize_flavors([flavor])[0]
 
     def delete_flavor(self, name_or_id):
         """Delete a flavor
@@ -1466,7 +1466,7 @@ class OperatorCloud(openstackcloud.OpenStackCloud):
         """
         try:
             flavor = self.manager.submitTask(
-                _tasks.FlavorGet(flavor=flavor_id)
+                _tasks.FlavorGet(flavor=flavor_id), raw=True
             )
         except nova_exceptions.NotFound:
             self.log.debug(
diff --git a/shade/task_manager.py b/shade/task_manager.py
index 620347b15..3b9d0c9a1 100644
--- a/shade/task_manager.py
+++ b/shade/task_manager.py
@@ -70,7 +70,7 @@ class Task(object):
         self._traceback = tb
         self._finished.set()
 
-    def wait(self):
+    def wait(self, raw):
         self._finished.wait()
         # TODO(mordred): We store the raw requests response if there is
         # one now. So we should probably do an error handler to throw
@@ -79,6 +79,10 @@ class Task(object):
             six.reraise(type(self._exception), self._exception,
                         self._traceback)
 
+        if raw:
+            # Do NOT convert the result.
+            return self._result
+
         # NOTE(Shrews): Since the client API might decide to subclass one
         # of these result types, we use isinstance() here instead of type().
         if (isinstance(self._result, list) or
@@ -126,7 +130,13 @@ class TaskManager(object):
         """ This is a direct action passthrough TaskManager """
         pass
 
-    def submitTask(self, task):
+    def submitTask(self, task, raw=False):
+        """Submit and execute the given task.
+
+        :param task: The task to execute.
+        :param bool raw: If True, return the raw result as received from the
+            underlying client call.
+        """
         self.log.debug(
             "Manager %s running task %s" % (self.name, type(task).__name__))
         start = time.time()
@@ -135,4 +145,4 @@ class TaskManager(object):
         self.log.debug(
             "Manager %s ran task %s in %ss" % (
                 self.name, type(task).__name__, (end - start)))
-        return task.wait()
+        return task.wait(raw)
diff --git a/shade/tests/fakes.py b/shade/tests/fakes.py
index f14314f75..1f3444aaf 100644
--- a/shade/tests/fakes.py
+++ b/shade/tests/fakes.py
@@ -41,9 +41,13 @@ class FakeEndpointv3(object):
 
 
 class FakeFlavor(object):
-    def __init__(self, id, name):
+    def __init__(self, id, name, ram):
         self.id = id
         self.name = name
+        self.ram = ram
+
+    def get_keys(self):
+        return {}
 
 
 class FakeFloatingIP(object):
diff --git a/shade/tests/functional/test_flavor.py b/shade/tests/functional/test_flavor.py
index 123b3c850..aefc00452 100644
--- a/shade/tests/functional/test_flavor.py
+++ b/shade/tests/functional/test_flavor.py
@@ -21,9 +21,6 @@ test_flavor
 Functional tests for `shade` flavor resource.
 """
 
-import string
-import random
-
 import shade
 from shade.exc import OpenStackCloudException
 from shade.tests import base
@@ -37,8 +34,7 @@ class TestFlavor(base.TestCase):
         self.operator_cloud = shade.operator_cloud(cloud='devstack-admin')
 
         # Generate a random name for flavors in this test
-        self.new_item_name = 'flavor_' + ''.join(
-            random.choice(string.ascii_lowercase) for _ in range(5))
+        self.new_item_name = self.getUniqueString('flavor')
 
         self.addCleanup(self._cleanup_flavors)
 
@@ -67,6 +63,12 @@ class TestFlavor(base.TestCase):
         flavor = self.operator_cloud.create_flavor(**flavor_kwargs)
 
         self.assertIsNotNone(flavor['id'])
+
+        # When properly normalized, we should always get an extra_specs
+        # and expect empty dict on create.
+        self.assertIn('extra_specs', flavor)
+        self.assertEqual({}, flavor['extra_specs'])
+
         for key in flavor_kwargs.keys():
             self.assertIn(key, flavor)
         for key, value in flavor_kwargs.items():
@@ -93,6 +95,8 @@ class TestFlavor(base.TestCase):
         # to make sure both of the flavors we just created are present.
         found = []
         for f in flavors:
+            # extra_specs should be added within list_flavors()
+            self.assertIn('extra_specs', f)
             if f['name'] in (pub_flavor_name, priv_flavor_name):
                 found.append(f)
         self.assertEqual(2, len(found))
@@ -125,3 +129,32 @@ class TestFlavor(base.TestCase):
                                                  project['id'])
         flavors = self.demo_cloud.search_flavors(priv_flavor_name)
         self.assertEqual(0, len(flavors))
+
+    def test_set_unset_flavor_specs(self):
+        """
+        Test setting and unsetting flavor extra specs
+        """
+        flavor_name = self.new_item_name + '_spec_test'
+        kwargs = dict(
+            name=flavor_name, ram=1024, vcpus=2, disk=10
+        )
+        new_flavor = self.operator_cloud.create_flavor(**kwargs)
+
+        # Expect no extra_specs
+        self.assertEqual({}, new_flavor['extra_specs'])
+
+        # Now set them
+        extra_specs = {'foo': 'aaa', 'bar': 'bbb'}
+        self.operator_cloud.set_flavor_specs(new_flavor['id'], extra_specs)
+        mod_flavor = self.operator_cloud.get_flavor(new_flavor['id'])
+
+        # Verify extra_specs were set
+        self.assertIn('extra_specs', mod_flavor)
+        self.assertEqual(extra_specs, mod_flavor['extra_specs'])
+
+        # Unset the 'foo' value
+        self.operator_cloud.unset_flavor_specs(mod_flavor['id'], ['foo'])
+        mod_flavor = self.operator_cloud.get_flavor(new_flavor['id'])
+
+        # Verify 'foo' is unset and 'bar' is still set
+        self.assertEqual({'bar': 'bbb'}, mod_flavor['extra_specs'])
diff --git a/shade/tests/unit/test_caching.py b/shade/tests/unit/test_caching.py
index 76ae9cf22..3b30cdfd3 100644
--- a/shade/tests/unit/test_caching.py
+++ b/shade/tests/unit/test_caching.py
@@ -303,8 +303,10 @@ class TestMemoryCache(base.TestCase):
         nova_mock.flavors.list.return_value = []
         self.assertEqual([], self.cloud.list_flavors())
 
-        fake_flavor = fakes.FakeFlavor('555', 'vanilla')
-        fake_flavor_dict = meta.obj_to_dict(fake_flavor)
+        fake_flavor = fakes.FakeFlavor('555', 'vanilla', 100)
+        fake_flavor_dict = _utils.normalize_flavors(
+            [meta.obj_to_dict(fake_flavor)]
+        )[0]
         nova_mock.flavors.list.return_value = [fake_flavor]
         self.cloud.list_flavors.invalidate(self.cloud)
         self.assertEqual([fake_flavor_dict], self.cloud.list_flavors())
diff --git a/shade/tests/unit/test_flavors.py b/shade/tests/unit/test_flavors.py
index c44b29717..9342ab55f 100644
--- a/shade/tests/unit/test_flavors.py
+++ b/shade/tests/unit/test_flavors.py
@@ -40,7 +40,7 @@ class TestFlavors(base.TestCase):
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_delete_flavor(self, mock_nova):
         mock_nova.flavors.list.return_value = [
-            fakes.FakeFlavor('123', 'lemon')
+            fakes.FakeFlavor('123', 'lemon', 100)
         ]
         self.assertTrue(self.op_cloud.delete_flavor('lemon'))
         mock_nova.flavors.delete.assert_called_once_with(flavor='123')
@@ -54,7 +54,7 @@ class TestFlavors(base.TestCase):
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_delete_flavor_exception(self, mock_nova):
         mock_nova.flavors.list.return_value = [
-            fakes.FakeFlavor('123', 'lemon')
+            fakes.FakeFlavor('123', 'lemon', 100)
         ]
         mock_nova.flavors.delete.side_effect = Exception()
         self.assertRaises(shade.OpenStackCloudException,
diff --git a/shade/tests/unit/test_shade.py b/shade/tests/unit/test_shade.py
index 0ed6ba9f1..9cffa1fde 100644
--- a/shade/tests/unit/test_shade.py
+++ b/shade/tests/unit/test_shade.py
@@ -24,7 +24,6 @@ from os_client_config import cloud_config
 import shade
 from shade import _utils
 from shade import exc
-from shade import meta
 from shade.tests import fakes
 from shade.tests.unit import base
 
@@ -493,47 +492,21 @@ class TestShade(base.TestCase):
 
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_get_flavor_by_ram(self, mock_nova_client):
-
-        class Flavor1(object):
-            id = '1'
-            name = 'vanilla ice cream'
-            ram = 100
-
-        class Flavor2(object):
-            id = '2'
-            name = 'chocolate ice cream'
-            ram = 200
-
-        vanilla = meta.obj_to_dict(Flavor1())
-        chocolate = meta.obj_to_dict(Flavor2())
+        vanilla = fakes.FakeFlavor('1', 'vanilla ice cream', 100)
+        chocolate = fakes.FakeFlavor('1', 'chocolate ice cream', 200)
         mock_nova_client.flavors.list.return_value = [vanilla, chocolate]
         flavor = self.cloud.get_flavor_by_ram(ram=150)
-        self.assertEquals(chocolate, flavor)
+        self.assertEquals(chocolate.id, flavor['id'])
 
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_get_flavor_by_ram_and_include(self, mock_nova_client):
-        class Flavor1(object):
-            id = '1'
-            name = 'vanilla ice cream'
-            ram = 100
-
-        class Flavor2(object):
-            id = '2'
-            name = 'chocolate ice cream'
-            ram = 200
-
-        class Flavor3(object):
-            id = '3'
-            name = 'strawberry ice cream'
-            ram = 250
-
-        vanilla = meta.obj_to_dict(Flavor1())
-        chocolate = meta.obj_to_dict(Flavor2())
-        strawberry = meta.obj_to_dict(Flavor3())
+        vanilla = fakes.FakeFlavor('1', 'vanilla ice cream', 100)
+        chocolate = fakes.FakeFlavor('2', 'chocoliate ice cream', 200)
+        strawberry = fakes.FakeFlavor('3', 'strawberry ice cream', 250)
         mock_nova_client.flavors.list.return_value = [
             vanilla, chocolate, strawberry]
         flavor = self.cloud.get_flavor_by_ram(ram=150, include='strawberry')
-        self.assertEquals(strawberry, flavor)
+        self.assertEquals(strawberry.id, flavor['id'])
 
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_get_flavor_by_ram_not_found(self, mock_nova_client):
@@ -544,17 +517,12 @@ class TestShade(base.TestCase):
 
     @mock.patch.object(shade.OpenStackCloud, 'nova_client')
     def test_get_flavor_string_and_int(self, mock_nova_client):
-        class Flavor1(object):
-            id = '1'
-            name = 'vanilla ice cream'
-            ram = 100
-
-        vanilla = meta.obj_to_dict(Flavor1())
+        vanilla = fakes.FakeFlavor('1', 'vanilla ice cream', 100)
         mock_nova_client.flavors.list.return_value = [vanilla]
         flavor1 = self.cloud.get_flavor('1')
-        self.assertEquals(vanilla, flavor1)
+        self.assertEquals(vanilla.id, flavor1['id'])
         flavor2 = self.cloud.get_flavor(1)
-        self.assertEquals(vanilla, flavor2)
+        self.assertEquals(vanilla.id, flavor2['id'])
 
     def test__neutron_exceptions_resource_not_found(self):
         with mock.patch.object(