Merge pull request #61 from prilr/CLOS-2842-elevate-actor-add_upgrade_boot_entry-fai

CLOS-2842: Cover centos/cloudlinux EFI paths in hybrid BIOS+UEFI handling

Author: prilr

repos/system_upgrade/common/actors/addupgradebootentry/actor.py

@@ -1,8 +1,10 @@
-import os
-
 from leapp.actors import Actor
 from leapp.exceptions import StopActorExecutionError
-from leapp.libraries.actor.addupgradebootentry import add_boot_entry, fix_grub_config_error
+from leapp.libraries.actor.addupgradebootentry import (
+    add_boot_entry,
+    fix_grub_config_error,
+    get_hybrid_bios_efi_configs,
+)
 from leapp.models import BootContent, FirmwareFacts, GrubConfigError, TargetKernelCmdlineArgTasks, TransactionDryRun
 from leapp.tags import InterimPreparationPhaseTag, IPUWorkflowTag
 
@@ -24,16 +26,11 @@ def process(self):
             if grub_config_error.error_detected:
                 fix_grub_config_error('/etc/default/grub', grub_config_error.error_type)
 
-        configs = None
         ff = next(self.consume(FirmwareFacts), None)
         if not ff:
             raise StopActorExecutionError(
                 'Could not identify system firmware',
                 details={'details': 'Actor did not receive FirmwareFacts message.'}
             )
 
-        # related to issue with hybrid BIOS and UEFI images
-        # https://bugzilla.redhat.com/show_bug.cgi?id=1667028
-        if ff.firmware == 'bios' and os.path.ismount('/boot/efi') and os.path.isfile('/boot/efi/EFI/redhat/grub.cfg'):
-            configs = ['/boot/grub2/grub.cfg', '/boot/efi/EFI/redhat/grub.cfg']
-        add_boot_entry(configs)
+        add_boot_entry(get_hybrid_bios_efi_configs(ff.firmware))

repos/system_upgrade/common/actors/addupgradebootentry/libraries/addupgradebootentry.py

@@ -7,21 +7,44 @@
 from leapp.models import BootContent, KernelCmdlineArg, TargetKernelCmdlineArgTasks
 
 
+def get_hybrid_bios_efi_configs(firmware):
+    """
+    On a BIOS-firmware system with /boot/efi mounted, the EFI directory may also hold
+    a grub.cfg that GRUB consults at boot. grubby --copy-default --make-default only
+    updates /boot/grub2/grub.cfg by default; if the EFI-side grub.cfg is not also
+    updated, the new entry may not become the actual boot default.
+
+    The OS-name subdirectory under /boot/efi/EFI/ varies by distribution. Check the
+    known options and return the configs list for add_boot_entry, or None if no
+    EFI-side grub.cfg is present.
+
+    Related: https://bugzilla.redhat.com/show_bug.cgi?id=1667028
+    """
+    if firmware != 'bios' or not os.path.ismount('/boot/efi'):
+        return None
+    for efi_subdir in ('redhat', 'centos', 'cloudlinux', 'almalinux'):
+        efi_cfg = '/boot/efi/EFI/{0}/grub.cfg'.format(efi_subdir)
+        if os.path.isfile(efi_cfg):
+            return ['/boot/grub2/grub.cfg', efi_cfg]
+    return None
+
+
 def add_boot_entry(configs=None):
     debug = 'debug' if os.getenv('LEAPP_DEBUG', '0') == '1' else ''
     enable_network = os.getenv('LEAPP_DEVEL_INITRAM_NETWORK') in ('network-manager', 'scripts')
-    ip_arg = ' ip=dhcp rd.neednet=1' if enable_network else ''
+    ip_args = 'ip=dhcp rd.neednet=1' if enable_network else ''
     kernel_dst_path, initram_dst_path = get_boot_file_paths()
     _remove_old_upgrade_boot_entry(kernel_dst_path, configs=configs)
     try:
+        args_parts = [x for x in [debug, ip_args, 'enforcing=0 rd.plymouth=0 plymouth.enable=0'] if x]
         cmd = [
             '/usr/sbin/grubby',
             '--add-kernel', '{0}'.format(kernel_dst_path),
             '--initrd', '{0}'.format(initram_dst_path),
             '--title', 'ELevate-Upgrade-Initramfs',
             '--copy-default',
             '--make-default',
-            '--args', '{DEBUG}{NET} enforcing=0 rd.plymouth=0 plymouth.enable=0'.format(DEBUG=debug, NET=ip_arg)
+            '--args', ' '.join(args_parts)
         ]
         if configs:
             for config in configs:

repos/system_upgrade/common/actors/addupgradebootentry/tests/unit_test_addupgradebootentry.py

@@ -38,7 +38,7 @@ def __call__(self, filename, content):
     '--remove-kernel', '/abc'
 ]
 
-run_args_add = [
+run_args_add_debug = [
     '/usr/sbin/grubby',
     '--add-kernel', '/abc',
     '--initrd', '/def',
@@ -49,16 +49,27 @@ def __call__(self, filename, content):
     'debug enforcing=0 rd.plymouth=0 plymouth.enable=0'
     ]
 
+run_args_add_no_debug = [
+    '/usr/sbin/grubby',
+    '--add-kernel', '/abc',
+    '--initrd', '/def',
+    '--title', 'ELevate-Upgrade-Initramfs',
+    '--copy-default',
+    '--make-default',
+    '--args',
+    'enforcing=0 rd.plymouth=0 plymouth.enable=0'
+    ]
+
 run_args_zipl = ['/usr/sbin/zipl']
 
 
 @pytest.mark.parametrize('run_args, arch', [
     # non s390x
-    (RunArgs(run_args_remove, run_args_add, None, 2), ARCH_X86_64),
+    (RunArgs(run_args_remove, run_args_add_debug, None, 2), ARCH_X86_64),
     # s390x
-    (RunArgs(run_args_remove, run_args_add, run_args_zipl, 3), ARCH_S390X),
+    (RunArgs(run_args_remove, run_args_add_debug, run_args_zipl, 3), ARCH_S390X),
     # config file specified
-    (RunArgs(run_args_remove, run_args_add, None, 2), ARCH_X86_64),
+    (RunArgs(run_args_remove, run_args_add_debug, None, 2), ARCH_X86_64),
 ])
 def test_add_boot_entry(monkeypatch, run_args, arch):
     def get_boot_file_paths_mocked():
@@ -84,6 +95,27 @@ def get_boot_file_paths_mocked():
         assert addupgradebootentry.run.args[2] == run_args.args_zipl
 
 
+def test_add_boot_entry_no_debug(monkeypatch):
+    """--args value must not have a leading space when LEAPP_DEBUG is unset (the common path)."""
+    def get_boot_file_paths_mocked():
+        return '/abc', '/def'
+
+    monkeypatch.setattr(addupgradebootentry, 'get_boot_file_paths', get_boot_file_paths_mocked)
+    monkeypatch.setattr(api, 'produce', produce_mocked())
+    monkeypatch.setenv('LEAPP_DEBUG', '0')
+    monkeypatch.setattr(addupgradebootentry, 'run', run_mocked())
+    monkeypatch.setattr(api, 'current_actor', CurrentActorMocked(ARCH_X86_64))
+
+    addupgradebootentry.add_boot_entry()
+
+    assert len(addupgradebootentry.run.args) == 2
+    assert addupgradebootentry.run.args[0] == run_args_remove
+    assert addupgradebootentry.run.args[1] == run_args_add_no_debug
+    # Confirm the --args value has no leading space.
+    grubby_args_value = addupgradebootentry.run.args[1][-1]
+    assert not grubby_args_value.startswith(' ')
+
+
 @pytest.mark.parametrize('is_leapp_invoked_with_debug', [True, False])
 def test_debug_kernelopt_removal_task_production(monkeypatch, is_leapp_invoked_with_debug):
     def get_boot_file_paths_mocked():
@@ -124,8 +156,8 @@ def get_boot_file_paths_mocked():
     assert len(addupgradebootentry.run.args) == 4
     assert addupgradebootentry.run.args[0] == run_args_remove + ['-c', CONFIGS[0]]
     assert addupgradebootentry.run.args[1] == run_args_remove + ['-c', CONFIGS[1]]
-    assert addupgradebootentry.run.args[2] == run_args_add + ['-c', CONFIGS[0]]
-    assert addupgradebootentry.run.args[3] == run_args_add + ['-c', CONFIGS[1]]
+    assert addupgradebootentry.run.args[2] == run_args_add_debug + ['-c', CONFIGS[0]]
+    assert addupgradebootentry.run.args[3] == run_args_add_debug + ['-c', CONFIGS[1]]
     assert api.produce.model_instances == [
         TargetKernelCmdlineArgTasks(to_remove=[KernelCmdlineArg(key='debug')]),
         TargetKernelCmdlineArgTasks(to_remove=[KernelCmdlineArg(key='enforcing', value='0')])
@@ -153,6 +185,38 @@ def consume_no_message_mocked(*models):
         addupgradebootentry.get_boot_file_paths()
 
 
+@pytest.mark.parametrize(
+    'firmware, mount, existing_efi_cfgs, expected',
+    [
+        # EFI firmware: hybrid path does not apply, even if /boot/efi is mounted
+        ('efi', True, ['/boot/efi/EFI/redhat/grub.cfg'], None),
+        # BIOS firmware but /boot/efi not mounted
+        ('bios', False, ['/boot/efi/EFI/centos/grub.cfg'], None),
+        # BIOS + /boot/efi mounted, redhat path (RHEL-derived systems)
+        ('bios', True, ['/boot/efi/EFI/redhat/grub.cfg'],
+         ['/boot/grub2/grub.cfg', '/boot/efi/EFI/redhat/grub.cfg']),
+        # BIOS + /boot/efi mounted, centos path (CL7 hybrid setups - was missed before)
+        ('bios', True, ['/boot/efi/EFI/centos/grub.cfg'],
+         ['/boot/grub2/grub.cfg', '/boot/efi/EFI/centos/grub.cfg']),
+        # BIOS + /boot/efi mounted, cloudlinux path (CL8+ hybrid setups - was missed before)
+        ('bios', True, ['/boot/efi/EFI/cloudlinux/grub.cfg'],
+         ['/boot/grub2/grub.cfg', '/boot/efi/EFI/cloudlinux/grub.cfg']),
+        # BIOS + /boot/efi mounted, almalinux path
+        ('bios', True, ['/boot/efi/EFI/almalinux/grub.cfg'],
+         ['/boot/grub2/grub.cfg', '/boot/efi/EFI/almalinux/grub.cfg']),
+        # BIOS + /boot/efi mounted, no known grub.cfg present
+        ('bios', True, [], None),
+        # BIOS + /boot/efi mounted, multiple paths present - redhat wins (checked first)
+        ('bios', True, ['/boot/efi/EFI/redhat/grub.cfg', '/boot/efi/EFI/centos/grub.cfg'],
+         ['/boot/grub2/grub.cfg', '/boot/efi/EFI/redhat/grub.cfg']),
+    ]
+)
+def test_get_hybrid_bios_efi_configs(monkeypatch, firmware, mount, existing_efi_cfgs, expected):
+    monkeypatch.setattr(os.path, 'ismount', lambda p: p == '/boot/efi' and mount)
+    monkeypatch.setattr(os.path, 'isfile', lambda p: p in existing_efi_cfgs)
+    assert addupgradebootentry.get_hybrid_bios_efi_configs(firmware) == expected
+
+
 @pytest.mark.skip("Broken test")
 @pytest.mark.parametrize(
     ('error_type', 'test_file_name'),