Merge "Fix error message with UEFI-incompatible images"

ironic_python_agent/extensions/image.py

@@ -280,6 +280,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
 
     :param device: the device to be checked.
     :param efi_system_part_uuid: efi partition uuid.
+    :raises: DeviceNotFound if the efi partition cannot be found.
     :return: True - if it founds any efi bootloader and the nvram was updated
              using the efibootmgr.
              False - if no efi bootloader is found.
@@ -295,38 +296,45 @@ def _manage_uefi(device, efi_system_part_uuid=None):
         local_path = tempfile.mkdtemp()
         # Trust the contents on the disk in the event of a whole disk image.
         efi_partition = utils.get_efi_part_on_device(device)
-        if not efi_partition:
+        if not efi_partition and efi_system_part_uuid:
             # _get_partition returns <device>+<partition> and we only need the
             # partition number
             partition = _get_partition(device, uuid=efi_system_part_uuid)
             efi_partition = int(partition.replace(device, ""))
 
-        if efi_partition:
+        if not efi_partition:
-            efi_partition_mount_point = os.path.join(local_path, "boot/efi")
+            # NOTE(dtantsur): we cannot have a valid EFI deployment without an
-            if not os.path.exists(efi_partition_mount_point):
+            # EFI partition at all. This code path is easily hit when using an
-                os.makedirs(efi_partition_mount_point)
+            # image that is not UEFI compatible (which sadly applies to most
+            # cloud images out there, with a nice exception of Ubuntu).
+            raise errors.DeviceNotFound(
+                "No EFI partition could be detected on device %s and "
+                "EFI partition UUID has not been recorded during deployment "
+                "(which is often the case for whole disk images). "
+                "Are you using a UEFI-compatible image?" % device)
 
-            # The mount needs the device with the partition, in case the
+        efi_partition_mount_point = os.path.join(local_path, "boot/efi")
-            # device ends with a digit we add a `p` and the partition number we
+        if not os.path.exists(efi_partition_mount_point):
-            # found, otherwise we just join the device and the partition number
+            os.makedirs(efi_partition_mount_point)
-            if device[-1].isdigit():
+
-                efi_device_part = '{}p{}'.format(device, efi_partition)
+        # The mount needs the device with the partition, in case the
-                utils.execute('mount', efi_device_part,
+        # device ends with a digit we add a `p` and the partition number we
-                              efi_partition_mount_point)
+        # found, otherwise we just join the device and the partition number
-            else:
+        if device[-1].isdigit():
-                efi_device_part = '{}{}'.format(device, efi_partition)
+            efi_device_part = '{}p{}'.format(device, efi_partition)
-                utils.execute('mount', efi_device_part,
+            utils.execute('mount', efi_device_part, efi_partition_mount_point)
-                              efi_partition_mount_point)
-            efi_mounted = True
         else:
-            # If we can't find the partition we need to decide what should
+            efi_device_part = '{}{}'.format(device, efi_partition)
-            # happen
+            utils.execute('mount', efi_device_part, efi_partition_mount_point)
-            return False
+        efi_mounted = True
+
         valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point)
         if valid_efi_bootloaders:
             _run_efibootmgr(valid_efi_bootloaders, device, efi_partition)
             return True
         else:
+            # NOTE(dtantsur): if we have an empty EFI partition, try to use
+            # grub-install to populate it.
             return False
 
     except processutils.ProcessExecutionError as e:

ironic_python_agent/tests/unit/extensions/test_image.py

@@ -2107,6 +2107,16 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
                                        mock_get_part_uuid,
                                        mock_execute, mock_dispatch):
         mock_utils_efi_part.return_value = None
+        self.assertRaises(errors.DeviceNotFound,
+                          image._manage_uefi, self.fake_dev, None)
+        self.assertFalse(mock_get_part_uuid.called)
+
+    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
+    def test__manage_uefi_empty_partition_by_uuid(self, mock_utils_efi_part,
+                                                  mock_get_part_uuid,
+                                                  mock_execute, mock_dispatch):
+        mock_utils_efi_part.return_value = None
         mock_get_part_uuid.return_value = self.fake_root_part
         result = image._manage_uefi(self.fake_dev, self.fake_root_uuid)
         self.assertFalse(result)

releasenotes/notes/uefi-images-38c8536db189ffc1.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Provides a more specific error message if a UEFI-incompatible image
+    is used in the UEFI mode.