Remove neutron client workarounds

We have a workarounds for cases when calling neutronclient.create_port()
or neutronclient.list_ports() may return ports with empty id, or empty
data, without raising an exception. These workarounds mask the neutron
bugs rather than fixing the root cause.
Also for example Nova doesn't have such workarounds, please see
references. This changes removes the workarounds.
Also provisioning will be interrupted with NetworkError() only when we
failed to create all Neutron ports in provisioning network, if at least
one port is created successfully we proceed with deployment.

Reference:
[0]https://github.com/openstack/nova/blob/71c007d9/nova/network/neutronv2/api.py#L1071
[1]https://github.com/openstack/nova/blob/71c007d9/nova/network/neutronv2/api.py#L315

Change-Id: I96cab32b1262efe710de06596626e01c50e457b0

ironic/common/neutron.py

@@ -127,24 +127,20 @@ def add_ports_to_network(task, network_uuid, is_flat=False):
         try:
             port = client.create_port(body)
         except neutron_exceptions.NeutronClientException as e:
-            rollback_ports(task, network_uuid)
+            failures.append(ironic_port.uuid)
-            msg = (_('Could not create neutron port for ironic port '
+            LOG.warning(_LW("Could not create neutron port for node's "
-                     '%(ir-port)s on given network %(net)s from node '
+                            "%(node)s port %(ir-port) on the neutron "
-                     '%(node)s. %(exc)s') %
+                            "network %(net)s. %(exc)s"),
                         {'net': network_uuid, 'node': node.uuid,
                          'ir-port': ironic_port.uuid, 'exc': e})
-            LOG.exception(msg)
+        else:
-            raise exception.NetworkError(msg)
-
-        try:
             ports[ironic_port.uuid] = port['port']['id']
-        except KeyError:
-            failures.append(ironic_port.uuid)
 
     if failures:
         if len(failures) == len(pxe_enabled_ports):
+            rollback_ports(task, network_uuid)
             raise exception.NetworkError(_(
-                "Failed to update vif_port_id for any PXE enabled port "
+                "Failed to create neutron ports for any PXE enabled port "
                 "on node %s.") % node.uuid)
         else:
             LOG.warning(_LW("Some errors were encountered when updating "
@@ -203,14 +199,6 @@ def remove_neutron_ports(task, params):
         return
 
     for port in ports:
-        if not port['id']:
-            # TODO(morgabra) client.list_ports() sometimes returns
-            # port objects with null ids. It's unclear why this happens.
-            LOG.warning(_LW("Deleting neutron port failed, missing 'id'. "
-                            "Node: %(node)s, neutron port: %(port)s."),
-                        {'node': node_uuid, 'port': port})
-            continue
-
         LOG.debug('Deleting neutron port %(vif_port_id)s of node '
                   '%(node_id)s.',
                   {'vif_port_id': port['id'], 'node_id': node_uuid})

ironic/tests/unit/common/test_neutron.py

@@ -223,31 +223,6 @@ class TestNeutronNetworkActions(db_base.DbTestCase):
     def test_add_ports_with_client_id_to_flat_network(self):
         self._test_add_ports_to_flat_network(is_client_id=True)
 
-    def test_add_ports_to_flat_network_no_neutron_port_id(self):
-        port = self.ports[0]
-        expected_body = {
-            'port': {
-                'network_id': self.network_uuid,
-                'admin_state_up': True,
-                'binding:vnic_type': 'baremetal',
-                'device_owner': 'baremetal:none',
-                'device_id': self.node.uuid,
-                'mac_address': port.address,
-                'binding:profile': {
-                    'local_link_information': [port.local_link_connection]
-                }
-            }
-        }
-        del self.neutron_port['id']
-        self.client_mock.create_port.return_value = {
-            'port': self.neutron_port}
-        with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.assertRaises(exception.NetworkError,
-                              neutron.add_ports_to_network,
-                              task, self.network_uuid, is_flat=True)
-            self.client_mock.create_port.assert_called_once_with(
-                expected_body)
-
     def test_add_ports_to_vlan_network_instance_uuid(self):
         self.node.instance_uuid = uuidutils.generate_uuid()
         self.node.save()
@@ -275,44 +250,33 @@ class TestNeutronNetworkActions(db_base.DbTestCase):
             self.client_mock.create_port.assert_called_once_with(expected_body)
 
     @mock.patch.object(neutron, 'rollback_ports')
-    def test_add_network_fail(self, rollback_mock):
+    def test_add_network_all_ports_fail(self, rollback_mock):
         # Check that if creating a port fails, the ports are cleaned up
         self.client_mock.create_port.side_effect = \
             neutron_client_exc.ConnectionFailed
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.assertRaisesRegex(
+            self.assertRaises(
-                exception.NetworkError, 'Could not create neutron port',
+                exception.NetworkError, neutron.add_ports_to_network, task,
-                neutron.add_ports_to_network, task, self.network_uuid)
+                self.network_uuid)
             rollback_mock.assert_called_once_with(task, self.network_uuid)
 
-    @mock.patch.object(neutron, 'rollback_ports')
-    def test_add_network_fail_create_any_port_empty(self, rollback_mock):
-        self.client_mock.create_port.return_value = {}
-        with task_manager.acquire(self.context, self.node.uuid) as task:
-            self.assertRaisesRegex(
-                exception.NetworkError, 'any PXE enabled port',
-                neutron.add_ports_to_network, task, self.network_uuid)
-            self.assertFalse(rollback_mock.called)
-
     @mock.patch.object(neutron, 'LOG')
-    @mock.patch.object(neutron, 'rollback_ports')
+    def test_add_network_create_some_ports_fail(self, log_mock):
-    def test_add_network_fail_create_some_ports_empty(self, rollback_mock,
+        object_utils.create_test_port(
-                                                      log_mock):
-        port2 = object_utils.create_test_port(
             self.context, node_id=self.node.id,
             uuid=uuidutils.generate_uuid(),
             address='52:54:55:cf:2d:32',
             extra={'vif_port_id': uuidutils.generate_uuid()}
         )
         self.client_mock.create_port.side_effect = [
-            {'port': self.neutron_port}, {}]
+            {'port': self.neutron_port}, neutron_client_exc.ConnectionFailed]
         with task_manager.acquire(self.context, self.node.uuid) as task:
             neutron.add_ports_to_network(task, self.network_uuid)
-            self.assertIn(str(port2.uuid),
+            self.assertIn("Could not create neutron port for node's",
-                          # Call #0, argument #1
+                          log_mock.warning.call_args_list[0][0][0])
-                          log_mock.warning.call_args[0][1]['ports'])
+            self.assertIn("Some errors were encountered when updating",
-            self.assertFalse(rollback_mock.called)
+                          log_mock.warning.call_args_list[1][0][0])
 
     @mock.patch.object(neutron, 'remove_neutron_ports')
     def test_remove_ports_from_network(self, remove_mock):

releasenotes/notes/remove-neutron-client-workarounds-996c59623684929b.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - Update create provisioning ports logic to fail only
+    when no neutron ports were created. If we created at
+    least one neutron port, proceed with the deployment.
+    It was the default behaviour for flat scenario.