Fix "server create" command failed when --nic auto or none

"auto" and "none" options was added into --nic argument of server create
command in patch https://review.openstack.org/#/c/412698/ , but that
don't work and raise internal error when execute command. The patch
fix that issue and add unit and functional tests.

Change-Id: Ia718c3bac0a5172a0cdbe9f0d97972a9346c1172
Co-Authored-By: Kevin_Zheng <zhengzhenyu@huawei.com>
Closes-Bug: #1663520

Author: kiwik

doc/source/command-objects/server.rst

@@ -173,6 +173,8 @@ Create a new server
     v6-fixed-ip: IPv6 fixed address for NIC (optional).
     none: (v2.37+) no network is attached.
     auto: (v2.37+) the compute service will automatically allocate a network.
+    Specifying a --nic of auto or none cannot be used with any other
+    --nic value.
 
 .. option:: --hint <key=value>
 

openstackclient/compute/v2/server.py

@@ -402,7 +402,7 @@ def get_parser(self, prog_name):
         parser.add_argument(
             '--nic',
             metavar="<net-id=net-uuid,v4-fixed-ip=ip-addr,v6-fixed-ip=ip-addr,"
-                    "port-id=port-uuid>",
+                    "port-id=port-uuid,auto,none>",
             action='append',
             default=[],
             help=_("Create a NIC on the server. "
@@ -414,7 +414,8 @@ def get_parser(self, prog_name):
                    "v6-fixed-ip: IPv6 fixed address for NIC (optional), "
                    "none: (v2.37+) no network is attached, "
                    "auto: (v2.37+) the compute service will automatically "
-                   "allocate a network."),
+                   "allocate a network. Specifying a --nic of auto or none "
+                   "cannot be used with any other --nic value."),
         )
         parser.add_argument(
             '--hint',
@@ -547,14 +548,21 @@ def take_action(self, parsed_args):
                 block_device_mapping_v2.append(mapping)
 
         nics = []
-        if parsed_args.nic in ('auto', 'none'):
-            nics = [parsed_args.nic]
-        else:
-            for nic_str in parsed_args.nic:
+        auto_or_none = False
+        for nic_str in parsed_args.nic:
+            # Handle the special auto/none cases
+            if nic_str in ('auto', 'none'):
+                auto_or_none = True
+                nics.append(nic_str)
+            else:
                 nic_info = {"net-id": "", "v4-fixed-ip": "",
                             "v6-fixed-ip": "", "port-id": ""}
-                nic_info.update(dict(kv_str.split("=", 1)
-                                for kv_str in nic_str.split(",")))
+                try:
+                    nic_info.update(dict(kv_str.split("=", 1)
+                                    for kv_str in nic_str.split(",")))
+                except ValueError:
+                    msg = _('Invalid --nic argument %s.') % nic_str
+                    raise exceptions.CommandError(msg)
                 if bool(nic_info["net-id"]) == bool(nic_info["port-id"]):
                     msg = _("either net-id or port-id should be specified "
                             "but not both")
@@ -581,6 +589,18 @@ def take_action(self, parsed_args):
                         raise exceptions.CommandError(msg)
                 nics.append(nic_info)
 
+        if nics:
+            if auto_or_none:
+                if len(nics) > 1:
+                    msg = _('Specifying a --nic of auto or none cannot '
+                            'be used with any other --nic value.')
+                    raise exceptions.CommandError(msg)
+                nics = nics[0]
+        else:
+            # Default to empty list if nothing was specified, let nova side to
+            # decide the default behavior.
+            nics = []
+
         hints = {}
         for hint in parsed_args.hint:
             key, _sep, value = hint.partition('=')

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

@@ -14,10 +14,10 @@
 import time
 
 from tempest.lib.common.utils import data_utils
+from tempest.lib import exceptions
 
 from openstackclient.tests.functional import base
 from openstackclient.tests.functional.volume.v2 import test_volume
-from tempest.lib import exceptions
 
 
 class ServerTests(base.TestCase):
@@ -319,7 +319,7 @@ def test_server_reboot(self):
         self.assertEqual("", raw_output)
         self.wait_for_status("ACTIVE")
 
-    def test_server_boot_from_volume(self):
+    def test_server_create_from_volume(self):
         """Test server create from volume, server delete
 
         Test steps:
@@ -437,14 +437,57 @@ def test_server_boot_from_volume(self):
             cmd_output['status'],
         )
 
-    def wait_for_status(self, expected_status='ACTIVE', wait=900, interval=30):
+    def test_server_create_with_none_network(self):
+        """Test server create with none network option."""
+        server_name = data_utils.rand_name('TestServer')
+        server = json.loads(self.openstack(
+            # auto/none enable in nova micro version (v2.37+)
+            '--os-compute-api-version 2.latest ' +
+            'server create -f json ' +
+            '--flavor ' + self.flavor_name + ' ' +
+            '--image ' + self.image_name + ' ' +
+            '--nic none ' +
+            server_name
+        ))
+        self.assertIsNotNone(server["id"])
+        self.addCleanup(self.openstack, 'server delete --wait ' + server_name)
+        self.assertEqual(server_name, server['name'])
+        self.wait_for_status(server_name=server_name)
+        server = json.loads(self.openstack(
+            'server show -f json ' + server_name
+        ))
+        self.assertIsNotNone(server['addresses'])
+        self.assertEqual('', server['addresses'])
+
+    def test_server_create_with_empty_network_option_latest(self):
+        """Test server create with empty network option in nova 2.latest."""
+        server_name = data_utils.rand_name('TestServer')
+        try:
+            self.openstack(
+                # auto/none enable in nova micro version (v2.37+)
+                '--os-compute-api-version 2.latest ' +
+                'server create -f json ' +
+                '--flavor ' + self.flavor_name + ' ' +
+                '--image ' + self.image_name + ' ' +
+                server_name
+            )
+        except exceptions.CommandFailed as e:
+            self.assertIn('nics are required after microversion 2.36',
+                          e.stderr)
+        else:
+            self.fail('CommandFailed should be raised.')
+
+    def wait_for_status(self, expected_status='ACTIVE',
+                        wait=900, interval=30, server_name=None):
         """Wait until server reaches expected status."""
         # TODO(thowe): Add a server wait command to osc
         failures = ['ERROR']
         total_sleep = 0
         opts = self.get_opts(['status'])
+        if not server_name:
+            server_name = self.NAME
         while total_sleep < wait:
-            status = self.openstack('server show ' + self.NAME + opts)
+            status = self.openstack('server show ' + server_name + opts)
             status = status.rstrip()
             print('Waiting for {} current status: {}'.format(expected_status,
                                                              status))
@@ -454,7 +497,7 @@ def wait_for_status(self, expected_status='ACTIVE', wait=900, interval=30):
             time.sleep(interval)
             total_sleep += interval
 
-        status = self.openstack('server show ' + self.NAME + opts)
+        status = self.openstack('server show ' + server_name + opts)
         status = status.rstrip()
         self.assertEqual(status, expected_status)
         # give it a little bit more time

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

@@ -467,6 +467,152 @@ def test_server_create_with_network(self):
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.datalist(), data)
 
+    def test_server_create_with_auto_network(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', 'flavor1',
+            '--nic', 'auto',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', 'flavor1'),
+            ('nic', ['auto']),
+            ('config_drive', False),
+            ('server_name', self.new_server.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[],
+            nics='auto',
+            scheduler_hints={},
+            config_drive=None,
+        )
+        # ServerManager.create(name, image, flavor, **kwargs)
+        self.servers_mock.create.assert_called_with(
+            self.new_server.name,
+            self.image,
+            self.flavor,
+            **kwargs
+        )
+
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.datalist(), data)
+
+    def test_server_create_with_none_network(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', 'flavor1',
+            '--nic', 'none',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', 'flavor1'),
+            ('nic', ['none']),
+            ('config_drive', False),
+            ('server_name', self.new_server.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # Set expected values
+        kwargs = dict(
+            meta=None,
+            files={},
+            reservation_id=None,
+            min_count=1,
+            max_count=1,
+            security_groups=[],
+            userdata=None,
+            key_name=None,
+            availability_zone=None,
+            block_device_mapping_v2=[],
+            nics='none',
+            scheduler_hints={},
+            config_drive=None,
+        )
+        # ServerManager.create(name, image, flavor, **kwargs)
+        self.servers_mock.create.assert_called_with(
+            self.new_server.name,
+            self.image,
+            self.flavor,
+            **kwargs
+        )
+
+        self.assertEqual(self.columns, columns)
+        self.assertEqual(self.datalist(), data)
+
+    def test_server_create_with_conflict_network_options(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', 'flavor1',
+            '--nic', 'none',
+            '--nic', 'auto',
+            '--nic', 'port-id=port1',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', 'flavor1'),
+            ('nic', ['none', 'auto', 'port-id=port1']),
+            ('config_drive', False),
+            ('server_name', self.new_server.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        get_endpoints = mock.Mock()
+        get_endpoints.return_value = {'network': []}
+        self.app.client_manager.auth_ref = mock.Mock()
+        self.app.client_manager.auth_ref.service_catalog = mock.Mock()
+        self.app.client_manager.auth_ref.service_catalog.get_endpoints = (
+            get_endpoints)
+
+        find_port = mock.Mock()
+        network_client = self.app.client_manager.network
+        network_client.find_port = find_port
+        port_resource = mock.Mock()
+        port_resource.id = 'port1_uuid'
+        find_port.return_value = port_resource
+
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+        self.assertNotCalled(self.servers_mock.create)
+
+    def test_server_create_with_invalid_network_options(self):
+        arglist = [
+            '--image', 'image1',
+            '--flavor', 'flavor1',
+            '--nic', 'abcdefgh',
+            self.new_server.name,
+        ]
+        verifylist = [
+            ('image', 'image1'),
+            ('flavor', 'flavor1'),
+            ('nic', ['abcdefgh']),
+            ('config_drive', False),
+            ('server_name', self.new_server.name),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        self.assertRaises(exceptions.CommandError,
+                          self.cmd.take_action, parsed_args)
+        self.assertNotCalled(self.servers_mock.create)
+
     @mock.patch.object(common_utils, 'wait_for_status', return_value=True)
     def test_server_create_with_wait_ok(self, mock_wait_for_status):
         arglist = [

releasenotes/notes/bug-1663520-d880bfa51ca7b798.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fix ``server create`` command failed when ``--nic`` auto or none.
+    ``auto`` and ``none`` options was added into --nic argument of server
+    create command, but that don't work and raise internal error when execute
+    command. The patch fix that issue.
+    [Bug `1663520 <https://bugs.launchpad.net/python-openstackclient/+bug/1663520>`_]