openstack/python-openstackclient
Code Issues Proposed changes

compute: Fix formatting of 'server show'

Browse Source 
In change Ic253184ee5f911ec2052419d328260dc4664b273, we switched to
using the SDK for the 'server show' command. There were a couple of
issues with this change, which we address here:

- openstacksdk uses different names for fields than the nova API. We
  opted to output both the original names and the openstacksdk aliases
  in the output. With testing, however, it's become obvious that the
  resulting output is very long and rather unfriendly from a UX
  perspective. We opt to only show fields with their original names.

- A number of fields included in the output are only valid in requests
  and will never be present in responses. These are removed.

- A number of fields are not present in later API microversions or are
  only present under certain conditions. These are removed from output
  when not included in responses.

- The image and flavor fields both had errant logic that resulted in
  unnecessary or incorrect information being show. This logic is
  corrected.

With these changes, the output now resembles the output seen before the
migration to openstacksdk. In the future we may wish to build on this
further and switch from a blacklist model (removing the fields we do not
wish to show from output) to a whitelist model (specifically stating
which fields to show) but that's a change for another day.

Change-Id: I7e3eaa0149bff202c8fd4538356cbc75b4f7e708
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This commit is contained in:
Stephen Finucane 2023-03-20 12:16:17 +00:00 committed by Stephen Finucane
parent 31ae635ffe
commit f43e2ed20d
2 changed files with 76 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

103
openstackclient/compute/v2/server.py
View File

@ -150,16 +150,13 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
# Some commands using this routine were originally implemented with the
# nova python wrappers, and were later migrated to use the SDK. Map the
# SDK's property names to the original property names to maintain backward
# compatibility for existing users. Data is duplicated under both the old
# and new name so users can consume the data by either name.
# compatibility for existing users.
column_map = {
'access_ipv4': 'accessIPv4',
'access_ipv6': 'accessIPv6',
'admin_password': 'adminPass',
'admin_password': 'adminPass',
'volumes': 'os-extended-volumes:volumes_attached',
'attached_volumes': 'volumes_attached',
'availability_zone': 'OS-EXT-AZ:availability_zone',
'block_device_mapping': 'block_device_mapping_v2',
'compute_host': 'OS-EXT-SRV-ATTR:host',
'created_at': 'created',
'disk_config': 'OS-DCF:diskConfig',
@ -169,7 +166,6 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
'fault': 'fault',
'hostname': 'OS-EXT-SRV-ATTR:hostname',
'hypervisor_hostname': 'OS-EXT-SRV-ATTR:hypervisor_hostname',
'image_id': 'imageRef',
'instance_name': 'OS-EXT-SRV-ATTR:instance_name',
'is_locked': 'locked',
'kernel_id': 'OS-EXT-SRV-ATTR:kernel_id',
@ -180,21 +176,56 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
'ramdisk_id': 'OS-EXT-SRV-ATTR:ramdisk_id',
'reservation_id': 'OS-EXT-SRV-ATTR:reservation_id',
'root_device_name': 'OS-EXT-SRV-ATTR:root_device_name',
'scheduler_hints': 'OS-SCH-HNT:scheduler_hints',
'task_state': 'OS-EXT-STS:task_state',
'terminated_at': 'OS-SRV-USG:terminated_at',
'updated_at': 'updated',
'user_data': 'OS-EXT-SRV-ATTR:user_data',
'vm_state': 'OS-EXT-STS:vm_state',
}
# Some columns returned by openstacksdk should not be shown because they're
# either irrelevant or duplicates
ignored_columns = {
# computed columns
'interface_ip',
'location',
'private_v4',
'private_v6',
'public_v4',
'public_v6',
# create-only columns
'block_device_mapping',
'image_id',
'max_count',
'min_count',
'scheduler_hints',
# aliases
'volumes',
# unnecessary
'links',
}
# 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
}
info.update(
{
column_map[column]: data
for column, data in info.items()
if column in column_map
}
)
data = {}
for key, value in info.items():
if key in ignored_columns:
continue
if key in optional_columns:
if info[key] is None:
continue
alias = column_map.get(key)
data[alias or key] = value
info = data
# Convert the image blob to a name
image_info = info.get('image', {})
@ -215,46 +246,57 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
# Convert the flavor blob to a name
flavor_info = info.get('flavor', {})
# Microversion 2.47 puts the embedded flavor into the server response
# body but omits the id, so if not present we just expose the flavor
# dict in the server output.
if 'id' in flavor_info:
# body. The presence of the 'original_name' attribute indicates this.
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)
info['flavor'] = "%s (%s)" % (flavor.name, flavor_id)
except Exception:
info['flavor'] = flavor_id
else:
else: # microversion >= 2.47
info['flavor'] = format_columns.DictColumn(flavor_info)
if 'os-extended-volumes:volumes_attached' in info:
# there's a lot of redundant information in BDMs - strip it
if 'volumes_attached' in info:
info.update(
{
'volumes_attached': format_columns.ListDictColumn(
info.pop('os-extended-volumes:volumes_attached')
[
{
k: v
for k, v in volume.items()
if v is not None and k != 'location'
}
for volume in info.pop('volumes_attached') or []
]
)
}
)
if 'security_groups' in info:
info.update(
{
'security_groups': format_columns.ListDictColumn(
info.pop('security_groups')
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'] = (
AddressesColumn(info['addresses'])
if 'addresses' in info
else format_columns.DictListColumn(info.get('networks'))
)
# Map 'networks' to 'addresses', if present. Note that the 'networks' key
# is used for create responses, otherwise it's 'addresses'. We know it'll
# be set because this is one of our optional columns.
if 'networks' in info:
info['addresses'] = format_columns.DictListColumn(
info.pop('networks', {}),
)
else:
info['addresses'] = AddressesColumn(info.get('addresses', {}))
# Map 'metadata' field to 'properties'
# Map 'metadata' field to 'properties' and format
info['properties'] = format_columns.DictColumn(info.pop('metadata'))
# Migrate tenant_id to project_id naming
@ -267,9 +309,6 @@ def _prep_server_detail(compute_client, image_client, server, refresh=True):
info['OS-EXT-STS:power_state']
)
# Remove values that are long and not too useful
info.pop('links', None)
return info

16
openstackclient/tests/unit/compute/v2/test_server.py
View File

@ -1380,7 +1380,6 @@ class TestServerCreate(TestServer):
'id',
'image',
'name',
'networks',
'properties',
)
@ -1394,7 +1393,6 @@ class TestServerCreate(TestServer):
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
@ -2399,7 +2397,7 @@ class TestServerCreate(TestServer):
self.new_server.name, self.image, self.flavor, **kwargs
)
self.assertEqual(self.columns, columns)
self.assertEqual(self.datalist(), data)
self.assertTupleEqual(self.datalist(), data)
@mock.patch.object(common_utils, 'wait_for_status', return_value=False)
def test_server_create_with_wait_fails(self, mock_wait_for_status):
@ -8245,7 +8243,7 @@ class TestServerShow(TestServer):
'image': {'id': self.image.id},
'flavor': {'id': self.flavor.id},
'tenant_id': 'tenant-id-xxx',
'networks': {'public': ['10.20.30.40', '2001:db8::f']},
'addresses': {'public': ['10.20.30.40', '2001:db8::f']},
}
self.sdk_client.get_server_diagnostics.return_value = {'test': 'test'}
server_method = {
@ -8270,7 +8268,6 @@ class TestServerShow(TestServer):
'id',
'image',
'name',
'networks',
'project_id',
'properties',
)
@ -8279,12 +8276,11 @@ class TestServerShow(TestServer):
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']},
server.AddressesColumn({'public': ['10.20.30.40', '2001:db8::f']}),
'tenant-id-xxx',
format_columns.DictColumn({}),
)
@ -9095,7 +9091,7 @@ class TestServerGeneral(TestServer):
'image': {u'id': _image.id},
'flavor': {u'id': _flavor.id},
'tenant_id': u'tenant-id-xxx',
'networks': {u'public': [u'10.20.30.40', u'2001:db8::f']},
'addresses': {u'public': [u'10.20.30.40', u'2001:db8::f']},
'links': u'http://xxx.yyy.com',
'properties': '',
'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
@ -9115,7 +9111,7 @@ class TestServerGeneral(TestServer):
),
'properties': '',
'volumes_attached': [{"id": "6344fe9d-ef20-45b2-91a6"}],
'addresses': format_columns.DictListColumn(_server.networks),
'addresses': format_columns.DictListColumn(_server.addresses),
'project_id': 'tenant-id-xxx',
}
@ -9125,8 +9121,6 @@ class TestServerGeneral(TestServer):
self.app.client_manager.image,
_server,
)
# 'networks' is used to create _server. Remove it.
server_detail.pop('networks')
# Check the results.
self.assertCountEqual(info, server_detail)