curtin-dev team mailing list archive

[Chris Peterson <mp+476799@xxxxxxxxxxxxxxxxxx>]

Chris Peterson has proposed merging ~cpete/curtin:kdump-manual-enable-look-for-script into curtin:master.

Commit message:
kdump: manual enable look for detection script

Make the manual enablement code path for kernel crash dumps check for the
enablement script. Previously we assumed any ProcessExecutionError
that resulted was from the script not existing, but it may be the case
that it fails for other reasons. Let's check if it exists first and just
not call it if it doesn't. This means we can let any
ProcessExecutionErrors bubble up instead of catching all of them.

Requested reviews:
  Server Team CI bot (server-team-bot): continuous-integration
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~cpete/curtin/+git/curtin/+merge/476799
-- 
Your team curtin developers is requested to review the proposed merge of ~cpete/curtin:kdump-manual-enable-look-for-script into curtin:master.

diff --git a/curtin/kernel_crash_dumps.py b/curtin/kernel_crash_dumps.py
index 9c7ef4d..5012945 100644
--- a/curtin/kernel_crash_dumps.py
+++ b/curtin/kernel_crash_dumps.py
@@ -4,7 +4,7 @@ from pathlib import Path
 
 from curtin import distro
 from curtin.log import LOG
-from curtin.util import ChrootableTarget, ProcessExecutionError
+from curtin.util import ChrootableTarget
 
 ENABLEMENT_SCRIPT = "/usr/share/kdump-tools/kdump_set_default"
 
@@ -38,15 +38,20 @@ def detection_script_available(target: Path) -> bool:
 def manual_enable(target: Path) -> None:
     """Manually enable kernel crash dumps with kdump-tools on target."""
     ensure_kdump_installed(target)
-    try:
+    if detection_script_available(target):
         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.
+    else:
+        # Enablement script not found. Likely scenario is that enablement was
+        # requested on a pre-24.10 series but the script hasn't been SRU'd yet.
+        # This is OK since installing on these series will mean kdump-tools
+        # is enabled by default.
+        # Let's not block the install on this but at least warn the user.
         LOG.warning(
-            "Unable to run kernel-crash-dumps enablement script: %s",
-            exc,
+            (
+                "kernel-crash-dumps enablement requested but enablement "
+                "script not found. Not running."
+            ),
         )
 
 
diff --git a/tests/unittests/test_kernel_crash_dumps.py b/tests/unittests/test_kernel_crash_dumps.py
index 54faf0a..5747100 100644
--- a/tests/unittests/test_kernel_crash_dumps.py
+++ b/tests/unittests/test_kernel_crash_dumps.py
@@ -10,6 +10,7 @@ from curtin.kernel_crash_dumps import (ENABLEMENT_SCRIPT, automatic_detect,
                                        detection_script_available,
                                        ensure_kdump_installed, manual_disable,
                                        manual_enable)
+from curtin.util import ProcessExecutionError
 from tests.unittests.helpers import CiTestCase
 
 
@@ -106,7 +107,13 @@ class TestKernelCrashDumpsUtilities(CiTestCase):
         else:
             do_install.assert_called_with(["kdump-tools"], target=str(target))
 
-    def test_manual_enable(self):
+    @parameterized.expand(
+        (
+            (True,),
+            (False,),
+        )
+    )
+    def test_manual_enable(self, detection_script_available):
         """Test manual enablement logic."""
         target = Path("/target")
         with (
@@ -117,13 +124,46 @@ class TestKernelCrashDumpsUtilities(CiTestCase):
                 "curtin.kernel_crash_dumps.ChrootableTarget",
                 new=MagicMock(),
             ) as chroot_mock,
+            patch(
+                "curtin.kernel_crash_dumps.detection_script_available",
+                return_value=detection_script_available,
+            ),
         ):
             manual_enable(target)
+
         ensure_mock.assert_called_once()
         subp_mock = chroot_mock.return_value.__enter__.return_value.subp
-        subp_mock.assert_called_with(
-            [ENABLEMENT_SCRIPT, "true"],
-        )
+
+        if detection_script_available:
+            subp_mock.assert_called_with(
+                [ENABLEMENT_SCRIPT, "true"],
+            )
+
+        else:
+            subp_mock.assert_not_called()
+
+    def test_manual_enable__exceptions_not_masked(self):
+        """Test ProcessExecutionErrors during manual enablement bubble up."""
+        target = Path("/target")
+        with (
+            patch(
+                "curtin.kernel_crash_dumps.ensure_kdump_installed",
+            ),
+            patch(
+                "curtin.kernel_crash_dumps.ChrootableTarget",
+                new=MagicMock(),
+            ) as ch_mock,
+            patch(
+                "curtin.kernel_crash_dumps.detection_script_available",
+                return_value=True,
+            ),
+        ):
+
+            ch_mock.return_value.__enter__.return_value.subp.side_effect = (
+                ProcessExecutionError()
+            )
+            with self.assertRaises(ProcessExecutionError):
+                manual_enable(target)
 
     @parameterized.expand(
         (