compute: Always use SDK client to display server

This affects the 'server create', 'server show', 'server rebuild' and
'server set' commands. We also fix a few mistakes around the fields
shown.

Change-Id: I9946e12146efff39f9ba1591c90a4a9bccd46919
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

openstackclient/compute/v2/server.py

@@ -128,23 +128,26 @@ def _get_ip_address(addresses, address_type, ip_address_family):
     )
 
 
-def _prep_server_detail(compute_client, image_client, server, refresh=True):
+def _prep_server_detail(compute_client, image_client, server, *, refresh=True):
     """Prepare the detailed server dict for printing
 
     :param compute_client: a compute client instance
     :param image_client: an image client instance
     :param server: a Server resource
     :param refresh: Flag indicating if ``server`` is already the latest version
-                    or if it needs to be refreshed, for example when showing
+        or if it needs to be refreshed, for example when showing the latest
-                    the latest details of a server after creating it.
+        details of a server after creating it.
     :rtype: a dict of server details
     """
-    # Note: Some callers of this routine pass a novaclient server, and others
-    # pass an SDK server. Column names may be different across those cases.
     info = server.to_dict()
+
     if refresh:
-        server = utils.find_resource(compute_client.servers, info['id'])
+        server = compute_client.get_server(info['id'])
-        info.update(server.to_dict())
+        # we only update if the field is not empty, to avoid overwriting
+        # existing values
+        info.update(
+            **{x: y for x, y in server.to_dict().items() if x not in info or y}
+        )
 
     # Some commands using this routine were originally implemented with the
     # nova python wrappers, and were later migrated to use the SDK. Map the
@@ -159,7 +162,6 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
         'compute_host': 'OS-EXT-SRV-ATTR:host',
         'created_at': 'created',
         'disk_config': 'OS-DCF:diskConfig',
-        'flavor_id': 'flavorRef',
         'has_config_drive': 'config_drive',
         'host_id': 'hostId',
         'fault': 'fault',
@@ -194,10 +196,12 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
         'public_v6',
         # create-only columns
         'block_device_mapping',
+        'flavor_id',
         'host',
         'image_id',
         'max_count',
         'min_count',
+        'networks',
         'personality',
         'scheduler_hints',
         # aliases
@@ -208,11 +212,12 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
     # Some columns are only present in certain responses and should not be
     # shown otherwise.
     optional_columns = {
-        'admin_password',  # removed in 2.14
+        # only in create responses if '[api] enable_instance_password' is set
-        'fault',  # only present in errored servers
+        'admin_password',
-        'flavor_id',  # removed in 2.47
+        # only present in errored servers
-        'networks',  # only present in create responses
+        'fault',
-        'security_groups',  # only present in create, detail responses
+        # only present in create, detail responses
+        'security_groups',
     }
 
     data = {}
@@ -252,7 +257,9 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
     if flavor_info.get('original_name') is None:  # microversion < 2.47
         flavor_id = flavor_info.get('id', '')
         try:
-            flavor = utils.find_resource(compute_client.flavors, flavor_id)
+            flavor = compute_client.find_flavor(
+                flavor_id, ignore_missing=False
+            )
             info['flavor'] = f"{flavor.name} ({flavor_id})"
         except Exception:
             info['flavor'] = flavor_id
@@ -2056,8 +2063,10 @@ class CreateServer(command.ShowOne):
                 msg = _('Error creating server: %s') % parsed_args.server_name
                 raise exceptions.CommandError(msg)
 
-        details = _prep_server_detail(compute_client, image_client, server)
+        # TODO(stephenfin): Remove when the whole command is using SDK
-        return zip(*sorted(details.items()))
+        compute_client = self.app.client_manager.sdk_connection.compute
+        data = _prep_server_detail(compute_client, image_client, server)
+        return zip(*sorted(data.items()))
 
 
 class CreateServerDump(command.Command):
@@ -3681,10 +3690,12 @@ class RebuildServer(command.ShowOne):
                 msg = _('Error rebuilding server: %s') % server.id
                 raise exceptions.CommandError(msg)
 
-        details = _prep_server_detail(
+        # TODO(stephenfin): Remove when the whole command is using SDK
+        compute_client = self.app.client_manager.sdk_connection.compute
+        data = _prep_server_detail(
             compute_client, image_client, server, refresh=False
         )
-        return zip(*sorted(details.items()))
+        return zip(*sorted(data.items()))
 
 
 class EvacuateServer(command.ShowOne):
@@ -3803,10 +3814,10 @@ host."""
                 msg = _('Error evacuating server: %s') % server.id
                 raise exceptions.CommandError(msg)
 
-        details = _prep_server_detail(
+        # TODO(stephenfin): Remove when the whole command is using SDK
-            compute_client, image_client, server, refresh=True
+        compute_client = self.app.client_manager.sdk_connection.compute
-        )
+        data = _prep_server_detail(compute_client, image_client, server)
-        return zip(*sorted(details.items()))
+        return zip(*sorted(data.items()))
 
 
 class RemoveFixedIP(command.Command):
@@ -4629,6 +4640,7 @@ information for the server."""
 
     def take_action(self, parsed_args):
         compute_client = self.app.client_manager.sdk_connection.compute
+        image_client = self.app.client_manager.image
 
         # Find by name or ID, then get the full details of the server
         server = compute_client.find_server(
@@ -4652,17 +4664,10 @@ information for the server."""
             topology = server.fetch_topology(compute_client)
 
         data = _prep_server_detail(
-            # TODO(dannosliwcd): Replace these clients with SDK clients after
+            compute_client, image_client, server, refresh=False
-            # all callers of _prep_server_detail() are using the SDK.
-            self.app.client_manager.compute,
-            self.app.client_manager.image,
-            server,
-            refresh=False,
         )
-
         if topology:
             data['topology'] = format_columns.DictColumn(topology)
-
         return zip(*sorted(data.items()))
 
 

openstackclient/tests/functional/compute/v2/test_server.py

@@ -210,10 +210,10 @@ class ServerTests(common.ComputeTestCase):
             self.flavor_name,
             flavor['name'],
         )
-        self.assertEqual(
+        # assume the v2.47+ output format
-            '{} ({})'.format(flavor['name'], flavor['id']),
+        self.assertIsInstance(cmd_output['flavor'], dict)
-            cmd_output["flavor"],
+        self.assertIn('name', cmd_output['flavor'])
-        )
+        self.assertEqual(flavor['name'], cmd_output['flavor']['name'])
         image = self.openstack(
             'image show ' + self.image_name,
             parse_output=True,

openstackclient/tests/unit/compute/v2/test_server.py

@@ -1286,49 +1286,125 @@ class TestServerAddSecurityGroup(compute_fakes.TestComputev2):
 
 class TestServerCreate(TestServer):
     columns = (
+        'OS-DCF:diskConfig',
+        'OS-EXT-AZ:availability_zone',
+        'OS-EXT-SRV-ATTR:host',
+        'OS-EXT-SRV-ATTR:hostname',
+        'OS-EXT-SRV-ATTR:hypervisor_hostname',
+        'OS-EXT-SRV-ATTR:instance_name',
+        'OS-EXT-SRV-ATTR:kernel_id',
+        'OS-EXT-SRV-ATTR:launch_index',
+        'OS-EXT-SRV-ATTR:ramdisk_id',
+        'OS-EXT-SRV-ATTR:reservation_id',
+        'OS-EXT-SRV-ATTR:root_device_name',
+        'OS-EXT-SRV-ATTR:user_data',
         'OS-EXT-STS:power_state',
+        'OS-EXT-STS:task_state',
+        'OS-EXT-STS:vm_state',
+        'OS-SRV-USG:launched_at',
+        'OS-SRV-USG:terminated_at',
+        'accessIPv4',
+        'accessIPv6',
         'addresses',
+        'config_drive',
+        'created',
+        'description',
         'flavor',
+        'hostId',
+        'host_status',
         'id',
         'image',
+        'key_name',
+        'locked',
+        'locked_reason',
         'name',
+        'pinned_availability_zone',
+        'progress',
+        'project_id',
         'properties',
+        'server_groups',
+        'status',
+        'tags',
+        'trusted_image_certificates',
+        'updated',
+        'user_id',
+        'volumes_attached',
     )
 
     def datalist(self):
-        datalist = (
+        return (
+            None,  # OS-DCF:diskConfig
+            None,  # OS-EXT-AZ:availability_zone
+            None,  # OS-EXT-SRV-ATTR:host
+            None,  # OS-EXT-SRV-ATTR:hostname
+            None,  # OS-EXT-SRV-ATTR:hypervisor_hostname
+            None,  # OS-EXT-SRV-ATTR:instance_name
+            None,  # OS-EXT-SRV-ATTR:kernel_id
+            None,  # OS-EXT-SRV-ATTR:launch_index
+            None,  # OS-EXT-SRV-ATTR:ramdisk_id
+            None,  # OS-EXT-SRV-ATTR:reservation_id
+            None,  # OS-EXT-SRV-ATTR:root_device_name
+            None,  # OS-EXT-SRV-ATTR:user_data
             server.PowerStateColumn(
-                getattr(self.new_server, 'OS-EXT-STS:power_state')
+                self.server.power_state
-            ),
+            ),  # OS-EXT-STS:power_state  # noqa: E501
-            format_columns.DictListColumn({}),
+            None,  # OS-EXT-STS:task_state
-            self.flavor.name + ' (' + self.new_server.flavor.get('id') + ')',
+            None,  # OS-EXT-STS:vm_state
-            self.new_server.id,
+            None,  # OS-SRV-USG:launched_at
-            self.image.name + ' (' + self.new_server.image.get('id') + ')',
+            None,  # OS-SRV-USG:terminated_at
-            self.new_server.name,
+            None,  # accessIPv4
-            format_columns.DictColumn(self.new_server.metadata),
+            None,  # accessIPv6
+            server.AddressesColumn({}),  # addresses
+            None,  # config_drive
+            None,  # created
+            None,  # description
+            self.flavor.name + " (" + self.flavor.id + ")",  # flavor
+            None,  # hostId
+            None,  # host_status
+            self.server.id,  # id
+            self.image.name + " (" + self.image.id + ")",  # image
+            None,  # key_name
+            None,  # locked
+            None,  # locked_reason
+            self.server.name,
+            None,  # pinned_availability_zone
+            None,  # progress
+            None,  # project_id
+            format_columns.DictColumn({}),  # properties
+            None,  # server_groups
+            None,  # status
+            format_columns.ListColumn([]),  # tags
+            None,  # trusted_image_certificates
+            None,  # updated
+            None,  # user_id
+            format_columns.ListDictColumn([]),  # volumes_attached
         )
-        return datalist
 
     def setUp(self):
         super().setUp()
 
-        attrs = {
-            'networks': {},
-        }
-        self.new_server = compute_fakes.create_one_server(attrs=attrs)
-
-        # This is the return value for utils.find_resource().
-        # This is for testing --wait option.
-        self.servers_mock.get.return_value = self.new_server
-
-        self.servers_mock.create.return_value = self.new_server
-
         self.image = image_fakes.create_one_image()
         self.image_client.find_image.return_value = self.image
         self.image_client.get_image.return_value = self.image
 
         self.flavor = compute_fakes.create_one_flavor()
         self.flavors_mock.get.return_value = self.flavor
+        self.compute_sdk_client.find_flavor.return_value = self.flavor
+
+        attrs = {
+            'addresses': {},
+            'networks': {},
+            'image': self.image,
+            'flavor': self.flavor,
+        }
+        self.new_server = compute_fakes.create_one_server(attrs=attrs)
+        self.servers_mock.create.return_value = self.new_server
+
+        # We need an SDK-style server object also for the get_server call in
+        # _prep_server_detail
+        # TODO(stephenfin): Remove when the whole command is using SDK
+        self.server = compute_fakes.create_one_sdk_server(attrs=attrs)
+        self.compute_sdk_client.get_server.return_value = self.server
 
         self.volume = volume_fakes.create_one_volume()
         self.volume_alt = volume_fakes.create_one_volume()
@@ -4706,9 +4782,6 @@ class TestServerList(_TestServerList):
         self.compute_sdk_client.servers.assert_called_with(**self.kwargs)
         self.image_client.images.assert_called()
         self.compute_sdk_client.flavors.assert_called()
-        # we did not pass image or flavor, so gets on those must be absent
-        self.assertFalse(self.flavors_mock.get.call_count)
-        self.assertFalse(self.image_client.get_image.call_count)
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.data, tuple(data))
 
@@ -6890,7 +6963,7 @@ class TestServerRebuildVolumeBacked(TestServer):
         )
 
 
-class TestEvacuateServer(TestServer):
+class TestServerEvacuate(TestServer):
     def setUp(self):
         super().setUp()
 
@@ -6918,6 +6991,12 @@ class TestEvacuateServer(TestServer):
         # Return value for utils.find_resource for server.
         self.servers_mock.get.return_value = self.server
 
+        # We need an SDK-style server object also for the get_server call in
+        # _prep_server_detail
+        # TODO(stephenfin): Remove when the whole command is using SDK
+        self.sdk_server = compute_fakes.create_one_sdk_server(attrs=attrs)
+        self.compute_sdk_client.get_server.return_value = self.sdk_server
+
         self.cmd = server.EvacuateServer(self.app, None)
 
     def _test_evacuate(self, args, verify_args, evac_args):
@@ -8300,7 +8379,11 @@ class TestServerShow(TestServer):
         super().setUp()
 
         self.image = image_fakes.create_one_image()
+        self.image_client.get_image.return_value = self.image
+
         self.flavor = compute_fakes.create_one_flavor()
+        self.compute_sdk_client.find_flavor.return_value = self.flavor
+
         self.topology = {
             'nodes': [{'vcpu_set': [0, 1]}, {'vcpu_set': [2, 3]}],
             'pagesize_kb': None,
@@ -8318,11 +8401,7 @@ class TestServerShow(TestServer):
             attrs=server_info,
         )
         self.server.fetch_topology = mock.MagicMock(return_value=self.topology)
-
-        # This is the return value for utils.find_resource()
         self.compute_sdk_client.get_server.return_value = self.server
-        self.image_client.get_image.return_value = self.image
-        self.flavors_mock.get.return_value = self.flavor
 
         # Get the command object to test
         self.cmd = server.ShowServer(self.app, None)
@@ -9241,15 +9320,13 @@ class TestServerGeneral(TestServer):
             [6],
         )
 
-    @mock.patch('osc_lib.utils.find_resource')
+    def test_prep_server_detail(self):
-    def test_prep_server_detail(self, find_resource):
-        # Setup mock method return value. utils.find_resource() will be called
-        # three times in _prep_server_detail():
-        # - The first time, return server info.
-        # - The second time, return image info.
-        # - The third time, return flavor info.
         _image = image_fakes.create_one_image()
+        self.image_client.get_image.return_value = _image
+
         _flavor = compute_fakes.create_one_flavor()
+        self.compute_sdk_client.find_flavor.return_value = _flavor
+
         server_info = {
             'image': {'id': _image.id},
             'flavor': {'id': _flavor.id},
@@ -9259,31 +9336,150 @@ class TestServerGeneral(TestServer):
             'properties': '',
             'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
         }
-        _server = compute_fakes.create_one_server(attrs=server_info)
+        _server = compute_fakes.create_one_sdk_server(server_info)
-        find_resource.side_effect = [_server, _flavor]
+        self.compute_sdk_client.get_server.return_value = _server
-        self.image_client.get_image.return_value = _image
 
-        # Prepare result data.
+        expected = {
-        info = {
+            'OS-DCF:diskConfig': None,
-            'id': _server.id,
+            'OS-EXT-AZ:availability_zone': None,
-            'name': _server.name,
+            'OS-EXT-SRV-ATTR:host': None,
-            'image': f'{_image.name} ({_image.id})',
+            'OS-EXT-SRV-ATTR:hostname': None,
-            'flavor': f'{_flavor.name} ({_flavor.id})',
+            'OS-EXT-SRV-ATTR:hypervisor_hostname': None,
+            'OS-EXT-SRV-ATTR:instance_name': None,
+            'OS-EXT-SRV-ATTR:kernel_id': None,
+            'OS-EXT-SRV-ATTR:launch_index': None,
+            'OS-EXT-SRV-ATTR:ramdisk_id': None,
+            'OS-EXT-SRV-ATTR:reservation_id': None,
+            'OS-EXT-SRV-ATTR:root_device_name': None,
+            'OS-EXT-SRV-ATTR:user_data': None,
             'OS-EXT-STS:power_state': server.PowerStateColumn(
-                getattr(_server, 'OS-EXT-STS:power_state')
+                _server.power_state
             ),
-            'properties': '',
+            'OS-EXT-STS:task_state': None,
-            'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
+            'OS-EXT-STS:vm_state': None,
+            'OS-SRV-USG:launched_at': None,
+            'OS-SRV-USG:terminated_at': None,
+            'accessIPv4': None,
+            'accessIPv6': None,
             'addresses': format_columns.DictListColumn(_server.addresses),
+            'config_drive': None,
+            'created': None,
+            'description': None,
+            'flavor': f'{_flavor.name} ({_flavor.id})',
+            'hostId': None,
+            'host_status': None,
+            'id': _server.id,
+            'image': f'{_image.name} ({_image.id})',
+            'key_name': None,
+            'locked': None,
+            'locked_reason': None,
+            'name': _server.name,
+            'pinned_availability_zone': None,
+            'progress': None,
             'project_id': 'tenant-id-xxx',
+            'properties': format_columns.DictColumn({}),
+            'server_groups': None,
+            'status': None,
+            'tags': format_columns.ListColumn([]),
+            'trusted_image_certificates': None,
+            'updated': None,
+            'user_id': None,
+            'volumes_attached': format_columns.ListDictColumn([]),
         }
 
-        # Call _prep_server_detail().
+        actual = server._prep_server_detail(
-        server_detail = server._prep_server_detail(
+            self.compute_sdk_client,
-            self.compute_client,
             self.image_client,
             _server,
         )
 
-        # Check the results.
+        self.assertCountEqual(expected, actual)
-        self.assertCountEqual(info, server_detail)
+        # this should be called since we need the flavor (< 2.47)
+        self.compute_sdk_client.find_flavor.assert_called_once_with(
+            _flavor.id, ignore_missing=False
+        )
+
+    def test_prep_server_detail_v247(self):
+        _image = image_fakes.create_one_image()
+        self.image_client.get_image.return_value = _image
+
+        _flavor = compute_fakes.create_one_flavor()
+        self.compute_sdk_client.find_flavor.return_value = _flavor
+
+        server_info = {
+            'image': {'id': _image.id},
+            'flavor': {
+                'vcpus': _flavor.vcpus,
+                'ram': _flavor.ram,
+                'disk': _flavor.disk,
+                'ephemeral': _flavor.ephemeral,
+                'swap': _flavor.swap,
+                'original_name': _flavor.name,
+                'extra_specs': {},
+            },
+            'tenant_id': 'tenant-id-xxx',
+            'addresses': {'public': ['10.20.30.40', '2001:db8::f']},
+            'links': 'http://xxx.yyy.com',
+            'properties': '',
+            'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
+        }
+        _server = compute_fakes.create_one_sdk_server(server_info)
+        self.compute_sdk_client.get_server.return_value = _server
+
+        expected = {
+            'OS-DCF:diskConfig': None,
+            'OS-EXT-AZ:availability_zone': None,
+            'OS-EXT-SRV-ATTR:host': None,
+            'OS-EXT-SRV-ATTR:hostname': None,
+            'OS-EXT-SRV-ATTR:hypervisor_hostname': None,
+            'OS-EXT-SRV-ATTR:instance_name': None,
+            'OS-EXT-SRV-ATTR:kernel_id': None,
+            'OS-EXT-SRV-ATTR:launch_index': None,
+            'OS-EXT-SRV-ATTR:ramdisk_id': None,
+            'OS-EXT-SRV-ATTR:reservation_id': None,
+            'OS-EXT-SRV-ATTR:root_device_name': None,
+            'OS-EXT-SRV-ATTR:user_data': None,
+            'OS-EXT-STS:power_state': server.PowerStateColumn(
+                _server.power_state
+            ),
+            'OS-EXT-STS:task_state': None,
+            'OS-EXT-STS:vm_state': None,
+            'OS-SRV-USG:launched_at': None,
+            'OS-SRV-USG:terminated_at': None,
+            'accessIPv4': None,
+            'accessIPv6': None,
+            'addresses': format_columns.DictListColumn(_server.addresses),
+            'config_drive': None,
+            'created': None,
+            'description': None,
+            'flavor': f'{_flavor.name} ({_flavor.id})',
+            'hostId': None,
+            'host_status': None,
+            'id': _server.id,
+            'image': f'{_image.name} ({_image.id})',
+            'key_name': None,
+            'locked': None,
+            'locked_reason': None,
+            'name': _server.name,
+            'pinned_availability_zone': None,
+            'progress': None,
+            'project_id': 'tenant-id-xxx',
+            'properties': format_columns.DictColumn({}),
+            'server_groups': None,
+            'status': None,
+            'tags': format_columns.ListColumn([]),
+            'trusted_image_certificates': None,
+            'updated': None,
+            'user_id': None,
+            'volumes_attached': format_columns.ListDictColumn([]),
+        }
+
+        actual = server._prep_server_detail(
+            self.compute_sdk_client,
+            self.image_client,
+            _server,
+        )
+
+        self.assertCountEqual(expected, actual)
+        # this shouldn't be called since we have a full flavor (>= 2.47)
+        self.compute_sdk_client.find_flavor.assert_not_called()