Create fstab entry with appropriate label

Depending on the how the stars align with partition images
being written to a remote system, we *may* end up with
*either* a Partition UUID value, or a Partition's UUID value.

Which are distinctly different.

This is becasue the value, when collected as a result of writing
an image to disk *falls* back and passes the value to enable
partition discovery and matching.

Later on, when we realized we ought to create an fstab entry,
we blindly re-used the value thinking it was, indeed, always
a Partition's UUID and not the Partition UUID. Obviously,
the label type is quite explicit, either UUID or PARTUUID
respectively, when initial ramdisk utilities such as dracut
are searching and mounting filesystems.

Adds capability to identify the correct label to utilize
based upon the current state of the block devices on disk.

Granted, we are likely only exposed to this because of IO
race conditions under high concurrecy load operations.
Normally this would only be seen on test VMs, but
systems being backed by a Storage Area Network *can*
exibit the same IO race conditions as virtual machines.

Change-Id: I953c936cbf8fad889108cbf4e50b1a15f511b38c
Resolves: rhbz#2058717
Story: #2009881
Task: 44623

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 6c45c455..fe24a79f 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 7b19931b..8a05f35b 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>`_.