Make NEC Plugin keep error resources

NEC Plugin used to ignore OFC errors while deleting resources from OFC,
and it could leave some unused resources on OFC. If OFC generates id
which is in remained resources when creating new resource, it will fail.

This commit makes NEC Plugin keep logical resource when it failed to
delete resource from OFC not to leave orphan resource on OFC, and raise
exception to tell the user that the resouce status is Error.

NOTE: The user can retry deletion. If the resouce was successfully
deleted from OFC in retries, the logical resource will be deleted.

Fixes: bug #1206416
Change-Id: Ifea38dfe3fe8b18d7ae1cedf86a23008549250cc

neutron/plugins/nec/nec_plugin.py

@@ -218,6 +218,7 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
         if port_status is not port['status']:
             self._update_resource_status(context, "port", port['id'],
                                          port_status)
+            port['status'] = port_status
 
         # deactivate packet_filters after the port has deleted from OFC.
         if self.packet_filter_enabled:
@@ -227,6 +228,8 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
             for pf in pfs:
                 self.deactivate_packet_filter(context, pf)
 
+        return port
+
     # Quantm Plugin Basic methods
 
     def create_network(self, context, network):
@@ -309,15 +312,25 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
         LOG.debug(_("NECPluginV2.delete_network() called, id=%s ."), id)
         net = super(NECPluginV2, self).get_network(context, id)
         tenant_id = net['tenant_id']
+        ports = self.get_ports(context, filters={'network_id': [id]})
+
+        # check if there are any tenant owned ports in-use
+        only_auto_del = all(p['device_owner'] in
+                            db_base_plugin_v2.AUTO_DELETE_PORT_OWNERS
+                            for p in ports)
+        if not only_auto_del:
+            raise q_exc.NetworkInUse(net_id=id)
 
         # Make sure auto-delete ports on OFC are deleted.
-        filter = {'network_id': [id],
+        _error_ports = []
-                  'device_owner': db_base_plugin_v2.AUTO_DELETE_PORT_OWNERS}
+        for port in ports:
-        auto_delete_ports = self.get_ports(context, filters=filter)
+            port = self.deactivate_port(context, port)
-        for port in auto_delete_ports:
+            if port['status'] == OperationalStatus.ERROR:
-            LOG.debug(_('delete_network(): deleting auto-delete port'
+                _error_ports.append(port['id'])
-                        ' from OFC: %s'), port)
+        if _error_ports:
-            self.deactivate_port(context, port)
+            reason = (_("Failed to delete port(s)=%s from OFC.") %
+                      ','.join(_error_ports))
+            raise nexc.OFCException(reason=reason)
 
         # delete all packet_filters of the network
         if self.packet_filter_enabled:
@@ -326,16 +339,17 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
             for pf in pfs:
                 self.delete_packet_filter(context, pf['id'])
 
-        super(NECPluginV2, self).delete_network(context, id)
         try:
             # 'net' parameter is required to lookup old OFC mapping
             self.ofc.delete_ofc_network(context, id, net)
         except (nexc.OFCException, nexc.OFCConsistencyBroken) as exc:
             reason = _("delete_network() failed due to %s") % exc
-            # NOTE: The OFC configuration of this network could be remained
+            LOG.error(reason)
-            #       as an orphan resource. But, it does NOT harm any other
+            self._update_resource_status(context, "network", net['id'],
-            #       resources, so this plugin just warns.
+                                         OperationalStatus.ERROR)
-            LOG.warn(reason)
+            raise
+
+        super(NECPluginV2, self).delete_network(context, id)
 
         # delete unnessary ofc_tenant
         filters = dict(tenant_id=[tenant_id])
@@ -407,7 +421,10 @@ class NECPluginV2(db_base_plugin_v2.NeutronDbPluginV2,
         # Thus we need to call self.get_port() instead of super().get_port()
         port = self.get_port(context, id)
 
-        self.deactivate_port(context, port)
+        port = self.deactivate_port(context, port)
+        if port['status'] == OperationalStatus.ERROR:
+            reason = _("Failed to delete port=%s from OFC.") % id
+            raise nexc.OFCException(reason=reason)
 
         # delete all packet_filters of the port
         if self.packet_filter_enabled:

neutron/tests/unit/nec/test_nec_plugin.py

@@ -17,10 +17,12 @@ import os
 
 import fixtures
 import mock
+import webob.exc
 
 from neutron.common.test_lib import test_config
 from neutron.common import topics
 from neutron import context
+from neutron.db import db_base_plugin_v2
 from neutron.extensions import portbindings
 from neutron import manager
 from neutron.plugins.nec.common import exceptions as nexc
@@ -530,6 +532,76 @@ class TestNecPluginOfcManager(NecPluginV2TestCase):
         ]
         self.ofc.assert_has_calls(expected)
 
+    def test_delete_network_with_ofc_deletion_failure(self):
+        self.ofc.set_raise_exc('delete_ofc_network',
+                               nexc.OFCException(reason='hoge'))
+
+        with self.network() as net:
+            net_id = net['network']['id']
+
+            self._delete('networks', net_id,
+                         expected_code=webob.exc.HTTPInternalServerError.code)
+
+            net_ref = self._show('networks', net_id)
+            self.assertEqual(net_ref['network']['status'], 'ERROR')
+
+            self.ofc.set_raise_exc('delete_ofc_network', None)
+
+        ctx = mock.ANY
+        tenant = mock.ANY
+        net_name = mock.ANY
+        net = mock.ANY
+        expected = [
+            mock.call.create_ofc_network(ctx, tenant, net_id, net_name),
+            mock.call.delete_ofc_network(ctx, net_id, net),
+            mock.call.delete_ofc_network(ctx, net_id, net),
+        ]
+        self.ofc.assert_has_calls(expected)
+        self.assertEqual(self.ofc.delete_ofc_network.call_count, 2)
+
+    def test_delete_network_with_deactivating_auto_delete_port_failure(self):
+        self.ofc.set_raise_exc('delete_ofc_port',
+                               nexc.OFCException(reason='hoge'))
+
+        with self.network(do_delete=False) as net:
+            net_id = net['network']['id']
+
+            device_owner = db_base_plugin_v2.AUTO_DELETE_PORT_OWNERS[0]
+            port = self._make_port(self.fmt, net_id, device_owner=device_owner)
+            port_id = port['port']['id']
+
+            portinfo = {'id': port_id, 'port_no': 123}
+            self.rpcapi_update_ports(added=[portinfo])
+
+        self._delete('networks', net_id,
+                     expected_code=webob.exc.HTTPInternalServerError.code)
+
+        net_ref = self._show('networks', net_id)
+        self.assertEqual(net_ref['network']['status'], 'ACTIVE')
+        port_ref = self._show('ports', port_id)
+        self.assertEqual(port_ref['port']['status'], 'ERROR')
+
+        self.ofc.set_raise_exc('delete_ofc_port', None)
+        self._delete('networks', net_id)
+
+        ctx = mock.ANY
+        tenant = mock.ANY
+        net_name = mock.ANY
+        net = mock.ANY
+        port = mock.ANY
+        expected = [
+            mock.call.create_ofc_network(ctx, tenant, net_id, net_name),
+            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),
+            mock.call.exists_ofc_port(ctx, port_id),
+            mock.call.delete_ofc_port(ctx, port_id, port),
+            mock.call.delete_ofc_network(ctx, net_id, net)
+        ]
+        self.ofc.assert_has_calls(expected)
+        self.assertEqual(self.ofc.delete_ofc_network.call_count, 1)
+
     def test_update_port(self):
         self._test_update_port_with_admin_state(resource='port')
 
@@ -592,3 +664,34 @@ class TestNecPluginOfcManager(NecPluginV2TestCase):
             mock.call.delete_ofc_tenant(ctx, self._tenant_id)
         ]
         self.ofc.assert_has_calls(expected)
+
+    def test_delete_port_with_ofc_deletion_failure(self):
+        self.ofc.set_raise_exc('delete_ofc_port',
+                               nexc.OFCException(reason='hoge'))
+
+        with self.port() 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,
+                         expected_code=webob.exc.HTTPInternalServerError.code)
+
+            port_ref = self._show('ports', port_id)
+            self.assertEqual(port_ref['port']['status'], 'ERROR')
+
+            self.ofc.set_raise_exc('delete_ofc_port', None)
+
+        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),
+            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, 2)