Prevent un-needed iscsi cleanup

When we added software raid support, we started calling bootloader
installation. As time went on, we ehnanced that code path for non
RAID cases in order to ensure that UEFI nvram was setup
for the instance to boot properly.

Somewhere in this process, we missed a possible failure case where
the iscsi client tgtadm may return failures. Obviously, the correct
path is to not call iscsi teardown if we don't need to.

Since it was always semi-opportunistic teardown, we can't blindly
catch any error, and if we started iSCSI and failed to tear the
connection down, we might want to still fail, so this change
moves the logic over to use a flag on the agent object which
one extension to set the flag and the other to read it and take
action based upon that.

Change-Id: Id3b1ae5e59282f4109f6246d5614d44c93aefa7c
Story: 2007937
Task: 40395

6 files changed, 47 insertions, 2 deletions

diff --git a/ironic_python_agent/agent.py b/ironic_python_agent/agent.py
index 207b9a45..c356f56e 100644
--- a/ironic_python_agent/agent.py
+++ b/ironic_python_agent/agent.py
@@ -213,6 +213,7 @@ class IronicPythonAgent(base.ExecuteCommandMixin):
         # in the event of long running ramdisks where the conductor
         # got upgraded somewhere along the way.
         self.agent_token_required = cfg.CONF.agent_token_required
+        self.iscsi_started = False
 
     def get_status(self):
         """Retrieve a serializable status.
diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 4a135331..512264ec 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -733,7 +733,8 @@ class ImageExtension(base.BaseAgentExtension):
 
         """
         device = hardware.dispatch_to_managers('get_os_install_device')
-        iscsi.clean_up(device)
+        if self.agent.iscsi_started:
+            iscsi.clean_up(device)
         boot = hardware.dispatch_to_managers('get_boot_info')
         # FIXME(arne_wiebalck): make software RAID work with efibootmgr
         if (boot.current_boot_mode == 'uefi'
diff --git a/ironic_python_agent/extensions/iscsi.py b/ironic_python_agent/extensions/iscsi.py
index 21dfba39..1b3b308c 100644
--- a/ironic_python_agent/extensions/iscsi.py
+++ b/ironic_python_agent/extensions/iscsi.py
@@ -210,7 +210,8 @@ class ISCSIExtension(base.BaseAgentExtension):
         else:
             _start_lio(iqn, portal_port, device)
             LOG.debug('Linux-IO configuration: %s', rts_root.dump())
-
+        # Mark iscsi as previously started
+        self.agent.iscsi_started = True
         LOG.info('Created iSCSI target with iqn %(iqn)s, portal port %(port)d,'
                  ' on device %(dev)s using %(method)s',
                  {'iqn': iqn, 'port': portal_port, 'dev': device,
diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py
index 154edbe0..02d6fc66 100644
--- a/ironic_python_agent/tests/unit/extensions/test_image.py
+++ b/ironic_python_agent/tests/unit/extensions/test_image.py
@@ -46,6 +46,8 @@ class TestImageExtension(base.IronicAgentTest):
         self.fake_efi_system_part_uuid = '45AB-2312'
         self.fake_prep_boot_part_uuid = '76937797-3253-8843-999999999999'
         self.fake_dir = '/tmp/fake-dir'
+        self.agent_extension.agent = mock.Mock()
+        self.agent_extension.agent.iscsi_started = True
 
     @mock.patch.object(iscsi, 'clean_up', autospec=True)
     @mock.patch.object(image, '_install_grub2', autospec=True)
@@ -323,6 +325,31 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
         )
         mock_iscsi_clean.assert_called_once_with(self.fake_dev)
 
+    @mock.patch.object(iscsi, 'clean_up', autospec=True)
+    @mock.patch.object(image, '_install_grub2', autospec=True)
+    def test__install_bootloader_prep_no_iscsi(
+            self, mock_grub2, mock_iscsi_clean,
+            mock_execute, mock_dispatch):
+        self.agent_extension.agent.iscsi_started = False
+        mock_dispatch.side_effect = [
+            self.fake_dev, hardware.BootInfo(current_boot_mode='bios')
+        ]
+        self.agent_extension.install_bootloader(
+            root_uuid=self.fake_root_uuid,
+            efi_system_part_uuid=None,
+            prep_boot_part_uuid=self.fake_prep_boot_part_uuid).join()
+        mock_dispatch.assert_any_call('get_os_install_device')
+        mock_dispatch.assert_any_call('get_boot_info')
+        self.assertEqual(2, mock_dispatch.call_count)
+        mock_grub2.assert_called_once_with(
+            self.fake_dev,
+            root_uuid=self.fake_root_uuid,
+            efi_system_part_uuid=None,
+            prep_boot_part_uuid=self.fake_prep_boot_part_uuid,
+            target_boot_mode='bios'
+        )
+        mock_iscsi_clean.assert_not_called()
+
     @mock.patch.object(hardware, 'is_md_device', lambda *_: False)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(iscsi, 'clean_up', autospec=True)
diff --git a/ironic_python_agent/tests/unit/extensions/test_iscsi.py b/ironic_python_agent/tests/unit/extensions/test_iscsi.py
index e914d4aa..131ad075 100644
--- a/ironic_python_agent/tests/unit/extensions/test_iscsi.py
+++ b/ironic_python_agent/tests/unit/extensions/test_iscsi.py
@@ -26,6 +26,9 @@ from ironic_python_agent import utils
 
 
 class FakeAgent(object):
+
+    iscsi_started = False
+
     def get_node_uuid(self):
         return 'my_node_uuid'
 
@@ -48,8 +51,11 @@ class TestISCSIExtensionTgt(base.IronicAgentTest):
                                 mock_destroy):
         mock_dispatch.return_value = self.fake_dev
         mock_execute.return_value = ('', '')
+        self.assertFalse(self.agent_extension.agent.iscsi_started)
+
         result = self.agent_extension.start_iscsi_target(iqn=self.fake_iqn)
 
+        self.assertTrue(self.agent_extension.agent.iscsi_started)
         expected = [mock.call('tgtd'),
                     mock.call('tgtadm', '--lld', 'iscsi', '--mode',
                               'target', '--op', 'show', attempts=10),
diff --git a/releasenotes/notes/prevent-needless-iscsi-cleanup-f8d602c0abc7e8ba.yaml b/releasenotes/notes/prevent-needless-iscsi-cleanup-f8d602c0abc7e8ba.yaml
new file mode 100644
index 00000000..25f6c443
--- /dev/null
+++ b/releasenotes/notes/prevent-needless-iscsi-cleanup-f8d602c0abc7e8ba.yaml
@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Fixes an issue with the ironic-python-agent where we would call to
+    setup the bootloader, which is necessary with software raid, but also
+    attempt to clean up iSCSI. This can cause issues when using the ``direct``
+    ``deploy_interface``. Now the agent will only clean up iSCSI connections
+    if iSCSI was explicitly started. For more information, please see
+    `story 2007937 <https://storyboard.openstack.org/#!/story/2007937>`_.