Normalize server objects

There are a set of things we always want to do to server dicts because
they are no-cost. This is in line with how we manage other resource
dicts, so make a normalize function. However, because we need to add
cloud and region info from the cloud object, make it a member method
rather than an friend function. Then, ensure it's always called on
server related calls.

Change-Id: Ied869efcb7d83c4f1a753962ca3127ce52172a24

shade/_utils.py

@@ -141,6 +141,36 @@ def _get_entity(func, name_or_id, filters):
     return entities[0]
 
 
+def normalize_servers(servers, cloud_name, region_name):
+    # Here instead of _utils because we need access to region and cloud
+    # name from the cloud object
+    ret = []
+    for server in servers:
+        ret.append(normalize_server(server, cloud_name, region_name))
+    return ret
+
+
+def normalize_server(server, cloud_name, region_name):
+    server.pop('links', None)
+    server['flavor'].pop('links', None)
+    # OpenStack can return image as a string when you've booted
+    # from volume
+    if str(server['image']) != server['image']:
+        server['image'].pop('links', None)
+
+    server['region'] = region_name
+    server['cloud'] = cloud_name
+
+    az = server.get('OS-EXT-AZ:availability_zone', None)
+    if az:
+        server['az'] = az
+
+    # Ensure volumes is always in the server dict, even if empty
+    server['volumes'] = []
+
+    return server
+
+
 def normalize_keystone_services(services):
     """Normalize the structure of keystone services
 

shade/inventory.py

@@ -61,9 +61,7 @@ class OpenStackInventory(object):
                 if expand:
                     server_vars = cloud.get_openstack_vars(server)
                 else:
-                    # expand_server_vars gets renamed in a follow on
+                    server_vars = meta.add_server_interfaces(cloud, server)
-                    # patch which should make this a bit clearer.
-                    server_vars = meta.expand_server_vars(cloud, server)
                 hostvars.append(server_vars)
 
         return hostvars

shade/meta.py

@@ -224,57 +224,47 @@ def get_groups_from_server(cloud, server, server_vars):
 
 
 def expand_server_vars(cloud, server):
-    """Add and clean up the server dict with information that is essential.
+    """Backwards compatibility function."""
+    return add_server_interfaces(cloud, server)
 
-    This function should not make additional calls to the cloud with one
+
-    exception - which is getting external/internal IPs, which have to do
+def add_server_interfaces(cloud, server):
-    some calls to neutron in some cases to find out which IP is routable. This
+    """Add network interface information to server.
-    is essential info as you can't otherwise connect to the server without
+
-    it.
+    Query the cloud as necessary to add information to the server record
+    about the network information needed to interface with the server.
+
+    Ensures that public_v4, public_v6, private_v4, private_v6, interface_ip,
+                 accessIPv4 and accessIPv6 are always set.
     """
-    server_vars = server
-    server_vars.pop('links', None)
-    server_vars['flavor'].pop('links', None)
-    # OpenStack can return image as a string when you've booted from volume
-    if str(server['image']) != server['image']:
-        server_vars['image'].pop('links', None)
-
     # First, add an IP address. Set it to '' rather than None if it does
     # not exist to remain consistent with the pre-existing missing values
-    server_vars['public_v4'] = get_server_external_ipv4(cloud, server) or ''
+    server['public_v4'] = get_server_external_ipv4(cloud, server) or ''
-    server_vars['public_v6'] = get_server_external_ipv6(server) or ''
+    server['public_v6'] = get_server_external_ipv6(server) or ''
-    server_vars['private_v4'] = get_server_private_ip(server, cloud) or ''
+    server['private_v4'] = get_server_private_ip(server, cloud) or ''
     interface_ip = None
-    if cloud.private and server_vars['private_v4']:
+    if cloud.private and server['private_v4']:
-        interface_ip = server_vars['private_v4']
+        interface_ip = server['private_v4']
     else:
-        if (server_vars['public_v6'] and cloud._local_ipv6
+        if (server['public_v6'] and cloud._local_ipv6
             and not cloud.force_ipv4):
-            interface_ip = server_vars['public_v6']
+            interface_ip = server['public_v6']
         else:
-            interface_ip = server_vars['public_v4']
+            interface_ip = server['public_v4']
     if interface_ip:
-        server_vars['interface_ip'] = interface_ip
+        server['interface_ip'] = interface_ip
 
     # Some clouds do not set these, but they're a regular part of the Nova
     # server record. Since we know them, go ahead and set them. In the case
     # where they were set previous, we use the values, so this will not break
     # clouds that provide the information
-    if cloud.private and server_vars['private_v4']:
+    if cloud.private and server['private_v4']:
-        server_vars['accessIPv4'] = server_vars['private_v4']
+        server['accessIPv4'] = server['private_v4']
     else:
-        server_vars['accessIPv4'] = server_vars['public_v4']
+        server['accessIPv4'] = server['public_v4']
-    server_vars['accessIPv6'] = server_vars['public_v6']
+    server['accessIPv6'] = server['public_v6']
 
-    server_vars['region'] = cloud.region_name
+    return server
-    server_vars['cloud'] = cloud.name
-
-    az = server_vars.get('OS-EXT-AZ:availability_zone', None)
-    if az:
-        server_vars['az'] = az
-    # Ensure volumes is always in the server dict, even if empty
-    server_vars['volumes'] = []
-    return server_vars
 
 
 def expand_server_security_groups(cloud, server):
@@ -293,7 +283,7 @@ def get_hostvars_from_server(cloud, server, mounts=None):
     expand_server_vars if caching is not set up. If caching is set up,
     the extra cost should be minimal.
     """
-    server_vars = expand_server_vars(cloud, server)
+    server_vars = add_server_interfaces(cloud, server)
 
     flavor_id = server['flavor']['id']
     flavor_name = cloud.get_flavor_name(flavor_id)

shade/openstackcloud.py

@@ -1130,7 +1130,9 @@ class OpenStackCloud(object):
                 "Error fetching server list on {cloud}:{region}:".format(
                     cloud=self.name,
                     region=self.region_name)):
-            servers = self.manager.submitTask(_tasks.ServerList())
+            servers = _utils.normalize_servers(
+                self.manager.submitTask(_tasks.ServerList()),
+                cloud_name=self.name, region_name=self.region_name)
 
             if detailed:
                 return [
@@ -1485,7 +1487,9 @@ class OpenStackCloud(object):
         return _utils._get_entity(searchfunc, name_or_id, filters)
 
     def get_server_by_id(self, id):
-        return self.manager.submitTask(_tasks.ServerGet(server=id))
+        return _utils.normalize_server(
+            self.manager.submitTask(_tasks.ServerGet(server=id)),
+            cloud_name=self.name, region_name=self.region_name)
 
     def get_image(self, name_or_id, filters=None):
         """Get an image by name or ID.

shade/tests/fakes.py

@@ -72,7 +72,11 @@ class FakeServer(object):
         self.name = name
         self.status = status
         self.addresses = addresses
+        if not flavor:
+            flavor = {}
         self.flavor = flavor
+        if not image:
+            image = {}
         self.image = image
         self.accessIPv4 = accessIPv4
         self.accessIPv6 = accessIPv6

shade/tests/unit/test_create_server.py

@@ -21,6 +21,7 @@ Tests for the `create_server` command.
 
 from mock import patch, Mock
 import os_client_config
+from shade import _utils
 from shade import meta
 from shade import OpenStackCloud
 from shade.exc import (OpenStackCloudException, OpenStackCloudTimeout)
@@ -132,10 +133,14 @@ class TestCreateServer(base.TestCase):
                 "servers.get.return_value": fake_server
             }
             OpenStackCloud.nova_client = Mock(**config)
-            self.assertEqual(meta.obj_to_dict(fake_server),
+            self.assertEqual(
-                             self.client.create_server(
+                _utils.normalize_server(
-                                 name='server-name', image='image=id',
+                    meta.obj_to_dict(fake_server),
-                                 flavor='flavor-id'))
+                    cloud_name=self.client.name,
+                    region_name=self.client.region_name),
+                self.client.create_server(
+                    name='server-name', image='image=id',
+                    flavor='flavor-id'))
 
     def test_create_server_wait(self):
         """

shade/tests/unit/test_inventory.py

@@ -16,7 +16,10 @@
 import mock
 import os_client_config
 
+from shade import _utils
 from shade import inventory
+from shade import meta
+from shade.tests import fakes
 from shade.tests.unit import base
 
 
@@ -59,14 +62,17 @@ class TestInventory(base.TestCase):
         self.assertEqual([server], ret)
 
     @mock.patch("os_client_config.config.OpenStackConfig")
-    @mock.patch("shade.meta.expand_server_vars")
+    @mock.patch("shade.meta.add_server_interfaces")
     @mock.patch("shade.OpenStackCloud")
-    def test_list_hosts_no_detail(self, mock_cloud, mock_expand, mock_config):
+    def test_list_hosts_no_detail(self, mock_cloud, mock_add, mock_config):
         mock_config.return_value.get_all_clouds.return_value = [{}]
 
         inv = inventory.OpenStackInventory()
 
-        server = dict(id='server_id', name='server_name')
+        server = _utils.normalize_server(
+            meta.obj_to_dict(fakes.FakeServer(
+                '1234', 'test', 'ACTIVE', addresses={})),
+            region_name='', cloud_name='')
         self.assertIsInstance(inv.clouds, list)
         self.assertEqual(1, len(inv.clouds))
         inv.clouds[0].list_servers.return_value = [server]
@@ -75,7 +81,7 @@ class TestInventory(base.TestCase):
 
         inv.clouds[0].list_servers.assert_called_once_with()
         self.assertFalse(inv.clouds[0].get_openstack_vars.called)
-        mock_expand.assert_called_once_with(inv.clouds[0], server)
+        mock_add.assert_called_once_with(inv.clouds[0], server)
 
     @mock.patch("os_client_config.config.OpenStackConfig")
     @mock.patch("shade.OpenStackCloud")

shade/tests/unit/test_meta.py

@@ -19,6 +19,7 @@ import warlock
 from neutronclient.common import exceptions as neutron_exceptions
 
 import shade
+from shade import _utils
 from shade import meta
 from shade.tests import fakes
 
@@ -359,14 +360,17 @@ class TestMeta(testtools.TestCase):
         mock_get_server_external_ipv6.return_value = PUBLIC_V6
 
         hostvars = meta.get_hostvars_from_server(
-            FakeCloud(), meta.obj_to_dict(FakeServer()))
+            FakeCloud(), _utils.normalize_server(
+                meta.obj_to_dict(FakeServer()),
+                cloud_name='CLOUD_NAME',
+                region_name='REGION_NAME'))
         self.assertNotIn('links', hostvars)
         self.assertEqual(PRIVATE_V4, hostvars['private_v4'])
         self.assertEqual(PUBLIC_V4, hostvars['public_v4'])
         self.assertEqual(PUBLIC_V6, hostvars['public_v6'])
         self.assertEqual(PUBLIC_V6, hostvars['interface_ip'])
-        self.assertEquals(FakeCloud.region_name, hostvars['region'])
+        self.assertEquals('REGION_NAME', hostvars['region'])
-        self.assertEquals(FakeCloud.name, hostvars['cloud'])
+        self.assertEquals('CLOUD_NAME', hostvars['cloud'])
         self.assertEquals("test-image-name", hostvars['image']['name'])
         self.assertEquals(FakeServer.image['id'], hostvars['image']['id'])
         self.assertNotIn('links', hostvars['image'])
@@ -411,14 +415,13 @@ class TestMeta(testtools.TestCase):
             FakeCloud(), meta.obj_to_dict(server))
         self.assertEquals('fake-image-id', hostvars['image']['id'])
 
-    @mock.patch.object(shade.meta, 'get_server_external_ipv4')
+    def test_az(self):
-    def test_az(self, mock_get_server_external_ipv4):
-        mock_get_server_external_ipv4.return_value = PUBLIC_V4
-
         server = FakeServer()
         server.__dict__['OS-EXT-AZ:availability_zone'] = 'az1'
-        hostvars = meta.get_hostvars_from_server(
+
-            FakeCloud(), meta.obj_to_dict(server))
+        hostvars = _utils.normalize_server(
+            meta.obj_to_dict(server),
+            cloud_name='', region_name='')
         self.assertEquals('az1', hostvars['az'])
 
     def test_has_volume(self):

shade/tests/unit/test_rebuild_server.py

@@ -21,6 +21,7 @@ Tests for the `rebuild_server` command.
 
 from mock import patch, Mock
 import os_client_config
+from shade import _utils
 from shade import meta
 from shade import OpenStackCloud
 from shade.exc import (OpenStackCloudException, OpenStackCloudTimeout)
@@ -108,5 +109,9 @@ class TestRebuildServer(base.TestCase):
                 "servers.get.return_value": active_server
             }
             OpenStackCloud.nova_client = Mock(**config)
-            self.assertEqual(meta.obj_to_dict(active_server),
+            self.client.name = 'cloud-name'
-                             self.client.rebuild_server("a", "b", wait=True))
+            self.assertEqual(
+                _utils.normalize_server(
+                    meta.obj_to_dict(active_server),
+                    cloud_name='cloud-name', region_name=''),
+                self.client.rebuild_server("a", "b", wait=True))

shade/tests/unit/test_shade.py

@@ -25,6 +25,7 @@ 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
 
 
@@ -499,7 +500,9 @@ class TestShade(base.TestCase):
         to the ServerList task.'''
         mock_serverlist.return_value = [
             munch.Munch({'name': 'testserver',
-                         'id': '1'})
+                         'id': '1',
+                         'flavor': {},
+                         'image': ''})
         ]
 
         r = self.cloud.list_servers()
@@ -514,7 +517,10 @@ class TestShade(base.TestCase):
         '''This test verifies that when list_servers is called with
         `detailed=True` that it calls `get_hostvars_from_server` for each
         server in the list.'''
-        mock_serverlist.return_value = ['server1', 'server2']
+        mock_serverlist.return_value = [
+            fakes.FakeServer('server1', '', 'ACTIVE'),
+            fakes.FakeServer('server2', '', 'ACTIVE'),
+        ]
         mock_get_hostvars_from_server.side_effect = [
             {'name': 'server1', 'id': '1'},
             {'name': 'server2', 'id': '2'},