Avoid loadbalancer stuck in PENDING_X if delete_vip_port fails

If either on a failed loadbalancer creation action, or in a loadbalancer
deletion, the delete_vip_port fails with a different exception than
PortNotFound (for instance, if neutron API is unreachable at that time),
the loadbalancer will remain stuck in PENDING_X, as the ERROR status
is not returned

Also modified coverage to 88% just to allow backport on xena stable
branch. Following patch [1] includes several tests over the code base
of this current patch and will restore the threshold again to 90 when
it is be backported.

[1] https://review.opendev.org/c/openstack/ovn-octavia-provider/+/836033

Closes-Bug: #1965732
Change-Id: I7701ac3ab7358d4ab061645a7221a541cd183aa7
(cherry picked from commit 28c50d9147)

ovn_octavia_provider/helper.py

@@ -362,8 +362,8 @@ class OvnProviderHelper():
     @tenacity.retry(
         retry=tenacity.retry_if_exception_type(
             driver_exceptions.UpdateStatusError),
-        wait=tenacity.wait_exponential(),
-        stop=tenacity.stop_after_delay(10),
+        wait=tenacity.wait_exponential(max=75),
+        stop=tenacity.stop_after_attempt(15),
         reraise=True)
     def _update_status_to_octavia(self, status):
         status = OvnProviderHelper._delete_disabled_from_status(status)
@@ -1005,9 +1005,15 @@ class OvnProviderHelper():
             LOG.exception(ovn_const.EXCEPTION_MSG, "creation of loadbalancer")
             # Any Exception set the status to ERROR
             if isinstance(port, dict):
-                self.delete_vip_port(port.get('id'))
-                LOG.warning("Deleting the VIP port %s since LB went into "
-                            "ERROR state", str(port.get('id')))
+                try:
+                    self.delete_vip_port(port.get('id'))
+                    LOG.warning("Deleting the VIP port %s since LB went into "
+                                "ERROR state", str(port.get('id')))
+                except Exception:
+                    LOG.exception("Error deleting the VIP port %s upon "
+                                  "loadbalancer %s creation failure",
+                                  str(port.get('id')),
+                                  str(loadbalancer[constants.ID]))
             status = {
                 constants.LOADBALANCERS: [
                     {constants.ID: loadbalancer[constants.ID],
@@ -1034,6 +1040,23 @@ class OvnProviderHelper():
             LOG.warning("Loadbalancer %s not found in OVN Northbound DB. "
                         "Setting the Loadbalancer status to DELETED "
                         "in Octavia", str(loadbalancer[constants.ID]))
+
+            # NOTE(ltomasbo): In case the previous loadbalancer deletion
+            # action failed at VIP deletion step, this ensures the VIP
+            # is not leaked
+            try:
+                vip_port_id = self._get_vip_port_from_loadbalancer_id(
+                    loadbalancer[constants.ID])
+                if vip_port_id:
+                    LOG.warning("Deleting the VIP port %s associated to LB "
+                                "missing in OVN DBs", str(vip_port_id))
+                    self.delete_vip_port(vip_port_id)
+            except Exception:
+                LOG.exception("Error deleting the VIP port %s",
+                              str(vip_port_id))
+                lbalancer_status[constants.PROVISIONING_STATUS] = (
+                    constants.ERROR)
+                lbalancer_status[constants.OPERATING_STATUS] = constants.ERROR
             return status
 
         try:
@@ -1046,12 +1069,13 @@ class OvnProviderHelper():
             # dict while iterating over it. So first get a list of keys.
             # https://cito.github.io/blog/never-iterate-a-changing-dict/
             status = {key: value for key, value in status.items() if value}
+            # Delete VIP port from neutron.
+            self.delete_vip_port(port_id)
         except Exception:
             LOG.exception(ovn_const.EXCEPTION_MSG, "deletion of loadbalancer")
             lbalancer_status[constants.PROVISIONING_STATUS] = constants.ERROR
             lbalancer_status[constants.OPERATING_STATUS] = constants.ERROR
-        # Delete VIP port from neutron.
-        self.delete_vip_port(port_id)
+
         return status
 
     def _lb_delete(self, loadbalancer, ovn_lb, status):
@@ -1154,6 +1178,11 @@ class OvnProviderHelper():
             lb_status[constants.OPERATING_STATUS] = constants.ERROR
         return status
 
+    def _get_vip_port_from_loadbalancer_id(self, lb_id):
+        lb = self._octavia_driver_lib.get_loadbalancer(lb_id)
+        if lb and lb.vip_port_id:
+            return lb.vip_port_id
+
     def listener_create(self, listener):
         ovn_lb = self._get_or_create_ovn_lb(
             listener[constants.LOADBALANCER_ID],
@@ -1897,6 +1926,12 @@ class OvnProviderHelper():
             LOG.debug('VIP Port already exists, uuid: %s', port['id'])
             return {'port': port}
 
+    @tenacity.retry(
+        retry=tenacity.retry_if_exception_type(
+            n_exc.NeutronClientException),
+        wait=tenacity.wait_exponential(max=75),
+        stop=tenacity.stop_after_attempt(15),
+        reraise=True)
     def delete_vip_port(self, port_id):
         neutron_client = clients.get_neutron_client()
         try:

ovn_octavia_provider/tests/unit/test_helper.py

@@ -543,9 +543,12 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase):
             self.ovn_lb.uuid)
         del_port.assert_called_once_with('foo_port')
 
+    @mock.patch.object(ovn_helper.OvnProviderHelper,
+                       '_get_vip_port_from_loadbalancer_id')
     @mock.patch.object(ovn_helper.OvnProviderHelper, 'delete_vip_port')
-    def test_lb_delete_row_not_found(self, del_port):
+    def test_lb_delete_row_not_found(self, del_port, get_vip_port):
         self.helper._find_ovn_lbs.side_effect = [idlutils.RowNotFound]
+        get_vip_port.return_value = None
         status = self.helper.lb_delete(self.lb)
         self.assertEqual(status['loadbalancers'][0]['provisioning_status'],
                          constants.DELETED)
@@ -553,6 +556,22 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase):
                          constants.OFFLINE)
         self.helper.ovn_nbdb_api.lb_del.assert_not_called()
         del_port.assert_not_called()
+        get_vip_port.assert_called_once_with(self.lb['id'])
+
+    @mock.patch.object(ovn_helper.OvnProviderHelper,
+                       '_get_vip_port_from_loadbalancer_id')
+    @mock.patch.object(ovn_helper.OvnProviderHelper, 'delete_vip_port')
+    def test_lb_delete_row_not_found_vip_leak(self, del_port, get_vip_port):
+        self.helper._find_ovn_lbs.side_effect = [idlutils.RowNotFound]
+        get_vip_port.return_value = 'foo_port'
+        status = self.helper.lb_delete(self.lb)
+        self.assertEqual(status['loadbalancers'][0]['provisioning_status'],
+                         constants.DELETED)
+        self.assertEqual(status['loadbalancers'][0]['operating_status'],
+                         constants.OFFLINE)
+        self.helper.ovn_nbdb_api.lb_del.assert_not_called()
+        del_port.assert_called_once_with('foo_port')
+        get_vip_port.assert_called_once_with(self.lb['id'])
 
     @mock.patch.object(ovn_helper.OvnProviderHelper, 'delete_vip_port')
     def test_lb_delete_exception(self, del_port):
@@ -564,7 +583,7 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase):
                          constants.ERROR)
         self.helper.ovn_nbdb_api.lb_del.assert_called_once_with(
             self.ovn_lb.uuid)
-        del_port.assert_called_once_with('foo_port')
+        del_port.assert_not_called()
 
     @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client')
     @mock.patch.object(ovn_helper.OvnProviderHelper, 'delete_vip_port')
@@ -580,6 +599,18 @@ class TestOvnProviderHelper(ovn_base.TestOvnOctaviaBase):
             self.ovn_lb.uuid)
         del_port.assert_called_once_with('foo_port')
 
+    @mock.patch.object(ovn_helper.OvnProviderHelper, 'delete_vip_port')
+    def test_lb_delete_port_exception(self, del_port):
+        del_port.side_effect = [Exception]
+        status = self.helper.lb_delete(self.ovn_lb)
+        self.assertEqual(status['loadbalancers'][0]['provisioning_status'],
+                         constants.ERROR)
+        self.assertEqual(status['loadbalancers'][0]['operating_status'],
+                         constants.ERROR)
+        self.helper.ovn_nbdb_api.lb_del.assert_called_once_with(
+            self.ovn_lb.uuid)
+        del_port.assert_called_once_with('foo_port')
+
     @mock.patch('ovn_octavia_provider.common.clients.get_neutron_client')
     def test_lb_delete_cascade(self, net_cli):
         net_cli.return_value.delete_port.return_value = None

tox.ini

@@ -62,7 +62,7 @@ setenv =
 commands =
   stestr run --no-subunit-trace {posargs}
   coverage combine
-  coverage report --fail-under=90 --skip-covered
+  coverage report --fail-under=88 --skip-covered
   coverage html -d cover
   coverage xml -o cover/coverage.xml