From 628ac48901b8d50d165ece98588e510b5a703612 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 9 May 2024 13:57:38 +0100 Subject: [PATCH] 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 --- openstackclient/compute/v2/server.py | 65 ++-- .../functional/compute/v2/test_server.py | 8 +- .../tests/unit/compute/v2/test_server.py | 302 +++++++++++++++--- 3 files changed, 288 insertions(+), 87 deletions(-) diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 026feaccd4..079e752a77 100644 --- a/openstackclient/compute/v2/server.py +++ b/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 - the latest details of a server after creating it. + or if it needs to be refreshed, for example when showing the latest + 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']) - info.update(server.to_dict()) + server = compute_client.get_server(info['id']) + # 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 - 'fault', # only present in errored servers - 'flavor_id', # removed in 2.47 - 'networks', # only present in create responses - 'security_groups', # only present in create, detail responses + # only in create responses if '[api] enable_instance_password' is set + 'admin_password', + # only present in errored servers + 'fault', + # 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) - return zip(*sorted(details.items())) + # 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) + 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( - compute_client, image_client, server, refresh=True - ) - return zip(*sorted(details.items())) + # 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) + 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 - # all callers of _prep_server_detail() are using the SDK. - self.app.client_manager.compute, - self.app.client_manager.image, - server, - refresh=False, + compute_client, image_client, server, refresh=False ) - if topology: data['topology'] = format_columns.DictColumn(topology) - return zip(*sorted(data.items())) diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py index e465acc4a5..d6638d2f71 100644 --- a/openstackclient/tests/functional/compute/v2/test_server.py +++ b/openstackclient/tests/functional/compute/v2/test_server.py @@ -210,10 +210,10 @@ class ServerTests(common.ComputeTestCase): self.flavor_name, flavor['name'], ) - self.assertEqual( - '{} ({})'.format(flavor['name'], flavor['id']), - cmd_output["flavor"], - ) + # assume the v2.47+ output format + self.assertIsInstance(cmd_output['flavor'], dict) + 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, diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 1384dfe8ad..bc3b3e8875 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/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') - ), - format_columns.DictListColumn({}), - self.flavor.name + ' (' + self.new_server.flavor.get('id') + ')', - self.new_server.id, - self.image.name + ' (' + self.new_server.image.get('id') + ')', - self.new_server.name, - format_columns.DictColumn(self.new_server.metadata), + self.server.power_state + ), # OS-EXT-STS:power_state # noqa: E501 + None, # 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 + 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, 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. + def test_prep_server_detail(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': {'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) - find_resource.side_effect = [_server, _flavor] - self.image_client.get_image.return_value = _image + _server = compute_fakes.create_one_sdk_server(server_info) + self.compute_sdk_client.get_server.return_value = _server - # Prepare result data. - info = { - 'id': _server.id, - 'name': _server.name, - 'image': f'{_image.name} ({_image.id})', - 'flavor': f'{_flavor.name} ({_flavor.id})', + 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( - getattr(_server, 'OS-EXT-STS:power_state') + _server.power_state ), - 'properties': '', - 'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}], + '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([]), } - # Call _prep_server_detail(). - server_detail = server._prep_server_detail( - self.compute_client, + actual = server._prep_server_detail( + self.compute_sdk_client, self.image_client, _server, ) - # Check the results. - self.assertCountEqual(info, server_detail) + self.assertCountEqual(expected, actual) + # 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()