From f45c1c52423dcbb2ea7690d56b3edc232d16636e Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Sun, 16 Mar 2014 00:16:23 +0900 Subject: [PATCH] nec plugin: allow to delete resource with ERROR status Previously if a resource is in ERROR status and there is no corresponding resource on OpenFlow controller, the resource cannot be deleted through an API request. This commit rearrange ERROR status check to allow resource with ERROR status to be deleted. Closes-Bug: #1295754 Change-Id: I709f5e2066eb5d12ec0f42dff15797acddc2009e --- neutron/plugins/nec/nec_plugin.py | 84 +++++++++++--------- neutron/plugins/nec/packet_filter.py | 55 +++++++------ neutron/plugins/nec/router_drivers.py | 2 + neutron/tests/unit/nec/test_nec_plugin.py | 61 +++++++++++++- neutron/tests/unit/nec/test_packet_filter.py | 28 ++++++- 5 files changed, 161 insertions(+), 69 deletions(-) diff --git a/neutron/plugins/nec/nec_plugin.py b/neutron/plugins/nec/nec_plugin.py index 811250ca1..ed2bd9984 100644 --- a/neutron/plugins/nec/nec_plugin.py +++ b/neutron/plugins/nec/nec_plugin.py @@ -167,6 +167,14 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, obj_db = obj_getter(context, id) obj_db.update(request) + def _update_resource_status_if_changed(self, context, resource_type, + resource_dict, new_status): + if resource_dict['status'] != new_status: + self._update_resource_status(context, resource_type, + resource_dict['id'], + new_status) + resource_dict['status'] = new_status + def _check_ofc_tenant_in_use(self, context, tenant_id): """Check if the specified tenant is used.""" # All networks are created on OFC @@ -234,16 +242,18 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, return port - def deactivate_port(self, context, port): + def deactivate_port(self, context, port, raise_exc=True): """Deactivate port by deleting port from OFC if exists.""" if not self.ofc.exists_ofc_port(context, port['id']): - LOG.debug(_("deactivate_port(): skip, ofc_port does not " - "exist.")) + LOG.debug(_("deactivate_port(): skip, ofc_port for port=%s " + "does not exist."), port['id']) return port try: self.ofc.delete_ofc_port(context, port['id'], port) - port_status = const.PORT_STATUS_DOWN + self._update_resource_status_if_changed( + context, "port", port, const.PORT_STATUS_DOWN) + return port except (nexc.OFCResourceNotFound, nexc.OFCMappingNotFound): # There is a case where multiple delete_port operation are # running concurrently. For example, delete_port from @@ -261,15 +271,14 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, 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 - - if port_status != port['status']: - self._update_resource_status(context, "port", port['id'], - port_status) - port['status'] = port_status - - return port + with excutils.save_and_reraise_exception() as ctxt: + LOG.error(_("Failed to delete port=%(port)s from OFC: " + "%(exc)s"), {'port': port['id'], 'exc': exc}) + self._update_resource_status_if_changed( + context, "port", port, const.PORT_STATUS_ERROR) + if not raise_exc: + ctxt.reraise = False + return port def _net_status(self, network): # NOTE: NEC Plugin accept admin_state_up. When it's False, this plugin @@ -336,7 +345,11 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, ports = super(NECPluginV2, self).get_ports(context, filters=filters) for port in ports: - self.deactivate_port(context, port) + # If some error occurs, status of errored port is set to ERROR. + # This is avoids too many rollback. + # TODO(amotoki): Raise an exception after all port operations + # are finished to inform the caller of API of the failure. + self.deactivate_port(context, port, raise_exc=False) elif changed and new_net['admin_state_up']: # enable ports of the network filters = dict(network_id=[id], status=[const.PORT_STATUS_DOWN], @@ -368,28 +381,25 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, raise n_exc.NetworkInUse(net_id=id) # Make sure auto-delete ports on OFC are deleted. - _error_ports = [] + # If an error occurs during port deletion, + # delete_network will be aborted. for port in ports: port = self.deactivate_port(context, port) - if port['status'] == const.PORT_STATUS_ERROR: - _error_ports.append(port['id']) - if _error_ports: - reason = (_("Failed to delete port(s)=%s from OFC.") % - ','.join(_error_ports)) - raise nexc.OFCException(reason=reason) # delete all packet_filters of the network from the controller for pf in net_db.packetfilters: self.delete_packet_filter(context, pf['id']) - try: - self.ofc.delete_ofc_network(context, id, net_db) - except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: - with excutils.save_and_reraise_exception(): - reason = _("delete_network() failed due to %s") % exc - LOG.error(reason) - self._update_resource_status(context, "network", net_db['id'], - const.NET_STATUS_ERROR) + if self.ofc.exists_ofc_network(context, id): + try: + self.ofc.delete_ofc_network(context, id, net_db) + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: + with excutils.save_and_reraise_exception(): + reason = _("delete_network() failed due to %s") % exc + LOG.error(reason) + self._update_resource_status( + context, "network", net_db['id'], + const.NET_STATUS_ERROR) super(NECPluginV2, self).delete_network(context, id) @@ -627,10 +637,8 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2, port = self._make_port_dict(port_db) handler = self._get_port_handler('delete', port['device_owner']) + # handler() raises an exception if an error occurs during processing. port = handler(context, port) - if port['status'] == const.PORT_STATUS_ERROR: - reason = _("Failed to delete port=%s from OFC.") % id - raise nexc.OFCException(reason=reason) # delete all packet_filters of the port from the controller for pf in port_db.packetfilters: @@ -738,9 +746,10 @@ class NECPluginV2RPCCallbacks(object): # NOTE: Make sure that packet filters on this port exist while # the port is active to avoid unexpected packet transfer. if portinfo: - self.plugin.deactivate_port(rpc_context, port) - self.plugin.deactivate_packet_filters_by_port(rpc_context, - id) + self.plugin.deactivate_port(rpc_context, port, + raise_exc=False) + self.plugin.deactivate_packet_filters_by_port( + rpc_context, id, raise_exc=False) self.plugin.activate_packet_filters_by_port(rpc_context, id) self.plugin.activate_port_if_ready(rpc_context, port) for id in kwargs.get('port_removed', []): @@ -761,8 +770,9 @@ class NECPluginV2RPCCallbacks(object): ndb.del_portinfo(session, id) port = self._get_port(rpc_context, id) if port: - self.plugin.deactivate_port(rpc_context, port) - self.plugin.deactivate_packet_filters_by_port(rpc_context, id) + self.plugin.deactivate_port(rpc_context, port, raise_exc=False) + self.plugin.deactivate_packet_filters_by_port( + rpc_context, id, raise_exc=False) def _get_port(self, context, port_id): try: diff --git a/neutron/plugins/nec/packet_filter.py b/neutron/plugins/nec/packet_filter.py index 9d32f980c..df48ebff8 100644 --- a/neutron/plugins/nec/packet_filter.py +++ b/neutron/plugins/nec/packet_filter.py @@ -148,12 +148,9 @@ class PacketFilterMixin(pf_db.PacketFilterDbMixin): # validate ownership pf = self.get_packet_filter(context, id) + # deactivate_packet_filter() raises an exception + # if an error occurs during processing. pf = self.deactivate_packet_filter(context, pf) - if pf['status'] == pf_db.PF_STATUS_ERROR: - msg = _("Failed to delete packet_filter id=%s which remains in " - "error status.") % id - LOG.error(msg) - raise nexc.OFCException(reason=msg) super(PacketFilterMixin, self).delete_packet_filter(context, id) @@ -205,30 +202,28 @@ class PacketFilterMixin(pf_db.PacketFilterDbMixin): LOG.debug(_("deactivate_packet_filter_if_ready() called, " "packet_filter=%s."), packet_filter) pf_id = packet_filter['id'] - current = packet_filter['status'] - pf_status = current - if self.ofc.exists_ofc_packet_filter(context, pf_id): - LOG.debug(_("deactivate_packet_filter(): " - "deleting packet_filter id=%s from OFC."), pf_id) - try: - self.ofc.delete_ofc_packet_filter(context, pf_id) - pf_status = pf_db.PF_STATUS_DOWN - 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': exc}) - pf_status = pf_db.PF_STATUS_ERROR - else: + if not self.ofc.exists_ofc_packet_filter(context, pf_id): LOG.debug(_("deactivate_packet_filter(): skip, " "Not found OFC Mapping for packet_filter id=%s."), pf_id) + return packet_filter - if pf_status != current: - self._update_resource_status(context, "packet_filter", pf_id, - pf_status) - packet_filter.update({'status': pf_status}) - - return packet_filter + LOG.debug(_("deactivate_packet_filter(): " + "deleting packet_filter id=%s from OFC."), pf_id) + try: + self.ofc.delete_ofc_packet_filter(context, pf_id) + self._update_resource_status_if_changed( + context, "packet_filter", packet_filter, pf_db.PF_STATUS_DOWN) + return packet_filter + except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: + with excutils.save_and_reraise_exception(): + LOG.error(_("Failed to delete packet_filter id=%(id)s " + "from OFC: %(exc)s"), + {'id': pf_id, 'exc': str(exc)}) + self._update_resource_status_if_changed( + context, "packet_filter", packet_filter, + pf_db.PF_STATUS_ERROR) def activate_packet_filters_by_port(self, context, port_id): if not self.packet_filter_enabled: @@ -240,14 +235,22 @@ class PacketFilterMixin(pf_db.PacketFilterDbMixin): for pf in pfs: self.activate_packet_filter_if_ready(context, pf) - def deactivate_packet_filters_by_port(self, context, port_id): + def deactivate_packet_filters_by_port(self, context, port_id, + raise_exc=True): if not self.packet_filter_enabled: return filters = {'in_port': [port_id], 'status': [pf_db.PF_STATUS_ACTIVE]} pfs = self.get_packet_filters(context, filters=filters) + error = False for pf in pfs: - self.deactivate_packet_filter(context, pf) + try: + self.deactivate_packet_filter(context, pf) + except (nexc.OFCException, nexc.OFCMappingNotFound): + error = True + if raise_exc and error: + raise nexc.OFCException(_('Error occurred while disabling packet ' + 'filter(s) for port %s'), port_id) def get_packet_filters_for_port(self, context, port): if self.packet_filter_enabled: diff --git a/neutron/plugins/nec/router_drivers.py b/neutron/plugins/nec/router_drivers.py index f1ac10cf3..407ea5365 100644 --- a/neutron/plugins/nec/router_drivers.py +++ b/neutron/plugins/nec/router_drivers.py @@ -162,6 +162,8 @@ class RouterOpenFlowDriver(RouterDriverBase): @call_log.log def delete_router(self, context, router_id, router): + if not self.ofc.exists_ofc_router(context, router_id): + return try: self.ofc.delete_ofc_router(context, router_id, router) except (nexc.OFCException, nexc.OFCMappingNotFound) as exc: diff --git a/neutron/tests/unit/nec/test_nec_plugin.py b/neutron/tests/unit/nec/test_nec_plugin.py index 182e98abd..3b8a6478b 100644 --- a/neutron/tests/unit/nec/test_nec_plugin.py +++ b/neutron/tests/unit/nec/test_nec_plugin.py @@ -361,6 +361,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.create_ofc_tenant(ctx, self._tenant_id), mock.call.create_ofc_network(ctx, self._tenant_id, net['id'], net['name']), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -379,6 +380,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.create_ofc_tenant(ctx, self._tenant_id), mock.call.create_ofc_network(ctx, self._tenant_id, net['id'], net['name']), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -403,7 +405,9 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.create_ofc_network(ctx, self._tenant_id, nets[1]['id'], nets[1]['name']), + mock.call.exists_ofc_network(ctx, nets[1]['id']), mock.call.delete_ofc_network(ctx, nets[1]['id'], mock.ANY), + mock.call.exists_ofc_network(ctx, nets[0]['id']), mock.call.delete_ofc_network(ctx, nets[0]['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -467,6 +471,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.create_ofc_tenant(ctx, self._tenant_id), mock.call.create_ofc_network(ctx, self._tenant_id, net['id'], net['name']), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -495,6 +500,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): net['name']), mock.call.exists_ofc_port(ctx, p1['id']), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -538,6 +544,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.exists_ofc_port(ctx, p1['id']), mock.call.delete_ofc_port(ctx, p1['id'], mock.ANY), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -570,12 +577,37 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.create_ofc_port(ctx, p['id'], mock.ANY), mock.call.exists_ofc_port(ctx, p['id']), mock.call.delete_ofc_port(ctx, p['id'], mock.ANY), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) ] self.ofc.assert_has_calls(expected) + def test_delete_network_with_error_status(self): + self.ofc.set_raise_exc('create_ofc_network', + nexc.OFCException(reason='fake error')) + + with self.network() as net: + net_id = net['network']['id'] + net_ref = self._show('networks', net_id) + self.assertEqual(net_ref['network']['status'], 'ERROR') + + ctx = mock.ANY + tenant_id = self._tenant_id + net_name = mock.ANY + net = mock.ANY + expected = [ + mock.call.exists_ofc_tenant(ctx, tenant_id), + mock.call.create_ofc_tenant(ctx, tenant_id), + mock.call.create_ofc_network(ctx, tenant_id, net_id, net_name), + mock.call.exists_ofc_network(ctx, net_id), + mock.call.exists_ofc_tenant(ctx, tenant_id), + mock.call.delete_ofc_tenant(ctx, tenant_id), + ] + self.ofc.assert_has_calls(expected) + self.assertFalse(self.ofc.delete_ofc_network.call_count) + def test_delete_network_with_ofc_deletion_failure(self): self.ofc.set_raise_exc('delete_ofc_network', nexc.OFCException(reason='hoge')) @@ -597,7 +629,9 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): net = mock.ANY expected = [ mock.call.create_ofc_network(ctx, tenant, net_id, net_name), + mock.call.exists_ofc_network(ctx, net_id), mock.call.delete_ofc_network(ctx, net_id, net), + mock.call.exists_ofc_network(ctx, net_id), mock.call.delete_ofc_network(ctx, net_id, net), ] self.ofc.assert_has_calls(expected) @@ -641,6 +675,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.delete_ofc_port(ctx, port_id, port), mock.call.exists_ofc_port(ctx, port_id), mock.call.delete_ofc_port(ctx, port_id, port), + mock.call.exists_ofc_network(ctx, net_id), mock.call.delete_ofc_network(ctx, net_id, net) ] self.ofc.assert_has_calls(expected) @@ -707,6 +742,7 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): mock.call.delete_ofc_port(ctx, p1['id'], mock.ANY), mock.call.exists_ofc_port(ctx, p1['id']), + mock.call.exists_ofc_network(ctx, net['id']), mock.call.delete_ofc_network(ctx, net['id'], mock.ANY), mock.call.exists_ofc_tenant(ctx, self._tenant_id), mock.call.delete_ofc_tenant(ctx, self._tenant_id) @@ -766,8 +802,8 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): nexc.OFCException(reason='hoge')) body = {'port': {'admin_state_up': False}} - res = self._update('ports', port_id, body) - self.assertEqual(res['port']['status'], 'ERROR') + self._update('ports', port_id, body, + expected_code=webob.exc.HTTPInternalServerError.code) port_ref = self._show('ports', port_id) self.assertEqual(port_ref['port']['status'], 'ERROR') @@ -799,6 +835,27 @@ class TestNecPluginOfcManager(NecPluginV2TestCase): self.ofc.assert_has_calls(expected) self.assertEqual(self.ofc.delete_ofc_port.call_count, 2) + def test_delete_port_with_error_status(self): + self.ofc.set_raise_exc('create_ofc_port', + nexc.OFCException(reason='fake')) + + with self.port() as port: + port_id = port['port']['id'] + portinfo = {'id': port_id, 'port_no': 123} + self.rpcapi_update_ports(added=[portinfo]) + port_ref = self._show('ports', port_id) + self.assertEqual(port_ref['port']['status'], 'ERROR') + + 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), + ] + self.ofc.assert_has_calls(expected) + self.assertFalse(self.ofc.delete_ofc_port.call_count) + def test_delete_port_with_ofc_deletion_failure(self): self.ofc.set_raise_exc('delete_ofc_port', nexc.OFCException(reason='hoge')) diff --git a/neutron/tests/unit/nec/test_packet_filter.py b/neutron/tests/unit/nec/test_packet_filter.py index 148727a8f..475de26de 100644 --- a/neutron/tests/unit/nec/test_packet_filter.py +++ b/neutron/tests/unit/nec/test_packet_filter.py @@ -388,6 +388,25 @@ class TestNecPluginPacketFilter(TestNecPluginPacketFilterBase): self.ofc.delete_ofc_packet_filter.assert_called_once_with( ctx, pf_id) + def test_delete_pf_with_error_status(self): + self.ofc.set_raise_exc('create_ofc_packet_filter', + nexc.OFCException(reason='fake')) + with self.packet_filter_on_network() as pf: + pf_id = pf['packet_filter']['id'] + pf_ref = self._show('packet_filters', pf_id) + self.assertEqual(pf_ref['packet_filter']['status'], 'ERROR') + + ctx = mock.ANY + pf_dict = mock.ANY + expected = [ + mock.call.exists_ofc_packet_filter(ctx, pf_id), + mock.call.create_ofc_packet_filter(ctx, pf_id, pf_dict), + mock.call.exists_ofc_packet_filter(ctx, pf_id), + ] + self.ofc.assert_has_calls(expected) + self.assertEqual(1, self.ofc.create_ofc_packet_filter.call_count) + self.assertEqual(0, self.ofc.delete_ofc_packet_filter.call_count) + def test_activate_pf_on_port_triggered_by_update_port(self): ctx = mock.ANY pf_dict = mock.ANY @@ -437,7 +456,8 @@ class TestNecPluginPacketFilter(TestNecPluginPacketFilterBase): # This update request will make plugin reactivate pf. data = {'packet_filter': {'priority': 1000}} - self._update('packet_filters', pf_id, data) + self._update('packet_filters', pf_id, data, + expected_code=webob.exc.HTTPInternalServerError.code) self.ofc.set_raise_exc('delete_ofc_packet_filter', None) @@ -451,8 +471,7 @@ class TestNecPluginPacketFilter(TestNecPluginPacketFilterBase): mock.call.delete_ofc_packet_filter(ctx, pf_id), mock.call.exists_ofc_packet_filter(ctx, pf_id), - - mock.call.exists_ofc_packet_filter(ctx, pf_id), + mock.call.delete_ofc_packet_filter(ctx, pf_id), ] self.ofc.assert_has_calls(expected) self.assertEqual(self.ofc.delete_ofc_packet_filter.call_count, 2) @@ -466,7 +485,8 @@ class TestNecPluginPacketFilter(TestNecPluginPacketFilterBase): nexc.OFCException(reason='hoge')) data = {'packet_filter': {'admin_state_up': False}} - self._update('packet_filters', pf_id, data) + self._update('packet_filters', pf_id, data, + expected_code=webob.exc.HTTPInternalServerError.code) pf_ref = self._show('packet_filters', pf_id) self.assertEqual(pf_ref['packet_filter']['status'], 'ERROR') -- 2.45.2