summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJulia Kreger <juliaashleykreger@gmail.com>2021-06-10 11:23:14 -0700
committerRiccardo Pittau <elfosardo@gmail.com>2021-06-11 09:25:24 +0000
commiteeffffcd4793232f4488e33460446488e859045d (patch)
treebaa9f27d446eaba104a88370ff7a17097a6a96cd
parent346e5c95796209b11b3514c3e624f232a6a5ff07 (diff)
downloadironic-python-agent-eeffffcd4793232f4488e33460446488e859045d.tar.gz
Utilize CSV file for EFI loader selection
Adds support to identify and utilize a CSV file to signal which bootloader to utilize, and set it when the OS is running as opposed to when EFI is running. This works around EFI loader potentially crashing some vendors hardware types when entry stored in the image does not match the EFI loader record which was utilzied to boot. Grub2+shim specifically specifically needs the CSV file name and entry label to match what the system was booted with in order to prevent the machine from potentially crashing. See https://storyboard.openstack.org/#!/story/2008962 and https://bugzilla.redhat.com/show_bug.cgi?id=1966129#c37 for more information. Change-Id: Ibf1ef4fe0764c0a6f1a39cb7eebc23ecc0ee177d Story: 2008962 Task: 42598 Co-Authored-By: Bob Fournier <bfournie@redhat.com> (cherry picked from commit 2fab70c36ba40a345a9dd01aeb5019681e567aa5)
-rw-r--r--ironic_python_agent/extensions/image.py53
-rw-r--r--ironic_python_agent/tests/unit/extensions/test_image.py76
-rw-r--r--releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml22
3 files changed, 137 insertions, 14 deletions
diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py
index 9f776524..3a5a24c9 100644
--- a/ironic_python_agent/extensions/image.py
+++ b/ironic_python_agent/extensions/image.py
@@ -37,12 +37,16 @@ CONF = cfg.CONF
BIND_MOUNTS = ('/dev', '/proc', '/run')
+# NOTE(TheJulia): Do not add bootia32.csv to this list. That is 32bit
+# EFI booting and never really became popular.
BOOTLOADERS_EFI = [
+ 'bootx64.csv', # Used by GRUB2 shim loader (Ubuntu, Red Hat)
+ 'boot.csv', # Used by rEFInd, Centos7 Grub2
'bootia32.efi',
- 'bootx64.efi',
+ 'bootx64.efi', # x86_64 Default
'bootia64.efi',
'bootarm.efi',
- 'bootaa64.efi',
+ 'bootaa64.efi', # Arm64 Default
'bootriscv32.efi',
'bootriscv64.efi',
'bootriscv128.efi',
@@ -223,9 +227,10 @@ def _is_bootloader_loaded(dev):
def _get_efi_bootloaders(location):
"""Get all valid efi bootloaders in a given location
- :param location: the location where it should start looking for the
+ :param location: the location where it should start looking for the
efi files.
- :return: a list of relative paths to valid efi bootloaders
+ :return: a list of relative paths to valid efi bootloaders or reference
+ files.
"""
# Let's find all files with .efi or .EFI extension
LOG.debug('Looking for all efi files on %s', location)
@@ -241,15 +246,30 @@ def _get_efi_bootloaders(location):
v_bl = efi_f.split(location)[-1][1:]
LOG.debug('%s is a valid bootloader', v_bl)
valid_bootloaders.append(v_bl)
+ if 'csv' in efi_f.lower():
+ v_bl = efi_f.split(location)[-1][1:]
+ LOG.debug('%s is a pointer to a bootloader', v_bl)
+ # The CSV files are intended to be authortative as
+ # to the bootloader and the label to be used. Since
+ # we found one, we're going to point directly to it.
+ # centos7 did ship with 2, but with the same contents.
+ # TODO(TheJulia): Perhaps we extend this to make a list
+ # of CSVs instead and only return those?! But then the
+ # question is which is right/first/preferred.
+ return [v_bl]
return valid_bootloaders
-def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
+def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
+ mount_point):
"""Executes efibootmgr and removes duplicate entries.
:param valid_efi_bootloaders: the list of valid efi bootloaders
:param device: the device to be used
:param efi_partition: the efi partition on the device
+ :param mount_point: The mountpoint for the EFI partition so we can
+ read contents of files if necessary to perform
+ proper bootloader injection operations.
"""
# Before updating let's get information about the bootorder
@@ -262,13 +282,25 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
r'Boot([0-9a-f-A-F]+)\s.*$')
label_id = 1
for v_bl in valid_efi_bootloaders:
- v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
- # Update the nvram using efibootmgr
- # https://linux.die.net/man/8/efibootmgr
- label = 'ironic' + str(label_id)
+ if 'csv' in v_bl.lower():
+ # These files are always UTF-16 encoded, sometimes have a header.
+ # Positive bonus is python silently drops the FEFF header.
+ with open(mount_point + '/' + v_bl, 'r', encoding='utf-16') as csv:
+ contents = str(csv.read())
+ csv_contents = contents.split(',', maxsplit=3)
+ csv_filename = v_bl.split('/')[-1]
+ v_efi_bl_path = v_bl.replace(csv_filename, str(csv_contents[0]))
+ v_efi_bl_path = '\\' + v_efi_bl_path.replace('/', '\\')
+ label = csv_contents[1]
+ else:
+ v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
+ label = 'ironic' + str(label_id)
+
LOG.debug("Adding loader %(path)s on partition %(part)s of device "
" %(dev)s", {'path': v_efi_bl_path, 'part': efi_partition,
'dev': device})
+ # Update the nvram using efibootmgr
+ # https://linux.die.net/man/8/efibootmgr
cmd = utils.execute('efibootmgr', '-c', '-d', device,
'-p', efi_partition, '-w', '-L', label,
'-l', v_efi_bl_path)
@@ -343,7 +375,8 @@ def _manage_uefi(device, efi_system_part_uuid=None):
valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point)
if valid_efi_bootloaders:
- _run_efibootmgr(valid_efi_bootloaders, device, efi_partition)
+ _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
+ efi_partition_mount_point)
return True
else:
# NOTE(dtantsur): if we have an empty EFI partition, try to use
diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py
index e2b633a7..5724c86c 100644
--- a/ironic_python_agent/tests/unit/extensions/test_image.py
+++ b/ironic_python_agent/tests/unit/extensions/test_image.py
@@ -2104,6 +2104,51 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
@mock.patch.object(image, '_get_partition', autospec=True)
@mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
@mock.patch.object(os, 'makedirs', autospec=True)
+ def test__manage_uefi_found_csv(self, mkdir_mock, mock_utils_efi_part,
+ mock_get_part_uuid, mock_efi_bl,
+ mock_execute, mock_dispatch):
+ mock_utils_efi_part.return_value = '1'
+ mock_get_part_uuid.return_value = self.fake_dev
+ mock_efi_bl.return_value = ['EFI/vendor/BOOTX64.CSV']
+
+ # Format is <file>,<entry_name>,<options>,humanfriendlytextnotused
+ # https://www.rodsbooks.com/efi-bootloaders/fallback.html
+ # Mild difference, Ubuntu ships a file without a 0xFEFF delimiter
+ # at the start of the file, where as Red Hat *does*
+ csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n'
+
+ mock_execute.side_effect = iter([('', ''), ('', ''),
+ ('', ''), ('', ''),
+ ('', ''), ('', ''),
+ ('', '')])
+
+ expected = [mock.call('partx', '-u', '/dev/fake', attempts=3,
+ delay_on_retry=True),
+ mock.call('udevadm', 'settle'),
+ mock.call('mount', self.fake_efi_system_part,
+ self.fake_dir + '/boot/efi'),
+ mock.call('efibootmgr'),
+ mock.call('efibootmgr', '-c', '-d', self.fake_dev,
+ '-p', '1', '-w',
+ '-L', 'Vendor String', '-l',
+ '\\EFI\\vendor\\shimx64.efi'),
+ mock.call('umount', self.fake_dir + '/boot/efi',
+ attempts=3, delay_on_retry=True),
+ mock.call('sync')]
+ with mock.patch('builtins.open',
+ mock.mock_open(read_data=csv_file_data)):
+ result = image._manage_uefi(self.fake_dev, self.fake_root_uuid)
+ self.assertTrue(result)
+ mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
+ mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
+ mock_execute.assert_has_calls(expected)
+ self.assertEqual(7, mock_execute.call_count)
+
+ @mock.patch.object(os.path, 'exists', lambda *_: False)
+ @mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
+ @mock.patch.object(image, '_get_partition', autospec=True)
+ @mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
+ @mock.patch.object(os, 'makedirs', autospec=True)
def test__manage_uefi_nvme_device(self, mkdir_mock, mock_utils_efi_part,
mock_get_part_uuid, mock_efi_bl,
mock_execute, mock_dispatch):
@@ -2187,7 +2232,7 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
('/boot/efi', ['EFI'], []),
('/boot/efi/EFI', ['centos', 'BOOT'], []),
('/boot/efi/EFI/centos', ['fw', 'fonts'],
- ['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
+ ['shimx64-centos.efi',
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
'grub.cfg']),
@@ -2208,7 +2253,28 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
('/boot/efi', ['EFI'], []),
('/boot/efi/EFI', ['centos', 'BOOT'], []),
('/boot/efi/EFI/centos', ['fw', 'fonts'],
- ['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
+ ['shimx64-centos.efi', 'BOOTX64.CSV',
+ 'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
+ 'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
+ 'grub.cfg']),
+ ('/boot/efi/EFI/centos/fw', [], []),
+ ('/boot/efi/EFI/centos/fonts', [], ['unicode.pf2']),
+ ('/boot/efi/EFI/BOOT', [],
+ ['BOOTX64.EFI', 'fallback.efi', 'fbx64.efi'])
+ ]
+ mock_access.return_value = True
+ result = image._get_efi_bootloaders("/boot/efi")
+ self.assertEqual(result[0], 'EFI/centos/BOOTX64.CSV')
+
+ @mock.patch.object(os, 'walk', autospec=True)
+ @mock.patch.object(os, 'access', autospec=True)
+ def test__get_efi_bootloaders_no_csv(
+ self, mock_access, mock_walk, mock_execute, mock_dispatch):
+ mock_walk.return_value = [
+ ('/boot/efi', ['EFI'], []),
+ ('/boot/efi/EFI', ['centos', 'BOOT'], []),
+ ('/boot/efi/EFI/centos', ['fw', 'fonts'],
+ ['shimx64-centos.efi',
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
'grub.cfg']),
@@ -2237,7 +2303,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
def test__run_efibootmgr_no_bootloaders(self, mock_execute, mock_dispatch):
result = image._run_efibootmgr([], self.fake_dev,
- self.fake_efi_system_part)
+ self.fake_efi_system_part,
+ self.fake_dir)
expected = []
self.assertIsNone(result)
mock_execute.assert_has_calls(expected)
@@ -2245,7 +2312,8 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n
def test__run_efibootmgr(self, mock_execute, mock_dispatch):
result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'],
self.fake_dev,
- self.fake_efi_system_part)
+ self.fake_efi_system_part,
+ self.fake_dir)
expected = [mock.call('efibootmgr'),
mock.call('efibootmgr', '-c', '-d', self.fake_dev,
'-p', self.fake_efi_system_part, '-w',
diff --git a/releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml b/releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml
new file mode 100644
index 00000000..c337c0d3
--- /dev/null
+++ b/releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml
@@ -0,0 +1,22 @@
+---
+features:
+ - |
+ Adds the capability into the agent to read and act upon bootloader CSV
+ files which serve as authoritative indicators of what bootloader to load
+ instead of leaning towards utilizing the default.
+fixes:
+ - |
+ Fixes nodes failing after deployment completes due to issues in the Grub2
+ EFI loader entry addition where a ``BOOT.CSV`` file provides the
+ authoritative pointer to the bootloader to be used for booting the OS. The
+ base issue with Grub2 is that it would update the UEFI bootloader NVRAM
+ entries with whatever is present in a vendor specific ``BOOT.CSV`` or
+ ``BOOTX64.CSV`` file. In some cases, a baremetal machine *can* crash when
+ this occurs. More information can be found at
+ `story 2008962 <https://storyboard.openstack.org/#!/story/2008962>`_.
+issues:
+ - |
+ If multiple bootloader CSV files are present on the EFI filesystem, the
+ first CSV file discovered will be utilized. The Ironic team considers
+ multiple files to be a defect in the image being deployed. This may be
+ changed in the future.