Use grubby to update kernel command line arguments - #511
mihaelabalutoiu wants to merge 4 commits into
Conversation
Removes cmdline entries with `grubby --remove-args` semantics: a bare name drops the argument whichever value it holds, a name/value pair drops it only on an exact match. Signed-off-by: Mihaela Balutoiu <mbalutoiu@cloudbasesolutions.com>
17e0300 to
f6177de
Compare
| @@ -69,33 +69,7 @@ def check_os_supported(cls, detected_os_info): | |||
| return False | |||
|
|
|||
| def disable_predictable_nic_names(self): | |||
There was a problem hiding this comment.
Worth taking into account to modify custom implementations of disable_predictable_nic_names in the other providers as well.
Besides that LGTM
There was a problem hiding this comment.
Besides OCI, there's no other implementations. We should be fine, since that's already handled here: https://github.com/cloudbase/coriolis-provider-oci/pull/3/files (the extra implementations seem to be removed, as they should :D )
Signed-off-by: Mihaela Balutoiu <mbalutoiu@cloudbasesolutions.com>
Use `grubby --update-kernel=ALL` to update kernel arguments on BLS-based systems such as RHEL 10, where `grub2-mkconfig` does not propagate them to `/boot/loader/entries`. Signed-off-by: Mihaela Balutoiu <mbalutoiu@cloudbasesolutions.com>
Existing `name=value` arguments are parsed as `key_val`, so appending them as `single` values duplicated them. Signed-off-by: Mihaela Balutoiu <mbalutoiu@cloudbasesolutions.com>
f6177de to
a56f89f
Compare
| self.assertFalse(result) | ||
| mock_get_grub_default_conf.assert_not_called() | ||
|
|
||
| @mock.patch.object(base.BaseLinuxOSMorphingTools, '_schedule_grub2_update') |
There was a problem hiding this comment.
I'd appreciate it more if this wasn't mocked and in fact actually checked (i.e. instead of using assert_called methods, use assertTrue/False on self.os_morphing_tools._grub2_update_scheduled .
If you go ahead and do this, please also make sure that the teardown or setup includes the reset of this property (basically set it back to False, so that all other tests start with that).
| @mock.patch.object( | ||
| base.BaseLinuxOSMorphingTools, | ||
| '_get_grub_default_conf', | ||
| return_value='/etc/default/grub', |
There was a problem hiding this comment.
This return value could be saved as a constant in this file.
| mock_schedule_grub2_update, | ||
| ): | ||
| mock_test_path_chroot.return_value = True | ||
| mock_read_file_sudo.return_value = ( |
There was a problem hiding this comment.
I don't think there's any point in having multiple tests for disable_predictable_nic_names anymore. Just mock update_kernel_cmdline_args and assert it got called with whatever expected args to add. That method already has enough tests itself, here you're basically just retesting that method.
| @mock.patch.object(redhat.BaseRedHatMorphingTools, '_update_kernel_cmdline_args') | ||
| def test_disable_predictable_nic_names(self, mock_update_kernel_cmdline_args): | ||
| self.morphing_tools.disable_predictable_nic_names() | ||
| mock_update_kernel_cmdline_args.assert_called_once_with( |
There was a problem hiding this comment.
Yep, this is exactly how the debian test should've looked like as well.
|
|
||
| @mock.patch.object(base.BaseLinuxOSMorphingTools, '_exec_cmd_chroot') | ||
| def test_disable_predictable_nic_names(self, mock_exec_cmd_chroot): | ||
| def test_disable_predictable_nic_names_updates_all_kernels( |
| @mock.patch.object(suse.BaseSUSEMorphingTools, '_schedule_grub2_update') | ||
| @mock.patch.object(base.BaseLinuxOSMorphingTools, '_read_file_sudo') | ||
| @mock.patch.object(base.BaseLinuxOSMorphingTools, '_test_path') | ||
| @mock.patch.object(base.BaseLinuxOSMorphingTools, '_test_path_chroot') |
There was a problem hiding this comment.
Same as for debian, just assert that _update_kernel_cmdline_args has been called with the right args :D
| 'GRUB_CMDLINE_LINUX="console=ttyS0"\n', | ||
| ), | ||
| ( | ||
| 'GRUB_CMDLINE_LINUX="cloud-init=enabled console=ttyS0"', |
There was a problem hiding this comment.
Is this case duplicated? Looks the same as the first one.
3 participants
Failing to update the kernel console options breaks text console functionality on affected platforms.
The issue is caused by BLS being enabled by default in
/etc/default/grub. OnRHEL 10, kernel arguments are managed through/boot/loader/entries, andgrub2-mkconfigdoes not propagate the updated options there.This PR implements the following:
Grub2ConfigEditor.remove_from_option, withgrubby --remove-argssemantics: a bare name drops the argument whichever value it holds, a name/value pair drops it only on an exact match._update_kernel_cmdline_argsto the baseOSMorphingtools, editing bothGRUB2cmdline options throughGrub2ConfigEditor. Debian and SUSE each had their own copy of these edits and now share this one.grubby --update-kernel=ALL, which reaches the BLS boot entries.