diff options
author | Julia Kreger <juliaashleykreger@gmail.com> | 2021-06-28 15:16:01 -0700 |
---|---|---|
committer | Julia Kreger <juliaashleykreger@gmail.com> | 2021-06-29 14:14:52 -0700 |
commit | e5d552474b21137ae2a66f17bdab5fc1bbf31ec6 (patch) | |
tree | 8169dc4fd7891add84b39cf687722189d008b0e2 | |
parent | 20e145e4da853cd759387e8d8727086f399e51b3 (diff) | |
download | ironic-python-agent-e5d552474b21137ae2a66f17bdab5fc1bbf31ec6.tar.gz |
Catch ismount not being handled
While investigating another grub issue, I was confused by the path
taken in the logs reported, and noticed that on a ramdisk, we might
not actually have a valid response to os.path.ismount, I'm guessing
depending on what in memory filesystem is in use while also coupled
with attempting to check a filesystem.
Adds a test to validate that exceptions raised on these commands
where this issue can be encountered, are properly bypassed, and also
adds additional logging to make it easier to figure out what is
going on in the entire bootloader setup sequence.
Change-Id: Ibd3060bef2e56468ada6b1a5c1cc1632a42803c3
-rw-r--r-- | ironic_python_agent/extensions/image.py | 41 | ||||
-rw-r--r-- | ironic_python_agent/tests/unit/extensions/test_image.py | 121 |
2 files changed, 153 insertions, 9 deletions
diff --git a/ironic_python_agent/extensions/image.py b/ironic_python_agent/extensions/image.py index 03fc7636..84e86f17 100644 --- a/ironic_python_agent/extensions/image.py +++ b/ironic_python_agent/extensions/image.py @@ -273,7 +273,7 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, """ # Before updating let's get information about the bootorder - LOG.debug("Getting information about boot order") + LOG.debug("Getting information about boot order.") utils.execute('efibootmgr') # NOTE(iurygregory): regex used to identify the Warning in the stderr after # we add the new entry. Example: @@ -283,6 +283,8 @@ def _run_efibootmgr(valid_efi_bootloaders, device, efi_partition, label_id = 1 for v_bl in valid_efi_bootloaders: if 'csv' in v_bl.lower(): + LOG.debug('A CSV file has been identified as a bootloader hint. ' + 'File: %s', v_bl) # 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: @@ -328,7 +330,7 @@ def _manage_uefi(device, efi_system_part_uuid=None): """ efi_partition_mount_point = None efi_mounted = False - + LOG.debug('Attempting UEFI loader autodetection and NVRAM record setup.') try: # Force UEFI to rescan the device. _rescan_device(device) @@ -381,6 +383,7 @@ def _manage_uefi(device, efi_system_part_uuid=None): else: # NOTE(dtantsur): if we have an empty EFI partition, try to use # grub-install to populate it. + LOG.warning('Empty EFI partition detected.') return False except processutils.ProcessExecutionError as e: @@ -551,6 +554,29 @@ def _umount_all_partitions(path, path_variable, umount_warn_msg): return umount_binds_success +def _mount_partition(partition, path): + if not os.path.ismount(path): + LOG.debug('Attempting to mount %(device)s to %(path)s to ' + 'partition.', + {'device': partition, + 'path': path}) + try: + utils.execute('mount', partition, path) + except processutils.ProcessExecutionError as e: + # NOTE(TheJulia): It seems in some cases, + # the python os.path.ismount can return False + # even *if* it is actually mounted. This appears + # to be becasue it tries to rely on inode on device + # logic, yet the rules are sometimes different inside + # ramdisks. So lets check the error first. + if 'already mounted' not in e: + # Raise the error, since this is not a known + # failure case + raise + else: + LOG.debug('Partition already mounted, proceeding.') + + def _install_grub2(device, root_uuid, efi_system_part_uuid=None, prep_boot_part_uuid=None, target_boot_mode='bios'): """Install GRUB2 bootloader on a given device.""" @@ -638,11 +664,9 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None, # remounted. LOG.debug('No EFI assets were preserved for setup or the ' 'ramdisk was unable to complete the setup. ' - 'falling back to bootloader installation from' + 'falling back to bootloader installation from ' 'deployed image.') - if not os.path.ismount(path): - LOG.debug('Re-mounting the root partition.') - utils.execute('mount', root_partition, path) + _mount_partition(root_partition, path) binary_name = "grub" if os.path.exists(os.path.join(path, 'usr/sbin/grub2-install')): @@ -662,9 +686,8 @@ def _install_grub2(device, root_uuid, efi_system_part_uuid=None, LOG.warning("GRUB2 will be installed for UEFI on efi partition " "%s using the install command which does not place " "Secure Boot signed binaries.", efi_partition) - if not os.path.ismount(efi_partition_mount_point): - utils.execute('mount', efi_partition, - efi_partition_mount_point) + + _mount_partition(efi_partition, efi_partition_mount_point) efi_mounted = True try: utils.execute('chroot %(path)s /bin/sh -c ' diff --git a/ironic_python_agent/tests/unit/extensions/test_image.py b/ironic_python_agent/tests/unit/extensions/test_image.py index 9661e6f3..05e937b8 100644 --- a/ironic_python_agent/tests/unit/extensions/test_image.py +++ b/ironic_python_agent/tests/unit/extensions/test_image.py @@ -1154,6 +1154,127 @@ efibootmgr: ** Warning ** : Boot0005 has same label ironic1\n mock_append_to_fstab.assert_called_with(self.fake_dir, self.fake_efi_system_part_uuid) + @mock.patch.object(os.path, 'ismount', lambda *_: False) + @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False) + @mock.patch.object(image, '_append_uefi_to_fstab', autospec=True) + @mock.patch.object(image, '_preserve_efi_assets', autospec=True) + @mock.patch.object(image, '_efi_boot_setup', autospec=True) + @mock.patch.object(os.path, 'exists', autospec=True) + @mock.patch.object(hardware, 'is_md_device', autospec=True) + @mock.patch.object(hardware, 'md_get_raid_devices', autospec=True) + @mock.patch.object(os, 'environ', autospec=True) + @mock.patch.object(os, 'makedirs', autospec=True) + @mock.patch.object(image, '_get_partition', autospec=True) + def test__install_grub2_uefi_partition_image_with_preserve_failure2( + self, mock_get_part_uuid, mkdir_mock, + environ_mock, mock_md_get_raid_devices, + mock_is_md_device, mock_exists, + mock_efi_setup, + mock_preserve_efi_assets, + mock_append_to_fstab, + mock_execute, mock_dispatch): + mock_exists.return_value = True + mock_efi_setup.side_effect = Exception('meow') + mock_get_part_uuid.side_effect = [self.fake_root_part, + self.fake_efi_system_part] + environ_mock.get.return_value = '/sbin' + mock_is_md_device.return_value = False + mock_md_get_raid_devices.return_value = {} + mock_preserve_efi_assets.return_value = None + exec_results = [('', '')] * 21 + already_exists = processutils.ProcessExecutionError( + '/dev is already mounted at /path') + # Mark mounts as already mounted, which is where os.path.ismount + # usage corresponds. + exec_results[6] = already_exists + exec_results[8] = already_exists + + image._install_grub2( + self.fake_dev, root_uuid=self.fake_root_uuid, + efi_system_part_uuid=self.fake_efi_system_part_uuid, + target_boot_mode='uefi') + self.assertFalse(mock_efi_setup.called) + + expected = [mock.call('mount', '/dev/fake2', self.fake_dir), + mock.call('mount', '-o', 'bind', '/dev', + self.fake_dir + '/dev'), + mock.call('mount', '-o', 'bind', '/proc', + self.fake_dir + '/proc'), + mock.call('mount', '-o', 'bind', '/run', + self.fake_dir + '/run'), + mock.call('mount', '-t', 'sysfs', 'none', + self.fake_dir + '/sys'), + mock.call(('chroot %s /bin/sh -c ' + '"grub2-mkconfig -o ' + '/boot/grub2/grub.cfg"' % self.fake_dir), + shell=True, + env_variables={ + 'PATH': '/sbin:/bin:/usr/sbin:/sbin', + 'GRUB_DISABLE_OS_PROBER': 'true', + 'GRUB_SAVEDEFAULT': 'true'}, + use_standard_locale=True), + mock.call('mount', '/dev/fake2', self.fake_dir), + mock.call(('chroot %s /bin/sh -c "mount -a -t vfat"' % + (self.fake_dir)), shell=True, + env_variables={ + 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), + mock.call('mount', self.fake_efi_system_part, + self.fake_dir + '/boot/efi'), + mock.call(('chroot %s /bin/sh -c "grub2-install"' % + self.fake_dir), shell=True, + env_variables={ + 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), + mock.call(('chroot %s /bin/sh -c ' + '"grub2-install --removable"' % + self.fake_dir), shell=True, + env_variables={ + 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), + mock.call( + 'umount', self.fake_dir + '/boot/efi', + attempts=3, delay_on_retry=True), + mock.call('mount', self.fake_efi_system_part, + '/tmp/fake-dir/boot/efi'), + mock.call(('chroot %s /bin/sh -c ' + '"grub2-mkconfig -o ' + '/boot/grub2/grub.cfg"' % self.fake_dir), + shell=True, + env_variables={ + 'PATH': '/sbin:/bin:/usr/sbin:/sbin', + 'GRUB_DISABLE_OS_PROBER': 'true', + 'GRUB_SAVEDEFAULT': 'true'}, + use_standard_locale=True), + mock.call('umount', self.fake_dir + '/boot/efi', + attempts=3, delay_on_retry=True), + mock.call(('chroot %s /bin/sh -c "umount -a -t vfat"' % + (self.fake_dir)), shell=True, + env_variables={ + 'PATH': '/sbin:/bin:/usr/sbin:/sbin'}), + mock.call('umount', self.fake_dir + '/dev', + attempts=3, delay_on_retry=True), + mock.call('umount', self.fake_dir + '/proc', + attempts=3, delay_on_retry=True), + mock.call('umount', self.fake_dir + '/run', + attempts=3, delay_on_retry=True), + mock.call('umount', self.fake_dir + '/sys', + attempts=3, delay_on_retry=True), + mock.call('umount', self.fake_dir, attempts=3, + delay_on_retry=True)] + + mkdir_mock.assert_not_called() + mock_execute.assert_has_calls(expected) + mock_get_part_uuid.assert_any_call(self.fake_dev, + uuid=self.fake_root_uuid) + mock_get_part_uuid.assert_any_call(self.fake_dev, + uuid=self.fake_efi_system_part_uuid) + self.assertFalse(mock_dispatch.called) + mock_preserve_efi_assets.assert_called_with( + self.fake_dir, + self.fake_dir + '/boot/efi/EFI', + '/dev/fake1', + self.fake_dir + '/boot/efi') + mock_append_to_fstab.assert_called_with(self.fake_dir, + self.fake_efi_system_part_uuid) + @mock.patch.object(image, '_is_bootloader_loaded', lambda *_: False) @mock.patch.object(os, 'listdir', autospec=True) @mock.patch.object(shutil, 'copy2', autospec=True) |