fix: Fix breaking changes in package install (#5069)

fix: Fix breaking changes in package install

Ensure:
* Apt can still install packages that contain `-`, `/`, or `=`
* If a specified package manager fails we don't retry as a generic
  package
* We do not attempt to invoke a package manager that doesn't exist
* Packages that are unable to be installed are tracked and
  reported appropriately

Fixes GH-5066

cloudinit/config/cc_package_update_upgrade_install.py

@@ -120,7 +120,9 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None:
         try:
             cloud.distro.install_packages(pkglist)
         except Exception as e:
-            util.logexc(LOG, "Failed to install packages: %s", pkglist)
+            util.logexc(
+                LOG, "Failure when attempting to install packages: %s", pkglist
+            )
             errors.append(e)
 
     # TODO(smoser): handle this less violently

cloudinit/distros/__init__.py

@@ -239,36 +239,44 @@ def install_packages(self, pkglist: PackageList):
 
         # First install packages using package manager(s)
         # supported by the distro
-        uninstalled = []
+        total_failed: Set[str] = set()
         for manager in self.package_managers:
-            to_try = (
-                packages_by_manager.get(manager.__class__, set())
-                | generic_packages
+
+            manager_packages = packages_by_manager.get(
+                manager.__class__, set()
             )
+
+            to_try = manager_packages | generic_packages
+            # Remove any failed we will try for this package manager
+            total_failed.difference_update(to_try)
+            if not manager.available():
+                LOG.debug("Package manager '%s' not available", manager.name)
+                total_failed.update(to_try)
+                continue
             if not to_try:
                 continue
-            uninstalled = manager.install_packages(to_try)
-            failed = {
-                pkg for pkg in uninstalled if pkg not in generic_packages
-            }
+            failed = manager.install_packages(to_try)
+            total_failed.update(failed)
             if failed:
                 LOG.info(error_message, failed)
-            generic_packages = set(uninstalled)
+            # Ensure we don't attempt to install packages specific to
+            # one particular package manager using another package manager
+            generic_packages = set(failed) - manager_packages
 
         # Now attempt any specified package managers not explicitly supported
         # by distro
         for manager_type, packages in packages_by_manager.items():
             if manager_type.name in [p.name for p in self.package_managers]:
                 # We already installed/attempted these; don't try again
                 continue
-            uninstalled.extend(
+            total_failed.update(
                 manager_type.from_config(
                     self._runner, self._cfg
                 ).install_packages(pkglist=packages)
             )
 
-        if uninstalled:
-            raise PackageInstallerError(error_message % uninstalled)
+        if total_failed:
+            raise PackageInstallerError(error_message % total_failed)
 
     @property
     def dhcp_client(self) -> dhcp.DhcpClient:

cloudinit/distros/package_management/apt.py

@@ -3,6 +3,7 @@
 import functools
 import logging
 import os
+import re
 import time
 from typing import Any, Iterable, List, Mapping, Optional, Sequence, cast
 
@@ -83,11 +84,11 @@ def __init__(
     ):
         super().__init__(runner)
         if apt_get_command is None:
-            apt_get_command = APT_GET_COMMAND
+            self.apt_get_command = APT_GET_COMMAND
         if apt_get_upgrade_subcommand is None:
             apt_get_upgrade_subcommand = "dist-upgrade"
         self.apt_command = tuple(apt_get_wrapper_command) + tuple(
-            apt_get_command
+            self.apt_get_command
         )
 
         self.apt_get_upgrade_subcommand = apt_get_upgrade_subcommand
@@ -104,6 +105,9 @@ def from_config(cls, runner: helpers.Runners, cfg: Mapping) -> "Apt":
             apt_get_upgrade_subcommand=cfg.get("apt_get_upgrade_subcommand"),
         )
 
+    def available(self) -> bool:
+        return bool(subp.which(self.apt_get_command[0]))
+
     def update_package_sources(self):
         self.runner.run(
             "update-sources",
@@ -123,19 +127,30 @@ def get_all_packages(self):
         return set(resp.splitlines())
 
     def get_unavailable_packages(self, pkglist: Iterable[str]):
-        return [pkg for pkg in pkglist if pkg not in self.get_all_packages()]
+        # Packages ending with `-` signify to apt to not install a transitive
+        # dependency.
+        # Anything after "/" refers to a target release
+        # "=" allows specifying a specific version
+        # Strip all off when checking for availability
+        return [
+            pkg
+            for pkg in pkglist
+            if re.split("/|=", pkg)[0].rstrip("-")
+            not in self.get_all_packages()
+        ]
 
     def install_packages(self, pkglist: Iterable) -> UninstalledPackages:
         self.update_package_sources()
         pkglist = util.expand_package_list("%s=%s", list(pkglist))
         unavailable = self.get_unavailable_packages(
             [x.split("=")[0] for x in pkglist]
         )
-        LOG.debug(
-            "The following packages were not found by APT so APT will "
-            "not attempt to install them: %s",
-            unavailable,
-        )
+        if unavailable:
+            LOG.debug(
+                "The following packages were not found by APT so APT will "
+                "not attempt to install them: %s",
+                unavailable,
+            )
         to_install = [p for p in pkglist if p not in unavailable]
         if to_install:
             self.run_package_command("install", pkgs=to_install)

cloudinit/distros/package_management/package_manager.py

@@ -17,6 +17,10 @@ def __init__(self, runner: helpers.Runners, **kwargs):
     def from_config(cls, runner: helpers.Runners, cfg) -> "PackageManager":
         return cls(runner)
 
+    @abstractmethod
+    def available(self) -> bool:
+        """Return if package manager is installed on system."""
+
     @abstractmethod
     def update_package_sources(self):
         ...

cloudinit/distros/package_management/snap.py

@@ -14,6 +14,9 @@
 class Snap(PackageManager):
     name = "snap"
 
+    def available(self) -> bool:
+        return bool(subp.which("snap"))
+
     def update_package_sources(self):
         pass
 

tests/unittests/config/test_cc_package_update_upgrade_install.py

@@ -37,6 +37,14 @@ def common_mocks(mocker):
         "cloudinit.distros.package_management.apt.Apt._apt_lock_available",
         return_value=True,
     )
+    mocker.patch(
+        "cloudinit.distros.package_management.apt.Apt.available",
+        return_value=True,
+    )
+    mocker.patch(
+        "cloudinit.distros.package_management.snap.Snap.available",
+        return_value=True,
+    )
 
 
 class TestRebootIfRequired:
@@ -275,7 +283,7 @@ def _new_subp(*args, **kwargs):
         assert caplog.records[-3].levelname == "WARNING"
         assert (
             caplog.records[-3].message
-            == "Failed to install packages: ['pkg1']"
+            == "Failure when attempting to install packages: ['pkg1']"
         )
 
 

tests/unittests/distros/package_management/test_apt.py

@@ -7,6 +7,8 @@
 from cloudinit import subp
 from cloudinit.distros.package_management.apt import APT_GET_COMMAND, Apt
 
+M_PATH = "cloudinit.distros.package_management.apt.Apt."
+
 
 @mock.patch.dict("os.environ", {}, clear=True)
 @mock.patch("cloudinit.distros.debian.subp.which", return_value=True)
@@ -86,3 +88,21 @@ def test_lock_exception_timeout(
         )
         with pytest.raises(TimeoutError):
             apt._wait_for_apt_command("stub", {"args": "stub2"}, timeout=5)
+
+    def test_search_stem(self, m_subp, m_which, mocker):
+        """Test that containing `-`, `/`, or `=` is handled correctly."""
+        mocker.patch(f"{M_PATH}update_package_sources")
+        mocker.patch(
+            f"{M_PATH}get_all_packages",
+            return_value=["cloud-init", "pkg2", "pkg3", "pkg4"],
+        )
+        m_install = mocker.patch(f"{M_PATH}run_package_command")
+
+        apt = Apt(runner=mock.Mock())
+        apt.install_packages(
+            ["cloud-init", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"]
+        )
+        m_install.assert_called_with(
+            "install",
+            pkgs=["cloud-init", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"],
+        )

tests/unittests/distros/test_init.py

@@ -256,6 +256,17 @@ def test_valid_substitution(
 class TestInstall:
     """Tests for cloudinit.distros.Distro.install_packages."""
 
+    @pytest.fixture(autouse=True)
+    def ensure_available(self, mocker):
+        mocker.patch(
+            "cloudinit.distros.package_management.apt.Apt.available",
+            return_value=True,
+        )
+        mocker.patch(
+            "cloudinit.distros.package_management.snap.Snap.available",
+            return_value=True,
+        )
+
     @pytest.fixture
     def m_apt_install(self, mocker):
         return mocker.patch(
@@ -318,7 +329,7 @@ def test_non_default_package_manager_fail(
         )
         with pytest.raises(
             PackageInstallerError,
-            match="Failed to install the following packages: \\['pkg3'\\]",
+            match="Failed to install the following packages: {'pkg3'}",
         ):
             _get_distro("debian").install_packages(
                 [{"apt": ["pkg1"]}, "pkg2", {"snap": ["pkg3"]}]
@@ -339,3 +350,120 @@ def test_default_and_specific_package_manager(
         assert "pkg3" not in apt_install_args
 
         m_snap_install.assert_not_called()
+
+    def test_specific_package_manager_fail_doesnt_retry(
+        self, mocker, m_snap_install
+    ):
+        """Test fail from package manager doesn't retry as generic."""
+        m_apt_install = mocker.patch(
+            "cloudinit.distros.package_management.apt.Apt.install_packages",
+            return_value=["pkg1"],
+        )
+        with pytest.raises(PackageInstallerError):
+            _get_distro("ubuntu").install_packages([{"apt": ["pkg1"]}])
+        apt_install_args = m_apt_install.call_args_list[0][0][0]
+        assert "pkg1" in apt_install_args
+        m_snap_install.assert_not_called()
+
+    def test_no_attempt_if_no_package_manager(
+        self, mocker, m_apt_install, m_snap_install, caplog
+    ):
+        """Test that no attempt is made if there are no package manager."""
+        mocker.patch(
+            "cloudinit.distros.package_management.apt.Apt.available",
+            return_value=False,
+        )
+        mocker.patch(
+            "cloudinit.distros.package_management.snap.Snap.available",
+            return_value=False,
+        )
+        with pytest.raises(PackageInstallerError):
+            _get_distro("ubuntu").install_packages(
+                ["pkg1", "pkg2", {"other": "pkg3"}]
+            )
+        m_apt_install.assert_not_called()
+        m_snap_install.assert_not_called()
+
+        assert "Package manager 'apt' not available" in caplog.text
+        assert "Package manager 'snap' not available" in caplog.text
+
+    @pytest.mark.parametrize(
+        "distro,pkg_list,apt_available,apt_failed,snap_failed,total_failed",
+        [
+            pytest.param(
+                "debian",
+                ["pkg1", {"apt": ["pkg2"], "snap": ["pkg3"]}],
+                False,
+                [],
+                ["pkg1", "pkg3"],
+                ["pkg1", "pkg2", "pkg3"],
+                id="debian_no_apt",
+            ),
+            pytest.param(
+                "debian",
+                ["pkg1", {"apt": ["pkg2"], "snap": ["pkg3"]}],
+                True,
+                ["pkg2"],
+                ["pkg3"],
+                ["pkg2", "pkg3"],
+                id="debian_with_apt",
+            ),
+            pytest.param(
+                "ubuntu",
+                ["pkg1", {"apt": ["pkg2"], "snap": ["pkg3"]}],
+                False,
+                [],
+                [],
+                ["pkg2"],
+                id="ubuntu_no_apt",
+            ),
+            pytest.param(
+                "ubuntu",
+                ["pkg1", {"apt": ["pkg2"], "snap": ["pkg3"]}],
+                True,
+                ["pkg1"],
+                ["pkg3"],
+                ["pkg3"],
+                id="ubuntu_with_apt",
+            ),
+        ],
+    )
+    def test_failed(
+        self,
+        distro,
+        pkg_list,
+        apt_available,
+        apt_failed,
+        snap_failed,
+        total_failed,
+        mocker,
+        m_apt_install,
+        m_snap_install,
+    ):
+        """Test that failed packages are properly tracked.
+
+        We need to ensure that the failed packages are properly tracked:
+            1. When package install fails normally
+            2. When package manager is not available
+            3. When package manager is not explicitly supported by the distro
+
+        So test various combinations of these scenarios.
+        """
+        mocker.patch(
+            "cloudinit.distros.package_management.apt.Apt.available",
+            return_value=apt_available,
+        )
+        mocker.patch(
+            "cloudinit.distros.package_management.apt.Apt.install_packages",
+            return_value=apt_failed,
+        )
+        mocker.patch(
+            "cloudinit.distros.package_management.snap.Snap.install_packages",
+            return_value=snap_failed,
+        )
+        with pytest.raises(PackageInstallerError) as exc:
+            _get_distro(distro).install_packages(pkg_list)
+        message = exc.value.args[0]
+        assert "Failed to install the following packages" in message
+        for pkg in total_failed:
+            assert pkg in message