Merge "Refactor efi_utils for easier maintaining and debugging"

ironic_python_agent/efi_utils.py

@@ -12,7 +12,7 @@
 
 import os
 import re
-import shutil
+import sys
 import tempfile
 
 from ironic_lib import disk_utils
@@ -45,11 +45,10 @@ def manage_uefi(device, efi_system_part_uuid=None):
     efi_partition_mount_point = None
     efi_mounted = False
     LOG.debug('Attempting UEFI loader autodetection and NVRAM record setup.')
-    try:
+
     # Force UEFI to rescan the device.
     utils.rescan_device(device)
 
-        local_path = tempfile.mkdtemp()
     # Trust the contents on the disk in the event of a whole disk image.
     efi_partition = disk_utils.find_efi_partition(device)
     if efi_partition:
@@ -78,6 +77,7 @@ def manage_uefi(device, efi_system_part_uuid=None):
             "(which is often the case for whole disk images). "
             "Are you using a UEFI-compatible image?" % device)
 
+    local_path = tempfile.mkdtemp()
     efi_partition_mount_point = os.path.join(local_path, "boot/efi")
     if not os.path.exists(efi_partition_mount_point):
         os.makedirs(efi_partition_mount_point)
@@ -87,9 +87,10 @@ def manage_uefi(device, efi_system_part_uuid=None):
     # found, otherwise we just join the device and the partition number
     if device[-1].isdigit():
         efi_device_part = '{}p{}'.format(device, efi_partition)
-            utils.execute('mount', efi_device_part, efi_partition_mount_point)
     else:
         efi_device_part = '{}{}'.format(device, efi_partition)
+
+    try:
         utils.execute('mount', efi_device_part, efi_partition_mount_point)
         efi_mounted = True
 
@@ -149,34 +150,28 @@ def manage_uefi(device, efi_system_part_uuid=None):
         return True
 
     except processutils.ProcessExecutionError as e:
-        error_msg = ('Could not verify uefi on device %(dev)s, '
+        error_msg = ('Could not configure UEFI boot on device %(dev)s: %(err)s'
-                     'failed with %(err)s.' % {'dev': device, 'err': e})
+                     % {'dev': device, 'err': e})
-        LOG.error(error_msg)
+        LOG.exception(error_msg)
         raise errors.CommandExecutionError(error_msg)
     finally:
-        LOG.debug('Executing _manage_uefi clean-up.')
-        umount_warn_msg = "Unable to umount %(local_path)s. Error: %(error)s"
-
-        try:
         if efi_mounted:
+            try:
                 utils.execute('umount', efi_partition_mount_point,
                               attempts=3, delay_on_retry=True)
             except processutils.ProcessExecutionError as e:
                 error_msg = ('Umounting efi system partition failed. '
                              'Attempted 3 times. Error: %s' % e)
                 LOG.error(error_msg)
+                # Do not mask the actual failure, if any
+                if sys.exc_info()[0] is None:
                     raise errors.CommandExecutionError(error_msg)
 
             else:
-            # If umounting the binds succeed then we can try to delete it
                 try:
                     utils.execute('sync')
                 except processutils.ProcessExecutionError as e:
-                LOG.warning(umount_warn_msg, {'path': local_path, 'error': e})
+                    LOG.warning('Unable to sync the local disks: %s', e)
-            else:
-                # After everything is umounted we can then remove the
-                # temporary directory
-                shutil.rmtree(local_path)
 
 
 # NOTE(TheJulia): Do not add bootia32.csv to this list. That is 32bit

ironic_python_agent/tests/unit/test_efi_utils.py

@@ -16,6 +16,7 @@ import tempfile
 from unittest import mock
 
 from ironic_lib import disk_utils
+from oslo_concurrency import processutils
 
 from ironic_python_agent import efi_utils
 from ironic_python_agent import errors
@@ -371,3 +372,100 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
         mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
         mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
         mock_execute.assert_has_calls(expected)
+
+    @mock.patch.object(os.path, 'exists', lambda *_: False)
+    @mock.patch.object(hardware, 'is_md_device', autospec=True)
+    @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True)
+    @mock.patch.object(os, 'makedirs', autospec=True)
+    def test_failure(self, mkdir_mock, mock_efi_bl, mock_is_md_device,
+                     mock_utils_efi_part, mock_get_part_uuid, mock_execute,
+                     mock_rescan):
+        mock_utils_efi_part.return_value = {'number': '1'}
+        mock_get_part_uuid.return_value = self.fake_dev
+        mock_is_md_device.return_value = False
+
+        mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
+
+        mock_execute.side_effect = processutils.ProcessExecutionError('boom')
+
+        self.assertRaisesRegex(errors.CommandExecutionError, 'boom',
+                               efi_utils.manage_uefi,
+                               self.fake_dev, self.fake_root_uuid)
+        mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_efi_bl.assert_not_called()
+        mock_execute.assert_called_once_with(
+            'mount', self.fake_efi_system_part, self.fake_dir + '/boot/efi')
+        mock_rescan.assert_called_once_with(self.fake_dev)
+
+    @mock.patch.object(os.path, 'exists', lambda *_: False)
+    @mock.patch.object(hardware, 'is_md_device', autospec=True)
+    @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True)
+    @mock.patch.object(os, 'makedirs', autospec=True)
+    def test_failure_after_mount(self, mkdir_mock, mock_efi_bl,
+                                 mock_is_md_device, mock_utils_efi_part,
+                                 mock_get_part_uuid, mock_execute,
+                                 mock_rescan):
+        mock_utils_efi_part.return_value = {'number': '1'}
+        mock_get_part_uuid.return_value = self.fake_dev
+        mock_is_md_device.return_value = False
+
+        mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
+
+        mock_execute.side_effect = [
+            ('', ''),
+            processutils.ProcessExecutionError('boom'),
+            ('', ''),
+            ('', ''),
+        ]
+
+        expected = [mock.call('mount', self.fake_efi_system_part,
+                              self.fake_dir + '/boot/efi'),
+                    mock.call('efibootmgr', '-v'),
+                    mock.call('umount', self.fake_dir + '/boot/efi',
+                              attempts=3, delay_on_retry=True),
+                    mock.call('sync')]
+
+        self.assertRaisesRegex(errors.CommandExecutionError, 'boom',
+                               efi_utils.manage_uefi,
+                               self.fake_dev, self.fake_root_uuid)
+        mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_execute.assert_has_calls(expected)
+        self.assertEqual(4, mock_execute.call_count)
+        mock_rescan.assert_called_once_with(self.fake_dev)
+
+    @mock.patch.object(os.path, 'exists', lambda *_: False)
+    @mock.patch.object(hardware, 'is_md_device', autospec=True)
+    @mock.patch.object(efi_utils, '_get_efi_bootloaders', autospec=True)
+    @mock.patch.object(os, 'makedirs', autospec=True)
+    def test_failure_after_failure(self, mkdir_mock, mock_efi_bl,
+                                   mock_is_md_device, mock_utils_efi_part,
+                                   mock_get_part_uuid, mock_execute,
+                                   mock_rescan):
+        mock_utils_efi_part.return_value = {'number': '1'}
+        mock_get_part_uuid.return_value = self.fake_dev
+        mock_is_md_device.return_value = False
+
+        mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
+
+        mock_execute.side_effect = [
+            ('', ''),
+            processutils.ProcessExecutionError('boom'),
+            processutils.ProcessExecutionError('no umount'),
+            ('', ''),
+        ]
+
+        expected = [mock.call('mount', self.fake_efi_system_part,
+                              self.fake_dir + '/boot/efi'),
+                    mock.call('efibootmgr', '-v'),
+                    mock.call('umount', self.fake_dir + '/boot/efi',
+                              attempts=3, delay_on_retry=True)]
+
+        self.assertRaisesRegex(errors.CommandExecutionError, 'boom',
+                               efi_utils.manage_uefi,
+                               self.fake_dev, self.fake_root_uuid)
+        mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
+        mock_execute.assert_has_calls(expected)
+        self.assertEqual(3, mock_execute.call_count)
+        mock_rescan.assert_called_once_with(self.fake_dev)