From ff49b04e28b74d89a5fa6334411c56628f502bee Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 15 Apr 2020 12:44:37 +0200 Subject: [PATCH] A boot partition on a GPT disk should be considered an EFI partition DIB builds instance images with EFI partitions that only have the boot flag, but not esp. According to parted documentation, boot is an alias for esp on GPT, so accept it as well. To avoid complexities when parsing parted output, the implementation is switched to existing utils and ironic-lib functions. Change-Id: I5f57535e5a89528c38d0879177b59db6c0f5c06e Story: #2007455 Task: #39423 --- ironic_python_agent/tests/unit/test_utils.py | 101 ++++++++---------- ironic_python_agent/utils.py | 22 ++-- .../notes/uefi-esp-660fc2c650e6af92.yaml | 5 + 3 files changed, 58 insertions(+), 70 deletions(-) create mode 100644 releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml diff --git a/ironic_python_agent/tests/unit/test_utils.py b/ironic_python_agent/tests/unit/test_utils.py index 48948fc35..214072921 100644 --- a/ironic_python_agent/tests/unit/test_utils.py +++ b/ironic_python_agent/tests/unit/test_utils.py @@ -23,6 +23,7 @@ import tarfile import tempfile from unittest import mock +from ironic_lib import disk_utils from ironic_lib import utils as ironic_utils from oslo_concurrency import processutils from oslo_serialization import base64 @@ -45,30 +46,6 @@ Number Start End Size File system Name Flags 1 116MB 2361MB 2245MB ext4 ''' -PARTED_OUTPUT_UNFORMATTED_NOFS = '''Model: whatever -Disk /dev/sda: 480GB -Sector size (logical/physical): 512B/512B -Partition Table: gpt -Disk Flags: - -Number Start End Size File system Name Flags -1 1049kB 9437kB 8389kB ESP boot, esp -2 9437kB 17.8MB 8389kB BSP bios_grub -3 17.8MB 40.0GB 40.0GB -4 479GB 480GB 68.1MB -''' - -PARTED_OUTPUT_NO_EFI = '''Model: whatever -Disk /dev/sda: 450GB -Sector size (logical/physical): 512B/512B -Partition Table: gpt -Disk Flags: - -Number Start End Size File system Name Flags -14 1049kB 5243kB 4194kB bios_grub - 1 116MB 2361MB 2245MB ext4 -''' - class ExecuteTestCase(ironic_agent_base.IronicAgentTest): # This test case does call utils.execute(), so don't block access to the @@ -630,40 +607,6 @@ class TestUtils(testtools.TestCase): utils.extract_device('whatevernotmatchin12a') ) - @mock.patch.object(utils, 'execute', autospec=True) - def test_get_efi_part_on_device(self, mocked_execute): - parted_ret = PARTED_OUTPUT_UNFORMATTED.format('gpt') - mocked_execute.side_effect = [ - (parted_ret, None) - ] - ret = utils.get_efi_part_on_device('/dev/sda') - mocked_execute.assert_has_calls( - [mock.call('parted', '-s', '/dev/sda', '--', 'print')] - ) - self.assertEqual('15', ret) - - @mock.patch.object(utils, 'execute', autospec=True) - def test_get_efi_part_on_device_without_fs(self, mocked_execute): - parted_ret = PARTED_OUTPUT_UNFORMATTED_NOFS - mocked_execute.side_effect = [ - (parted_ret, None) - ] - ret = utils.get_efi_part_on_device('/dev/sda') - mocked_execute.assert_has_calls( - [mock.call('parted', '-s', '/dev/sda', '--', 'print')] - ) - self.assertEqual('1', ret) - - @mock.patch.object(utils, 'execute', autospec=True) - def test_get_efi_part_on_device_not_found(self, mocked_execute): - mocked_execute.side_effect = [ - (PARTED_OUTPUT_NO_EFI, None) - ] - self.assertIsNone(utils.get_efi_part_on_device('/dev/sda')) - mocked_execute.assert_has_calls( - [mock.call('parted', '-s', '/dev/sda', '--', 'print')] - ) - def test_extract_capability_from_dict(self): expected_dict = {"hello": "world"} root = {"capabilities": expected_dict} @@ -1040,3 +983,45 @@ class TestClockSyncUtils(ironic_agent_base.IronicAgentTest): mock_time_method.return_value = None utils.sync_clock() self.assertEqual(0, mock_execute.call_count) + + +@mock.patch.object(disk_utils, 'list_partitions', autospec=True) +@mock.patch.object(utils, 'scan_partition_table_type', autospec=True) +class TestGetEfiPart(testtools.TestCase): + + def test_get_efi_part_on_device(self, mocked_type, mocked_parts): + mocked_parts.return_value = [ + {'number': '1', 'flags': ''}, + {'number': '14', 'flags': 'bios_grub'}, + {'number': '15', 'flags': 'esp, boot'}, + ] + ret = utils.get_efi_part_on_device('/dev/sda') + self.assertEqual('15', ret) + + def test_get_efi_part_on_device_only_boot_flag_gpt(self, mocked_type, + mocked_parts): + mocked_type.return_value = 'gpt' + mocked_parts.return_value = [ + {'number': '1', 'flags': ''}, + {'number': '14', 'flags': 'bios_grub'}, + {'number': '15', 'flags': 'boot'}, + ] + ret = utils.get_efi_part_on_device('/dev/sda') + self.assertEqual('15', ret) + + def test_get_efi_part_on_device_only_boot_flag_mbr(self, mocked_type, + mocked_parts): + mocked_type.return_value = 'msdos' + mocked_parts.return_value = [ + {'number': '1', 'flags': ''}, + {'number': '14', 'flags': 'bios_grub'}, + {'number': '15', 'flags': 'boot'}, + ] + self.assertIsNone(utils.get_efi_part_on_device('/dev/sda')) + + def test_get_efi_part_on_device_not_found(self, mocked_type, mocked_parts): + mocked_parts.return_value = [ + {'number': '1', 'flags': ''}, + {'number': '14', 'flags': 'bios_grub'}, + ] + self.assertIsNone(utils.get_efi_part_on_device('/dev/sda')) diff --git a/ironic_python_agent/utils.py b/ironic_python_agent/utils.py index e74cc1f16..dca48d118 100644 --- a/ironic_python_agent/utils.py +++ b/ironic_python_agent/utils.py @@ -25,6 +25,7 @@ import tarfile import tempfile import time +from ironic_lib import disk_utils from ironic_lib import utils as ironic_utils from oslo_concurrency import processutils from oslo_config import cfg @@ -73,7 +74,6 @@ DEVICE_EXTRACTOR = re.compile(r'^(?:(.*\d)p|(.*\D))(?:\d+)$') PARTED_TABLE_TYPE_REGEX = re.compile(r'^.*partition\s+table\s*:\s*(gpt|msdos)', re.IGNORECASE) -PARTED_ESP_PATTERN = re.compile(r'^\s*(\d+)\s.*\s\s.*\s.*esp(,|\s|$).*$') def execute(*cmd, **kwargs): @@ -611,23 +611,21 @@ def scan_partition_table_type(device): def get_efi_part_on_device(device): - """Looks for the efi partition on a given device + """Looks for the efi partition on a given device. + + A boot partition on a GPT disk is assumed to be an EFI partition as well. :param device: lock device upon which to check for the efi partition :return: the efi partition or None """ - efi_part = None - out, _u = execute('parted', '-s', device, '--', 'print') - for line in out.splitlines(): - m = PARTED_ESP_PATTERN.match(line) - if m: - efi_part = m.group(1) - - LOG.debug("Found efi partition %s on device %s.", efi_part, device) - break + is_gpt = scan_partition_table_type(device) == 'gpt' + for part in disk_utils.list_partitions(device): + flags = {x.strip() for x in part['flags'].split(',')} + if 'esp' in flags or ('boot' in flags and is_gpt): + LOG.debug("Found EFI partition %s on device %s.", part, device) + return part['number'] else: LOG.debug("No efi partition found on device %s", device) - return efi_part _LARGE_KEYS = frozenset(['configdrive', 'system_logs']) diff --git a/releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml b/releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml new file mode 100644 index 000000000..8701019e4 --- /dev/null +++ b/releasenotes/notes/uefi-esp-660fc2c650e6af92.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + No longer tries to use GRUB2 for configuring boot for whole disk images + with an EFI partition present but only marked as ``boot`` (not ``esp``).