From bfddae67056a8040b61b7137b9eb9deb6e6565fb Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Fri, 12 Aug 2016 11:35:14 +0300 Subject: [PATCH] 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 | 30 +++------- ironic/tests/unit/common/test_neutron.py | 58 ++++--------------- ...n-client-workarounds-996c59623684929b.yaml | 6 ++ 3 files changed, 26 insertions(+), 68 deletions(-) create mode 100644 releasenotes/notes/remove-neutron-client-workarounds-996c59623684929b.yaml diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index 0b16acdd85..5a7509efae 100644 --- a/ironic/common/neutron.py +++ b/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) - msg = (_('Could not create neutron port for ironic port ' - '%(ir-port)s on given network %(net)s from node ' - '%(node)s. %(exc)s') % - {'net': network_uuid, 'node': node.uuid, - 'ir-port': ironic_port.uuid, 'exc': e}) - LOG.exception(msg) - raise exception.NetworkError(msg) - - try: - ports[ironic_port.uuid] = port['port']['id'] - except KeyError: failures.append(ironic_port.uuid) + LOG.warning(_LW("Could not create neutron port for node's " + "%(node)s port %(ir-port) on the neutron " + "network %(net)s. %(exc)s"), + {'net': network_uuid, 'node': node.uuid, + 'ir-port': ironic_port.uuid, 'exc': e}) + else: + ports[ironic_port.uuid] = port['port']['id'] 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}) diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index ef18f5733d..bc9c032691 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/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( - exception.NetworkError, 'Could not create neutron port', - neutron.add_ports_to_network, task, self.network_uuid) + self.assertRaises( + exception.NetworkError, neutron.add_ports_to_network, task, + 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_fail_create_some_ports_empty(self, rollback_mock, - log_mock): - port2 = object_utils.create_test_port( + def test_add_network_create_some_ports_fail(self, log_mock): + 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), - # Call #0, argument #1 - log_mock.warning.call_args[0][1]['ports']) - self.assertFalse(rollback_mock.called) + self.assertIn("Could not create neutron port for node's", + log_mock.warning.call_args_list[0][0][0]) + self.assertIn("Some errors were encountered when updating", + 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): diff --git a/releasenotes/notes/remove-neutron-client-workarounds-996c59623684929b.yaml b/releasenotes/notes/remove-neutron-client-workarounds-996c59623684929b.yaml new file mode 100644 index 0000000000..9644ca3fd0 --- /dev/null +++ b/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.