Make ML2 ensure_dvr_port_binding more robust

There is a remote chance that this operation may
be prone to DB integrity errors, in case the binding
is attempted on the same port twice.

Ideally getter methods should not create, but this
is a common Neutron (anti)-pattern that would be
difficult to eradicate (at least in a single patch);
so for now let's make this code more defensive.

Related-bug: #1269131
Related-bug: #1335226

Change-Id: Ie6c57fd46f0752839814dbac5b14fae2364f973d

diff --git a/neutron/plugins/ml2/db.py b/neutron/plugins/ml2/db.py
index c312f7d0901d084992fd23e0999e054191ebdbdd..242b19ae9621cbf93fac3ce9c9f8412ec1947010 100644 (file)
--- a/neutron/plugins/ml2/db.py
+++ b/neutron/plugins/ml2/db.py
@@ -15,6 +15,8 @@
 
 from sqlalchemy.orm import exc
 
+from oslo.db import exception as db_exc
+
 from neutron.common import constants as n_const
 from neutron.db import api as db_api
 from neutron.db import models_v2
@@ -90,16 +92,13 @@ def get_locked_port_and_binding(session, port_id):
 
 
 def ensure_dvr_port_binding(session, port_id, host, router_id=None):
-    # FIXME(armando-migliaccio): take care of LP #1335226
-    # DVR ports are slightly different from the others in
-    # that binding happens at a later stage via L3 agent
-    # therefore we need to keep this logic of creation on
-    # missing binding.
-    with session.begin(subtransactions=True):
-        try:
-            record = (session.query(models.DVRPortBinding).
-                      filter_by(port_id=port_id, host=host).one())
-        except exc.NoResultFound:
+    record = (session.query(models.DVRPortBinding).
+              filter_by(port_id=port_id, host=host).first())
+    if record:
+        return record
+
+    try:
+        with session.begin(subtransactions=True):
             record = models.DVRPortBinding(
                 port_id=port_id,
                 host=host,
@@ -109,7 +108,11 @@ def ensure_dvr_port_binding(session, port_id, host, router_id=None):
                 cap_port_filter=False,
                 status=n_const.PORT_STATUS_DOWN)
             session.add(record)
-        return record
+            return record
+    except db_exc.DBDuplicateEntry:
+        LOG.debug("DVR Port %s already bound", port_id)
+        return (session.query(models.DVRPortBinding).
+                filter_by(port_id=port_id, host=host).one())
 
 
 def delete_dvr_port_binding(session, port_id, host):

diff --git a/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py b/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py
index b3e89148c6461b49d3188405402a5b83508c7de5..847d28de9a878675384cb3b76e97aafeb9c98321 100644 (file)
--- a/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py
+++ b/neutron/tests/unit/ml2/db/test_ml2_dvr_db.py
@@ -13,6 +13,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import mock
+
+from sqlalchemy.orm import query
+
 from neutron import context
 from neutron.db import api as db_api
 from neutron.db import l3_db
@@ -66,6 +70,22 @@ class Ml2DBTestCase(base.BaseTestCase):
             self.ctx.session.add(record)
             return record
 
+    def test_ensure_dvr_port_binding_deals_with_db_duplicate(self):
+        network_id = 'foo_network_id'
+        port_id = 'foo_port_id'
+        router_id = 'foo_router_id'
+        host_id = 'foo_host_id'
+        self._setup_neutron_network(network_id, [port_id])
+        self._setup_dvr_binding(network_id, port_id, router_id, host_id)
+        with mock.patch.object(query.Query, 'first') as query_first:
+            query_first.return_value = []
+            with mock.patch.object(ml2_db.LOG, 'debug') as log_trace:
+                binding = ml2_db.ensure_dvr_port_binding(
+                    self.ctx.session, port_id, host_id, router_id)
+        self.assertTrue(query_first.called)
+        self.assertTrue(log_trace.called)
+        self.assertEqual(port_id, binding.port_id)
+
     def test_ensure_dvr_port_binding(self):
         network_id = 'foo_network_id'
         port_id = 'foo_port_id'