From 5c6808763dc028f006e7dab8111a21cbf6ee2ebf Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Tue, 18 Feb 2014 17:01:01 +0900 Subject: [PATCH] Handle racing condition in OFC port deletion Multiple delete_port operations can run in parallel. For such case later operation(s) will receive 404 (NotFound) error from OFC or NotResultFound sqlalchemy exception. These are valid exceptions and they should be ignored in delete_port operations. Closes-Bug: #1281574 OFCConsistencyBroken is renamed to OFCMappingNotFound because when multiple delete_port operations run in parallel OFCConsistencyBroken can occur and it is a valid case so the excepion name looks inappropriate. Change-Id: I1511d55994c88b8828f0ff62610c18ddc6dfac8f --- neutron/plugins/nec/common/exceptions.py | 11 +++++-- neutron/plugins/nec/common/ofc_client.py | 4 +++ neutron/plugins/nec/db/api.py | 5 ++- neutron/plugins/nec/nec_plugin.py | 26 +++++++++++++--- neutron/plugins/nec/packet_filter.py | 4 +-- neutron/plugins/nec/router_drivers.py | 10 +++--- neutron/tests/unit/nec/test_nec_plugin.py | 37 +++++++++++++++++++++++ neutron/tests/unit/nec/test_ofc_client.py | 5 +++ 8 files changed, 84 insertions(+), 18 deletions(-) diff --git a/neutron/plugins/nec/common/exceptions.py b/neutron/plugins/nec/common/exceptions.py index d6b2b3b500..0045b54607 100644 --- a/neutron/plugins/nec/common/exceptions.py +++ b/neutron/plugins/nec/common/exceptions.py @@ -28,13 +28,18 @@ class OFCException(qexc.NeutronException): self.err_code = kwargs.get('err_code') +class OFCResourceNotFound(qexc.NotFound): + message = _("The specified OFC resource (%(resource)s) is not found.") + + class NECDBException(qexc.NeutronException): message = _("An exception occurred in NECPluginV2 DB: %(reason)s") -class OFCConsistencyBroken(qexc.NeutronException): - message = _("Consistency of neutron-OFC resource map is broken: " - "%(reason)s") +class OFCMappingNotFound(qexc.NotFound): + message = _("Neutron-OFC resource mapping for " + "%(resource)s %(neutron_id)s is not found. " + "It may be deleted during processing.") class PortInfoNotFound(qexc.NotFound): diff --git a/neutron/plugins/nec/common/ofc_client.py b/neutron/plugins/nec/common/ofc_client.py index 4bc49b42a9..7fbfe9ec45 100644 --- a/neutron/plugins/nec/common/ofc_client.py +++ b/neutron/plugins/nec/common/ofc_client.py @@ -95,6 +95,10 @@ class OFCClient(object): httplib.ACCEPTED, httplib.NO_CONTENT): return data + elif res.status == httplib.NOT_FOUND: + LOG.info(_("Specified resource %s does not exist on OFC "), + action) + raise nexc.OFCResourceNotFound(resource=action) else: LOG.warning(_("Operation on OFC failed: " "status=%(status)s, detail=%(detail)s"), diff --git a/neutron/plugins/nec/db/api.py b/neutron/plugins/nec/db/api.py index e606861ff2..85c5f8601f 100644 --- a/neutron/plugins/nec/db/api.py +++ b/neutron/plugins/nec/db/api.py @@ -144,9 +144,8 @@ def get_ofc_id_lookup_both(session, resource, neutron_id): ofc_id = get_ofc_id(session, resource, neutron_id, old_style=True) if not ofc_id: - reason = (_("NotFound %(resource)s for neutron_id=%(id)s.") - % {'resource': resource, 'id': neutron_id}) - raise nexc.OFCConsistencyBroken(reason=reason) + raise nexc.OFCMappingNotFound(resource=resource, + neutron_id=neutron_id) return ofc_id diff --git a/neutron/plugins/nec/nec_plugin.py b/neutron/plugins/nec/nec_plugin.py index 41aaf61f7c..519da2ccfd 100644 --- a/neutron/plugins/nec/nec_plugin.py +++ b/neutron/plugins/nec/nec_plugin.py @@ -187,7 +187,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, else: LOG.debug(_('_cleanup_ofc_tenant: No OFC tenant for %s'), tenant_id) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: reason = _("delete_ofc_tenant() failed due to %s") % exc LOG.warn(reason) @@ -223,7 +223,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, try: self.ofc.create_ofc_port(context, port['id'], port) port_status = const.PORT_STATUS_ACTIVE - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: LOG.error(_("create_ofc_port() failed due to %s"), exc) port_status = const.PORT_STATUS_ERROR @@ -244,7 +244,23 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, try: self.ofc.delete_ofc_port(context, port['id'], port) port_status = const.PORT_STATUS_DOWN - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCResourceNotFound, nexc.OFCMappingNotFound): + # There is a case where multiple delete_port operation are + # running concurrently. For example, delete_port from + # release_dhcp_port and deletion of network owned ports in + # delete_network. In such cases delete_ofc_port may receive + # 404 error from OFC. + # Also there is a case where neutron port is deleted + # between exists_ofc_port and get_ofc_id in delete_ofc_port. + # In this case OFCMappingNotFound is raised. + # These two cases are valid situations. + LOG.info(_("deactivate_port(): OFC port for port=%s is " + "already removed."), port['id']) + # The port is already removed, so there is no need + # to update status in the database. + port['status'] = const.PORT_STATUS_DOWN + return port + except nexc.OFCException as exc: LOG.error(_("delete_ofc_port() failed due to %s"), exc) port_status = const.PORT_STATUS_ERROR @@ -282,7 +298,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, if not self.ofc.exists_ofc_tenant(context, tenant_id): self.ofc.create_ofc_tenant(context, tenant_id) self.ofc.create_ofc_network(context, tenant_id, net_id, net_name) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: LOG.error(_("Failed to create network id=%(id)s on " "OFC: %(exc)s"), {'id': net_id, 'exc': exc}) network['network']['status'] = const.NET_STATUS_ERROR @@ -368,7 +384,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, try: self.ofc.delete_ofc_network(context, id, net_db) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: reason = _("delete_network() failed due to %s") % exc LOG.error(reason) self._update_resource_status(context, "network", net_db['id'], diff --git a/neutron/plugins/nec/packet_filter.py b/neutron/plugins/nec/packet_filter.py index 8c30bd420f..2c831e641a 100644 --- a/neutron/plugins/nec/packet_filter.py +++ b/neutron/plugins/nec/packet_filter.py @@ -127,7 +127,7 @@ class PacketFilterMixin(pf_db.PacketFilterDbMixin): self.ofc.create_ofc_packet_filter(context, pf_id, packet_filter) pf_status = pf_db.PF_STATUS_ACTIVE - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: LOG.error(_("Failed to create packet_filter id=%(id)s on " "OFC: %(exc)s"), {'id': pf_id, 'exc': str(exc)}) pf_status = pf_db.PF_STATUS_ERROR @@ -153,7 +153,7 @@ class PacketFilterMixin(pf_db.PacketFilterDbMixin): try: self.ofc.delete_ofc_packet_filter(context, pf_id) pf_status = pf_db.PF_STATUS_DOWN - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: LOG.error(_("Failed to delete packet_filter id=%(id)s from " "OFC: %(exc)s"), {'id': pf_id, 'exc': str(exc)}) pf_status = pf_db.PF_STATUS_ERROR diff --git a/neutron/plugins/nec/router_drivers.py b/neutron/plugins/nec/router_drivers.py index 9781f0b6e0..f1ac10cf36 100644 --- a/neutron/plugins/nec/router_drivers.py +++ b/neutron/plugins/nec/router_drivers.py @@ -119,7 +119,7 @@ class RouterOpenFlowDriver(RouterDriverBase): new_status) router['status'] = new_status return router - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: with excutils.save_and_reraise_exception(): if (isinstance(exc, nexc.OFCException) and exc.status == httplib.CONFLICT): @@ -151,7 +151,7 @@ class RouterOpenFlowDriver(RouterDriverBase): self.plugin._update_resource_status( context, "router", router_id, new_status) new_router['status'] = new_status - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: with excutils.save_and_reraise_exception(): reason = _("_update_ofc_routes() failed due to %s") % exc LOG.error(reason) @@ -164,7 +164,7 @@ class RouterOpenFlowDriver(RouterDriverBase): def delete_router(self, context, router_id, router): try: self.ofc.delete_ofc_router(context, router_id, router) - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: with excutils.save_and_reraise_exception(): LOG.error(_("delete_router() failed due to %s"), exc) self.plugin._update_resource_status( @@ -195,7 +195,7 @@ class RouterOpenFlowDriver(RouterDriverBase): self.plugin._update_resource_status( context, "port", port_id, new_status) return port - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: with excutils.save_and_reraise_exception(): reason = _("add_router_interface() failed due to %s") % exc LOG.error(reason) @@ -213,7 +213,7 @@ class RouterOpenFlowDriver(RouterDriverBase): new_status) port['status'] = new_status return port - except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc: + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: with excutils.save_and_reraise_exception(): reason = _("delete_router_interface() failed due to %s") % exc LOG.error(reason) diff --git a/neutron/tests/unit/nec/test_nec_plugin.py b/neutron/tests/unit/nec/test_nec_plugin.py index 727308a9ea..fa061cfad3 100644 --- a/neutron/tests/unit/nec/test_nec_plugin.py +++ b/neutron/tests/unit/nec/test_nec_plugin.py @@ -843,3 +843,40 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): ] self.ofc.assert_has_calls(expected) self.assertEqual(self.ofc.delete_ofc_port.call_count, 2) + + def _test_delete_port_for_disappeared_ofc_port(self, raised_exc): + self.ofc.set_raise_exc('delete_ofc_port', raised_exc) + + with self.port(no_delete=True) as port: + port_id = port['port']['id'] + + portinfo = {'id': port_id, 'port_no': 123} + self.rpcapi_update_ports(added=[portinfo]) + + self._delete('ports', port_id) + + # Check the port on neutron db is deleted. NotFound for + # neutron port itself should be handled by called. It is + # consistent with ML2 behavior, but it may need to be + # revisit. + self._show('ports', port_id, + expected_code=webob.exc.HTTPNotFound.code) + + ctx = mock.ANY + port = mock.ANY + expected = [ + mock.call.exists_ofc_port(ctx, port_id), + mock.call.create_ofc_port(ctx, port_id, port), + mock.call.exists_ofc_port(ctx, port_id), + mock.call.delete_ofc_port(ctx, port_id, port), + ] + self.ofc.assert_has_calls(expected) + self.assertEqual(self.ofc.delete_ofc_port.call_count, 1) + + def test_delete_port_for_nonexist_ofc_port(self): + self._test_delete_port_for_disappeared_ofc_port( + nexc.OFCResourceNotFound(resource='ofc_port')) + + def test_delete_port_for_noofcmap_ofc_port(self): + self._test_delete_port_for_disappeared_ofc_port( + nexc.OFCMappingNotFound(resource='port', neutron_id='port1')) diff --git a/neutron/tests/unit/nec/test_ofc_client.py b/neutron/tests/unit/nec/test_ofc_client.py index 1567ae9e0e..aba037edfa 100644 --- a/neutron/tests/unit/nec/test_ofc_client.py +++ b/neutron/tests/unit/nec/test_ofc_client.py @@ -72,6 +72,11 @@ class OFCClientTest(base.BaseTestCase): for status in [201, 202, 204]: self._test_do_request(status, None, None) + def test_do_request_returns_404(self): + resbody = '' + errmsg = _("The specified OFC resource (/somewhere) is not found.") + self._test_do_request(404, resbody, errmsg, nexc.OFCResourceNotFound) + def test_do_request_error_no_body(self): errmsg = _("An OFC exception has occurred: Operation on OFC failed") exc_checks = {'status': 400, 'err_code': None, 'err_msg': None}