OnMetal Driver Can't Handle 404s on Port Delete

When quark tries to delete an onmetal port that no longer
exists downstream, it should happily ignore the 404 and
move on.Instead, it complains and retries before eventually
giving up. This is due to not catching the correctexception
from the neutron client.This doesn't present as a 500 to
any clients, as the exception is caught and ignored,
but it gums up the logs and wastes time making requests
multiple times.

Implements: Exception handling
Closes-Bug: #1738277
Change-Id: I4ccd9be891e6c533ceb68427d47ed658bf27f4a8

quark/drivers/ironic_driver.py

@@ -20,6 +20,7 @@ import json
 import netaddr
 
 from neutron_lib import exceptions as n_exc
+from neutronclient.neutron import client as neutron_client
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import importutils
@@ -397,13 +398,13 @@ class IronicDriver(base.BaseDriver):
     def _delete_port(self, context, port_id):
         try:
             return self._client.delete_port(port_id)
+        except neutron_client.utils.exceptions.PortNotFoundClient:
+            # Port not found exception encountered, such as port deleted
+            LOG.error("port %s not found downstream."
+                      "ignoring delete." % (port_id))
+            return None
         except Exception as e:
-            # This doesn't get wrapped by the client unfortunately.
-            if "404 not found" in str(e).lower():
-                LOG.error("port %s not found downstream. ignoring delete."
-                          % (port_id))
-                return
-
+            # Any generic exception (say 500 or others)
             msg = ("failed to delete downstream port. "
                    "exception: %s" % (e))
             LOG.exception(msg)

quark/tests/test_ironic_driver.py

@@ -14,18 +14,17 @@
 #  under the License.
 
 import contextlib
-
-import netaddr
-
 import json
-
 import mock
+import netaddr
+from neutronclient.neutron import client as neutron_client
 from oslo_config import cfg
-
 import quark.drivers.ironic_driver
 from quark import network_strategy
 from quark.tests import test_base
 
+CONF = cfg.CONF
+
 
 class TestIronicDriverBase(test_base.TestBase):
 
@@ -569,6 +568,29 @@ class TestIronicDriverCreatePort(TestIronicDriverBase):
 
 class TestIronicDriverDeletePort(TestIronicDriverBase):
 
+    @contextlib.contextmanager
+    def test_port_not_found_exception_no_retry(self):
+        '''When port not found we must only call delete once.'''
+        ironic_driver = quark.drivers.ironic_driver.IronicDriver()
+        ironic_driver._client = mock.MagicMock()
+        ironic_driver._client.delete_port.return_value = None
+        side_effect = neutron_client.utils.exceptions.PortNotFoundClient
+        ironic_driver._client.delete_port.side_effect = side_effect
+        port_id = 'very_fake_port_id'
+        ironic_driver.delete_port(self.context, port_id)
+        ironic_driver._client.delete_port.assert_called_once_with(port_id)
+
+    @contextlib.contextmanager
+    def test_port_not_found_exception_with_retry(self):
+        '''When port delete fails for unknown reasons, retry.'''
+        ironic_driver = quark.drivers.ironic_driver.IronicDriver()
+        ironic_driver._client = mock.MagicMock()
+        ironic_driver._client.delete_port.side_effect = Exception
+        port_id = 'very_fake_port_id'
+        ironic_driver.delete_port(self.context, port_id)
+        self.assertEqual(ironic_driver._client.delete_port.call_count,
+                         CONF.IRONIC.operation_retries)
+
     def test_delete_port(self):
         with self._stubs(delete_port=[None]) as (driver, client,
                                                  _, delete):
@@ -576,6 +598,7 @@ class TestIronicDriverDeletePort(TestIronicDriverBase):
             delete.assert_called_once_with("foo")
 
     def test_delete_port_retries(self):
+
         delete_port = [Exception("uhoh"), None]
         with self._stubs(delete_port=delete_port) as (driver,
                                                       client, _,
@@ -601,12 +624,18 @@ class TestIronicDriverDeletePort(TestIronicDriverBase):
                  mock.call("foo")])
 
     def test_delete_port_ignores_404(self):
+
+        '''Any exception other than neutronclient.PortNotFound would be
+
+        considered as generic exception, one retry for generic exception
+        '''
+
         delete_port = [Exception("404 not found"), None]
         with self._stubs(delete_port=delete_port) as (driver,
                                                       client, _,
                                                       delete):
             driver.delete_port(self.context, "foo")
-            delete.assert_called_once_with("foo")
+            self.assertEqual(delete.call_count, 2)
 
 
 class TestIronicDriverUpdatePort(TestIronicDriverBase):