From ba3caa6c644460a76518e6a0b1977b03516bc323 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 2 Jul 2020 17:27:49 +0200 Subject: [PATCH] Increase the ESP partition size to 550 MiB when using software RAID This has been a popular guidance, and diskimage-builder has recently started following it. Change-Id: I794c846fb191c15b0a30546bf64d624dfbde0fd4 --- ironic_python_agent/extensions/image.py | 3 ++- ironic_python_agent/raid_utils.py | 14 +++++++++----- .../tests/unit/extensions/test_image.py | 12 ++++++------ ironic_python_agent/tests/unit/test_hardware.py | 8 ++++---- .../notes/raid-esp-size-2c322adb2d3b9ce7.yaml | 9 +++++++++ 5 files changed, 30 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/raid-esp-size-2c322adb2d3b9ce7.yaml diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index e0138a7c2..75e3f320d 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -28,6 +28,7 @@ from ironic_python_agent import errors from ironic_python_agent.extensions import base from ironic_python_agent.extensions import iscsi from ironic_python_agent import hardware +from ironic_python_agent import raid_utils from ironic_python_agent import utils LOG = log.getLogger(__name__) @@ -391,7 +392,7 @@ def _prepare_boot_partitions_for_softraid(device, holders, efi_part, # We know that we kept this space when configuring raid,see # hardware.GenericHardwareManager.create_configuration. # We could also directly get the EFI partition size. - partsize_mib = 128 + partsize_mib = raid_utils.ESP_SIZE_MIB partlabel_prefix = 'uefi-holder-' for number, holder in enumerate(holders): # NOTE: see utils.get_partition_table_type_from_specs diff --git a/ironic_python_agent/raid_utils.py b/ironic_python_agent/raid_utils.py index 2ad49e0a6..d62b4280a 100644 --- a/ironic_python_agent/raid_utils.py +++ b/ironic_python_agent/raid_utils.py @@ -22,6 +22,11 @@ from ironic_python_agent import utils LOG = logging.getLogger(__name__) +# NOTE(dtantsur): 550 MiB is used by DIB and seems a common guidance: +# https://www.rodsbooks.com/efi-bootloaders/principles.html +ESP_SIZE_MIB = 550 + + def get_block_devices_for_raid(block_devices, logical_disks): """Get block devices that are involved in the RAID configuration. @@ -88,11 +93,10 @@ def calculate_raid_start(target_boot_mode, partition_table_type, dev_name): # granularity is GiB, so you lose up to 1GiB just for a bios boot # partition... if target_boot_mode == 'uefi': - # Leave 129MiB - start_sector s for the esp (approx 128MiB) - # NOTE: any image efi partition is expected to be less - # than 128MiB - # TBD: 129MiB is a waste in most cases. - raid_start = '129MiB' + # Leave 551MiB - start_sector s for the esp (approx 550 MiB) + # TODO(dtantsur): 550 MiB is a waste in most cases, make it + # configurable? + raid_start = '%sMiB' % (ESP_SIZE_MIB + 1) else: if partition_table_type == 'gpt': # Leave 8MiB - start_sector s (approx 7MiB) diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index d84307fb0..154edbe07 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -682,7 +682,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock_efi_part.assert_called_once_with('/dev/md0') expected = [ mock.call('sgdisk', '-F', '/dev/sda'), - mock.call('sgdisk', '-n', '0:451s:+128MiB', '-t', '0:ef00', '-c', + mock.call('sgdisk', '-n', '0:451s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-0', '/dev/sda'), mock.call('partprobe'), mock.call('blkid'), @@ -690,7 +690,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n '/dev/sda'), mock.call('cp', '/dev/md0p12', '/dev/sda12'), mock.call('sgdisk', '-F', '/dev/sdb'), - mock.call('sgdisk', '-n', '0:452s:+128MiB', '-t', '0:ef00', '-c', + mock.call('sgdisk', '-n', '0:452s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-1', '/dev/sdb'), mock.call('partprobe'), mock.call('blkid'), @@ -726,14 +726,14 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock_efi_part.assert_called_once_with('/dev/md0') expected = [ mock.call('sgdisk', '-F', '/dev/sda'), - mock.call('sgdisk', '-n', '0:451s:+128MiB', '-t', '0:ef00', '-c', + mock.call('sgdisk', '-n', '0:451s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-0', '/dev/sda'), mock.call('partprobe'), mock.call('blkid'), mock.call('blkid', '-l', '-t', 'PARTLABEL=uefi-holder-0', '/dev/sda'), mock.call('sgdisk', '-F', '/dev/sdb'), - mock.call('sgdisk', '-n', '0:452s:+128MiB', '-t', '0:ef00', '-c', + mock.call('sgdisk', '-n', '0:452s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-1', '/dev/sdb'), mock.call('partprobe'), mock.call('blkid'), @@ -770,7 +770,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n expected = [ mock.call('sgdisk', '-F', '/dev/sda'), - mock.call('sgdisk', '-n', '0:451s:+128MiB', '-t', '0:ef00', '-c', + mock.call('sgdisk', '-n', '0:451s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-0', '/dev/sda'), mock.call('partprobe'), mock.call('blkid'), @@ -778,7 +778,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n '/dev/sda'), mock.call('cp', '/dev/md0p15', '/dev/sda12'), mock.call('sgdisk', '-F', '/dev/sdb'), - mock.call('sgdisk', '-n', '0:452s:+128MiB', '-t', '0:ef00', '-c', + mock.call('sgdisk', '-n', '0:452s:+550MiB', '-t', '0:ef00', '-c', '0:uefi-holder-1', '/dev/sdb'), mock.call('partprobe'), mock.call('blkid'), diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 0be427e52..29a46939b 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -3078,10 +3078,10 @@ class TestGenericHardwareManager(base.IronicAgentTest): mock.call('parted', '/dev/sda', '-s', '--', 'mklabel', 'gpt'), mock.call('parted', '/dev/sdb', '-s', '--', 'mklabel', 'gpt'), mock.call('parted', '/dev/sda', '-s', '-a', 'optimal', '--', - 'mkpart', 'primary', '129MiB', '10GiB'), + 'mkpart', 'primary', '551MiB', '10GiB'), mock.call('partx', '-u', '/dev/sda', check_exit_code=False), mock.call('parted', '/dev/sdb', '-s', '-a', 'optimal', '--', - 'mkpart', 'primary', '129MiB', '10GiB'), + 'mkpart', 'primary', '551MiB', '10GiB'), mock.call('partx', '-u', '/dev/sdb', check_exit_code=False), mock.call('parted', '/dev/sda', '-s', '-a', 'optimal', '--', 'mkpart', 'primary', '10GiB', '-1'), @@ -3693,10 +3693,10 @@ class TestGenericHardwareManager(base.IronicAgentTest): mock.call('parted', '/dev/nvme1n1', '-s', '--', 'mklabel', 'gpt'), mock.call('parted', '/dev/nvme0n1', '-s', '-a', 'optimal', '--', - 'mkpart', 'primary', '129MiB', '10GiB'), + 'mkpart', 'primary', '551MiB', '10GiB'), mock.call('partx', '-u', '/dev/nvme0n1', check_exit_code=False), mock.call('parted', '/dev/nvme1n1', '-s', '-a', 'optimal', '--', - 'mkpart', 'primary', '129MiB', '10GiB'), + 'mkpart', 'primary', '551MiB', '10GiB'), mock.call('partx', '-u', '/dev/nvme1n1', check_exit_code=False), mock.call('parted', '/dev/nvme0n1', '-s', '-a', 'optimal', '--', 'mkpart', 'primary', '10GiB', '-1'), diff --git a/releasenotes/notes/raid-esp-size-2c322adb2d3b9ce7.yaml b/releasenotes/notes/raid-esp-size-2c322adb2d3b9ce7.yaml new file mode 100644 index 000000000..9f75b8c64 --- /dev/null +++ b/releasenotes/notes/raid-esp-size-2c322adb2d3b9ce7.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + The size of the ESP partition created for software RAID has been increased + from 128 MiB to 550 MiB. This change is in line with the recent + `diskimage-builder change + `_ + as well as the `guidance from the author of gdisk + `_.