From 881015acef90451754144f56d681cad3d4152cbe Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Fri, 25 Feb 2022 12:18:39 -0800 Subject: 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 (cherry picked from commit 99ca1086dbfc7b6e41cf800b0bd899565e2e8922) --- ironic_python_agent/extensions/image.py | 51 +++++++++++++++++++--- ironic_python_agent/hardware.py | 4 +- .../tests/unit/extensions/test_image.py | 47 ++++++++++++++++++-- .../tests/unit/samples/hardware_samples.py | 7 +++ ironic_python_agent/tests/unit/test_hardware.py | 26 +++++++++++ ...handle-partuuid-for-fstab-e0aadea20a056982.yaml | 10 +++++ 6 files changed, 135 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/handle-partuuid-for-fstab-e0aadea20a056982.yaml diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index 1f587dcd..64776a48 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -891,17 +891,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 f2156f84..c0a8e85a 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -530,6 +530,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 @@ -597,7 +598,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 @@ -610,6 +611,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 3c5c69a6..8b6a8514 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -804,6 +804,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, @@ -858,6 +870,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"' % @@ -902,8 +917,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', @@ -969,6 +997,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"' % @@ -1079,6 +1110,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) @@ -1095,7 +1127,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, @@ -1159,6 +1192,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) @@ -1276,6 +1312,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) @@ -1293,8 +1330,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, @@ -1363,6 +1400,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 573d28d5..01825d05 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): @@ -4143,6 +4150,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 `_. -- cgit v1.2.1