From 4746f09b3dfa765684e6e20cf4b9742e1bb3bd62 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Mon, 20 Jul 2020 14:24:06 -0700 Subject: [PATCH] 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 (cherry picked from commit 2a56ee03b6ffb2d8c8f5ff553e90ecf6ee07f9af) --- ironic_python_agent/agent.py | 1 + ironic_python_agent/extensions/image.py | 3 ++- ironic_python_agent/extensions/iscsi.py | 3 ++- .../tests/unit/extensions/test_image.py | 27 +++++++++++++++++++ .../tests/unit/extensions/test_iscsi.py | 6 +++++ ...edless-iscsi-cleanup-f8d602c0abc7e8ba.yaml | 9 +++++++ 6 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/prevent-needless-iscsi-cleanup-f8d602c0abc7e8ba.yaml diff --git a/ironic_python_agent/agent.py b/ironic_python_agent/agent.py index dfe77b04b..64b5009fe 100644 --- a/ironic_python_agent/agent.py +++ b/ironic_python_agent/agent.py @@ -219,6 +219,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 5b54df7d7..d25b8e620 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -710,7 +710,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 21dfba392..1b3b308cb 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 11ef29f15..e6db55543 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 e914d4aa7..131ad0751 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 000000000..25f6c443d --- /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 `_.