From 1f7b3c075d61112bc2cb7d5dc84562deefe0f102 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 18 Mar 2024 21:16:09 -0500 Subject: [PATCH 1/5] 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 --- .../cc_package_update_upgrade_install.py | 4 +- cloudinit/distros/__init__.py | 30 ++-- cloudinit/distros/package_management/apt.py | 30 ++-- .../package_management/package_manager.py | 4 + cloudinit/distros/package_management/snap.py | 3 + .../test_cc_package_update_upgrade_install.py | 10 +- .../distros/package_management/test_apt.py | 19 +++ tests/unittests/distros/test_init.py | 130 +++++++++++++++++- 8 files changed, 209 insertions(+), 21 deletions(-) diff --git a/cloudinit/config/cc_package_update_upgrade_install.py b/cloudinit/config/cc_package_update_upgrade_install.py index 3bf1ce0b3b7..42d8c004379 100644 --- a/cloudinit/config/cc_package_update_upgrade_install.py +++ b/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 diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 4b51c0d8b85..c644e329099 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -239,21 +239,31 @@ def install_packages(self, pkglist: PackageList): # First install packages using package manager(s) # supported by the distro - uninstalled = [] + uninstalled = 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 + uninstalled.difference_update(to_try) + if not manager.available(): + LOG.debug("Package manager '%s' not available", manager.name) + uninstalled.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) + uninstalled.update(failed) + unexpected_failed = { + pkg for pkg in failed if pkg not in generic_packages } - if failed: + if unexpected_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 @@ -261,7 +271,7 @@ def install_packages(self, pkglist: PackageList): 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( + uninstalled.update( manager_type.from_config( self._runner, self._cfg ).install_packages(pkglist=packages) diff --git a/cloudinit/distros/package_management/apt.py b/cloudinit/distros/package_management/apt.py index 627c0348e69..5ef94b26386 100644 --- a/cloudinit/distros/package_management/apt.py +++ b/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,7 +127,16 @@ 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] not in self.get_all_packages() + ] def install_packages(self, pkglist: Iterable) -> UninstalledPackages: self.update_package_sources() @@ -131,11 +144,12 @@ def install_packages(self, pkglist: Iterable) -> UninstalledPackages: 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) diff --git a/cloudinit/distros/package_management/package_manager.py b/cloudinit/distros/package_management/package_manager.py index 864555f6a4d..d92b11d5a6d 100644 --- a/cloudinit/distros/package_management/package_manager.py +++ b/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): ... diff --git a/cloudinit/distros/package_management/snap.py b/cloudinit/distros/package_management/snap.py index 92eb1af8f5f..a5fc2a89db1 100644 --- a/cloudinit/distros/package_management/snap.py +++ b/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 diff --git a/tests/unittests/config/test_cc_package_update_upgrade_install.py b/tests/unittests/config/test_cc_package_update_upgrade_install.py index a77c305eb99..08db05a03fb 100644 --- a/tests/unittests/config/test_cc_package_update_upgrade_install.py +++ b/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']" ) diff --git a/tests/unittests/distros/package_management/test_apt.py b/tests/unittests/distros/package_management/test_apt.py index d261e757ae6..1bd053c3ee4 100644 --- a/tests/unittests/distros/package_management/test_apt.py +++ b/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,20 @@ 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=["pkg1", "pkg2", "pkg3", "pkg4"], + ) + m_install = mocker.patch(f"{M_PATH}run_package_command") + + apt = Apt(runner=mock.Mock()) + apt.install_packages( + ["pkg1", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"] + ) + m_install.assert_called_with( + "install", pkgs=["pkg1", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"] + ) diff --git a/tests/unittests/distros/test_init.py b/tests/unittests/distros/test_init.py index f7994f1ca7c..062f0ea9b5d 100644 --- a/tests/unittests/distros/test_init.py +++ b/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,uninstalled", + [ + 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_uninstalled( + self, + distro, + pkg_list, + apt_available, + apt_failed, + snap_failed, + uninstalled, + mocker, + m_apt_install, + m_snap_install, + ): + """Test that uninstalled packages are properly tracked. + + We need to ensure that the uninstalled packages are properly tracked: + 1. When package install fails + 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 uninstalled: + assert pkg in message From 6cd3978e9ffaecb82aa58f770e233a1f271e1e8c Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 19 Mar 2024 08:17:24 -0500 Subject: [PATCH 2/5] comments --- cloudinit/distros/__init__.py | 14 +++++++------- cloudinit/distros/package_management/apt.py | 3 ++- .../distros/package_management/test_apt.py | 7 ++++--- tests/unittests/distros/test_init.py | 14 +++++++------- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index c644e329099..8a762c7e5a1 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -239,7 +239,7 @@ def install_packages(self, pkglist: PackageList): # First install packages using package manager(s) # supported by the distro - uninstalled = set() + total_failed = set() for manager in self.package_managers: manager_packages = packages_by_manager.get( @@ -247,15 +247,15 @@ def install_packages(self, pkglist: PackageList): ) to_try = manager_packages | generic_packages - uninstalled.difference_update(to_try) + total_failed.difference_update(to_try) if not manager.available(): LOG.debug("Package manager '%s' not available", manager.name) - uninstalled.update(to_try) + total_failed.update(to_try) continue if not to_try: continue failed = manager.install_packages(to_try) - uninstalled.update(failed) + total_failed.update(failed) unexpected_failed = { pkg for pkg in failed if pkg not in generic_packages } @@ -271,14 +271,14 @@ def install_packages(self, pkglist: PackageList): if manager_type.name in [p.name for p in self.package_managers]: # We already installed/attempted these; don't try again continue - uninstalled.update( + 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: diff --git a/cloudinit/distros/package_management/apt.py b/cloudinit/distros/package_management/apt.py index 5ef94b26386..7f39b808c58 100644 --- a/cloudinit/distros/package_management/apt.py +++ b/cloudinit/distros/package_management/apt.py @@ -135,7 +135,8 @@ def get_unavailable_packages(self, pkglist: Iterable[str]): return [ pkg for pkg in pkglist - if re.split("/|=|-", pkg)[0] not in self.get_all_packages() + if re.split("/|=", pkg)[0].rstrip("-") + not in self.get_all_packages() ] def install_packages(self, pkglist: Iterable) -> UninstalledPackages: diff --git a/tests/unittests/distros/package_management/test_apt.py b/tests/unittests/distros/package_management/test_apt.py index 1bd053c3ee4..8b5c707aa15 100644 --- a/tests/unittests/distros/package_management/test_apt.py +++ b/tests/unittests/distros/package_management/test_apt.py @@ -94,14 +94,15 @@ def test_search_stem(self, m_subp, m_which, mocker): mocker.patch(f"{M_PATH}update_package_sources") mocker.patch( f"{M_PATH}get_all_packages", - return_value=["pkg1", "pkg2", "pkg3", "pkg4"], + 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( - ["pkg1", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"] + ["cloud-init", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"] ) m_install.assert_called_with( - "install", pkgs=["pkg1", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"] + "install", + pkgs=["cloud-init", "pkg2-", "pkg3/jammy-updates", "pkg4=1.2"], ) diff --git a/tests/unittests/distros/test_init.py b/tests/unittests/distros/test_init.py index 062f0ea9b5d..986ccafcbca 100644 --- a/tests/unittests/distros/test_init.py +++ b/tests/unittests/distros/test_init.py @@ -388,7 +388,7 @@ def test_no_attempt_if_no_package_manager( assert "Package manager 'snap' not available" in caplog.text @pytest.mark.parametrize( - "distro,pkg_list,apt_available,apt_failed,snap_failed,uninstalled", + "distro,pkg_list,apt_available,apt_failed,snap_failed,total_failed", [ pytest.param( "debian", @@ -428,22 +428,22 @@ def test_no_attempt_if_no_package_manager( ), ], ) - def test_uninstalled( + def test_failed( self, distro, pkg_list, apt_available, apt_failed, snap_failed, - uninstalled, + total_failed, mocker, m_apt_install, m_snap_install, ): - """Test that uninstalled packages are properly tracked. + """Test that failed packages are properly tracked. - We need to ensure that the uninstalled packages are properly tracked: - 1. When package install fails + 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 @@ -465,5 +465,5 @@ def test_uninstalled( _get_distro(distro).install_packages(pkg_list) message = exc.value.args[0] assert "Failed to install the following packages" in message - for pkg in uninstalled: + for pkg in total_failed: assert pkg in message From d06bc9dbe03a3238af8e6337880aa7168d4a3173 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 19 Mar 2024 11:17:55 -0500 Subject: [PATCH 3/5] mypy likes to randomly yell --- cloudinit/distros/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 8a762c7e5a1..610c6b3debe 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -239,7 +239,7 @@ def install_packages(self, pkglist: PackageList): # First install packages using package manager(s) # supported by the distro - total_failed = set() + total_failed: Set[str] = set() for manager in self.package_managers: manager_packages = packages_by_manager.get( From bdd8e03733e7c8cd8800e97ebca6c2befb6a60f1 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 19 Mar 2024 15:30:54 -0500 Subject: [PATCH 4/5] update failed logging --- cloudinit/distros/__init__.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 610c6b3debe..fcdccad5dc9 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -256,10 +256,7 @@ def install_packages(self, pkglist: PackageList): continue failed = manager.install_packages(to_try) total_failed.update(failed) - unexpected_failed = { - pkg for pkg in failed if pkg not in generic_packages - } - if unexpected_failed: + if failed: LOG.info(error_message, failed) # Ensure we don't attempt to install packages specific to # one particular package manager using another package manager From ea179a257ce96f4c1d255cab70cd20602a98bec6 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 19 Mar 2024 15:35:10 -0500 Subject: [PATCH 5/5] Update cloudinit/distros/__init__.py Co-authored-by: Chad Smith --- cloudinit/distros/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index fcdccad5dc9..2c4d54871ea 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -247,6 +247,7 @@ def install_packages(self, pkglist: PackageList): ) 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)