curtin-dev team mailing list archive

[Dan Bungert <mp+473169@xxxxxxxxxxxxxxxxxx>]

nitpicks, also comments about naming
actual functionality looks correct, thanks!

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 0539c83..07575b7 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -1569,6 +1570,29 @@ def configure_nvme_over_tcp(cfg, target: pathlib.Path) -> None:
>          nvme_tcp.configure_nvme_stas(cfg, target)
>  
>  
> +def configure_kernel_crash_dumps(cfg, target: pathlib.Path) -> None:
> +    """Configure kernel crash dumps on target system.
> +
> +    kernel-crash-dumps:
> +        enabled: bool | None
> +
> +    If `enabled` is `None` then kernel crash dumps will be dynamically enabled
> +    if both the kdump-tools package is installed on the target system and it
> +    also provides the expected enablement script (Starting in 24.10).
> +    """
> +    kdump_cfg = cfg.get("kernel-crash-dumps", {})

I won't block this MP on this request, but for new stuff we're moving to Attrs classes instead of relying on dictionaries.

> +
> +    enabled: bool = kdump_cfg.get("enabled")
> +    automatic = enabled is None
> +
> +    if automatic:
> +        kernel_crash_dumps.automatic_enable(target)

nitpick: maybe "detect" instead of "automatic_enable", having "automatic_enable" function as a disable is modestly unexpected.

> +    elif enabled:
> +        kernel_crash_dumps.manual_enable(target)
> +    else:
> +        kernel_crash_dumps.manual_disable(target)
> +
> +
>  def handle_cloudconfig(cfg, base_dir=None):
>      """write cloud-init configuration files into base_dir.
>  
> diff --git a/curtin/kernel_crash_dumps.py b/curtin/kernel_crash_dumps.py
> new file mode 100644
> index 0000000..e05838a
> --- /dev/null
> +++ b/curtin/kernel_crash_dumps.py
> @@ -0,0 +1,70 @@
> +# This file is part of curtin. See LICENSE file for copyright and license info.
> +
> +from pathlib import Path
> +
> +from curtin import distro
> +from curtin.log import LOG
> +from curtin.util import ChrootableTarget, ProcessExecutionError
> +
> +ENABLEMENT_SCRIPT = "/usr/share/kdump-tools/kdump_set_default"
> +
> +
> +def ensure_kdump_installed(target: Path) -> None:
> +    """Ensure kdump-tools installed on target system and install it if not.
> +
> +    kdump-tools is theoretically part of the base-install in >=24.10
> +    but we may need to dynamically install it if manual enablement is
> +    requested.
> +    """
> +    if "kdump-tools" not in distro.get_installed_packages(str(target)):
> +        distro.install_packages(["kdump-tools"], target=str(target))
> +
> +
> +def automatic_enable_available(target: Path) -> bool:
> +    """Detect existence of the enablement script.
> +
> +    Enablement script will only be found on targets where kdump-tools is
> +    pre-installed and it's a version which contains the script.
> +    """
> +    path = target / ENABLEMENT_SCRIPT[1:]
> +    if path.exists():
> +        LOG.debug("kernel-crash-dumps enablement script found.")
> +        return True
> +    else:
> +        LOG.debug("kernel-crash-dumps enablement script missing.")
> +        return False
> +
> +
> +def manual_enable(target: Path) -> None:
> +    """Manually enable kernel crash dumps with kdump-tools on target."""
> +    ensure_kdump_installed(target)
> +    try:
> +        with ChrootableTarget(str(target)) as in_target:
> +            in_target.subp([ENABLEMENT_SCRIPT, "true"])
> +    except ProcessExecutionError as exc:
> +        # Likely the enablement script hasn't been SRU'd
> +        # Let's not block the install on this.
> +        LOG.warning(
> +            "Unable to run kernel-crash-dumps enablement script: %s",
> +            exc,
> +        )
> +
> +
> +def manual_disable(target: Path) -> None:
> +    """Manually disable kernel crash dumps with kdump-tools on target."""
> +    if "kdump-tools" in distro.get_installed_packages(str(target)):
> +        with ChrootableTarget(str(target)) as in_target:
> +            in_target.subp([ENABLEMENT_SCRIPT, "false"])
> +    return

nitpick: redundant

> +
> +
> +def automatic_enable(target: Path) -> None:
> +    """Perform automatic kernel crash dumps enable with kdump-tools on target.
> +
> +    Uses the enablement script provided by kdump-tools to dynamically enable
> +    or disable kernel-crash-dumps if the enablement script is found.
> +    """
> +    if automatic_enable_available(target):
> +        LOG.debug("Running dynamic enablement script...")
> +        with ChrootableTarget(str(target)) as in_target:
> +            in_target.subp([ENABLEMENT_SCRIPT])
> diff --git a/doc/topics/config.rst b/doc/topics/config.rst
> index b579f37..bfd3ad4 100644
> --- a/doc/topics/config.rst
> +++ b/doc/topics/config.rst
> @@ -475,6 +476,41 @@ After kernel installation, remove the listed packages.
>      package: linux-image-generic-hwe-24.04
>      remove: ["linux-generic"]
>  
> +kernel-crash-dumps
> +~~~~~~~~~~~~~~~~~~
> +Configure how Curtin will configure kernel crash dumps in the target system
> +using the ``kdump-tools`` package. If ``kernel-crash-dumps`` is not configured,
> +Curtin will attempt to use ``kdump-tools`` to dynamically enable kernel crash

Do we want to say something like "if certain criteria are met" ?  I think we can be vague on the details and link to the doc content when we have it, but I think we should say something.

> +dumps on the target machine. This requires ``kdump-tools`` to be installed in
> +the target system before the hook is ran. It will not install ``kdump-tools``
> +by default.
> +
> +**enabled**: *<boolean or None: default None>*
> +
> +Enable, disable, or allow dynamic enablement of kernel crash dumps on the target

I'm not sure "dynamic enablement" is the best phrasing, what do you think about focusing on the "detection" part?  Forcing the setting to be On is kind of dynamic enablement, since we're doing that at install time, and like mentioned in a previous comment, "dynamic enablement" is not necessarily obvious that it will sometimes disable.

> +system for values of ``true``, ``false``, and ``None``, respectively.
> +
> +If ``enabled`` is set to ``true``, Curtin will install the ``kdump-tools``
> +package if it is not installed already and then enable kernel crash dumps in
> +the target system.
> +
> +If ``enabled`` is set to ``false``, Curtin will ensure kernel crash dumps are
> +disabled in the target system but it **will not uninstall the package**.
> +
> +**Examples**::
> +
> +  # Default: dynamically enable kernel crash dumps if kdump-tools is installed.
> +  # on the target system.
> +  kernel-crash-dumps:
> +    enabled: null
> +
> +  # Unconditionally enable kernel-crash-dumps.
> +  kernel-crash-dumps:
> +    enabled: true
> +
> +  # Unconditionally disable kernel-crash-dumps.
> +  kernel-crash-dumps:
> +    enabled: false
>  
>  kexec
>  ~~~~~


-- 
https://code.launchpad.net/~cpete/curtin/+git/curtin/+merge/473169
Your team curtin developers is requested to review the proposed merge of ~cpete/curtin:kernel-crash-dumps-introduction into curtin:master.