diff --git a/neutron/plugins/nec/nec_plugin.py b/neutron/plugins/nec/nec_plugin.py index 2fe59d01a6..94220c4e42 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) @@ -630,10 +640,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: @@ -741,9 +749,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', []): @@ -764,8 +773,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 9d32f980cd..df48ebff86 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 f1ac10cf36..407ea5365d 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 182e98abd4..3b8a6478b4 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 148727a8f2..475de26de7 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')