curtin-dev team mailing list archive

[Dan Bungert <mp+473473@xxxxxxxxxxxxxxxxxx>]

Dan Bungert has proposed merging ~dbungert/curtin:kernel-remove-tweaks into curtin:master.

Commit message:
Do not squash

Update config kernel interface to merge the two removal options into one keyword.
Modest test coverage improvements.
Comment improvements.
Minor logic clarification.

Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/473473
-- 
Your team curtin developers is requested to review the proposed merge of ~dbungert/curtin:kernel-remove-tweaks into curtin:master.

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 25568b0..a8cc4c6 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -374,10 +374,10 @@ def install_kernel(cfg, target):
     kernel_cfg = config.fromdict(config.KernelConfig, kernel_cfg_d)
 
     with contextlib.ExitStack() as exitstack:
-        if kernel_cfg.remove_existing or kernel_cfg.remove:
+        if kernel_cfg.remove_needed():
             exitstack.enter_context(
                 distro.ensure_one_kernel(
-                    target=target, before=kernel_cfg.remove,
+                    target=target, before=kernel_cfg.kernels_to_remove(),
                 )
             )
         if not kernel_cfg.install:
diff --git a/curtin/config.py b/curtin/config.py
index 74451c8..037b4bb 100644
--- a/curtin/config.py
+++ b/curtin/config.py
@@ -160,8 +160,17 @@ class KernelConfig:
     fallback_package: str = "linux-generic"
     mapping: dict = attr.Factory(dict)
     install: bool = attr.ib(default=True, converter=value_as_boolean)
-    remove_existing: bool = attr.ib(default=False, converter=value_as_boolean)
-    remove: list = attr.Factory(list)
+    remove: typing.Union[None | list | str] = None
+
+    def remove_needed(self) -> bool:
+        if to_remove := self.kernels_to_remove():
+            return bool(to_remove)
+        return self.remove == "existing"
+
+    def kernels_to_remove(self) -> typing.Optional[list]:
+        if isinstance(self.remove, list):
+            return self.remove
+        return None
 
 
 class SerializationError(Exception):
@@ -225,14 +234,18 @@ class Deserializer:
             ]
 
     def _walk_Union(self, meth, args, context):
+        if context.cur is None:
+            return context.cur
         NoneType = type(None)
         if NoneType in args:
             args = [a for a in args if a is not NoneType]
             if len(args) == 1:
                 # I.e. Optional[thing]
-                if context.cur is None:
-                    return context.cur
                 return meth(args[0], context)
+        if isinstance(context.cur, list):
+            return meth(list, context)
+        if isinstance(context.cur, str):
+            return meth(str, context)
         context.error(f"cannot serialize Union[{args}]")
 
     def _deserialize_attr(self, annotation, context):
diff --git a/curtin/distro.py b/curtin/distro.py
index 219b854..e6e13d5 100644
--- a/curtin/distro.py
+++ b/curtin/distro.py
@@ -519,7 +519,7 @@ def list_kernels(osfamily=None, target=None):
 def ensure_one_kernel(osfamily=None, target=None, before=None):
     """ensure_one_kernel is a context manager that evalutates the state of
     installed kernels, before and after doing package operations.  With that
-    information, kernels that only appear before and not after are removed.
+    information, kernels that are unique to the before set are removed.
     """
     if bool(before):
         before = set(before)
@@ -538,10 +538,12 @@ def ensure_one_kernel(osfamily=None, target=None, before=None):
     # that the kernel we asked to install was installed already, so we
     # shouldn't be removing one.  This should work fine for a single kernel
     # being preinstalled, but will fail to remove in the case of 2 kernels
-    # preinstalled but only one of them is intended.
+    # preinstalled but only one of them is intended.  We seed the before list
+    # externally to handle that case, when the above `before = list_kernels()`
+    # is too late to accurately capture the initial state.
     after = set(list_kernels(osfamily=osfamily, target=target))
     LOG.debug('ensure_one_kernel: kernels after install %s', after)
-    if not bool(after - before):
+    if before == after:
         LOG.debug(
             'No kernels to remove - kernel to install was already installed'
         )
diff --git a/doc/topics/config.rst b/doc/topics/config.rst
index fb0916f..4fbcc5a 100644
--- a/doc/topics/config.rst
+++ b/doc/topics/config.rst
@@ -439,17 +439,21 @@ Specify the exact package to install in the target OS.
 
 Defaults to True.  If False, no kernel install is attempted.
 
-**remove_existing**: *<boolean>*
 
-Supported on Debian and Ubuntu OSes.  Defaults to False.  If True, known
-kernels in .deb packages are removed from the target system (packages which
-``Provides: linux-image``), followed by an ``apt-get autoremove``.  If no
-kernel is being installed, this also implies the removal of the
-``linux-firmware`` package.
+**remove**: *<List of kernel package names> | "existing"*
 
-**remove**: *<List of kernel package names>*
+After kernel installation, remove the listed packages.  Defaults to no action.
 
-After kernel installation, remove the listed packages.
+Instead of a list, the keyword ``existing`` may be supplied, indicating that
+all existing kernels must be removed.  (Supported on Debian and Ubuntu OSes)
+Known kernels in .deb packages are removed from the target system (packages
+which ``Provides: linux-image``), followed by an ``apt-get autoremove``.
+It's expected that some sort of other kernel is specified with ``package`` to
+be the replacement.
+
+The kernel removal code has a guardrail against the case where the target
+system is left without a kernel installed, in which case this ``remove``
+directive may be ignored.
 
 **Examples**::
 
@@ -464,12 +468,7 @@ After kernel installation, remove the listed packages.
   # and remove other kernels if present
   kernel:
     package: linux-image-generic-hwe-24.04
-    remove_existing: true
-
-  # install nothing and remove existing kernels
-  kernel:
-    install: false
-    remove_existing: true
+    remove: existing
 
   # install hwe kernel, remove generic kernel
   kernel:
diff --git a/tests/unittests/test_config.py b/tests/unittests/test_config.py
index 603a5c9..1db3191 100644
--- a/tests/unittests/test_config.py
+++ b/tests/unittests/test_config.py
@@ -206,5 +206,24 @@ class TestDeserializer(CiTestCase):
             DashToUnderscore(a_b=True),
             deserializer.deserialize(DashToUnderscore, {"a-b": True}))
 
+    def test_union_str_list(self):
+        deserializer = config.Deserializer()
+
+        @attr.s(auto_attribs=True)
+        class UnionClass:
+            val: typing.Union[str | list | None]
+
+        self.assertEqual(
+            UnionClass(val="a"),
+            deserializer.deserialize(UnionClass, {"val": "a"}))
+
+        self.assertEqual(
+            UnionClass(val=["b"]),
+            deserializer.deserialize(UnionClass, {"val": ["b"]}))
+
+        self.assertEqual(
+            UnionClass(val=None),
+            deserializer.deserialize(UnionClass, {"val": None}))
+
 
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index 4ee3a26..f0b263e 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -98,6 +98,7 @@ class TestCurthooksInstallKernel(CiTestCase):
 
             self.mock_instpkg.assert_called_with(
                 [kernel_package], target=self.target, env=self.fk_env)
+            self.mock_purgepkg.assert_not_called()
 
     def test__installs_kernel_fallback_package(self):
         fallback_package = "mock-linux-kernel-fallback"
@@ -111,6 +112,7 @@ class TestCurthooksInstallKernel(CiTestCase):
 
             self.mock_instpkg.assert_called_with(
                 [fallback_package], target=self.target, env=self.fk_env)
+            self.mock_purgepkg.assert_not_called()
 
     def test__installs_kernel_from_mapping(self):
         kernel_cfg = {
@@ -131,6 +133,7 @@ class TestCurthooksInstallKernel(CiTestCase):
             self.mock_instpkg.assert_called_with(
                 ["linux-flavor-lts-dapper"],
                 target=self.target, env=self.fk_env)
+            self.mock_purgepkg.assert_not_called()
 
     @parameterized.expand((
         [{'kernel': None}],
@@ -141,6 +144,7 @@ class TestCurthooksInstallKernel(CiTestCase):
             curthooks.install_kernel(kernel_cfg, self.target)
 
             self.mock_instpkg.assert_not_called()
+            self.mock_purgepkg.assert_not_called()
 
     def test__removes_and_installs_kernel(self):
         to_install_kernel_package = "mock-linux-kernel"
@@ -148,7 +152,7 @@ class TestCurthooksInstallKernel(CiTestCase):
         kernel_cfg = {
             'kernel': {
                 'package': to_install_kernel_package,
-                'remove_existing': 'true',
+                'remove': 'existing',
             }
         }
         self.mock_subp.return_value = ("warty", "")
@@ -175,7 +179,7 @@ class TestCurthooksInstallKernel(CiTestCase):
         kernel_cfg = {
             'kernel': {
                 'package': to_install_kernel_package,
-                'remove_existing': 'true',
+                'remove': 'existing',
             }
         }
         self.mock_subp.return_value = ("warty", "")
@@ -197,7 +201,7 @@ class TestCurthooksInstallKernel(CiTestCase):
         kernel_cfg = {
             'kernel': {
                 'package': to_install_kernel_package,
-                'remove_existing': 'true',
+                'remove': 'existing',
             }
         }
         self.mock_subp.return_value = ("warty", "")