Do not try to guess EFI partition path by its number

The logic of adding a partition number to the device path does not work
for devicemapper devices (e.g. a multipath storage device).

Conflicts:
	ironic_python_agent/efi_utils.py
	ironic_python_agent/tests/unit/extensions/test_image.py
	ironic_python_agent/tests/unit/test_efi_utils.py

Change-Id: I9a445e847d282c50adfa4bad5e7136776861005d
(cherry picked from commit f09f6c9f1a09c7062d0450b3e0a4d3164fd53f7f)

4 files changed, 117 insertions, 48 deletions

diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 0ec6de9d..3a37cde8 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -323,12 +323,43 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
         # Update the nvram using efibootmgr
         # https://linux.die.net/man/8/efibootmgr
         utils.execute('efibootmgr', '-v', '-c', '-d', device,
-                      '-p', efi_partition, '-w', '-L', label,
+                      '-p', str(efi_partition), '-w', '-L', label,
                       '-l', v_efi_bl_path)
         # Increment the ID in case the loop runs again.
         label_id += 1
 
 
+def get_partition_path_by_number(device, part_num):
+    """Get partition path (/dev/something) by a partition number on device.
+
+    Only works for GPT partition table.
+    """
+    uuid = None
+    partinfo, _ = utils.execute('sgdisk', '-i', str(part_num), device,
+                                use_standard_locale=True)
+    for line in partinfo.splitlines():
+        if not line.strip():
+            continue
+
+        try:
+            field, value = line.rsplit(':', 1)
+        except ValueError:
+            LOG.warning('Invalid sgdisk line: %s', line)
+            continue
+
+        if 'partition unique guid' in field.lower():
+            uuid = value.strip().lower()
+            LOG.debug('GPT partition number %s on device %s has UUID %s',
+                      part_num, device, uuid)
+            break
+
+    if uuid is not None:
+        return _get_partition(device, uuid)
+    else:
+        LOG.warning('No UUID information provided in sgdisk output for '
+                    'partition %s on device %s', part_num, device)
+
+
 def _manage_uefi(device, efi_system_part_uuid=None):
     """Manage the device looking for valid efi bootloaders to update the nvram.
 
@@ -344,6 +375,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
     """
     efi_partition_mount_point = None
     efi_mounted = False
+    efi_partition = None
     LOG.debug('Attempting UEFI loader autodetection and NVRAM record setup.')
     try:
         # Force UEFI to rescan the device.
@@ -351,17 +383,27 @@ 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)
+        efi_part_num = utils.get_efi_part_on_device(device)
+        if efi_part_num is not None:
+            efi_partition = get_partition_path_by_number(device, efi_part_num)
+
         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 = _get_partition(device, uuid=efi_system_part_uuid)
             try:
-                efi_partition = int(partition.replace(device, ""))
+                efi_part_num = int(efi_partition.replace(device, ""))
             except ValueError:
                 # NVMe Devices get a partitioning scheme that is different from
                 # traditional block devices like SCSI/SATA
-                efi_partition = int(partition.replace(device + 'p', ""))
+                try:
+                    efi_part_num = int(efi_partition.replace(device + 'p', ""))
+                except ValueError as exc:
+                    # At least provide a reasonable error message if the device
+                    # does not follow this procedure.
+                    raise errors.DeviceNotFound(
+                        "Cannot detect the partition number of the device "
+                        "%s: %s" % (device, exc))
 
         if not efi_partition:
             # NOTE(dtantsur): we cannot have a valid EFI deployment without an
@@ -378,15 +420,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
         if not os.path.exists(efi_partition_mount_point):
             os.makedirs(efi_partition_mount_point)
 
-        # The mount needs the device with the partition, in case the
-        # device ends with a digit we add a `p` and the partition number we
-        # 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)
-            utils.execute('mount', efi_device_part, efi_partition_mount_point)
+        utils.execute('mount', efi_partition, efi_partition_mount_point)
         efi_mounted = True
 
         valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point)
@@ -398,7 +432,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
 
         if not hardware.is_md_device(device):
             efi_devices = [device]
-            efi_partition_numbers = [efi_partition]
+            efi_partition_numbers = [efi_part_num]
             efi_label_suffix = ''
         else:
             # umount to allow for signature removal (to avoid confusion about
@@ -409,7 +443,7 @@ def _manage_uefi(device, efi_system_part_uuid=None):
 
             holders = hardware.get_holder_disks(device)
             efi_md_device = prepare_boot_partitions_for_softraid(
-                device, holders, efi_device_part, target_boot_mode='uefi'
+                device, holders, efi_partition, target_boot_mode='uefi'
             )
             efi_devices = hardware.get_component_devices(efi_md_device)
             efi_partition_numbers = []
@@ -426,12 +460,12 @@ def _manage_uefi(device, efi_system_part_uuid=None):
             efi_label_suffix = "(RAID, part%s)"
 
             # remount for _run_efibootmgr
-            utils.execute('mount', efi_device_part, efi_partition_mount_point)
+            utils.execute('mount', efi_partition, efi_partition_mount_point)
             efi_mounted = True
 
         efi_dev_part = zip(efi_devices, efi_partition_numbers)
         for i, (efi_dev, efi_part) in enumerate(efi_dev_part):
-            LOG.debug("Calling efibootmgr with dev %s part %s",
+            LOG.debug("Calling efibootmgr with dev %s partition number %s",
                       efi_dev, efi_part)
             if efi_label_suffix:
                 # NOTE (arne_wiebalck): uniqify the labels to prevent
diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py
index b36110b2..9ecdd6f1 100644
--- a/ironic_python_agent/tests/unit/extensions/test_image.py
+++ b/ironic_python_agent/tests/unit/extensions/test_image.py
@@ -27,6 +27,7 @@ from ironic_python_agent.extensions import image
 from ironic_python_agent import hardware
 from ironic_python_agent import raid_utils
 from ironic_python_agent.tests.unit import base
+from ironic_python_agent.tests.unit.samples import hardware_samples
 from ironic_python_agent import utils
 
 
@@ -218,14 +219,14 @@ class TestImageExtension(base.IronicAgentTest):
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=False)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__uefi_bootloader_given_partition(
-            self, mkdir_mock, mock_utils_efi_part, mock_partition,
+            self, mkdir_mock, mock_utils_efi_part, mock_get_partition,
             mock_efi_bl, mock_execute, mock_dispatch):
         mock_dispatch.side_effect = [
             self.fake_dev, hardware.BootInfo(current_boot_mode='uefi')
         ]
-        mock_partition.side_effect = [self.fake_dev, self.fake_efi_system_part]
+        mock_get_partition.return_value = self.fake_efi_system_part
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
-        mock_utils_efi_part.return_value = '1'
+        mock_utils_efi_part.return_value = None
 
         mock_execute.side_effect = iter([('', ''), ('', ''),
                                          ('', ''), ('', ''),
@@ -262,16 +263,16 @@ class TestImageExtension(base.IronicAgentTest):
     @mock.patch.object(hardware, 'is_md_device', lambda *_: False)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__uefi_bootloader_find_partition(
-            self, mkdir_mock, mock_utils_efi_part, mock_partition,
+            self, mkdir_mock, mock_utils_efi_part, mock_get_part_path,
             mock_efi_bl, mock_execute, mock_dispatch):
         mock_dispatch.side_effect = [
             self.fake_dev, hardware.BootInfo(current_boot_mode='uefi')
         ]
-        mock_partition.return_value = self.fake_dev
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_utils_efi_part.return_value = '1'
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
         mock_execute.side_effect = iter([('', ''), ('', ''),
@@ -309,16 +310,16 @@ class TestImageExtension(base.IronicAgentTest):
     @mock.patch.object(hardware, 'is_md_device', lambda *_: False)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__uefi_bootloader_with_entry_removal(
-            self, mkdir_mock, mock_utils_efi_part, mock_partition,
+            self, mkdir_mock, mock_utils_efi_part, mock_get_part_path,
             mock_efi_bl, mock_execute, mock_dispatch):
         mock_dispatch.side_effect = [
             self.fake_dev, hardware.BootInfo(current_boot_mode='uefi')
         ]
-        mock_partition.return_value = self.fake_dev
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_utils_efi_part.return_value = '1'
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
         stdout_msg = """
@@ -366,16 +367,16 @@ Boot0002 VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
     @mock.patch.object(hardware, 'is_md_device', lambda *_: False)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__uefi_bootloader_with_entry_removal_lenovo(
-            self, mkdir_mock, mock_utils_efi_part, mock_partition,
+            self, mkdir_mock, mock_utils_efi_part, mock_get_part_path,
             mock_efi_bl, mock_execute, mock_dispatch):
         mock_dispatch.side_effect = [
             self.fake_dev, hardware.BootInfo(current_boot_mode='uefi')
         ]
-        mock_partition.return_value = self.fake_dev
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_utils_efi_part.return_value = '1'
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
         # NOTE(TheJulia): This test string was derived from a lenovo SR650
@@ -428,16 +429,16 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
     @mock.patch.object(hardware, 'is_md_device', lambda *_: False)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__add_multi_bootloaders(
-            self, mkdir_mock, mock_utils_efi_part, mock_partition,
+            self, mkdir_mock, mock_utils_efi_part, mock_get_part_path,
             mock_efi_bl, mock_execute, mock_dispatch):
         mock_dispatch.side_effect = [
             self.fake_dev, hardware.BootInfo(current_boot_mode='uefi')
         ]
-        mock_partition.return_value = self.fake_dev
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_utils_efi_part.return_value = '1'
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI',
                                     'WINDOWS/system32/winload.efi']
@@ -2326,14 +2327,14 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__manage_uefi(self, mkdir_mock, mock_utils_efi_part,
-                          mock_get_part_uuid, mock_efi_bl, mock_is_md_device,
+                          mock_get_part_path, mock_efi_bl, mock_is_md_device,
                           mock_execute, mock_dispatch):
         mock_utils_efi_part.return_value = '1'
-        mock_get_part_uuid.return_value = self.fake_dev
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_is_md_device.return_value = False
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
 
@@ -2366,15 +2367,15 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__manage_uefi_found_csv(self, mkdir_mock, mock_utils_efi_part,
-                                    mock_get_part_uuid, mock_efi_bl,
+                                    mock_get_part_path, mock_efi_bl,
                                     mock_is_md_device, mock_execute,
                                     mock_dispatch):
         mock_utils_efi_part.return_value = '1'
-        mock_get_part_uuid.return_value = self.fake_dev
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_is_md_device.return_value = False
         mock_efi_bl.return_value = ['EFI/vendor/BOOTX64.CSV']
 
@@ -2427,15 +2428,15 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__manage_uefi_nvme_device(self, mkdir_mock, mock_utils_efi_part,
-                                      mock_get_part_uuid, mock_efi_bl,
+                                      mock_get_part_path, mock_efi_bl,
                                       mock_is_md_device, mock_execute,
                                       mock_dispatch):
         mock_utils_efi_part.return_value = '1'
-        mock_get_part_uuid.return_value = '/dev/fakenvme0p1'
+        mock_get_part_path.return_value = '/dev/fakenvme0p1'
         mock_is_md_device.return_value = False
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
 
@@ -2468,15 +2469,15 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
     @mock.patch.object(os.path, 'exists', lambda *_: False)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__manage_uefi_wholedisk(
             self, mkdir_mock, mock_utils_efi_part,
-            mock_get_part_uuid, mock_efi_bl, mock_is_md_device,
+            mock_get_part_path, mock_efi_bl, mock_is_md_device,
             mock_execute, mock_dispatch):
         mock_utils_efi_part.return_value = '1'
-        mock_get_part_uuid.side_effect = Exception
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_is_md_device.return_value = False
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
 
@@ -2514,16 +2515,16 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
     @mock.patch.object(hardware, 'get_holder_disks', autospec=True)
     @mock.patch.object(hardware, 'is_md_device', autospec=True)
     @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
-    @mock.patch.object(image, '_get_partition', autospec=True)
+    @mock.patch.object(image, 'get_partition_path_by_number', autospec=True)
     @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
     @mock.patch.object(os, 'makedirs', autospec=True)
     def test__manage_uefi_software_raid(
             self, mkdir_mock, mock_utils_efi_part,
-            mock_get_part_uuid, mock_efi_bl, mock_is_md_device,
+            mock_get_part_path, mock_efi_bl, mock_is_md_device,
             mock_get_holder_disks, mock_prepare, mock_get_component_devices,
             mock_execute, mock_dispatch):
         mock_utils_efi_part.return_value = '1'
-        mock_get_part_uuid.side_effect = Exception
+        mock_get_part_path.return_value = self.fake_efi_system_part
         mock_efi_bl.return_value = ['EFI/BOOT/BOOTX64.EFI']
         mock_is_md_device.return_value = True
         mock_get_holder_disks.return_value = ['/dev/sda', '/dev/sdb']
@@ -2671,3 +2672,21 @@ Boot0002: VENDMAGIC FvFile(9f3c6294-bf9b-4208-9808-be45dfc34b51)
             mock_open.side_effect = OSError('boom')
             image._append_uefi_to_fstab(
                 self.fake_dir, 'abcd-efgh')
+
+
+@mock.patch.object(image, '_get_partition', autospec=True)
+@mock.patch.object(utils, 'execute', autospec=True)
+class TestGetPartitionPathByNumber(base.IronicAgentTest):
+    def test_ok(self, mock_execute, mock_get_partition):
+        mock_execute.return_value = (hardware_samples.SGDISK_INFO_TEMPLATE, '')
+        mock_get_partition.return_value = '/dev/fake1'
+        result = image.get_partition_path_by_number('/dev/fake', 1)
+        self.assertEqual('/dev/fake1', result)
+        mock_execute.assert_called_once_with('sgdisk', '-i', '1', '/dev/fake',
+                                             use_standard_locale=True)
+
+    def test_broken(self, mock_execute, mock_get_partition):
+        mock_execute.return_value = ('I am a teaport', '')
+        self.assertIsNone(
+            image.get_partition_path_by_number('/dev/fake', 1))
+        mock_get_partition.assert_not_called()
diff --git a/ironic_python_agent/tests/unit/samples/hardware_samples.py b/ironic_python_agent/tests/unit/samples/hardware_samples.py
index 8cc9b1ea..660c61b5 100644
--- a/ironic_python_agent/tests/unit/samples/hardware_samples.py
+++ b/ironic_python_agent/tests/unit/samples/hardware_samples.py
@@ -1500,3 +1500,14 @@ NVME_CLI_INFO_TEMPLATE_FORMAT_UNSUPPORTED = ("""
   ]
 }
 """)
+
+
+SGDISK_INFO_TEMPLATE = ("""
+Partition GUID code: C12A7328-F81F-11D2-BA4B-00A0C93EC93B (EFI system partition)
+Partition unique GUID: FAED7408-6D92-4FC6-883B-9069E2274ECA
+First sector: 2048 (at 1024.0 KiB)
+Last sector: 1050623 (at 513.0 MiB)
+Partition size: 1048576 sectors (512.0 MiB)
+Attribute flags: 0000000000000000
+Partition name: 'EFI System Partition'
+""")  # noqa
diff --git a/releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml b/releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml
new file mode 100644
index 00000000..f1a7924f
--- /dev/null
+++ b/releasenotes/notes/efi-partuuid-5fe933a462eeede1.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes configuring UEFI boot when the EFI partition is located on a
+    devicemapper device.