From: armando-migliaccio <armamig@gmail.com>
Date: Fri, 4 Jul 2014 00:00:44 +0000 (-0700)
Subject: Make ML2 ensure_dvr_port_binding more robust
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=0b30651678e1e2e574553272c0f9ab69418efe96;p=openstack-build%2Fneutron-build.git

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 c312f7d09..242b19ae9 100644
--- 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 b3e89148c..847d28de9 100644
--- 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'