Skip to content

Commit 474d2a9

Browse files
juliakregerbfournie
juliakreger
and
bfournie
committed
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 2fab70c) (cherry picked from commit bfa97cb)
1 parent 350a67e commit 474d2a9

File tree

3 files changed

+137
-14
lines changed

3 files changed

+137
-14
lines changed

‎ironic_python_agent/extensions/image.py‎

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,16 @@
3838

3939
BIND_MOUNTS = ('/dev', '/proc', '/run')
4040

41+
# NOTE(TheJulia): Do not add bootia32.csv to this list. That is 32bit
42+
# EFI booting and never really became popular.
4143
BOOTLOADERS_EFI = [
44+
'bootx64.csv', # Used by GRUB2 shim loader (Ubuntu, Red Hat)
45+
'boot.csv', # Used by rEFInd, Centos7 Grub2
4246
'bootia32.efi',
43-
'bootx64.efi',
47+
'bootx64.efi', # x86_64 Default
4448
'bootia64.efi',
4549
'bootarm.efi',
46-
'bootaa64.efi',
50+
'bootaa64.efi', # Arm64 Default
4751
'bootriscv32.efi',
4852
'bootriscv64.efi',
4953
'bootriscv128.efi',
@@ -219,9 +223,10 @@ def _is_bootloader_loaded(dev):
219223
def _get_efi_bootloaders(location):
220224
"""Get all valid efi bootloaders in a given location
221225
222-
:param location: the location where it should start looking for the
226+
:param location: the location where it should start looking for the
223227
efi files.
224-
:return: a list of relative paths to valid efi bootloaders
228+
:return: a list of relative paths to valid efi bootloaders or reference
229+
files.
225230
"""
226231
# Let's find all files with .efi or .EFI extension
227232
LOG.debug('Looking for all efi files on %s', location)
@@ -237,15 +242,30 @@ def _get_efi_bootloaders(location):
237242
v_bl = efi_f.split(location)[-1][1:]
238243
LOG.debug('%s is a valid bootloader', v_bl)
239244
valid_bootloaders.append(v_bl)
245+
if 'csv' in efi_f.lower():
246+
v_bl = efi_f.split(location)[-1][1:]
247+
LOG.debug('%s is a pointer to a bootloader', v_bl)
248+
# The CSV files are intended to be authortative as
249+
# to the bootloader and the label to be used. Since
250+
# we found one, we're going to point directly to it.
251+
# centos7 did ship with 2, but with the same contents.
252+
# TODO(TheJulia): Perhaps we extend this to make a list
253+
# of CSVs instead and only return those?! But then the
254+
# question is which is right/first/preferred.
255+
return [v_bl]
240256
return valid_bootloaders
241257

242258

243-
def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
259+
def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
260+
mount_point):
244261
"""Executes efibootmgr and removes duplicate entries.
245262
246263
:param valid_efi_bootloaders: the list of valid efi bootloaders
247264
:param device: the device to be used
248265
:param efi_partition: the efi partition on the device
266+
:param mount_point: The mountpoint for the EFI partition so we can
267+
read contents of files if necessary to perform
268+
proper bootloader injection operations.
249269
"""
250270

251271
# Before updating let's get information about the bootorder
@@ -258,13 +278,25 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition):
258278
r'Boot([0-9a-f-A-F]+)\s.*$')
259279
label_id = 1
260280
for v_bl in valid_efi_bootloaders:
261-
v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
262-
# Update the nvram using efibootmgr
263-
# https://linux.die.net/man/8/efibootmgr
264-
label = 'ironic' + str(label_id)
281+
if 'csv' in v_bl.lower():
282+
# These files are always UTF-16 encoded, sometimes have a header.
283+
# Positive bonus is python silently drops the FEFF header.
284+
with open(mount_point + '/' + v_bl, 'r', encoding='utf-16') as csv:
285+
contents = str(csv.read())
286+
csv_contents = contents.split(',', maxsplit=3)
287+
csv_filename = v_bl.split('/')[-1]
288+
v_efi_bl_path = v_bl.replace(csv_filename, str(csv_contents[0]))
289+
v_efi_bl_path = '\\' + v_efi_bl_path.replace('/', '\\')
290+
label = csv_contents[1]
291+
else:
292+
v_efi_bl_path = '\\' + v_bl.replace('/', '\\')
293+
label = 'ironic' + str(label_id)
294+
265295
LOG.debug("Adding loader %(path)s on partition %(part)s of device "
266296
" %(dev)s", {'path': v_efi_bl_path, 'part': efi_partition,
267297
'dev': device})
298+
# Update the nvram using efibootmgr
299+
# https://linux.die.net/man/8/efibootmgr
268300
cmd = utils.execute('efibootmgr', '-c', '-d', device,
269301
'-p', efi_partition, '-w', '-L', label,
270302
'-l', v_efi_bl_path)
@@ -340,7 +372,8 @@ def _manage_uefi(device, efi_system_part_uuid=None):
340372

341373
valid_efi_bootloaders = _get_efi_bootloaders(efi_partition_mount_point)
342374
if valid_efi_bootloaders:
343-
_run_efibootmgr(valid_efi_bootloaders, device, efi_partition)
375+
_run_efibootmgr(valid_efi_bootloaders, device, efi_partition,
376+
efi_partition_mount_point)
344377
return True
345378
else:
346379
# NOTE(dtantsur): if we have an empty EFI partition, try to use

‎ironic_python_agent/tests/unit/extensions/test_image.py‎

Lines changed: 72 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2133,6 +2133,51 @@ def test__manage_uefi(self, mkdir_mock, mock_utils_efi_part,
21332133
mock_execute.assert_has_calls(expected)
21342134
self.assertEqual(7, mock_execute.call_count)
21352135

2136+
@mock.patch.object(os.path, 'exists', lambda *_: False)
2137+
@mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
2138+
@mock.patch.object(image, '_get_partition', autospec=True)
2139+
@mock.patch.object(utils, 'get_efi_part_on_device', autospec=True)
2140+
@mock.patch.object(os, 'makedirs', autospec=True)
2141+
def test__manage_uefi_found_csv(self, mkdir_mock, mock_utils_efi_part,
2142+
mock_get_part_uuid, mock_efi_bl,
2143+
mock_execute, mock_dispatch):
2144+
mock_utils_efi_part.return_value = '1'
2145+
mock_get_part_uuid.return_value = self.fake_dev
2146+
mock_efi_bl.return_value = ['EFI/vendor/BOOTX64.CSV']
2147+
2148+
# Format is <file>,<entry_name>,<options>,humanfriendlytextnotused
2149+
# https://www.rodsbooks.com/efi-bootloaders/fallback.html
2150+
# Mild difference, Ubuntu ships a file without a 0xFEFF delimiter
2151+
# at the start of the file, where as Red Hat *does*
2152+
csv_file_data = u'shimx64.efi,Vendor String,,Grub2MadeUSDoThis\n'
2153+
2154+
mock_execute.side_effect = iter([('', ''), ('', ''),
2155+
('', ''), ('', ''),
2156+
('', ''), ('', ''),
2157+
('', '')])
2158+
2159+
expected = [mock.call('partx', '-u', '/dev/fake', attempts=3,
2160+
delay_on_retry=True),
2161+
mock.call('udevadm', 'settle'),
2162+
mock.call('mount', self.fake_efi_system_part,
2163+
self.fake_dir + '/boot/efi'),
2164+
mock.call('efibootmgr'),
2165+
mock.call('efibootmgr', '-c', '-d', self.fake_dev,
2166+
'-p', '1', '-w',
2167+
'-L', 'Vendor String', '-l',
2168+
'\\EFI\\vendor\\shimx64.efi'),
2169+
mock.call('umount', self.fake_dir + '/boot/efi',
2170+
attempts=3, delay_on_retry=True),
2171+
mock.call('sync')]
2172+
with mock.patch('builtins.open',
2173+
mock.mock_open(read_data=csv_file_data)):
2174+
result = image._manage_uefi(self.fake_dev, self.fake_root_uuid)
2175+
self.assertTrue(result)
2176+
mkdir_mock.assert_called_once_with(self.fake_dir + '/boot/efi')
2177+
mock_efi_bl.assert_called_once_with(self.fake_dir + '/boot/efi')
2178+
mock_execute.assert_has_calls(expected)
2179+
self.assertEqual(7, mock_execute.call_count)
2180+
21362181
@mock.patch.object(os.path, 'exists', lambda *_: False)
21372182
@mock.patch.object(image, '_get_efi_bootloaders', autospec=True)
21382183
@mock.patch.object(image, '_get_partition', autospec=True)
@@ -2221,7 +2266,7 @@ def test__no_efi_bootloaders(self, mock_access, mock_walk, mock_execute,
22212266
('/boot/efi', ['EFI'], []),
22222267
('/boot/efi/EFI', ['centos', 'BOOT'], []),
22232268
('/boot/efi/EFI/centos', ['fw', 'fonts'],
2224-
['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
2269+
['shimx64-centos.efi',
22252270
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
22262271
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
22272272
'grub.cfg']),
@@ -2242,7 +2287,28 @@ def test__get_efi_bootloaders(self, mock_access, mock_walk, mock_execute,
22422287
('/boot/efi', ['EFI'], []),
22432288
('/boot/efi/EFI', ['centos', 'BOOT'], []),
22442289
('/boot/efi/EFI/centos', ['fw', 'fonts'],
2245-
['shimx64-centos.efi', 'BOOT.CSV', 'BOOTX64.CSV',
2290+
['shimx64-centos.efi', 'BOOTX64.CSV',
2291+
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
2292+
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
2293+
'grub.cfg']),
2294+
('/boot/efi/EFI/centos/fw', [], []),
2295+
('/boot/efi/EFI/centos/fonts', [], ['unicode.pf2']),
2296+
('/boot/efi/EFI/BOOT', [],
2297+
['BOOTX64.EFI', 'fallback.efi', 'fbx64.efi'])
2298+
]
2299+
mock_access.return_value = True
2300+
result = image._get_efi_bootloaders("/boot/efi")
2301+
self.assertEqual(result[0], 'EFI/centos/BOOTX64.CSV')
2302+
2303+
@mock.patch.object(os, 'walk', autospec=True)
2304+
@mock.patch.object(os, 'access', autospec=True)
2305+
def test__get_efi_bootloaders_no_csv(
2306+
self, mock_access, mock_walk, mock_execute, mock_dispatch):
2307+
mock_walk.return_value = [
2308+
('/boot/efi', ['EFI'], []),
2309+
('/boot/efi/EFI', ['centos', 'BOOT'], []),
2310+
('/boot/efi/EFI/centos', ['fw', 'fonts'],
2311+
['shimx64-centos.efi',
22462312
'MokManager.efi', 'mmx64.efi', 'shim.efi', 'fwupia32.efi',
22472313
'fwupx64.efi', 'shimx64.efi', 'grubenv', 'grubx64.efi',
22482314
'grub.cfg']),
@@ -2271,15 +2337,17 @@ def test__get_windows_efi_bootloaders(self, mock_access, mock_walk,
22712337

22722338
def test__run_efibootmgr_no_bootloaders(self, mock_execute, mock_dispatch):
22732339
result = image._run_efibootmgr([], self.fake_dev,
2274-
self.fake_efi_system_part)
2340+
self.fake_efi_system_part,
2341+
self.fake_dir)
22752342
expected = []
22762343
self.assertIsNone(result)
22772344
mock_execute.assert_has_calls(expected)
22782345

22792346
def test__run_efibootmgr(self, mock_execute, mock_dispatch):
22802347
result = image._run_efibootmgr(['EFI/BOOT/BOOTX64.EFI'],
22812348
self.fake_dev,
2282-
self.fake_efi_system_part)
2349+
self.fake_efi_system_part,
2350+
self.fake_dir)
22832351
expected = [mock.call('efibootmgr'),
22842352
mock.call('efibootmgr', '-c', '-d', self.fake_dev,
22852353
'-p', self.fake_efi_system_part, '-w',

‎releasenotes/notes/support-bootloader-csv-file-use-c815b520c600cd98.yaml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
features:
3+
- |
4+
Adds the capability into the agent to read and act upon bootloader CSV
5+
files which serve as authoritative indicators of what bootloader to load
6+
instead of leaning towards utilizing the default.
7+
fixes:
8+
- |
9+
Fixes nodes failing after deployment completes due to issues in the Grub2
10+
EFI loader entry addition where a ``BOOT.CSV`` file provides the
11+
authoritative pointer to the bootloader to be used for booting the OS. The
12+
base issue with Grub2 is that it would update the UEFI bootloader NVRAM
13+
entries with whatever is present in a vendor specific ``BOOT.CSV`` or
14+
``BOOTX64.CSV`` file. In some cases, a baremetal machine *can* crash when
15+
this occurs. More information can be found at
16+
`story 2008962 <https://storyboard.openstack.org/#!/story/2008962>`_.
17+
issues:
18+
- |
19+
If multiple bootloader CSV files are present on the EFI filesystem, the
20+
first CSV file discovered will be utilized. The Ironic team considers
21+
multiple files to be a defect in the image being deployed. This may be
22+
changed in the future.

0 commit comments

Comments
 (0)