[OVN] Retry retrieving LSP hosting information

There's a sync issue while trying to fetch the hosting information for
the LSP before we write it to the OVN database, sometimes the
information is not yet present and we end up with an empty string ("")
for the host attribute of portbindings. This patch adds a retry
mechanism to solve this sync issue.

Conflicts:
  neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py
  neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py

Change-Id: I52ec4b346271889ebaa7b7f84981eae5503d02d3
Related-Bug: #2020058
Signed-off-by: Lucas Alvares Gomes <[email protected]>
(cherry picked from commit 3044b93)

Author: umago

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py

@@ -42,13 +42,15 @@
 from ovsdbapp.backend.ovs_idl import idlutils
 import tenacity
 
+from neutron._i18n import _
 from neutron.common.ovn import acl as ovn_acl
 from neutron.common.ovn import constants as ovn_const
 from neutron.common.ovn import utils
 from neutron.common import utils as common_utils
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 from neutron.db import ovn_revision_numbers_db as db_rev
 from neutron.db import segments_db
+from neutron.plugins.ml2 import db as ml2_db
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.extensions \
     import placement as placement_extension
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb.extensions \
@@ -272,6 +274,29 @@ def determine_bind_host(self, port, port_context=None):
                     ovn_const.VIF_DETAILS_CARD_SERIAL_NUMBER]).hostname
         return ''
 
+    @tenacity.retry(retry=tenacity.retry_if_exception_type(RuntimeError),
+                    wait=tenacity.wait_random(min=2, max=3),
+                    stop=tenacity.stop_after_attempt(3),
+                    reraise=True)
+    def _wait_for_port_bindings_host(self, context, port_id):
+        db_port = ml2_db.get_port(context, port_id)
+        # This is already checked previously but, just to stay on
+        # the safe side in case the port is deleted mid-operation
+        if not db_port:
+            raise RuntimeError(
+                _('No port found with ID %s') % port_id)
+
+        if not db_port.port_bindings:
+            raise RuntimeError(
+                _('No port bindings information found for  '
+                  'port %s') % port_id)
+
+        if not db_port.port_bindings[0].host:
+            raise RuntimeError(
+                _('No hosting information found for port %s') % port_id)
+
+        return db_port
+
     def update_lsp_host_info(self, context, db_port, up=True):
         """Update the binding hosting information for the LSP.
 
@@ -287,8 +312,19 @@ def update_lsp_host_info(self, context, db_port, up=True):
         if up:
             if not db_port.port_bindings:
                 return
-            host = db_port.port_bindings[0].host
 
+            if not db_port.port_bindings[0].host:
+                # NOTE(lucasgomes): There might be a sync issue between
+                # the moment that this port was fetched from the database
+                # and the hosting information being set, retry a few times
+                try:
+                    db_port = self._wait_for_port_bindings_host(
+                        context, db_port.id)
+                except RuntimeError as e:
+                    LOG.warning(e)
+                    return
+
+            host = db_port.port_bindings[0].host
             ext_ids = ('external_ids',
                        {ovn_const.OVN_HOST_ID_EXT_ID_KEY: host})
             cmd.append(

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py

@@ -17,6 +17,7 @@
 
 from neutron.common.ovn import constants
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
+from neutron.plugins.ml2 import db as ml2_db
 from neutron.plugins.ml2.drivers.ovn.mech_driver.ovsdb import ovn_client
 from neutron.tests import base
 from neutron.tests.unit import fake_resources as fakes
@@ -25,6 +26,8 @@
 from neutron_lib.api.definitions import portbindings
 from neutron_lib.services.logapi import constants as log_const
 
+from tenacity import wait_none
+
 
 class TestOVNClientBase(base.BaseTestCase):
 
@@ -47,6 +50,9 @@ def setUp(self):
             fakes.FakeChassis.create(
                 attrs={'hostname': self.fake_smartnic_hostname}))
 
+        # Disable tenacity wait for UT
+        self.ovn_client._wait_for_port_bindings_host.retry.wait = wait_none()
+
     def test_vnic_normal_unbound_port(self):
         self.assertEqual(
             '',
@@ -139,6 +145,45 @@ def test_update_lsp_host_info_up(self):
             'Logical_Switch_Port', port_id,
             ('external_ids', {constants.OVN_HOST_ID_EXT_ID_KEY: host_id}))
 
+    def test_update_lsp_host_info_up_retry(self):
+        context = mock.MagicMock()
+        host_id = 'fake-binding-host-id'
+        port_id = 'fake-port-id'
+        db_port_no_host = mock.Mock(
+            id=port_id, port_bindings=[mock.Mock(host="")])
+        db_port = mock.Mock(
+            id=port_id, port_bindings=[mock.Mock(host=host_id)])
+
+        with mock.patch.object(
+                self.ovn_client, '_wait_for_port_bindings_host') as mock_wait:
+            mock_wait.return_value = db_port
+            self.ovn_client.update_lsp_host_info(context, db_port_no_host)
+
+            # Assert _wait_for_port_bindings_host was called
+            mock_wait.assert_called_once_with(context, port_id)
+
+        # Assert host_id was set
+        self.nb_idl.db_set.assert_called_once_with(
+            'Logical_Switch_Port', port_id,
+            ('external_ids', {constants.OVN_HOST_ID_EXT_ID_KEY: host_id}))
+
+    def test_update_lsp_host_info_up_retry_fail(self):
+        context = mock.MagicMock()
+        port_id = 'fake-port-id'
+        db_port_no_host = mock.Mock(
+            id=port_id, port_bindings=[mock.Mock(host="")])
+
+        with mock.patch.object(
+                self.ovn_client, '_wait_for_port_bindings_host') as mock_wait:
+            mock_wait.side_effect = RuntimeError("boom")
+            self.ovn_client.update_lsp_host_info(context, db_port_no_host)
+
+            # Assert _wait_for_port_bindings_host was called
+            mock_wait.assert_called_once_with(context, port_id)
+
+        # Assert host_id was NOT set
+        self.assertFalse(self.nb_idl.db_set.called)
+
     def test_update_lsp_host_info_down(self):
         context = mock.MagicMock()
         port_id = 'fake-port-id'
@@ -150,6 +195,47 @@ def test_update_lsp_host_info_down(self):
             'Logical_Switch_Port', port_id, 'external_ids',
             constants.OVN_HOST_ID_EXT_ID_KEY, if_exists=True)
 
+    @mock.patch.object(ml2_db, 'get_port')
+    def test__wait_for_port_bindings_host(self, mock_get_port):
+        context = mock.MagicMock()
+        host_id = 'fake-binding-host-id'
+        port_id = 'fake-port-id'
+        db_port_no_host = mock.Mock(
+            id=port_id, port_bindings=[mock.Mock(host="")])
+        db_port = mock.Mock(
+            id=port_id, port_bindings=[mock.Mock(host=host_id)])
+
+        mock_get_port.side_effect = (db_port_no_host, db_port)
+
+        ret = self.ovn_client._wait_for_port_bindings_host(
+            context, port_id)
+
+        self.assertEqual(ret, db_port)
+
+        expected_calls = [mock.call(context, port_id),
+                          mock.call(context, port_id)]
+        mock_get_port.assert_has_calls(expected_calls)
+
+    @mock.patch.object(ml2_db, 'get_port')
+    def test__wait_for_port_bindings_host_fail(self, mock_get_port):
+        context = mock.MagicMock()
+        port_id = 'fake-port-id'
+        db_port_no_pb = mock.Mock(id=port_id, port_bindings=[])
+        db_port_no_host = mock.Mock(
+            id=port_id, port_bindings=[mock.Mock(host="")])
+
+        mock_get_port.side_effect = (
+            db_port_no_pb, db_port_no_host, db_port_no_host)
+
+        self.assertRaises(
+            RuntimeError, self.ovn_client._wait_for_port_bindings_host,
+            context, port_id)
+
+        expected_calls = [mock.call(context, port_id),
+                          mock.call(context, port_id),
+                          mock.call(context, port_id)]
+        mock_get_port.assert_has_calls(expected_calls)
+
 
 class TestOVNClientFairMeter(TestOVNClientBase,
                              test_log_driver.TestOVNDriverBase):