From 43c8359db51d2367c6d40f9ff0b5c868547276f5 Mon Sep 17 00:00:00 2001
From: inimitableharish <inimitableharish@gmail.com>
Date: Tue, 12 Dec 2017 00:04:31 +0530
Subject: [PATCH] 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    | 13 +++++-----
 quark/tests/test_ironic_driver.py | 41 ++++++++++++++++++++++++++-----
 2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/quark/drivers/ironic_driver.py b/quark/drivers/ironic_driver.py
index ce5510d..1ca9cfa 100644
--- a/quark/drivers/ironic_driver.py
+++ b/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)
diff --git a/quark/tests/test_ironic_driver.py b/quark/tests/test_ironic_driver.py
index 13511ec..00b8f1e 100644
--- a/quark/tests/test_ironic_driver.py
+++ b/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):