compute: Migrate 'server create' to SDK

The final step. Future changes will clean up the remnants of the
novaclient usage. This is a rather large patch, owing to the number of
things that novaclient was handling for us which SDK does not, but the
combination of unit and functional tests mean we should be handling
all of these differences.

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

openstackclient/api/compute_v2.py

@@ -621,3 +621,40 @@ def find_security_group(compute_client, name_or_id):
         raise exceptions.NotFound(f'{name_or_id} not found')
 
     return found
+
+
+def find_network(compute_client, name_or_id):
+    """Find the ID for a given network name or ID
+
+    https://docs.openstack.org/api-ref/compute/#show-network-details
+
+    :param compute_client: A compute client
+    :param name_or_id: The name or ID of the network to look up
+    :returns: A network object
+    :raises exception.NotFound: If a matching network could not be found or
+        more than one match was found
+    """
+    response = compute_client.get(
+        f'/os-networks/{name_or_id}', microversion='2.1'
+    )
+    if response.status_code != http.HTTPStatus.NOT_FOUND:
+        # there might be other, non-404 errors
+        sdk_exceptions.raise_from_response(response)
+        return response.json()['network']
+
+    response = compute_client.get('/os-networks', microversion='2.1')
+    sdk_exceptions.raise_from_response(response)
+    found = None
+    networks = response.json()['networks']
+    for network in networks:
+        if network['label'] == name_or_id:
+            if found:
+                raise exceptions.NotFound(
+                    f'multiple matches found for {name_or_id}'
+                )
+            found = network
+
+    if not found:
+        raise exceptions.NotFound(f'{name_or_id} not found')
+
+    return found

openstackclient/compute/v2/server.py

@@ -21,10 +21,10 @@ import getpass
 import json
 import logging
 import os
+import typing as ty
 
 from cliff import columns as cliff_columns
 import iso8601
-from novaclient import api_versions
 from openstack import exceptions as sdk_exceptions
 from openstack import utils as sdk_utils
 from osc_lib.cli import format_columns
@@ -82,7 +82,7 @@ class AddressesColumn(cliff_columns.FormattableColumn):
     def machine_readable(self):
         return {
             k: [i['addr'] for i in v if 'addr' in i]
-            for k, v in self._value.items()
+            for k, v in (self._value.items() if self._value else [])
         }
 
 
@@ -1069,7 +1069,6 @@ class BDMAction(parseractions.MultiKeyValueAction):
             super().__call__(parser, namespace, values, option_string)
 
 
-# TODO(stephenfin): Migrate to SDK
 class CreateServer(command.ShowOne):
     _description = _("Create a new server")
 
@@ -1173,7 +1172,7 @@ class CreateServer(command.ShowOne):
         )
         parser.add_argument(
             '--block-device',
-            metavar='',
+            metavar='<block-device>',
             action=BDMAction,
             dest='block_devices',
             default=[],
@@ -1507,7 +1506,7 @@ class CreateServer(command.ShowOne):
                 self.app.stdout.write('\rProgress: %s' % progress)
                 self.app.stdout.flush()
 
-        compute_client = self.app.client_manager.compute
+        compute_client = self.app.client_manager.sdk_connection.compute
         volume_client = self.app.client_manager.volume
         image_client = self.app.client_manager.image
 
@@ -1602,12 +1601,12 @@ class CreateServer(command.ShowOne):
                 parsed_args.snapshot,
             ).id
 
-        flavor = utils.find_resource(
+        flavor = compute_client.find_flavor(
-            compute_client.flavors, parsed_args.flavor
+            parsed_args.flavor, ignore_missing=False
         )
 
         if parsed_args.file:
-            if compute_client.api_version >= api_versions.APIVersion('2.57'):
+            if sdk_utils.supports_microversion(compute_client, '2.57'):
                 msg = _(
                     'Personality files are deprecated and are not supported '
                     'for --os-compute-api-version greater than 2.56; use '
@@ -1638,10 +1637,12 @@ class CreateServer(command.ShowOne):
             msg = _("max instances should be > 0")
             raise exceptions.CommandError(msg)
 
-        userdata = None
+        user_data = None
         if parsed_args.user_data:
             try:
-                userdata = open(parsed_args.user_data)
+                with open(parsed_args.user_data, 'rb') as fh:
+                    # TODO(stephenfin): SDK should do this for us
+                    user_data = base64.b64encode(fh.read()).decode('utf-8')
             except OSError as e:
                 msg = _("Can't open '%(data)s': %(exception)s")
                 raise exceptions.CommandError(
@@ -1649,7 +1650,7 @@ class CreateServer(command.ShowOne):
                 )
 
         if parsed_args.description:
-            if compute_client.api_version < api_versions.APIVersion("2.19"):
+            if not sdk_utils.supports_microversion(compute_client, '2.19'):
                 msg = _(
                     '--os-compute-api-version 2.19 or greater is '
                     'required to support the --description option'
@@ -1657,26 +1658,7 @@ class CreateServer(command.ShowOne):
                 raise exceptions.CommandError(msg)
 
         block_device_mapping_v2 = []
-        if volume:
+        if parsed_args.boot_from_volume:
-            block_device_mapping_v2 = [
-                {
-                    'uuid': volume,
-                    'boot_index': 0,
-                    'source_type': 'volume',
-                    'destination_type': 'volume',
-                }
-            ]
-        elif snapshot:
-            block_device_mapping_v2 = [
-                {
-                    'uuid': snapshot,
-                    'boot_index': 0,
-                    'source_type': 'snapshot',
-                    'destination_type': 'volume',
-                    'delete_on_termination': False,
-                }
-            ]
-        elif parsed_args.boot_from_volume:
             # Tell nova to create a root volume from the image provided.
             if not image:
                 msg = _(
@@ -1695,6 +1677,35 @@ class CreateServer(command.ShowOne):
             ]
             # If booting from volume we do not pass an image to compute.
             image = None
+        elif image:
+            block_device_mapping_v2 = [
+                {
+                    'uuid': image.id,
+                    'boot_index': 0,
+                    'source_type': 'image',
+                    'destination_type': 'local',
+                    'delete_on_termination': True,
+                }
+            ]
+        elif volume:
+            block_device_mapping_v2 = [
+                {
+                    'uuid': volume,
+                    'boot_index': 0,
+                    'source_type': 'volume',
+                    'destination_type': 'volume',
+                }
+            ]
+        elif snapshot:
+            block_device_mapping_v2 = [
+                {
+                    'uuid': snapshot,
+                    'boot_index': 0,
+                    'source_type': 'snapshot',
+                    'destination_type': 'volume',
+                    'delete_on_termination': False,
+                }
+            ]
 
         if parsed_args.swap:
             block_device_mapping_v2.append(
@@ -1770,7 +1781,7 @@ class CreateServer(command.ShowOne):
                     raise exceptions.CommandError(msg)
 
             if 'tag' in mapping and (
-                compute_client.api_version < api_versions.APIVersion('2.42')
+                not sdk_utils.supports_microversion(compute_client, '2.42')
             ):
                 msg = _(
                     '--os-compute-api-version 2.42 or greater is '
@@ -1779,7 +1790,7 @@ class CreateServer(command.ShowOne):
                 raise exceptions.CommandError(msg)
 
             if 'volume_type' in mapping and (
-                compute_client.api_version < api_versions.APIVersion('2.67')
+                not sdk_utils.supports_microversion(compute_client, '2.67')
             ):
                 msg = _(
                     '--os-compute-api-version 2.67 or greater is '
@@ -1835,7 +1846,7 @@ class CreateServer(command.ShowOne):
 
             block_device_mapping_v2.append(mapping)
 
-        if not image and not any(
+        if not any(
             [bdm.get('boot_index') == 0 for bdm in block_device_mapping_v2]
         ):
             msg = _(
@@ -1844,10 +1855,12 @@ class CreateServer(command.ShowOne):
             )
             raise exceptions.CommandError(msg)
 
-        nics = parsed_args.nics
+        # Default to empty list if nothing was specified and let nova
+        # decide the default behavior.
+        networks: ty.Union[str, ty.List[ty.Dict[str, str]], None] = []
 
-        if 'auto' in nics or 'none' in nics:
+        if 'auto' in parsed_args.nics or 'none' in parsed_args.nics:
-            if len(nics) > 1:
+            if len(parsed_args.nics) > 1:
                 msg = _(
                     'Specifying a --nic of auto or none cannot '
                     'be used with any other --nic, --network '
@@ -1855,7 +1868,7 @@ class CreateServer(command.ShowOne):
                 )
                 raise exceptions.CommandError(msg)
 
-            if compute_client.api_version < api_versions.APIVersion('2.37'):
+            if not sdk_utils.supports_microversion(compute_client, '2.37'):
                 msg = _(
                     '--os-compute-api-version 2.37 or greater is '
                     'required to support explicit auto-allocation of a '
@@ -1863,12 +1876,12 @@ class CreateServer(command.ShowOne):
                 )
                 raise exceptions.CommandError(msg)
 
-            nics = nics[0]
+            networks = parsed_args.nics[0]
         else:
-            for nic in nics:
+            for nic in parsed_args.nics:
                 if 'tag' in nic:
-                    if compute_client.api_version < api_versions.APIVersion(
+                    if not sdk_utils.supports_microversion(
-                        '2.43'
+                        compute_client, '2.43'
                     ):
                         msg = _(
                             '--os-compute-api-version 2.43 or greater is '
@@ -1894,9 +1907,11 @@ class CreateServer(command.ShowOne):
                         nic['port-id'] = port.id
                 else:
                     if nic['net-id']:
-                        nic['net-id'] = compute_client.api.network_find(
+                        net = compute_v2.find_network(
+                            compute_client,
                             nic['net-id'],
-                        )['id']
+                        )
+                        nic['net-id'] = net['id']
 
                     if nic['port-id']:
                         msg = _(
@@ -1905,18 +1920,35 @@ class CreateServer(command.ShowOne):
                         )
                         raise exceptions.CommandError(msg)
 
-        if not nics:
+                # convert from the novaclient-derived "NIC" view to the actual
+                # "network" view
+                network = {}
+
+                if nic['net-id']:
+                    network['uuid'] = nic['net-id']
+
+                if nic['port-id']:
+                    network['port'] = nic['port-id']
+
+                if nic['v4-fixed-ip']:
+                    network['fixed'] = nic['v4-fixed-ip']
+                elif nic['v6-fixed-ip']:
+                    network['fixed'] = nic['v6-fixed-ip']
+
+                if nic.get('tag'):  # tags are optional
+                    network['tag'] = nic['tag']
+
+                networks.append(network)
+
+        if not parsed_args.nics and sdk_utils.supports_microversion(
+            compute_client, '2.37'
+        ):
             # Compute API version >= 2.37 requires a value, so default to
             # 'auto' to maintain legacy behavior if a nic wasn't specified.
-            if compute_client.api_version >= api_versions.APIVersion('2.37'):
+            networks = 'auto'
-                nics = 'auto'
-            else:
-                # Default to empty list if nothing was specified and let nova
-                # decide the default behavior.
-                nics = []
 
         # Check security group exist and convert ID to name
-        security_group_names = []
+        security_groups = []
         if self.app.client_manager.is_network_endpoint_enabled():
             network_client = self.app.client_manager.network
             for each_sg in parsed_args.security_group:
@@ -1925,12 +1957,12 @@ class CreateServer(command.ShowOne):
                 )
                 # Use security group ID to avoid multiple security group have
                 # same name in neutron networking backend
-                security_group_names.append(sg.id)
+                security_groups.append({'name': sg.id})
         else:
             # Handle nova-network case
             for each_sg in parsed_args.security_group:
-                sg = compute_client.api.security_group_find(each_sg)
+                sg = compute_v2.find_security_group(compute_client, each_sg)
-                security_group_names.append(sg['name'])
+                security_groups.append({'name': sg['name']})
 
         hints = {}
         for key, values in parsed_args.hints.items():
@@ -1941,9 +1973,8 @@ class CreateServer(command.ShowOne):
                 hints[key] = values
 
         if parsed_args.server_group:
-            server_group_obj = utils.find_resource(
+            server_group_obj = compute_client.find_server_group(
-                compute_client.server_groups,
+                parsed_args.server_group, ignore_missing=False
-                parsed_args.server_group,
             )
             hints['group'] = server_group_obj.id
 
@@ -1965,69 +1996,89 @@ class CreateServer(command.ShowOne):
             else:
                 config_drive = parsed_args.config_drive
 
-        boot_args = [parsed_args.server_name, image, flavor]
+        kwargs = {
-
+            'name': parsed_args.server_name,
-        boot_kwargs = dict(
+            'image_id': image.id if image else '',
-            meta=parsed_args.properties,
+            'flavor_id': flavor.id,
-            files=files,
+            'min_count': parsed_args.min,
-            reservation_id=None,
+            'max_count': parsed_args.max,
-            min_count=parsed_args.min,
+        }
-            max_count=parsed_args.max,
-            security_groups=security_group_names,
-            userdata=userdata,
-            key_name=parsed_args.key_name,
-            availability_zone=parsed_args.availability_zone,
-            admin_pass=parsed_args.password,
-            block_device_mapping_v2=block_device_mapping_v2,
-            nics=nics,
-            scheduler_hints=hints,
-            config_drive=config_drive,
-        )
 
         if parsed_args.description:
-            boot_kwargs['description'] = parsed_args.description
+            kwargs['description'] = parsed_args.description
+
+        if parsed_args.availability_zone:
+            kwargs['availability_zone'] = parsed_args.availability_zone
+
+        if parsed_args.password:
+            kwargs['admin_password'] = parsed_args.password
+
+        if parsed_args.properties:
+            kwargs['metadata'] = parsed_args.properties
+
+        if parsed_args.key_name:
+            kwargs['key_name'] = parsed_args.key_name
+
+        if user_data:
+            kwargs['user_data'] = user_data
+
+        if files:
+            kwargs['personality'] = files
+
+        if security_groups:
+            kwargs['security_groups'] = security_groups
+
+        if block_device_mapping_v2:
+            kwargs['block_device_mapping'] = block_device_mapping_v2
+
+        if hints:
+            kwargs['scheduler_hints'] = hints
+
+        if networks is not None:
+            kwargs['networks'] = networks
+
+        if config_drive is not None:
+            kwargs['config_drive'] = config_drive
 
         if parsed_args.tags:
-            if compute_client.api_version < api_versions.APIVersion('2.52'):
+            if not sdk_utils.supports_microversion(compute_client, '2.52'):
                 msg = _(
                     '--os-compute-api-version 2.52 or greater is required to '
                     'support the --tag option'
                 )
                 raise exceptions.CommandError(msg)
 
-            boot_kwargs['tags'] = parsed_args.tags
+            kwargs['tags'] = parsed_args.tags
 
         if parsed_args.host:
-            if compute_client.api_version < api_versions.APIVersion("2.74"):
+            if not sdk_utils.supports_microversion(compute_client, '2.74'):
                 msg = _(
                     '--os-compute-api-version 2.74 or greater is required to '
                     'support the --host option'
                 )
                 raise exceptions.CommandError(msg)
 
-            boot_kwargs['host'] = parsed_args.host
+            kwargs['host'] = parsed_args.host
 
         if parsed_args.hypervisor_hostname:
-            if compute_client.api_version < api_versions.APIVersion("2.74"):
+            if not sdk_utils.supports_microversion(compute_client, '2.74'):
                 msg = _(
                     '--os-compute-api-version 2.74 or greater is required to '
                     'support the --hypervisor-hostname option'
                 )
                 raise exceptions.CommandError(msg)
 
-            boot_kwargs['hypervisor_hostname'] = (
+            kwargs['hypervisor_hostname'] = parsed_args.hypervisor_hostname
-                parsed_args.hypervisor_hostname
-            )
 
         if parsed_args.hostname:
-            if compute_client.api_version < api_versions.APIVersion("2.90"):
+            if not sdk_utils.supports_microversion(compute_client, '2.90'):
                 msg = _(
                     '--os-compute-api-version 2.90 or greater is required to '
                     'support the --hostname option'
                 )
                 raise exceptions.CommandError(msg)
 
-            boot_kwargs['hostname'] = parsed_args.hostname
+            kwargs['hostname'] = parsed_args.hostname
 
         # TODO(stephenfin): Handle OS_TRUSTED_IMAGE_CERTIFICATE_IDS
         if parsed_args.trusted_image_certs:
@@ -2037,7 +2088,7 @@ class CreateServer(command.ShowOne):
                     'servers booted directly from images'
                 )
                 raise exceptions.CommandError(msg)
-            if compute_client.api_version < api_versions.APIVersion('2.63'):
+            if not sdk_utils.supports_microversion(compute_client, '2.63'):
                 msg = _(
                     '--os-compute-api-version 2.63 or greater is required to '
                     'support the --trusted-image-cert option'
@@ -2045,25 +2096,22 @@ class CreateServer(command.ShowOne):
                 raise exceptions.CommandError(msg)
 
             certs = parsed_args.trusted_image_certs
-            boot_kwargs['trusted_image_certificates'] = certs
+            kwargs['trusted_image_certificates'] = certs
 
-        LOG.debug('boot_args: %s', boot_args)
+        LOG.debug('boot_kwargs: %s', kwargs)
-        LOG.debug('boot_kwargs: %s', boot_kwargs)
 
         # Wrap the call to catch exceptions in order to close files
         try:
-            server = compute_client.servers.create(*boot_args, **boot_kwargs)
+            server = compute_client.create_server(**kwargs)
         finally:
             # Clean up open files - make sure they are not strings
             for f in files:
                 if hasattr(f, 'close'):
                     f.close()
-            if hasattr(userdata, 'close'):
-                userdata.close()
 
         if parsed_args.wait:
             if utils.wait_for_status(
-                compute_client.servers.get,
+                compute_client.get_server,
                 server.id,
                 callback=_show_progress,
             ):
@@ -2072,8 +2120,6 @@ class CreateServer(command.ShowOne):
                 msg = _('Error creating server: %s') % parsed_args.server_name
                 raise exceptions.CommandError(msg)
 
-        # 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()))
 

openstackclient/tests/unit/api/test_compute_v2.py

@@ -763,3 +763,102 @@ class TestFindSecurityGroup(utils.TestCase):
             self.compute_sdk_client,
             sg_name,
         )
+
+
+class TestFindNetwork(utils.TestCase):
+
+    def setUp(self):
+        super().setUp()
+
+        self.compute_sdk_client = mock.Mock(_proxy.Proxy)
+
+    def test_find_network_by_id(self):
+        net_id = uuid.uuid4().hex
+        net_name = 'name-' + uuid.uuid4().hex
+        data = {
+            'network': {
+                'id': net_id,
+                'label': net_name,
+                # other fields omitted for brevity
+            }
+        }
+        self.compute_sdk_client.get.side_effect = [
+            fakes.FakeResponse(data=data),
+        ]
+
+        result = compute.find_network(self.compute_sdk_client, net_id)
+
+        self.compute_sdk_client.get.assert_has_calls(
+            [
+                mock.call(f'/os-networks/{net_id}', microversion='2.1'),
+            ]
+        )
+        self.assertEqual(data['network'], result)
+
+    def test_find_network_by_name(self):
+        net_id = uuid.uuid4().hex
+        net_name = 'name-' + uuid.uuid4().hex
+        data = {
+            'networks': [
+                {
+                    'id': net_id,
+                    'label': net_name,
+                    # other fields omitted for brevity
+                }
+            ],
+        }
+        self.compute_sdk_client.get.side_effect = [
+            fakes.FakeResponse(status_code=http.HTTPStatus.NOT_FOUND),
+            fakes.FakeResponse(data=data),
+        ]
+
+        result = compute.find_network(self.compute_sdk_client, net_name)
+
+        self.compute_sdk_client.get.assert_has_calls(
+            [
+                mock.call(f'/os-networks/{net_name}', microversion='2.1'),
+                mock.call('/os-networks', microversion='2.1'),
+            ]
+        )
+        self.assertEqual(data['networks'][0], result)
+
+    def test_find_network_not_found(self):
+        data = {'networks': []}
+        self.compute_sdk_client.get.side_effect = [
+            fakes.FakeResponse(status_code=http.HTTPStatus.NOT_FOUND),
+            fakes.FakeResponse(data=data),
+        ]
+        self.assertRaises(
+            osc_lib_exceptions.NotFound,
+            compute.find_network,
+            self.compute_sdk_client,
+            'invalid-net',
+        )
+
+    def test_find_network_by_name_duplicate(self):
+        net_name = 'name-' + uuid.uuid4().hex
+        data = {
+            'networks': [
+                {
+                    'id': uuid.uuid4().hex,
+                    'label': net_name,
+                    # other fields omitted for brevity
+                },
+                {
+                    'id': uuid.uuid4().hex,
+                    'label': net_name,
+                    # other fields omitted for brevity
+                },
+            ],
+        }
+        self.compute_sdk_client.get.side_effect = [
+            fakes.FakeResponse(status_code=http.HTTPStatus.NOT_FOUND),
+            fakes.FakeResponse(data=data),
+        ]
+
+        self.assertRaises(
+            osc_lib_exceptions.NotFound,
+            compute.find_network,
+            self.compute_sdk_client,
+            net_name,
+        )

openstackclient/tests/unit/network/v2/fakes.py

@@ -32,8 +32,9 @@ from openstack.network.v2 import network_ip_availability as _ip_availability
 from openstack.network.v2 import network_segment_range as _segment_range
 from openstack.network.v2 import port as _port
 from openstack.network.v2 import rbac_policy as network_rbac
+from openstack.network.v2 import security_group as _security_group
 from openstack.network.v2 import segment as _segment
-from openstack.network.v2 import service_profile as _flavor_profile
+from openstack.network.v2 import service_profile as _service_profile
 from openstack.network.v2 import trunk as _trunk
 
 from openstackclient.tests.unit import fakes
@@ -1943,11 +1944,44 @@ def get_network_rbacs(rbac_policies=None, count=2):
     return mock.Mock(side_effect=rbac_policies)
 
 
-def create_one_service_profile(attrs=None):
+def create_one_security_group(attrs=None):
-    """Create flavor profile."""
+    """Create a security group."""
     attrs = attrs or {}
 
-    flavor_profile_attrs = {
+    security_group_attrs = {
+        'name': 'security-group-name-' + uuid.uuid4().hex,
+        'id': 'security-group-id-' + uuid.uuid4().hex,
+        'project_id': 'project-id-' + uuid.uuid4().hex,
+        'description': 'security-group-description-' + uuid.uuid4().hex,
+        'location': 'MUNCHMUNCHMUNCH',
+    }
+
+    security_group_attrs.update(attrs)
+
+    security_group = _security_group.SecurityGroup(**security_group_attrs)
+
+    return security_group
+
+
+def create_security_groups(attrs=None, count=2):
+    """Create multiple fake security groups.
+
+    :param dict attrs: A dictionary with all attributes
+    :param int count: The number of security groups to fake
+    :return: A list of fake SecurityGroup objects
+    """
+    security_groups = []
+    for i in range(0, count):
+        security_groups.append(create_one_security_group(attrs))
+
+    return security_groups
+
+
+def create_one_service_profile(attrs=None):
+    """Create service profile."""
+    attrs = attrs or {}
+
+    service_profile_attrs = {
         'id': 'flavor-profile-id' + uuid.uuid4().hex,
         'description': 'flavor-profile-description-' + uuid.uuid4().hex,
         'project_id': 'project-id-' + uuid.uuid4().hex,
@@ -1957,20 +1991,20 @@ def create_one_service_profile(attrs=None):
         'location': 'MUNCHMUNCHMUNCH',
     }
 
-    flavor_profile_attrs.update(attrs)
+    service_profile_attrs.update(attrs)
 
-    flavor_profile = _flavor_profile.ServiceProfile(**flavor_profile_attrs)
+    flavor_profile = _service_profile.ServiceProfile(**service_profile_attrs)
 
     return flavor_profile
 
 
 def create_service_profile(attrs=None, count=2):
-    """Create multiple flavor profiles."""
+    """Create multiple service profiles."""
 
-    flavor_profiles = []
+    service_profiles = []
     for i in range(0, count):
-        flavor_profiles.append(create_one_service_profile(attrs))
+        service_profiles.append(create_one_service_profile(attrs))
-    return flavor_profiles
+    return service_profiles
 
 
 def get_service_profile(flavor_profile=None, count=2):