diff options
3 files changed, 137 insertions, 14 deletions
diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index 3fe5cc59..ba7a2bb9 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 599a1973..4f06e465 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -2122,6 +2122,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): @@ -2205,7 +2250,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']), @@ -2226,7 +2271,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']), @@ -2255,7 +2321,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) @@ -2263,7 +2330,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. |