Thanks for this branch Ryan. It looks much better than and more modular now.
I've added a number of comments and questions for you.
Diff comments:
> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 4afe00c..f705711 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -682,28 +683,6 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
> else:
> instdevs = list(blockdevs)
>
> - env = os.environ.copy()
> -
> - replace_default = grubcfg.get('replace_linux_default', True)
> - if str(replace_default).lower() in ("0", "false"):
> - env['REPLACE_GRUB_LINUX_DEFAULT'] = "0"
> - else:
> - env['REPLACE_GRUB_LINUX_DEFAULT'] = "1"
Probably worth noting in the commit that curtin is writing config defaults to
/etc/default/grub file now instead of environment variables.
> -
> - probe_os = grubcfg.get('probe_additional_os', False)
> - if probe_os not in (False, True):
> - raise ValueError("Unexpected value %s for 'probe_additional_os'. "
> - "Value must be boolean" % probe_os)
> - env['DISABLE_OS_PROBER'] = "0" if probe_os else "1"
We have migrated from setting DISABLE_OS_PROBER in the environment to settings
GRUB_DISABLE_OS_PROBER. It's probably worth mentioning in the top-level commit
message.
> -
> - # if terminal is present in config, but unset, then don't
> - grub_terminal = grubcfg.get('terminal', 'console')
> - if not isinstance(grub_terminal, str):
> - raise ValueError("Unexpected value %s for 'terminal'. "
> - "Value must be a string" % grub_terminal)
> - if not grub_terminal.lower() == "unmodified":
> - env['GRUB_TERMINAL'] = grub_terminal
> -
> if instdevs:
> instdevs = [block.get_dev_name_entry(i)[1] for i in instdevs]
> if osfamily == DISTROS.debian:
> @@ -711,38 +690,15 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
> else:
> instdevs = ["none"]
>
> - if uefi_bootable and grubcfg.get('update_nvram', True):
> + update_nvram = grubcfg.get('update_nvram', True)
Should we be using config.value_as_boolean(grubcfg.get('update_nvram', True)
here too or are we moving away from the more flexible accept any true-ish
false-ish value and maybe we should tighten up both logical paths here and in
install_grub?
Also, docs say "update_nvram: <boolean: default False>" but it looks like it
defaults to True here.
> + if uefi_bootable and update_nvram:
> uefi_remove_old_loaders(grubcfg, target)
>
> LOG.debug("installing grub to %s [replace_default=%s]",
Can we move this debig log message inside our proper install_grub function now?
> - instdevs, replace_default)
> + instdevs, grubcfg.get('replace_default'))
> + install_grub(instdevs, target, uefi=uefi_bootable, grubcfg=grubcfg)
Should we be adding the ReportEventStack just as you've added in
curtin/install/install_grub.py main ?
>
> - with util.ChrootableTarget(target):
> - args = ['install-grub']
> - if uefi_bootable:
> - args.append("--uefi")
> - LOG.debug("grubcfg: %s", grubcfg)
> - if grubcfg.get('update_nvram', True):
> - LOG.debug("GRUB UEFI enabling NVRAM updates")
> - args.append("--update-nvram")
> - else:
> - LOG.debug("NOT enabling UEFI nvram updates")
> - LOG.debug("Target system may not boot")
> - if len(instdevs) > 1:
> - instdevs = [instdevs[0]]
> - LOG.debug("Selecting primary EFI boot device %s for install",
> - instdevs[0])
> -
> - args.append('--os-family=%s' % osfamily)
> - args.append(target)
> -
> - # capture stdout and stderr joined.
> - join_stdout_err = ['sh', '-c', 'exec "$0" "$@" 2>&1']
> - out, _err = util.subp(
> - join_stdout_err + args + instdevs, env=env, capture=True)
> - LOG.debug("%s\n%s\n", args + instdevs, out)
> -
> - if uefi_bootable and grubcfg.get('update_nvram', True):
> + if uefi_bootable and update_nvram:
> uefi_remove_duplicate_entries(grubcfg, target)
> uefi_reorder_loaders(grubcfg, target)
>
> diff --git a/curtin/commands/install_grub.py b/curtin/commands/install_grub.py
> new file mode 100644
> index 0000000..c1571b1
> --- /dev/null
> +++ b/curtin/commands/install_grub.py
> @@ -0,0 +1,366 @@
> +import os
> +import re
> +import shutil
> +import sys
> +
> +from curtin import block
> +from curtin import config
> +from curtin import distro
> +from curtin import util
> +from curtin.log import LOG
> +from curtin.paths import target_path
> +from curtin.reporter import events
> +from . import populate_one_subcmd
> +
> +CMD_ARGUMENTS = (
> + ((('-t', '--target'),
> + {'help': 'operate on target. default is env[TARGET_MOUNT_POINT]',
> + 'action': 'store', 'metavar': 'TARGET', 'default': None}),
> + (('-c', '--config'),
> + {'help': 'operate on config. default is env[CONFIG]',
> + 'action': 'store', 'metavar': 'CONFIG', 'default': None}),
> + )
> +)
> +
> +GRUB_MULTI_INSTALL = '/usr/lib/grub/grub-multi-install'
> +
> +
> +def get_grub_package(target_arch, uefi, rhel_ver=None):
> + if target_arch is None:
> + raise ValueError('Missing target_arch parameter')
> +
> + if uefi is None:
> + raise ValueError('Missing uefi parameter')
> +
> + if 'ppc64' in target_arch:
> + return ('grub-ieee1275', 'powerpc-ieee1275')
> + if uefi:
> + if target_arch == 'amd64':
> + grub_name = 'grub-efi-%s' % target_arch
> + grub_target = "x86_64-efi"
> + elif target_arch == 'x86_64':
> + # centos 7+, no centos6 support
> + # grub2-efi-x64 installs a signed grub bootloader while
> + # curtin uses grub2-efi-x64-modules to generate grubx64.efi.
> + # Either works just check that one of them is installed.
> + grub_name = "grub2-efi-x64"
> + grub_target = "x86_64-efi"
> + elif target_arch == 'arm64':
> + grub_name = 'grub-efi-%s' % target_arch
> + grub_target = "arm64-efi"
> + elif target_arch == 'i386':
> + grub_name = 'grub-efi-ia32'
> + grub_target = 'i386-efi'
> + else:
> + raise ValueError('Unsupported UEFI arch: %s' % target_arch)
> + else:
> + grub_target = 'i386-pc'
> + if target_arch in ['i386', 'amd64']:
> + grub_name = 'grub-pc'
> + elif target_arch == 'x86_64':
> + if rhel_ver == '6':
> + grub_name = 'grub'
> + elif rhel_ver in ['7', '8']:
> + grub_name = 'grub2-pc'
> + else:
> + raise ValueError('Unsupported RHEL version: %s', rhel_ver)
> + else:
> + raise ValueError('Unsupported arch: %s' % target_arch)
> +
> + return (grub_name, grub_target)
> +
> +
> +def get_grub_config_file(distroinfo):
> + if distroinfo.family == distro.DISTROS.redhat:
> + return '/etc/default/grub'
> + # ubuntu writes to /etc/default/grub.d/50-curtin-settings.cfg
> + # to avoid tripping prompts on upgrade LP: #564853
> + return '/etc/default/grub.d/50-curtin-settings.cfg'
> +
> +
> +def prepare_grub_dir(target, grub_cfg):
> + util.ensure_dir(os.path.dirname(target_path(target, grub_cfg)))
> +
> + # LP: #1179940 . The 50-cloudig-settings.cfg file is written by the cloud
> + # images build and defines/override some settings. Disable it.
> + ci_cfg = target_path(target,
> + os.path.join(
> + os.path.dirname(grub_cfg),
> + "50-cloudimg-settings.cfg"))
> +
> + if os.path.exists(ci_cfg):
> + LOG.debug('grub: moved %s out of the way', ci_cfg)
> + shutil.move(ci_cfg, ci_cfg + '.disabled')
> +
> +
> +def get_carryover_params(distroinfo):
> + # return a string to append to installed systems boot parameters
> + # it may include a '--' after a '---'
> + # see LP: 1402042 for some history here.
> + # this is similar to 'user-params' from d-i
> + cmdline = util.load_file('/proc/cmdline')
> + preferred_sep = '---' # KERNEL_CMDLINE_COPY_TO_INSTALL_SEP
> + legacy_sep = '--'
> +
> + def wrap(sep):
> + return ' ' + sep + ' '
> +
> + if wrap(preferred_sep) in cmdline:
> + lead, extra = cmdline.split(wrap(preferred_sep))
> + elif wrap(legacy_sep) in cmdline:
> + lead, extra = cmdline.split(wrap(legacy_sep))
> + else:
> + extra = ""
> + lead = cmdline
> +
> + carry_extra = []
> + if extra:
> + for tok in extra.split():
> + if re.match(r'(BOOTIF=.*|initrd=.*|BOOT_IMAGE=.*)', tok):
> + continue
> + carry_extra.append(tok)
> +
> + carry_lead = []
> + for tok in lead.split():
> + if tok in carry_extra:
> + continue
> + if tok.startswith('console='):
> + carry_lead.append(tok)
> +
> + # always append rd.auto=1 for redhat family
> + if distroinfo.family == distro.DISTROS.redhat:
> + carry_extra.append('rd.auto=1')
> +
> + return carry_lead + carry_extra
> +
> +
> +def replace_grub_cmdline_linux_default(target, new_args):
> + # we always update /etc/default/grub to avoid "hiding" the override in
> + # a grub.d directory.
> + newcontent = 'GRUB_CMDLINE_LINUX_DEFAULT="%s"' % " ".join(new_args)
> + target_grubconf = target_path(target, '/etc/default/grub')
> + content = ""
> + if os.path.exists(target_grubconf):
> + content = util.load_file(target_grubconf)
> + existing = re.search(
> + r'GRUB_CMDLINE_LINUX_DEFAULT=.*', content, re.MULTILINE)
> + if existing:
> + omode = 'w+'
> + updated_content = content[:existing.start()]
> + updated_content += newcontent
> + updated_content += content[existing.end():]
> + else:
> + omode = 'a+'
> + updated_content = newcontent + '\n'
> +
> + util.write_file(target_grubconf, updated_content, omode=omode)
> + LOG.debug('updated %s to set: %s', target_grubconf, newcontent)
> +
> +
> +def write_grub_config(target, grubcfg, grub_conf, new_params):
> + replace_default = grubcfg.get('replace_linux_default', True)
> + if replace_default:
> + replace_grub_cmdline_linux_default(target, new_params)
> +
> + probe_os = grubcfg.get('probe_additional_os', False)
> + if probe_os not in (False, True):
> + raise ValueError("Unexpected value %s for 'probe_additional_os'. "
> + "Value must be boolean" % probe_os)
> + if not probe_os:
> + probe_content = [
> + ('# Curtin disable grub os prober that might find other '
> + 'OS installs.'),
> + 'GRUB_DISABLE_OS_PROBER="true"',
> + '']
> + util.write_file(target_path(target, grub_conf),
> + "\n".join(probe_content), omode='a+')
> +
> + # if terminal is present in config, but unset, then don't
> + grub_terminal = grubcfg.get('terminal', 'console')
> + if not isinstance(grub_terminal, str):
> + raise ValueError("Unexpected value %s for 'terminal'. "
> + "Value must be a string" % grub_terminal)
> + if not grub_terminal.lower() == "unmodified":
> + terminal_content = [
> + '# Curtin configured GRUB_TERMINAL value',
> + 'GRUB_TERMINAL="%s"' % grub_terminal]
> + util.write_file(target_path(target, grub_conf),
> + "\n".join(terminal_content), omode='a+')
> +
> +
> +def find_efi_loader(target, bootid):
> + efi_path = '/boot/efi/EFI'
> + possible_loaders = [
> + os.path.join(efi_path, bootid, 'shimx64.efi'),
> + os.path.join(efi_path, 'BOOT', 'BOOTX64.EFI'),
> + os.path.join(efi_path, bootid, 'grubx64.efi'),
> + ]
> + for loader in possible_loaders:
> + tloader = target_path(target, path=loader)
> + if os.path.exists(tloader):
> + LOG.debug('find_efi_loader: found %s', loader)
> + return loader
> + return None
> +
> +
> +def get_efi_disk_part(devices):
> + for disk in devices:
> + (parent, partnum) = block.get_blockdev_for_partition(disk)
> + if partnum:
> + return (parent, partnum)
> +
> + return (None, None)
> +
> +
> +def get_grub_install_command(uefi, distroinfo, target):
> + grub_install_cmd = 'grub-install'
> + if distroinfo.family == distro.DISTROS.debian:
> + # prefer grub-multi-install if present
> + if uefi and os.path.exists(target_path(target, GRUB_MULTI_INSTALL)):
> + grub_install_cmd = GRUB_MULTI_INSTALL
> + elif distroinfo.family == distro.DISTROS.redhat:
> + grub_install_cmd = 'grub2-install'
> +
> + LOG.debug('Using grub install command: %s', grub_install_cmd)
> + return grub_install_cmd
> +
> +
> +def gen_uefi_install_commands(grub_name, grub_target, grub_cmd, update_nvram,
> + distroinfo, devices, target):
> + install_cmds = [['efibootmgr', '-v']]
> + post_cmds = []
> + bootid = distroinfo.variant
> + efidir = '/boot/efi'
> + if distroinfo.family == distro.DISTROS.debian:
> + install_cmds.append(['dpkg-reconfigure', grub_name])
> + install_cmds.append(['update-grub'])
> + elif distroinfo.family == distro.DISTROS.redhat:
> + loader = find_efi_loader(target, bootid)
> + if loader and update_nvram:
> + grub_cmd = None # don't install just add entry
> + efi_disk, efi_part_num = get_efi_disk_part(devices)
> + install_cmds.append(['efibootmgr', '--create',
> '--write-signature',
> + '--label', bootid, '--disk', efi_disk,
> + '--part', efi_part_num, '--loader', loader])
> + post_cmds.append(['grub2-mkconfig', '-o',
> + '/boot/efi/EFI/%s/grub.cfg' % bootid])
> + else:
> + post_cmds.append(['grub2-mkconfig', '-o',
> '/boot/grub2/grub.cfg'])
> + else:
> + raise ValueError("Unsupported os family for grub "
> + "install: %s" % distroinfo.family)
> +
> + if grub_cmd == GRUB_MULTI_INSTALL:
> + # grub-multi-install is called with no arguments
> + install_cmds.append([grub_cmd])
> + elif grub_cmd:
> + install_cmds.append(
> + [grub_cmd, '--target=%s' % grub_target,
> + '--efi-directory=%s' % efidir, '--bootloader-id=%s' % bootid,
> + '--recheck'] + ([] if update_nvram else ['--no-nvram']))
> +
> + # check efi boot menu before and after
> + post_cmds.append(['efibootmgr', '-v'])
> +
> + return (install_cmds, post_cmds)
> +
> +
> +def gen_install_commands(grub_name, grub_cmd, distroinfo, devices,
> + rhel_ver=None):
> + install_cmds = []
> + post_cmds = []
> + if distroinfo.family == distro.DISTROS.debian:
> + install_cmds.append(['dpkg-reconfigure', grub_name])
> + install_cmds.append(['update-grub'])
> + elif distroinfo.family == distro.DISTROS.redhat:
> + if rhel_ver in ["7", "8"]:
> + post_cmds.append(
> + ['grub2-mkconfig', '-o', '/boot/grub2/grub.cfg'])
> + else:
> + raise ValueError('Unsupported "rhel_ver" value: %s' % rhel_ver)
> + else:
> + raise ValueError("Unsupported os family for grub "
> + "install: %s" % distroinfo.family)
> + for dev in devices:
> + install_cmds.append([grub_cmd, dev])
> +
> + return (install_cmds, post_cmds)
> +
> +
> +def install_grub(devices, target, uefi=None, grubcfg=None):
> + """Install grub to devices inside target chroot.
> +
> + :param: devices: List of block device paths to install grub upon.
> + :param: target: A string specifying the path to the chroot mountpoint.
> + :param: uefi: A boolean set to True if system is UEFI bootable otherwise
> + False.
> + :param: grubcfg: An config dict with grub config options.
> + """
> +
> + if not devices:
> + raise ValueError("Invalid parameter 'devices': %s" % devices)
> +
> + if not target:
> + raise ValueError("Invalid parameter 'target': %s" % target)
> +
> + LOG.debug("install_grub: target=%s devices=%s", target, devices)
We could emit replace_default=%s here too right and consolidate log messages?
> + update_nvram = config.value_as_boolean(grubcfg.get('update_nvram', True))
> + distroinfo = distro.get_distroinfo(target=target)
> + target_arch = distro.get_architecture(target=target)
> + rhel_ver = (distro.rpm_get_dist_id(target)
> + if distroinfo.family == distro.DISTROS.redhat else None)
> +
> + if target_arch == "s390x":
> + raise RuntimeError("Grub is not valid for s390x")
How are consumers going to get down this path, the CLI? curthooks doesn't call
this path as it exits out early before calling setup_grub. Do we expect maybe
that custom curthooks would call into setup_grub directly?
Also if we are raising RuntimeError for that path, should be also raise on
aarch64, armv7 and not util.is_uefi_bootable ?
Also, even get_grub_package called by install_grub.py:main raises a ValueError
on architecture incompatibility, so we wouldn't specifically need this 390x
case as we call get_grub_package on the next line.
> + # verify_target_device_mount(target)
> + grub_name, grub_target = get_grub_package(target_arch, uefi, rhel_ver)
> + grub_conf = get_grub_config_file(distroinfo)
> + new_params = get_carryover_params(distroinfo)
> + prepare_grub_dir(target, grub_conf)
> + write_grub_config(target, grubcfg, grub_conf, new_params)
> + grub_cmd = get_grub_install_command(uefi, distroinfo, target)
> + if uefi:
> + install_cmds, post_cmds = gen_uefi_install_commands(
> + grub_name, grub_target, grub_cmd, update_nvram, distroinfo,
> + devices, target)
> + else:
> + install_cmds, post_cmds = gen_install_commands(
> + grub_name, grub_cmd, distroinfo, devices, rhel_ver)
> +
> + env = os.environ.copy()
> + env['DEBIAN_FRONTEND'] = 'noninteractive'
> +
> + LOG.debug('Grub install cmds:\n%s', str(install_cmds + post_cmds))
> + with util.ChrootableTarget(target) as in_chroot:
> + for cmd in install_cmds + post_cmds:
> + in_chroot.subp(cmd, env=env, capture=True)
> +
> +
> +def install_grub_main(args):
> + state = util.load_command_environment()
> +
> + if args.target is not None:
> + target = args.target
> + else:
> + target = state['target']
> +
> + if target is None:
> + sys.stderr.write("Unable to find target. "
> + "Use --target or set TARGET_MOUNT_POINT\n")
> + sys.exit(2)
> +
> + cfg = config.load_command_config(args, state)
> + stack_prefix = state.get('report_stack_prefix', '')
> + uefi = util.is_uefi_bootable()
> + grubcfg = cfg.get('grub')
> + with events.ReportEventStack(
> + name=stack_prefix, reporting_enabled=True, level="INFO",
> + description="Installing grub to target devices"):
> + install_grub(args.devices, target, uefi=uefi, grubcfg=grubcfg)
> + sys.exit(0)
> +
> +
> +def POPULATE_SUBCMD(parser):
> + populate_one_subcmd(parser, CMD_ARGUMENTS, install_grub_main)
> +
> +# vi: ts=4 expandtab syntax=python
> diff --git a/curtin/distro.py b/curtin/distro.py
> index 1f62e7a..061665b 100644
> --- a/curtin/distro.py
> +++ b/curtin/distro.py
> @@ -549,4 +550,29 @@ def fstab_header():
> #
> # <file system> <mount point> <type> <options> <dump> <pass>""")
>
> +
> +def dpkg_get_architecture(target=None):
> + out, _ = subp(['dpkg', '--print-architecture'], capture=True,
> + target=target)
> + return out.strip()
> +
> +
> +def rpm_get_architecture(target=None):
> + # rpm requires /dev mounts
> + with ChrootableTarget(target) as in_chroot:
> + out, _ = in_chroot.subp(['rpm', '-E', '%_arch'], capture=True)
Why are we using the ChrootableTarget context manager here and not passing
target=target to subp like dpkg_get_architecture does? which one is preferred
and why?
> + return out.strip()
> +
> +
> +def get_architecture(target=None, osfamily=None):
> + if not osfamily:
> + osfamily = get_osfamily(target=target)
> +
> + if osfamily == DISTROS.debian:
> + return dpkg_get_architecture(target=target)
> +
> + if osfamily == DISTROS.redhat:
> + return rpm_get_architecture(target=target)
> +
We potentially return None here if ! debian or redhat. I don't think all
callsites handle a potential None value, should this raise a ValueError or
RuntimeError?
> +
> # vi: ts=4 expandtab syntax=python
--
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/382931
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
