From a01b8d31f8364a3d86f016a2940df1b38598889d Mon Sep 17 00:00:00 2001 From: Brett Holman Date: Tue, 11 Jun 2024 16:23:25 -0600 Subject: [PATCH] fix(apt): Enable calling apt update multiple times (#5230) This is required to configure apt when dependency is not installed. Fixes GH-5223 Co-authored-by: James Falcon --- cloudinit/config/cc_apt_configure.py | 6 +-- cloudinit/distros/__init__.py | 4 +- cloudinit/distros/alpine.py | 6 +-- cloudinit/distros/amazon.py | 2 +- cloudinit/distros/arch.py | 9 ++-- cloudinit/distros/freebsd.py | 6 +-- cloudinit/distros/gentoo.py | 6 +-- cloudinit/distros/netbsd.py | 2 +- cloudinit/distros/opensuse.py | 6 +-- cloudinit/distros/package_management/apt.py | 12 +++-- .../package_management/package_manager.py | 2 +- cloudinit/distros/package_management/snap.py | 2 +- cloudinit/distros/photon.py | 6 +-- cloudinit/distros/rhel.py | 6 +-- tests/unittests/config/test_apt_source_v1.py | 2 +- tests/unittests/config/test_apt_source_v3.py | 29 ++++++------ .../distros/package_management/test_apt.py | 44 ++++++++++++++++++- tests/unittests/helpers.py | 20 ++++++--- tests/unittests/util.py | 2 +- 19 files changed, 111 insertions(+), 61 deletions(-) diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py index 99a8f556c4d..3460912077a 100644 --- a/cloudinit/config/cc_apt_configure.py +++ b/cloudinit/config/cc_apt_configure.py @@ -769,10 +769,6 @@ def add_apt_key(ent, cloud, gpg, hardened=False, file_name=None): ) -def update_packages(cloud): - cloud.distro.update_package_sources() - - def add_apt_sources( srcdict, cloud, gpg, template_params=None, aa_repo_match=None ): @@ -856,7 +852,7 @@ def add_apt_sources( LOG.exception("failed write to file %s: %s", sourcefn, detail) raise - update_packages(cloud) + cloud.distro.update_package_sources(force=True) return diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 4312d803805..789af4a7d69 100644 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -396,7 +396,7 @@ def package_command(self, command, args=None, pkgs=None): # managers. raise NotImplementedError() - def update_package_sources(self): + def update_package_sources(self, *, force=False): for manager in self.package_managers: if not manager.available(): LOG.debug( @@ -405,7 +405,7 @@ def update_package_sources(self): ) continue try: - manager.update_package_sources() + manager.update_package_sources(force=force) except Exception as e: LOG.error( "Failed to update package using %s: %s", manager.name, e diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py index 867963f2c78..a1d0d900c9f 100644 --- a/cloudinit/distros/alpine.py +++ b/cloudinit/distros/alpine.py @@ -15,7 +15,7 @@ from cloudinit import distros, helpers, subp, util from cloudinit.distros.parsers.hostname import HostnameConf -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -189,12 +189,12 @@ def package_command(self, command, args=None, pkgs=None): # Allow the output of this to flow outwards (ie not be captured) subp.subp(cmd, capture=False) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( "update-sources", self.package_command, ["update"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) @property diff --git a/cloudinit/distros/amazon.py b/cloudinit/distros/amazon.py index 52f47097d30..bdb2c08dcfb 100644 --- a/cloudinit/distros/amazon.py +++ b/cloudinit/distros/amazon.py @@ -20,5 +20,5 @@ class Distro(rhel.Distro): dhclient_lease_directory = "/var/lib/dhcp" dhclient_lease_file_regex = r"dhclient-[\w-]+\.lease" - def update_package_sources(self): + def update_package_sources(self, *, force=False): return None diff --git a/cloudinit/distros/arch.py b/cloudinit/distros/arch.py index 521c014d720..f09d34ca5e3 100644 --- a/cloudinit/distros/arch.py +++ b/cloudinit/distros/arch.py @@ -10,7 +10,7 @@ from cloudinit.distros import PackageList from cloudinit.distros.parsers.hostname import HostnameConf from cloudinit.net.netplan import CLOUDINIT_NETPLAN_FILE -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -141,7 +141,10 @@ def package_command(self, command, args=None, pkgs=None): # Allow the output of this to flow outwards (ie not be captured) subp.subp(cmd, capture=False) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( - "update-sources", self.package_command, ["-y"], freq=PER_INSTANCE + "update-sources", + self.package_command, + ["-y"], + freq=PER_ALWAYS if force else PER_INSTANCE, ) diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index 2d8fa02fea6..ba35b2e611f 100644 --- a/cloudinit/distros/freebsd.py +++ b/cloudinit/distros/freebsd.py @@ -12,7 +12,7 @@ import cloudinit.distros.bsd from cloudinit import subp, util from cloudinit.distros.networking import FreeBSDNetworking -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -203,12 +203,12 @@ def _get_pkg_cmd_environ(self): operations""" return {"ASSUME_ALWAYS_YES": "YES"} - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( "update-sources", self.package_command, ["update"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) @staticmethod diff --git a/cloudinit/distros/gentoo.py b/cloudinit/distros/gentoo.py index 37ac7b68cf7..5ab41bbd9db 100644 --- a/cloudinit/distros/gentoo.py +++ b/cloudinit/distros/gentoo.py @@ -11,7 +11,7 @@ from cloudinit import distros, helpers, subp, util from cloudinit.distros import PackageList from cloudinit.distros.parsers.hostname import HostnameConf -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -132,10 +132,10 @@ def package_command(self, command, args=None, pkgs=None): # Allow the output of this to flow outwards (ie not be captured) subp.subp(cmd, capture=False) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( "update-sources", self.package_command, ["--sync"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) diff --git a/cloudinit/distros/netbsd.py b/cloudinit/distros/netbsd.py index 972528c6df0..da8c1904028 100644 --- a/cloudinit/distros/netbsd.py +++ b/cloudinit/distros/netbsd.py @@ -153,7 +153,7 @@ def _get_pkg_cmd_environ(self): ) } - def update_package_sources(self): + def update_package_sources(self, *, force=False): pass diff --git a/cloudinit/distros/opensuse.py b/cloudinit/distros/opensuse.py index e8f85f97564..91620a98e07 100644 --- a/cloudinit/distros/opensuse.py +++ b/cloudinit/distros/opensuse.py @@ -15,7 +15,7 @@ from cloudinit.distros import PackageList from cloudinit.distros import rhel_util as rhutil from cloudinit.distros.parsers.hostname import HostnameConf -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -148,12 +148,12 @@ def set_timezone(self, tz): # This ensures that the correct tz will be used for the system util.copy(tz_file, self.tz_local_fn) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( "update-sources", self.package_command, ["refresh"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) def _read_hostname(self, filename, default=None): diff --git a/cloudinit/distros/package_management/apt.py b/cloudinit/distros/package_management/apt.py index 16f3b3dcab8..9546857b839 100644 --- a/cloudinit/distros/package_management/apt.py +++ b/cloudinit/distros/package_management/apt.py @@ -12,7 +12,7 @@ PackageManager, UninstalledPackages, ) -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -108,12 +108,12 @@ def from_config(cls, runner: helpers.Runners, cfg: Mapping) -> "Apt": def available(self) -> bool: return bool(subp.which(self.apt_get_command[0])) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self.runner.run( "update-sources", self.run_package_command, ["update"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) @functools.lru_cache(maxsize=1) @@ -182,14 +182,12 @@ def run_package_command(self, command, args=None, pkgs=None): }, ) - def _apt_lock_available(self, lock_files=None): + def _apt_lock_available(self): """Determines if another process holds any apt locks. If all locks are clear, return True else False. """ - if lock_files is None: - lock_files = APT_LOCK_FILES - for lock in lock_files: + for lock in APT_LOCK_FILES: if not os.path.exists(lock): # Only wait for lock files that already exist continue diff --git a/cloudinit/distros/package_management/package_manager.py b/cloudinit/distros/package_management/package_manager.py index d92b11d5a6d..32c4cac246c 100644 --- a/cloudinit/distros/package_management/package_manager.py +++ b/cloudinit/distros/package_management/package_manager.py @@ -22,7 +22,7 @@ def available(self) -> bool: """Return if package manager is installed on system.""" @abstractmethod - def update_package_sources(self): + def update_package_sources(self, *, force=False): ... @abstractmethod diff --git a/cloudinit/distros/package_management/snap.py b/cloudinit/distros/package_management/snap.py index a5fc2a89db1..baab9e3ca85 100644 --- a/cloudinit/distros/package_management/snap.py +++ b/cloudinit/distros/package_management/snap.py @@ -17,7 +17,7 @@ class Snap(PackageManager): def available(self) -> bool: return bool(subp.which("snap")) - def update_package_sources(self): + def update_package_sources(self, *, force=False): pass def install_packages(self, pkglist: Iterable) -> UninstalledPackages: diff --git a/cloudinit/distros/photon.py b/cloudinit/distros/photon.py index 2678be0a2a7..a9b1fc05d0a 100644 --- a/cloudinit/distros/photon.py +++ b/cloudinit/distros/photon.py @@ -7,7 +7,7 @@ from cloudinit import distros, helpers, net, subp, util from cloudinit.distros import PackageList from cloudinit.distros import rhel_util as rhutil -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -156,10 +156,10 @@ def package_command(self, command, args=None, pkgs=None): if ret: LOG.error("Error while installing packages: %s", err) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( "update-sources", self.package_command, ["makecache"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py index 0fac9660b7a..0e214150766 100644 --- a/cloudinit/distros/rhel.py +++ b/cloudinit/distros/rhel.py @@ -13,7 +13,7 @@ from cloudinit import distros, helpers, subp, util from cloudinit.distros import PackageList, rhel_util from cloudinit.distros.parsers.hostname import HostnameConf -from cloudinit.settings import PER_INSTANCE +from cloudinit.settings import PER_ALWAYS, PER_INSTANCE LOG = logging.getLogger(__name__) @@ -208,10 +208,10 @@ def package_command(self, command, args=None, pkgs=None): # Allow the output of this to flow outwards (ie not be captured) subp.subp(cmd, capture=False) - def update_package_sources(self): + def update_package_sources(self, *, force=False): self._runner.run( "update-sources", self.package_command, ["makecache"], - freq=PER_INSTANCE, + freq=PER_ALWAYS if force else PER_INSTANCE, ) diff --git a/tests/unittests/config/test_apt_source_v1.py b/tests/unittests/config/test_apt_source_v1.py index f27850078bb..93d0cc0832a 100644 --- a/tests/unittests/config/test_apt_source_v1.py +++ b/tests/unittests/config/test_apt_source_v1.py @@ -42,7 +42,7 @@ class FakeDistro: """Fake Distro helper object""" - def update_package_sources(self): + def update_package_sources(self, *, force=False): """Fake update_package_sources helper method""" return diff --git a/tests/unittests/config/test_apt_source_v3.py b/tests/unittests/config/test_apt_source_v3.py index a1c75b328ef..9bdfa9548d3 100644 --- a/tests/unittests/config/test_apt_source_v3.py +++ b/tests/unittests/config/test_apt_source_v3.py @@ -65,8 +65,8 @@ def setup(self, mocker, tmpdir): @staticmethod def _add_apt_sources(cfg, cloud, gpg, **kwargs): - with mock.patch.object(cc_apt_configure, "update_packages"): - cc_apt_configure.add_apt_sources(cfg, cloud, gpg, **kwargs) + # with mock.patch.object(cloud.distro, "update_package_sources"): + cc_apt_configure.add_apt_sources(cfg, cloud, gpg, **kwargs) @staticmethod def _get_default_params(): @@ -89,7 +89,7 @@ def _apt_src_basic(self, filename, cfg, tmpdir, gpg): self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=gpg, template_params=params, aa_repo_match=self.matcher, @@ -185,7 +185,7 @@ def _apt_src_replacement(self, filename, cfg, tmpdir, gpg): params = self._get_default_params() self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=gpg, template_params=params, aa_repo_match=self.matcher, @@ -261,10 +261,11 @@ def _apt_src_keyid( """ params = self._get_default_params() + cloud = get_cloud() with mock.patch.object(cc_apt_configure, "add_apt_key") as mockobj: self._add_apt_sources( cfg, - cloud=None, + cloud=cloud, gpg=gpg, template_params=params, aa_repo_match=self.matcher, @@ -274,9 +275,9 @@ def _apt_src_keyid( calls = [] for key in cfg: if is_hardened is not None: - calls.append(call(cfg[key], None, gpg, hardened=is_hardened)) + calls.append(call(cfg[key], cloud, gpg, hardened=is_hardened)) else: - calls.append(call(cfg[key], None, gpg)) + calls.append(call(cfg[key], cloud, gpg)) mockobj.assert_has_calls(calls, any_order=True) @@ -389,7 +390,7 @@ def test_apt_v3_src_key(self, mocker, m_gpg): mockobj = mocker.patch.object(cc_apt_configure, "apt_key") self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=m_gpg, template_params=params, aa_repo_match=self.matcher, @@ -427,7 +428,7 @@ def test_apt_v3_src_keyonly(self, mocker, m_gpg): mockobj = mocker.patch.object(cc_apt_configure, "apt_key") self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=m_gpg, template_params=params, aa_repo_match=self.matcher, @@ -458,7 +459,7 @@ def test_apt_v3_src_keyidonly(self, m_gpg): with mock.patch.object(cc_apt_configure, "apt_key") as mockobj: self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=m_gpg, template_params=params, aa_repo_match=self.matcher, @@ -494,7 +495,7 @@ def apt_src_keyid_real(self, cfg, expectedkey, gpg, is_hardened=None): ) as mockgetkey: self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=gpg, template_params=params, aa_repo_match=self.matcher, @@ -559,7 +560,7 @@ def test_apt_v3_src_keyid_keyserver(self, m_gpg): with mock.patch.object(cc_apt_configure, "add_apt_key_raw") as mockadd: self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=m_gpg, template_params=params, aa_repo_match=self.matcher, @@ -584,7 +585,7 @@ def test_apt_v3_src_ppa(self, m_gpg): with mock.patch("cloudinit.subp.subp") as mockobj: self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=m_gpg, template_params=params, aa_repo_match=self.matcher, @@ -614,7 +615,7 @@ def test_apt_v3_src_ppa_tri(self, m_gpg): with mock.patch("cloudinit.subp.subp") as mockobj: self._add_apt_sources( cfg, - cloud=None, + cloud=mock.Mock(), gpg=m_gpg, template_params=params, aa_repo_match=self.matcher, diff --git a/tests/unittests/distros/package_management/test_apt.py b/tests/unittests/distros/package_management/test_apt.py index 570bd7df010..f882b5a75d5 100644 --- a/tests/unittests/distros/package_management/test_apt.py +++ b/tests/unittests/distros/package_management/test_apt.py @@ -1,13 +1,18 @@ # This file is part of cloud-init. See LICENSE file for license information. +import tempfile from itertools import count, cycle from unittest import mock import pytest -from cloudinit import subp +from cloudinit import helpers, subp +from cloudinit.distros.package_management import apt from cloudinit.distros.package_management.apt import APT_GET_COMMAND, Apt +from tests.unittests.helpers import MockPaths +from tests.unittests.util import FakeDataSource M_PATH = "cloudinit.distros.package_management.apt.Apt." +TMP_DIR = tempfile.TemporaryDirectory() @mock.patch.dict("os.environ", {}, clear=True) @@ -112,3 +117,40 @@ def test_search_stem(self, m_subp, m_which, mocker): "pkg5^", ], ) + + +@mock.patch.object( + apt, + "APT_LOCK_FILES", + [f"{TMP_DIR}/{FILE}" for FILE in apt.APT_LOCK_FILES], +) +class TestUpdatePackageSources: + @mock.patch.object(apt.subp, "which", return_value=True) + @mock.patch.object(apt.subp, "subp") + def test_force_update_calls_twice(self, m_subp, m_which): + """Ensure that force=true calls apt update again""" + instance = apt.Apt(helpers.Runners(MockPaths({}, FakeDataSource()))) + instance.update_package_sources() + instance.update_package_sources(force=True) + assert 2 == len(m_subp.call_args_list) + TMP_DIR.cleanup() + + @mock.patch.object(apt.subp, "which", return_value=True) + @mock.patch.object(apt.subp, "subp") + def test_force_update_twice_calls_twice(self, m_subp, m_which): + """Ensure that force=true calls apt update again when called twice""" + instance = apt.Apt(helpers.Runners(MockPaths({}, FakeDataSource()))) + instance.update_package_sources(force=True) + instance.update_package_sources(force=True) + assert 2 == len(m_subp.call_args_list) + TMP_DIR.cleanup() + + @mock.patch.object(apt.subp, "which", return_value=True) + @mock.patch.object(apt.subp, "subp") + def test_no_force_update_calls_once(self, m_subp, m_which): + """Ensure that apt-get update calls are deduped unless expected""" + instance = apt.Apt(helpers.Runners(MockPaths({}, FakeDataSource()))) + instance.update_package_sources() + instance.update_package_sources() + assert 1 == len(m_subp.call_args_list) + TMP_DIR.cleanup() diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py index 1ad5b8897ed..d89b0b830d2 100644 --- a/tests/unittests/helpers.py +++ b/tests/unittests/helpers.py @@ -273,11 +273,7 @@ def tmp_cloud(self, distro, sys_cfg=None, metadata=None): self.new_root = self.tmp_dir() if not sys_cfg: sys_cfg = {} - tmp_paths = {} - for var in ["templates_dir", "run_dir", "cloud_dir"]: - tmp_paths[var] = self.tmp_path(var, dir=self.new_root) - util.ensure_dir(tmp_paths[var]) - self.paths = ch.Paths(tmp_paths) + self.paths = MockPaths({}) cls = distros.fetch(distro) mydist = cls(distro, sys_cfg, self.paths) myds = DataSourceNone.DataSourceNone(sys_cfg, mydist, self.paths) @@ -464,6 +460,20 @@ def _ensure_url_default_path(url): ) +class MockPaths(ch.Paths): + def __init__(self, path_cfgs: dict, ds=None): + super().__init__(path_cfgs=path_cfgs, ds=ds) + self.temp = tempfile.TemporaryDirectory() + + self.cloud_dir: str = path_cfgs.get( + "cloud_dir", f"{self.temp}/var/lib/cloud" + ) + self.run_dir: str = path_cfgs.get("run_dir", f"{self.temp}/run/cloud/") + self.template_dir: str = path_cfgs.get( + "templates_dir", f"{self.temp}/etc/cloud/templates/" + ) + + class ResponsesTestCase(CiTestCase): def setUp(self): super().setUp() diff --git a/tests/unittests/util.py b/tests/unittests/util.py index ca59477ef80..c779ae56076 100644 --- a/tests/unittests/util.py +++ b/tests/unittests/util.py @@ -163,7 +163,7 @@ def shutdown_command(self, *, mode, delay, message): def package_command(self, command, args=None, pkgs=None): pass - def update_package_sources(self): + def update_package_sources(self, *, force=False): return (True, "yay") def do_as(self, command, args=None, **kwargs):