From 03776d82e58622b30b90260ed9c374b0cfc70f2b Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 4 Nov 2020 10:14:00 +0000 Subject: [PATCH] compute: Fix 'server * -f yaml' output Make use of 'FormattableColumn'-derived formatters, which provide better output than what we were using before, particularly for the YAML output format. For example, compare before for the 'server show' command: $ openstack --os-compute-api-version 2.79 server show test-server -f yaml ... addresses: private=fdff:77e3:9bb4:0:f816:3eff:fe6d:a944, 10.0.0.44 flavor: disk='1', ephemeral='0', extra_specs.hw_rng:allowed='True', original_name='m1.tiny', ram='512', swap='0', vcpus='1' ... To after: $ openstack --os-compute-api-version 2.79 server show test-server -f yaml ... addresses: private: - fdff:77e3:9bb4:0:f816:3eff:fe6d:a944 - 10.0.0.44 flavor: disk: 1 ephemeral: 0 extra_specs: hw_rng:allowed: 'True' original_name: m1.tiny ram: 512 swap: 0 vcpus: 1 ... Similarly, compare before for 'server list': $ openstack --os-compute-api-version 2.79 server list -f yaml - ... Networks: private=fdff:77e3:9bb4:0:f816:3eff:fe6d:a944, 10.0.0.44 Power State: Running Properties: '' ... To after: $ openstack --os-compute-api-version 2.79 server list -f yaml - ... Networks: private: - fdff:77e3:9bb4:0:f816:3eff:fe6d:a944 - 10.0.0.44 Power State: 1 Properties: {} ... We also fix the human-readable output for the 'tags' field. Before: $ openstack --os-compute-api-version 2.79 server list ... | tags | ['bar', 'foo'] | After: $ openstack --os-compute-api-version 2.79 server list ... | tags | bar, foo | Change-Id: I7a8349106e211c57c4577b75326b39b88bd9ac1e Signed-off-by: Stephen Finucane --- openstackclient/compute/v2/server.py | 58 +++----- .../functional/compute/v2/test_server.py | 20 ++- .../tests/unit/compute/v2/test_server.py | 126 ++++++++---------- ...proved-server-output-6965b664f6abda8d.yaml | 10 ++ 4 files changed, 99 insertions(+), 115 deletions(-) create mode 100644 releasenotes/notes/improved-server-output-6965b664f6abda8d.yaml diff --git a/openstackclient/compute/v2/server.py b/openstackclient/compute/v2/server.py index 2512a8778c..ba0243ef3e 100644 --- a/openstackclient/compute/v2/server.py +++ b/openstackclient/compute/v2/server.py @@ -21,6 +21,7 @@ import io import logging import os +from cliff import columns as cliff_columns import iso8601 from novaclient import api_versions from novaclient.v2 import servers @@ -41,28 +42,9 @@ LOG = logging.getLogger(__name__) IMAGE_STRING_FOR_BFV = 'N/A (booted from volume)' -def _format_servers_list_networks(networks): - """Return a formatted string of a server's networks +class PowerStateColumn(cliff_columns.FormattableColumn): + """Generate a formatted string of a server's power state.""" - :param networks: a Server.networks field - :rtype: a string of formatted network addresses - """ - output = [] - for (network, addresses) in networks.items(): - if not addresses: - continue - addresses_csv = ', '.join(addresses) - group = "%s=%s" % (network, addresses_csv) - output.append(group) - return '; '.join(output) - - -def _format_servers_list_power_state(state): - """Return a formatted string of a server's power state - - :param state: the power state number of a server - :rtype: a string mapped to the power state number - """ power_states = [ 'NOSTATE', # 0x00 'Running', # 0x01 @@ -74,10 +56,11 @@ def _format_servers_list_power_state(state): 'Suspended' # 0x07 ] - try: - return power_states[state] - except Exception: - return 'N/A' + def human_readable(self): + try: + return self.power_states[self._value] + except Exception: + return 'N/A' def _get_ip_address(addresses, address_type, ip_address_family): @@ -169,7 +152,7 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True): except Exception: info['flavor'] = flavor_id else: - info['flavor'] = utils.format_dict(flavor_info) + info['flavor'] = format_columns.DictColumn(flavor_info) if 'os-extended-volumes:volumes_attached' in info: info.update( @@ -185,19 +168,15 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True): info.pop('security_groups')) } ) + if 'tags' in info: + info.update({'tags': format_columns.ListColumn(info.pop('tags'))}) + # NOTE(dtroyer): novaclient splits these into separate entries... # Format addresses in a useful way - info['addresses'] = _format_servers_list_networks(server.networks) + info['addresses'] = format_columns.DictListColumn(server.networks) # Map 'metadata' field to 'properties' - if not info['metadata']: - info.update( - {'properties': utils.format_dict(info.pop('metadata'))} - ) - else: - info.update( - {'properties': format_columns.DictColumn(info.pop('metadata'))} - ) + info['properties'] = format_columns.DictColumn(info.pop('metadata')) # Migrate tenant_id to project_id naming if 'tenant_id' in info: @@ -205,7 +184,7 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True): # Map power state num to meaningful string if 'OS-EXT-STS:power_state' in info: - info['OS-EXT-STS:power_state'] = _format_servers_list_power_state( + info['OS-EXT-STS:power_state'] = PowerStateColumn( info['OS-EXT-STS:power_state']) # Remove values that are long and not too useful @@ -1873,10 +1852,9 @@ class ListServer(command.Lister): s, columns, mixed_case_fields=mixed_case_fields, formatters={ - 'OS-EXT-STS:power_state': - _format_servers_list_power_state, - 'Networks': _format_servers_list_networks, - 'Metadata': utils.format_dict, + 'OS-EXT-STS:power_state': PowerStateColumn, + 'Networks': format_columns.DictListColumn, + 'Metadata': format_columns.DictColumn, }, ) for s in data ), diff --git a/openstackclient/tests/functional/compute/v2/test_server.py b/openstackclient/tests/functional/compute/v2/test_server.py index 44d9c61ffb..bad3f93d47 100644 --- a/openstackclient/tests/functional/compute/v2/test_server.py +++ b/openstackclient/tests/functional/compute/v2/test_server.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import itertools import json import time import uuid @@ -346,6 +347,14 @@ class ServerTests(common.ComputeTestCase): # DevStack without cells. self.skipTest("No Network service present") + def _chain_addresses(addresses): + # Flatten a dict of lists mapping network names to IP addresses, + # returning only the IP addresses + # + # >>> _chain_addresses({'private': ['10.1.0.32', '172.24.5.41']}) + # ['10.1.0.32', '172.24.5.41'] + return itertools.chain(*[*addresses.values()]) + cmd_output = self.server_create() name = cmd_output['name'] self.wait_for_status(name, "ACTIVE") @@ -387,7 +396,7 @@ class ServerTests(common.ComputeTestCase): 'server show -f json ' + name )) - if floating_ip not in cmd_output['addresses']: + if floating_ip not in _chain_addresses(cmd_output['addresses']): # Hang out for a bit and try again print('retrying floating IP check') wait_time += 10 @@ -397,7 +406,7 @@ class ServerTests(common.ComputeTestCase): self.assertIn( floating_ip, - cmd_output['addresses'], + _chain_addresses(cmd_output['addresses']), ) # detach ip @@ -417,7 +426,7 @@ class ServerTests(common.ComputeTestCase): 'server show -f json ' + name )) - if floating_ip in cmd_output['addresses']: + if floating_ip in _chain_addresses(cmd_output['addresses']): # Hang out for a bit and try again print('retrying floating IP check') wait_time += 10 @@ -431,7 +440,7 @@ class ServerTests(common.ComputeTestCase): )) self.assertNotIn( floating_ip, - cmd_output['addresses'], + _chain_addresses(cmd_output['addresses']), ) def test_server_reboot(self): @@ -856,8 +865,7 @@ class ServerTests(common.ComputeTestCase): server = json.loads(self.openstack( 'server show -f json ' + server_name )) - self.assertIsNotNone(server['addresses']) - self.assertEqual('', server['addresses']) + self.assertEqual({}, server['addresses']) def test_server_create_with_security_group(self): """Test server create with security group ID and name""" diff --git a/openstackclient/tests/unit/compute/v2/test_server.py b/openstackclient/tests/unit/compute/v2/test_server.py index 1c8541936b..efc105e565 100644 --- a/openstackclient/tests/unit/compute/v2/test_server.py +++ b/openstackclient/tests/unit/compute/v2/test_server.py @@ -22,6 +22,7 @@ from unittest.mock import call import iso8601 from novaclient import api_versions from openstack import exceptions as sdk_exceptions +from osc_lib.cli import format_columns from osc_lib import exceptions from osc_lib import utils as common_utils @@ -33,6 +34,29 @@ from openstackclient.tests.unit import utils from openstackclient.tests.unit.volume.v2 import fakes as volume_fakes +class TestPowerStateColumn(utils.TestCase): + + def test_human_readable(self): + self.assertEqual( + 'NOSTATE', server.PowerStateColumn(0x00).human_readable()) + self.assertEqual( + 'Running', server.PowerStateColumn(0x01).human_readable()) + self.assertEqual( + '', server.PowerStateColumn(0x02).human_readable()) + self.assertEqual( + 'Paused', server.PowerStateColumn(0x03).human_readable()) + self.assertEqual( + 'Shutdown', server.PowerStateColumn(0x04).human_readable()) + self.assertEqual( + '', server.PowerStateColumn(0x05).human_readable()) + self.assertEqual( + 'Crashed', server.PowerStateColumn(0x06).human_readable()) + self.assertEqual( + 'Suspended', server.PowerStateColumn(0x07).human_readable()) + self.assertEqual( + 'N/A', server.PowerStateColumn(0x08).human_readable()) + + class TestServer(compute_fakes.TestComputev2): def setUp(self): @@ -1015,15 +1039,15 @@ class TestServerCreate(TestServer): def datalist(self): datalist = ( - server._format_servers_list_power_state( + 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, self.new_server.networks, - '', + format_columns.DictColumn(self.new_server.metadata), ) return datalist @@ -3041,7 +3065,7 @@ class TestServerList(TestServer): s.id, s.name, s.status, - server._format_servers_list_networks(s.networks), + format_columns.DictListColumn(s.networks), # Image will be an empty string if boot-from-volume self.image.name if s.image else server.IMAGE_STRING_FOR_BFV, self.flavor.name, @@ -3051,10 +3075,10 @@ class TestServerList(TestServer): s.name, s.status, getattr(s, 'OS-EXT-STS:task_state'), - server._format_servers_list_power_state( + server.PowerStateColumn( getattr(s, 'OS-EXT-STS:power_state') ), - server._format_servers_list_networks(s.networks), + format_columns.DictListColumn(s.networks), # Image will be an empty string if boot-from-volume self.image.name if s.image else server.IMAGE_STRING_FOR_BFV, s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV, @@ -3062,13 +3086,13 @@ class TestServerList(TestServer): s.flavor['id'], getattr(s, 'OS-EXT-AZ:availability_zone'), getattr(s, 'OS-EXT-SRV-ATTR:host'), - s.Metadata, + format_columns.DictColumn({}), )) self.data_no_name_lookup.append(( s.id, s.name, s.status, - server._format_servers_list_networks(s.networks), + format_columns.DictListColumn(s.networks), # Image will be an empty string if boot-from-volume s.image['id'] if s.image else server.IMAGE_STRING_FOR_BFV, s.flavor['id'] @@ -3128,7 +3152,7 @@ class TestServerList(TestServer): self.servers_mock.list.assert_called_with(**self.kwargs) self.assertEqual(self.columns_long, columns) - self.assertEqual(tuple(self.data_long), tuple(data)) + self.assertCountEqual(tuple(self.data_long), tuple(data)) def test_server_list_column_option(self): arglist = [ @@ -3429,9 +3453,11 @@ class TestServerList(TestServer): def test_server_list_v269_with_partial_constructs(self): self.app.client_manager.compute.api_version = \ api_versions.APIVersion('2.69') + arglist = [] verifylist = [] parsed_args = self.check_parser(self.cmd, arglist, verifylist) + # include "partial results" from non-responsive part of # infrastructure. server_dict = { @@ -3454,10 +3480,10 @@ class TestServerList(TestServer): # it will fail at formatting the networks info later on. "networks": {} } - server = compute_fakes.fakes.FakeResource( + _server = compute_fakes.fakes.FakeResource( info=server_dict, ) - self.servers.append(server) + self.servers.append(_server) columns, data = self.cmd.take_action(parsed_args) # get the first three servers out since our interest is in the partial # server. @@ -3466,8 +3492,13 @@ class TestServerList(TestServer): next(data) partial_server = next(data) expected_row = ( - 'server-id-95a56bfc4xxxxxx28d7e418bfd97813a', '', - 'UNKNOWN', '', '', '') + 'server-id-95a56bfc4xxxxxx28d7e418bfd97813a', + '', + 'UNKNOWN', + format_columns.DictListColumn({}), + '', + '', + ) self.assertEqual(expected_row, partial_server) def test_server_list_with_tag(self): @@ -6292,15 +6323,16 @@ class TestServerShow(TestServer): ) self.data = ( - 'Running', - 'public=10.20.30.40, 2001:db8::f', + server.PowerStateColumn( + getattr(self.server, 'OS-EXT-STS:power_state')), + format_columns.DictListColumn(self.server.networks), self.flavor.name + " (" + self.flavor.id + ")", self.server.id, self.image.name + " (" + self.image.id + ")", self.server.name, {'public': ['10.20.30.40', '2001:db8::f']}, 'tenant-id-xxx', - '', + format_columns.DictColumn({}), ) def test_show_no_options(self): @@ -6323,7 +6355,7 @@ class TestServerShow(TestServer): columns, data = self.cmd.take_action(parsed_args) self.assertEqual(self.columns, columns) - self.assertEqual(self.data, data) + self.assertCountEqual(self.data, data) def test_show_embedded_flavor(self): # Tests using --os-compute-api-version >= 2.47 where the flavor @@ -6350,7 +6382,7 @@ class TestServerShow(TestServer): self.assertEqual(self.columns, columns) # Since the flavor details are in a dict we can't be sure of the # ordering so just assert that one of the keys is in the output. - self.assertIn('original_name', data[2]) + self.assertIn('original_name', data[2]._value) def test_show_diagnostics(self): arglist = [ @@ -6719,50 +6751,6 @@ class TestServerGeneral(TestServer): self.assertRaises(exceptions.CommandError, server._get_ip_address, self.OLD, 'private', [6]) - def test_format_servers_list_power_state(self): - self.assertEqual("NOSTATE", - server._format_servers_list_power_state(0x00)) - self.assertEqual("Running", - server._format_servers_list_power_state(0x01)) - self.assertEqual("", - server._format_servers_list_power_state(0x02)) - self.assertEqual("Paused", - server._format_servers_list_power_state(0x03)) - self.assertEqual("Shutdown", - server._format_servers_list_power_state(0x04)) - self.assertEqual("", - server._format_servers_list_power_state(0x05)) - self.assertEqual("Crashed", - server._format_servers_list_power_state(0x06)) - self.assertEqual("Suspended", - server._format_servers_list_power_state(0x07)) - self.assertEqual("N/A", - server._format_servers_list_power_state(0x08)) - - def test_format_servers_list_networks(self): - # Setup network info to test. - networks = { - u'public': [u'10.20.30.40', u'2001:db8::f'], - u'private': [u'2001:db8::f', u'10.20.30.40'], - } - - # Prepare expected data. - # Since networks is a dict, whose items are in random order, there - # could be two results after formatted. - data_1 = (u'private=2001:db8::f, 10.20.30.40; ' - u'public=10.20.30.40, 2001:db8::f') - data_2 = (u'public=10.20.30.40, 2001:db8::f; ' - u'private=2001:db8::f, 10.20.30.40') - - # Call _format_servers_list_networks(). - networks_format = server._format_servers_list_networks(networks) - - msg = ('Network string is not formatted correctly.\n' - 'reference = %s or %s\n' - 'actual = %s\n' % - (data_1, data_2, networks_format)) - self.assertIn(networks_format, (data_1, data_2), msg) - @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 @@ -6789,14 +6777,14 @@ class TestServerGeneral(TestServer): info = { 'id': _server.id, 'name': _server.name, - 'addresses': u'public=10.20.30.40, 2001:db8::f', - 'flavor': u'%s (%s)' % (_flavor.name, _flavor.id), - 'image': u'%s (%s)' % (_image.name, _image.id), - 'project_id': u'tenant-id-xxx', - 'properties': '', - 'OS-EXT-STS:power_state': server._format_servers_list_power_state( + 'image': '%s (%s)' % (_image.name, _image.id), + 'flavor': '%s (%s)' % (_flavor.name, _flavor.id), + 'OS-EXT-STS:power_state': server.PowerStateColumn( getattr(_server, 'OS-EXT-STS:power_state')), + 'properties': '', 'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}], + 'addresses': format_columns.DictListColumn(_server.networks), + 'project_id': 'tenant-id-xxx', } # Call _prep_server_detail(). @@ -6809,4 +6797,4 @@ class TestServerGeneral(TestServer): server_detail.pop('networks') # Check the results. - self.assertEqual(info, server_detail) + self.assertCountEqual(info, server_detail) diff --git a/releasenotes/notes/improved-server-output-6965b664f6abda8d.yaml b/releasenotes/notes/improved-server-output-6965b664f6abda8d.yaml new file mode 100644 index 0000000000..cbe950eace --- /dev/null +++ b/releasenotes/notes/improved-server-output-6965b664f6abda8d.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + The ``addresses`` and ``flavor`` fields of the ``server show`` command will + now be correctly rendered as a list of objects and an object, respectively. + - | + The ``networks`` and ``properties`` fields of the ``server list`` command + will now be rendered as objects. In addition, the ``power_state`` field + will now be humanized and rendered as a string value when using the table + formatter.