openstack/ironic-python-agent
Code Issues Proposed changes

Create fstab entry with appropriate label

Browse Source 
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
This commit is contained in:
Julia Kreger 2022-02-25 12:18:39 -08:00
parent 48f30ab528
commit 99ca1086db
6 changed files with 135 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
ironic_python_agent/extensions/image.py
View File

@ -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)

4
ironic_python_agent/hardware.py
View File

@ -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):

47
ironic_python_agent/tests/unit/extensions/test_image.py
View File

@ -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)

7
ironic_python_agent/tests/unit/samples/hardware_samples.py
View File

@ -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 = (

26
ironic_python_agent/tests/unit/test_hardware.py
View File

@ -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)

10
releasenotes/notes/handle-partuuid-for-fstab-e0aadea20a056982.yaml Normal file
View File

@ -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>`_.