fix(apt): Allow opt-in for calling apt update multiple times

[holmanb]

fix(apt): Enable calling apt update multiple times

This is required to configure apt when dependency is not installed.

Fixes GH-5223

Additional Context

note for reviewers: does this kwarg name make it obvious what it is supposed to do?

Calling this multiple times isn't ideal, but we support installing gpg as needed for apt-configure, and without calling apt-update after the gpg install we won't be able to actually install packages whose repositories are configured in apt_configure, so our hands are tied if we want to continue to support this for minimal images.

Fixes #5223

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[TheRealFalcon]

@holmanb , this solution doesn't work unfortunately.

package_manager.py is currently only used by apt and snap but still get called from the distro level. For that, we'd need to update
https://github.com/canonical/cloud-init/blob/main/cloudinit/distros/__init__.py#L398

Since that's the base class, if we update there, we should also update all of the child classes.

Additionally, the update doesn't need to be called after the dependencies are installed. Rather, sources need to be updated after the source file is written.

The following diff (based off main at 237d957) should fix this:

diff --git a/cloudinit/config/cc_apt_configure.py b/cloudinit/config/cc_apt_configure.py
index 99a8f556c..346091207 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 90227675e..60ba6eb75 100644
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -395,7 +395,7 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
         # 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(
@@ -404,7 +404,7 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
                 )
                 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 fa0bbf23c..50eb98ca3 100644
--- a/cloudinit/distros/alpine.py
+++ b/cloudinit/distros/alpine.py
@@ -13,7 +13,7 @@ from typing import Optional

 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_INSTANCE, PER_ALWAYS

 LOG = logging.getLogger(__name__)

@@ -186,12 +186,12 @@ class Distro(distros.Distro):
         # 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 52f47097d..bdb2c08dc 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 521c014d7..f09d34ca5 100644
--- a/cloudinit/distros/arch.py
+++ b/cloudinit/distros/arch.py
@@ -10,7 +10,7 @@ from cloudinit import distros, helpers, subp, util
 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 @@ class Distro(distros.Distro):
         # 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 2d8fa02fe..ba35b2e61 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -12,7 +12,7 @@ from io import StringIO
 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 @@ class Distro(cloudinit.distros.bsd.BSD):
         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 37ac7b68c..5ab41bbd9 100644
--- a/cloudinit/distros/gentoo.py
+++ b/cloudinit/distros/gentoo.py
@@ -11,7 +11,7 @@ import logging
 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 @@ class Distro(distros.Distro):
         # 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 e8b9bcd5b..b201df75a 100644
--- a/cloudinit/distros/netbsd.py
+++ b/cloudinit/distros/netbsd.py
@@ -153,7 +153,7 @@ class NetBSD(cloudinit.distros.bsd.BSD):
             )
         }

-    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 8cf41b5d6..9ecf97246 100644
--- a/cloudinit/distros/opensuse.py
+++ b/cloudinit/distros/opensuse.py
@@ -15,7 +15,7 @@ from cloudinit import distros, helpers, subp, util
 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__)

@@ -150,12 +150,12 @@ class Distro(distros.Distro):
             # 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 16f3b3dca..c2a1d7cf8 100644
--- a/cloudinit/distros/package_management/apt.py
+++ b/cloudinit/distros/package_management/apt.py
@@ -12,7 +12,7 @@ from cloudinit.distros.package_management.package_manager import (
     PackageManager,
     UninstalledPackages,
 )
-from cloudinit.settings import PER_INSTANCE
+from cloudinit.settings import PER_ALWAYS, PER_INSTANCE

 LOG = logging.getLogger(__name__)

@@ -108,12 +108,12 @@ class Apt(PackageManager):
     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)
diff --git a/cloudinit/distros/package_management/package_manager.py b/cloudinit/distros/package_management/package_manager.py
index d92b11d5a..32c4cac24 100644
--- a/cloudinit/distros/package_management/package_manager.py
+++ b/cloudinit/distros/package_management/package_manager.py
@@ -22,7 +22,7 @@ class PackageManager(ABC):
         """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 a5fc2a89d..baab9e3ca 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 2678be0a2..a9b1fc05d 100644
--- a/cloudinit/distros/photon.py
+++ b/cloudinit/distros/photon.py
@@ -7,7 +7,7 @@ import logging
 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 @@ class Distro(distros.Distro):
         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 3254cbe01..b3f566d09 100644
--- a/cloudinit/distros/rhel.py
+++ b/cloudinit/distros/rhel.py
@@ -13,7 +13,7 @@ import os
 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__)

@@ -209,10 +209,10 @@ class Distro(distros.Distro):
         # 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 5ae83c84f..b85ac3486 100644
--- a/tests/unittests/config/test_apt_source_v1.py
+++ b/tests/unittests/config/test_apt_source_v1.py
@@ -42,7 +42,7 @@ ADD_APT_REPO_MATCH = r"^[\w-]+:\w"
 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/util.py b/tests/unittests/util.py
index ca59477ef..c779ae560 100644
--- a/tests/unittests/util.py
+++ b/tests/unittests/util.py
@@ -163,7 +163,7 @@ class MockDistro(distros.Distro):
     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):

Let me know if you think this is the right approach.

[holmanb]

@TheRealFalcon yeah, my first attempt was just broken. This should work (better UI too). Thanks for this!

[TheRealFalcon]

I think it'd be good to have a test that asserts update is called with we have an apt source, even if update has already been previously called.

[holmanb]

I think it'd be good to have a test that asserts update is called with we have an apt source, even if update has already been previously called.

+1 will do.

Btw the original reporter confirmed that this fixes the issue.

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

[TheRealFalcon]

@holmanb any updates here?

[holmanb]

@holmanb any updates here?

Should be ready tomorrow, will ping for review when ready

[holmanb]

@TheRealFalcon I've added tests to ensure the expected behavior. PTAL