Merge "Create fstab entry with appropriate label"

6 files changed, 135 insertions, 10 deletions

diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 892aa288..de880f84 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -485,17 +485,58 @@ def _try_preserve_efi_assets(device, path,
 def _append_uefi_to_fstab(fs_path, efi_system_part_uuid):
     """Append the efi partition id to the filesystem table.
 
-    :param fs_path:
-    :param efi_system_part_uuid:
+    :param fs_path: The path to the filesystem.
+    :param efi_system_part_uuid: uuid to use to try and find the
+                                 partition. Warning: this may be
+                                 a partition uuid or a actual uuid.
     """
     fstab_file = os.path.join(fs_path, 'etc/fstab')
     if not os.path.exists(fstab_file):
         return
     try:
-        fstab_string = ("UUID=%s\t/boot/efi\tvfat\tumask=0077\t"
-                        "0\t1\n") % efi_system_part_uuid
+        # Collect all of the block devices so we appropriately match UUID
+        # or PARTUUID into an fstab entry.
+        block_devs = hardware.list_all_block_devices(block_type='part')
+
+        # Default to uuid, but if we find a partuuid instead, that is okay,
+        # we just need to know later on.
+        fstab_label = None
+        for bdev in block_devs:
+            # Check UUID first
+            if bdev.uuid and efi_system_part_uuid in bdev.uuid:
+                LOG.debug('Identified block device %(dev)s UUID %(uuid)s '
+                          'for UEFI boot. Proceeding with fstab update using '
+                          'a UUID.',
+                          {'dev': bdev.name,
+                           'uuid': efi_system_part_uuid})
+                # What we have works, and is correct, we can break the loop
+                fstab_label = 'UUID'
+                break
+            # Fallback to PARTUUID, since we don't know if the provided
+            # UUID matches a PARTUUID, or UUID field, and the fstab entry
+            # needs to match it.
+            if bdev.partuuid and efi_system_part_uuid in bdev.partuuid:
+                LOG.debug('Identified block device %(dev)s partition UUID '
+                          '%(uuid)s for UEFI boot. Proceeding with fstab '
+                          'update using a PARTUUID.',
+                          {'dev': bdev.name,
+                           'uuid': efi_system_part_uuid})
+                fstab_label = 'PARTUUID'
+                break
+
+        if not fstab_label:
+            # Fallback to prior behavior, which should generally be correct.
+            LOG.warning('Falling back to fstab entry addition label of UUID. '
+                        'We could not identify which UUID or PARTUUID '
+                        'identifier label should be used, thus UUID will be '
+                        'used.')
+            fstab_label = 'UUID'
+
+        fstab_string = ("%s=%s\t/boot/efi\tvfat\tumask=0077\t"
+                        "0\t1\n") % (fstab_label, efi_system_part_uuid)
         with open(fstab_file, "r+") as fstab:
-            if efi_system_part_uuid not in fstab.read():
+            already_present_string = fstab_label + '=' + efi_system_part_uuid
+            if already_present_string not in fstab.read():
                 fstab.writelines(fstab_string)
     except (OSError, EnvironmentError, IOError) as exc:
         LOG.debug('Failed to add entry to /etc/fstab. Error %s', exc)
diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py
index ceafe31e..62e39f1d 100644
--- a/ironic_python_agent/hardware.py
+++ b/ironic_python_agent/hardware.py
@@ -545,6 +545,7 @@ def list_all_block_devices(block_type='disk',
                                                            'block', 'vendor'),
                                    by_path=by_path_name,
                                    uuid=device['UUID'],
+                                   partuuid=device['PARTUUID'],
                                    **extra))
     return devices
 
@@ -612,7 +613,7 @@ class BlockDevice(encoding.SerializableComparable):
     def __init__(self, name, model, size, rotational, wwn=None, serial=None,
                  vendor=None, wwn_with_extension=None,
                  wwn_vendor_extension=None, hctl=None, by_path=None,
-                 uuid=None):
+                 uuid=None, partuuid=None):
         self.name = name
         self.model = model
         self.size = size
@@ -625,6 +626,7 @@ class BlockDevice(encoding.SerializableComparable):
         self.wwn_vendor_extension = wwn_vendor_extension
         self.hctl = hctl
         self.by_path = by_path
+        self.partuuid = partuuid
 
 
 class NetworkInterface(encoding.SerializableComparable):
diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py
index 1834559b..00aaf706 100644
--- a/ironic_python_agent/tests/unit/extensions/test_image.py
+++ b/ironic_python_agent/tests/unit/extensions/test_image.py
@@ -751,6 +751,18 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
         mock_is_md_device.return_value = False
         mock_md_get_raid_devices.return_value = {}
         mock_exists.side_effect = iter([False, True, False, True, True])
+        partuuid_device = ('KNAME="sda" MODEL="DRIVE 0" SIZE="10240000" '
+                           'ROTA="1" TYPE="disk" UUID="987654-3210" '
+                           'PARTUUID=""\n'
+                           'KNAME="sda0" MODEL="DRIVE 0" SIZE="102400" '
+                           'ROTA="1" TYPE="part" '
+                           'UUID="' + self.fake_efi_system_part_uuid + '" '
+                           'PARTUUID="1234-2918"\n')
+        exec_side_effect = [('', '')] * 16
+        exec_side_effect.append((partuuid_device, ''))
+        exec_side_effect.extend([('', '')] * 8)
+        mock_execute.side_effect = exec_side_effect
+
         with mock.patch('builtins.open', mock.mock_open()) as mock_open:
             image._install_grub2(
                 self.fake_dev, root_uuid=self.fake_root_uuid,
@@ -805,6 +817,9 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
                                   'GRUB_DISABLE_OS_PROBER': 'true',
                                   'GRUB_SAVEDEFAULT': 'true'},
                               use_standard_locale=True),
+                    mock.call('udevadm', 'settle'),
+                    mock.call('lsblk', '-Pbia',
+                              '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'),
                     mock.call('umount', self.fake_dir + '/boot/efi',
                               attempts=3, delay_on_retry=True),
                     mock.call(('chroot %s /bin/sh -c "umount -a -t vfat"' %
@@ -849,8 +864,21 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
         environ_mock.get.return_value = '/sbin'
         mock_is_md_device.return_value = False
         mock_md_get_raid_devices.return_value = {}
+        partuuid_device = ('KNAME="sda" MODEL="DRIVE 0" SIZE="10240000" '
+                           'ROTA="1" TYPE="disk" UUID="987654-3210" '
+                           'PARTUUID=""\n'
+                           'KNAME="sda0" MODEL="DRIVE 0" SIZE="102400" '
+                           'ROTA="1" TYPE="part" UUID="987654-3210" '
+                           'PARTUUID="' + self.fake_efi_system_part_uuid
+                           + '"\n')
+        exec_side_effect = [('', '')] * 16
+        exec_side_effect.append((partuuid_device, ''))
+        exec_side_effect.extend([('', '')] * 8)
+        mock_execute.side_effect = exec_side_effect
+        # Validates the complete opposite path *and* no-write behavior
+        # occurs if the entry already exists.
         fstab_data = (
-            'UUID=%s\tpath vfat option' % self.fake_efi_system_part_uuid)
+            'PARTUUID=%s\tpath vfat option' % self.fake_efi_system_part_uuid)
         mock_exists.side_effect = [True, False, True, True, True, False,
                                    True, True]
         with mock.patch('builtins.open',
@@ -916,6 +944,9 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
                                   'GRUB_DISABLE_OS_PROBER': 'true',
                                   'GRUB_SAVEDEFAULT': 'true'},
                               use_standard_locale=True),
+                    mock.call('udevadm', 'settle'),
+                    mock.call('lsblk', '-Pbia',
+                              '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID'),
                     mock.call('umount', self.fake_dir + '/boot/efi',
                               attempts=3, delay_on_retry=True),
                     mock.call(('chroot %s /bin/sh -c "umount -a -t vfat"' %
@@ -1026,6 +1057,7 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
 
     @mock.patch.object(os, 'listdir', lambda *_: ['file1', 'file2'])
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
+    @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True)
     @mock.patch.object(shutil, 'copy2', autospec=True)
     @mock.patch.object(os.path, 'isfile', autospec=True)
     @mock.patch.object(image, '_efi_boot_setup', autospec=True)
@@ -1042,7 +1074,8 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
             mock_is_md_device, mock_exists,
             mock_copytree, mock_efi_setup,
             mock_isfile, mock_copy2,
-            mock_execute, mock_dispatch):
+            mock_fstab_append, mock_execute,
+            mock_dispatch):
         mock_exists.return_value = True
         mock_efi_setup.return_value = True
         mock_get_part_uuid.side_effect = [self.fake_root_part,
@@ -1106,6 +1139,9 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
                                            uuid=self.fake_root_uuid)
         mock_get_part_uuid.assert_any_call(self.fake_dev,
                                            uuid=self.fake_efi_system_part_uuid)
+        mock_fstab_append.assert_called_once_with(
+            self.fake_dir,
+            self.fake_efi_system_part_uuid)
         self.assertFalse(mock_dispatch.called)
 
     @mock.patch.object(os.path, 'ismount', lambda *_: False)
@@ -1344,6 +1380,7 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
                                                 self.fake_efi_system_part_uuid)
 
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
+    @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True)
     @mock.patch.object(os, 'listdir', autospec=True)
     @mock.patch.object(shutil, 'copy2', autospec=True)
     @mock.patch.object(os.path, 'isfile', autospec=True)
@@ -1361,8 +1398,8 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
             mock_is_md_device, mock_exists,
             mock_copytree, mock_efi_setup,
             mock_isfile, mock_copy2,
-            mock_oslistdir, mock_execute,
-            mock_dispatch):
+            mock_oslistdir, mock_append_to_fstab,
+            mock_execute, mock_dispatch):
         mock_exists.return_value = True
         mock_efi_setup.return_value = True
         mock_get_part_uuid.side_effect = [self.fake_root_part,
@@ -1431,6 +1468,8 @@ Boot0004* ironic1      HD(1,GPT,55db8d03-c8f6-4a5b-9155-790dddc348fa,0x800,0x640
                                            uuid=self.fake_efi_system_part_uuid)
         self.assertFalse(mock_dispatch.called)
         self.assertEqual(2, mock_oslistdir.call_count)
+        mock_append_to_fstab.assert_called_with(self.fake_dir,
+                                                self.fake_efi_system_part_uuid)
 
     @mock.patch.object(os.path, 'ismount', lambda *_: False)
     @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False)
diff --git a/ironic_python_agent/tests/unit/samples/hardware_samples.py b/ironic_python_agent/tests/unit/samples/hardware_samples.py
index 3ab54153..8cc9b1ea 100644
--- a/ironic_python_agent/tests/unit/samples/hardware_samples.py
+++ b/ironic_python_agent/tests/unit/samples/hardware_samples.py
@@ -160,6 +160,13 @@ RAID_BLK_DEVICE_TEMPLATE = (
     'PARTUUID=""'
 )
 
+PARTUUID_DEVICE_TEMPLATE = (
+    'KNAME="sda" MODEL="DRIVE 0" SIZE="1765517033472" '
+    'ROTA="1" TYPE="disk" UUID="" PARTUUID=""\n'
+    'KNAME="sda1" MODEL="DRIVE 0" SIZE="107373133824" '
+    'ROTA="1" TYPE="part" UUID="987654-3210" PARTUUID="1234-5678"\n'
+)
+
 SHRED_OUTPUT_0_ITERATIONS_ZERO_FALSE = ()
 
 SHRED_OUTPUT_1_ITERATION_ZERO_TRUE = (
diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py
index a9deb79b..60820727 100644
--- a/ironic_python_agent/tests/unit/test_hardware.py
+++ b/ironic_python_agent/tests/unit/test_hardware.py
@@ -66,6 +66,13 @@ RAID_BLK_DEVICE_TEMPLATE_DEVICES = [
                          vendor="FooTastic", uuid=""),
 ]
 
+BLK_DEVICE_TEMPLATE_PARTUUID_DEVICE = [
+    hardware.BlockDevice(name='/dev/sda1', model='DRIVE 0',
+                         size=107373133824, rotational=True,
+                         vendor="FooTastic", uuid="987654-3210",
+                         partuuid="1234-5678"),
+]
+
 
 class FakeHardwareManager(hardware.GenericHardwareManager):
     def __init__(self, hardware_support):
@@ -4291,6 +4298,25 @@ class TestModuleFunctions(base.IronicAgentTest):
         self.assertEqual(RAID_BLK_DEVICE_TEMPLATE_DEVICES, result)
         mocked_udev.assert_called_once_with()
 
+    @mock.patch.object(os, 'readlink', autospec=True)
+    @mock.patch.object(hardware, '_get_device_info',
+                       lambda x, y, z: 'FooTastic')
+    @mock.patch.object(hardware, '_udev_settle', autospec=True)
+    @mock.patch.object(hardware.pyudev.Devices, "from_device_file",
+                       autospec=False)
+    def test_list_all_block_devices_partuuid_success(
+            self, mocked_fromdevfile,
+            mocked_udev, mocked_readlink,
+            mocked_execute):
+        mocked_readlink.return_value = '../../sda'
+        mocked_fromdevfile.return_value = {}
+        mocked_execute.return_value = (hws.PARTUUID_DEVICE_TEMPLATE, '')
+        result = hardware.list_all_block_devices(block_type='part')
+        mocked_execute.assert_called_once_with(
+            'lsblk', '-Pbia', '-oKNAME,MODEL,SIZE,ROTA,TYPE,UUID,PARTUUID')
+        self.assertEqual(BLK_DEVICE_TEMPLATE_PARTUUID_DEVICE, result)
+        mocked_udev.assert_called_once_with()
+
     @mock.patch.object(hardware, '_get_device_info',
                        lambda x, y: "FooTastic")
     @mock.patch.object(hardware, '_udev_settle', autospec=True)
diff --git a/releasenotes/notes/handle-partuuid-for-fstab-e0aadea20a056982.yaml b/releasenotes/notes/handle-partuuid-for-fstab-e0aadea20a056982.yaml
new file mode 100644
index 00000000..f9b6bbc5
--- /dev/null
+++ b/releasenotes/notes/handle-partuuid-for-fstab-e0aadea20a056982.yaml
@@ -0,0 +1,10 @@
+---
+fixes:
+  - |
+    Fixes handling of a Partition UUID being returned instead of a
+    Partition's UUID when the OS may not return the Partition's UUID in time.
+    These two fields are typically referred to as PARTUUID and UUID,
+    respectively. Often these sorts of issues arise under heavy IO load.
+    We now scan, and identify which "UUID" we identified, and update
+    a Linux fstab entry appropriately. For more information, please see
+    `story #2009881 <https://storyboard.openstack.org/#!/story/2009881>`_.