From c5956bdada2442a9051d33dd19b59fce3bdd45be Mon Sep 17 00:00:00 2001
From: Julia Kreger <juliaashleykreger@gmail.com>
Date: Fri, 1 Nov 2019 06:40:13 -0700
Subject: [PATCH] Suppress errors from iscsi session cleanup

In some cases, where the rts library is not installed, IPA
was recently changed to try and tear down the local side of
the iscsi connection by trying to tear down bond and target
being offered. The whole attempt with this is to ensure that
no disk locks are in place which can prevent partition table
updates.

Since we added this logic, in some cases these commands can
fail and cause the deployment process to fail when it would
have otherwise succeeded. As such, suppress the errors.

Change-Id: I0e04936ad337b394dd68e9b0396a9f1203218f9f
---
 ironic_python_agent/extensions/iscsi.py       | 30 ++++++++++++-------
 .../tests/unit/extensions/test_iscsi.py       | 13 ++++++++
 ...si-teardown-handling-0df2345318d3c843.yaml | 12 ++++++++
 3 files changed, 45 insertions(+), 10 deletions(-)
 create mode 100644 releasenotes/notes/fix-iscsi-teardown-handling-0df2345318d3c843.yaml

diff --git a/ironic_python_agent/extensions/iscsi.py b/ironic_python_agent/extensions/iscsi.py
index 7fca41542..21dfba392 100644
--- a/ironic_python_agent/extensions/iscsi.py
+++ b/ironic_python_agent/extensions/iscsi.py
@@ -114,18 +114,28 @@ def clean_up(device):
     try:
         rts_root = rtslib_fb.RTSRoot()
     except (OSError, EnvironmentError, rtslib_fb.RTSLibError) as exc:
-        LOG.info('Linux-IO is not available, attemting to stop tgtd mapping. '
-                 'Error: %s.', exc)
-        cmd = ['tgtadm', '--lld', 'iscsi', '--mode', 'target', '--op',
-               'unbind', '--tid', '1', '--initiator-address', 'ALL']
-        _execute(cmd, "Error when cleaning up iscsi binds.")
-
+        try:
+            LOG.info('Linux-IO is not available, attemting to stop tgtd '
+                     'mapping. Error: %s.', exc)
+            cmd = ['tgtadm', '--lld', 'iscsi', '--mode', 'target', '--op',
+                   'unbind', '--tid', '1', '--initiator-address', 'ALL']
+            _execute(cmd, "Error when cleaning up iscsi binds.")
+        except errors.ISCSICommandError:
+            # This command may fail if the target was already torn down
+            # and that is okay, we just want to ensure it has been torn
+            # down so there should be no disk locks persisting.
+            pass
         cmd = ['sync']
         _execute(cmd, "Error flushing buffers to disk.")
-
-        cmd = ['tgtadm', '--lld', 'iscsi', '--mode', 'target', '--op',
-               'delete', '--tid', '1']
-        _execute(cmd, "Error deleting the iscsi target configuration.")
+        try:
+            cmd = ['tgtadm', '--lld', 'iscsi', '--mode', 'target', '--op',
+                   'delete', '--tid', '1']
+            _execute(cmd, "Error deleting the iscsi target configuration.")
+        except errors.ISCSICommandError:
+            # This command should remove the target from being offered.
+            # It is just proper clean-up, and often previously the IPA
+            # side, or "target" was never really torn down in many cases.
+            pass
         return
 
     storage = None
diff --git a/ironic_python_agent/tests/unit/extensions/test_iscsi.py b/ironic_python_agent/tests/unit/extensions/test_iscsi.py
index f275c0fc3..36741683e 100644
--- a/ironic_python_agent/tests/unit/extensions/test_iscsi.py
+++ b/ironic_python_agent/tests/unit/extensions/test_iscsi.py
@@ -304,6 +304,19 @@ class TestISCSIExtensionCleanUpFallback(base.IronicAgentTest):
         iscsi.clean_up(self.fake_dev)
         mock_execute.assert_has_calls(expected)
 
+    def test_commands_fail(self, mock_execute):
+        mock_execute.side_effect = [processutils.ProcessExecutionError(),
+                                    ('', ''),
+                                    processutils.ProcessExecutionError()]
+        expected = [mock.call('tgtadm', '--lld', 'iscsi', '--mode',
+                              'target', '--op', 'unbind', '--tid', '1',
+                              '--initiator-address', 'ALL'),
+                    mock.call('sync'),
+                    mock.call('tgtadm', '--lld', 'iscsi', '--mode', 'target',
+                              '--op', 'delete', '--tid', '1')]
+        iscsi.clean_up(self.fake_dev)
+        mock_execute.assert_has_calls(expected)
+
 
 @mock.patch.object(iscsi.rtslib_fb, 'RTSRoot', autospec=True)
 class TestISCSIExtensionCleanUp(base.IronicAgentTest):
diff --git a/releasenotes/notes/fix-iscsi-teardown-handling-0df2345318d3c843.yaml b/releasenotes/notes/fix-iscsi-teardown-handling-0df2345318d3c843.yaml
new file mode 100644
index 000000000..070f2cc0a
--- /dev/null
+++ b/releasenotes/notes/fix-iscsi-teardown-handling-0df2345318d3c843.yaml
@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    Fixes an issue in fallback error handling where native iSCSI controls
+    are unavailable due to the composition of the IPA ramdisk and where direct
+    ``tgtadm`` commands also fails.
+
+    Before fallback error handling was added, the teardown was skipped
+    completely in the event of the native iSCSI controls being unavailable.
+    The end user behavior is now as it was previously prior to the fallback
+    error handling being added, but IPA will still continue to attempt to
+    clean up the iSCSI session.