Guard against concurrent port removal in DVR

The delete_csnat_router_interface_ports method constructs
a generator expression to retrieve the ports attached
to a router. If a concurrent process manages to delete
one of the ports referenced in the generator from the DB
after it is constructed (but before iteration), some of the
ports will be None objects on iteration.

This patch verifies that the port db object is there
before trying to extract the ID from it.

More details:

This is because the query expression[1] is evaluated immediately
at the time the generator is defined. If there are ports in the
DB at the time of this evaluation, RouterPort objects will be
prepared to be iterated over. However, before the iteration over the
generator happens, something else may delete the port from the DB.
If that happens, when the iteration starts and the 'port' attribute
is accessed on the router port, a SELECT statement will be issued to
the DB to get that port[2] and it will return None.

1. router.attached_ports.filter_by(port_type=DEVICE_OWNER_DVR_SNAT)
2. The RouterPorts table's relationship to the Ports table is using
   the default 'select' lazy loading.

Closes-Bug: #1381263
Change-Id: Ia371545d641aaa1cbaa0fd10cd233250ec5769e5

diff --git a/neutron/db/l3_dvr_db.py b/neutron/db/l3_dvr_db.py
index 1b39d5bed76214e9a769fbab4452e4a0fc4a37f7..2625d1139e9916eeda9aa2b015f0e334158f8901 100644 (file)
--- a/neutron/db/l3_dvr_db.py
+++ b/neutron/db/l3_dvr_db.py
@@ -578,6 +578,7 @@ class L3_NAT_with_dvr_db_mixin(l3_db.L3_NAT_db_mixin,
         ports = (
             rp.port.id for rp in
             router.attached_ports.filter_by(port_type=DEVICE_OWNER_DVR_SNAT)
+            if rp.port
         )
 
         c_snat_ports = self._core_plugin.get_ports(

diff --git a/neutron/tests/unit/ml2/test_ml2_plugin.py b/neutron/tests/unit/ml2/test_ml2_plugin.py
index 9bd8ccd9f61e96b29954f289902564b9f2b6d830..18f27e8f3e16bb69e61f69243e75b2b8df1ece46 100644 (file)
--- a/neutron/tests/unit/ml2/test_ml2_plugin.py
+++ b/neutron/tests/unit/ml2/test_ml2_plugin.py
@@ -24,6 +24,7 @@ from neutron.common import exceptions as exc
 from neutron.common import utils
 from neutron import context
 from neutron.db import db_base_plugin_v2 as base_plugin
+from neutron.db import l3_db
 from neutron.extensions import external_net as external_net
 from neutron.extensions import l3agentscheduler
 from neutron.extensions import multiprovidernet as mpnet
@@ -296,6 +297,38 @@ class TestMl2DvrPortsV2(TestMl2PortsV2):
         self._test_delete_dvr_serviced_port(
             device_owner=constants.DEVICE_OWNER_LOADBALANCER)
 
+    def test_concurrent_csnat_port_delete(self):
+        plugin = manager.NeutronManager.get_service_plugins()[
+            service_constants.L3_ROUTER_NAT]
+        r = plugin.create_router(
+            self.context,
+            {'router': {'name': 'router', 'admin_state_up': True}})
+        with self.subnet() as s:
+            p = plugin.add_router_interface(self.context, r['id'],
+                                            {'subnet_id': s['subnet']['id']})
+
+        # lie to turn the port into an SNAT interface
+        with self.context.session.begin():
+            rp = self.context.session.query(l3_db.RouterPort).filter_by(
+                port_id=p['port_id']).first()
+            rp.port_type = constants.DEVICE_OWNER_ROUTER_SNAT
+
+        # take the port away before csnat gets a chance to delete it
+        # to simulate a concurrent delete
+        orig_get_ports = plugin._core_plugin.get_ports
+
+        def get_ports_with_delete_first(*args, **kwargs):
+            plugin._core_plugin.delete_port(self.context,
+                                            p['port_id'],
+                                            l3_port_check=False)
+            return orig_get_ports(*args, **kwargs)
+        plugin._core_plugin.get_ports = get_ports_with_delete_first
+
+        # This should be able to handle a concurrent delete without raising
+        # an exception
+        router = plugin._get_router(self.context, r['id'])
+        plugin.delete_csnat_router_interface_ports(self.context, router)
+
 
 class TestMl2PortBinding(Ml2PluginV2TestCase,
                          test_bindings.PortBindingsTestCase):