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 <[email protected]>
canonical · Jun 11, 2024 · a01b8d3 · a01b8d3
1 parent fdc2505
commit a01b8d3
Show file tree

Hide file tree

Showing 19 changed files with 111 additions and 61 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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,