Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(snap): avoid refresh on package_upgrade: true and refresh.hold #5426

Merged
merged 2 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion cloudinit/distros/package_management/snap.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,23 @@ def install_packages(self, pkglist: Iterable) -> UninstalledPackages:

@staticmethod
def upgrade_packages():
subp.subp(["snap", "refresh"])
command = ["snap", "get", "system", "-d"]
snap_hold = None
try:
result = subp.subp(command)
snap_hold = (
util.load_json(result.stdout).get("refresh", {}).get("hold")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I start a new lxd container, I see a refresh hold immediately upon starting the container. I think a refresh hold exists regardless if the user has specified one and just points to the next time snap will refresh.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, yes you are correct. Just saw this on jammy and oracular images. Ok, we can't approach this feature from this angle. If someone wants temporary snap hold during boot, They can add a runcmd: snap refresh --unhold to user-data. I'll update docs and avoid the time-based hold since it looks to be set during boot anyway that ties directly to the Jul 10 22:33:58.917075 test-j systemd[1]: Started Snap Daemon. getting started in journalctl.

)
except subp.ProcessExecutionError as e:
LOG.info(
"Continuing to snap refresh. Unable to run command: %s: %s",
command,
e,
)
if snap_hold == "forever":
LOG.info(
"Skipping snap refresh because refresh.hold is set to '%s'",
snap_hold,
)
else:
subp.subp(["snap", "refresh"])
11 changes: 7 additions & 4 deletions doc/module-docs/cc_package_update_upgrade_install/data.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
cc_package_update_upgrade_install:
description: |
This module allows packages to be updated, upgraded or installed during
boot. If any packages are to be installed or an upgrade is to be performed
then the package cache will be updated first. If a package installation or
upgrade requires a reboot, then a reboot can be performed if
``package_reboot_if_required`` is specified.
boot using any available package manager present on a system such as apt,
pkg, snap, yum or zypper. If any packages are to be installed or an upgrade
is to be performed then the package cache will be updated first. If a
package installation or upgrade requires a reboot, then a reboot can be
performed if ``package_reboot_if_required`` is specified.
examples:
- comment: |
Example 1:
file: cc_package_update_upgrade_install/example1.yaml
- comment: "By default, ``package_upgrade: true`` performs upgrades on any installed package manager. To avoid calling ``snap refresh`` in images with snap installed, set snap refresh.hold to ``forever`` will prevent cloud-init's snap interaction during any boot"
file: cc_package_update_upgrade_install/example2.yaml
name: Package Update Upgrade Install
title: Update, upgrade, and install packages
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#cloud-config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It seems a bit odd to have 2 of our 3 examples dedicated to a workaround for a single package manager on a single OS. I don't think we really need an example showing how to unhold it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me. I'll drop that example. of needed it is a corner case for sure.

package_update: true
package_upgrade: true
snap:
commands:
00: snap refresh --hold=forever
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is a workaround, we should include a comment explaining why someone might want this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do include that comment in data.yaml for example2 which is rendered above the example in the modules documentation
- comment: "By default,package_upgrade: truewill trigger upgrades on any installed package manager. To avoid callingsnap refreshin images with snap installed, set snap refresh.hold toforever will prevent cloud-init's snap interaction during any boot". I'd updated this comment and the general desciprtion of the module to clarify that it interacts with any installed package managers which include apt/pkg/yum/zypper/snap

package_reboot_if_required: true
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,40 @@ def test_snap_packages_are_installed(self, class_client):
assert "curl" in output
assert "postman" in output

def test_snap_refresh_not_called_when_refresh_hold_forever(
self, class_client
):
"""Assert snap refresh is not called when snap refresh --hold is set.
Certain network-limited or secure environments may opt to avoid
contacting snap API endpoints. In those scenarios, it is expected
that automated snap refresh is held for all snaps. Typically, this is
done with snap refresh --hold in those environments.
Assert cloud-init does not attempt to call snap refresh when
refresh.hold is forever.
"""
assert class_client.execute(
[
"grep",
r"Running command \['snap', 'refresh'",
"/var/log/cloud-init.log",
]
).ok
assert class_client.execute("snap refresh --hold").ok
class_client.instance.clean()
class_client.restart()
assert class_client.execute(
[
"grep",
r"Running command \['snap', 'refresh']",
"/var/log/cloud-init.log",
]
).failed
assert class_client.execute(
"grep 'Skipping snap refresh' /var/log/cloud-init.log"
).ok


HELLO_VERSIONS_BY_RELEASE = {
"oracular": "2.10-3build2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def _isfile(filename: str):

caplog.set_level(logging.WARNING)
with mock.patch(
"cloudinit.subp.subp", return_value=("fakeout", "fakeerr")
"cloudinit.subp.subp", return_value=SubpResult("{}", "fakeerr")
) as m_subp:
with mock.patch("os.path.isfile", side_effect=_isfile):
with mock.patch(M_PATH + "time.sleep") as m_sleep:
Expand Down
84 changes: 81 additions & 3 deletions tests/unittests/distros/test_ubuntu.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
# This file is part of cloud-init. See LICENSE file for license information.
import logging

import pytest

from cloudinit.distros import fetch
from cloudinit.subp import SubpResult


class TestPackageCommand:
Expand All @@ -14,7 +17,7 @@ def test_package_command_only_refresh_snap_when_available(
"cloudinit.distros.ubuntu.Snap.available",
return_value=snap_available,
)
m_snap_upgrade_packges = mocker.patch(
m_snap_upgrade_packages = mocker.patch(
blackboxsw marked this conversation as resolved.
Show resolved Hide resolved
"cloudinit.distros.ubuntu.Snap.upgrade_packages",
return_value=snap_available,
)
Expand All @@ -27,6 +30,81 @@ def test_package_command_only_refresh_snap_when_available(
m_apt_run_package_command.assert_called_once_with("upgrade")
m_snap_available.assert_called_once()
if snap_available:
m_snap_upgrade_packges.assert_called_once()
m_snap_upgrade_packages.assert_called_once()
else:
m_snap_upgrade_packages.assert_not_called()

@pytest.mark.parametrize(
"subp_side_effect,expected_log",
(
pytest.param(
[
SubpResult(
stdout='{"refresh": {"hold": "forever"}}', stderr=None
)
],
"Skipping snap refresh because refresh.hold is set to"
" 'forever'",
id="skip_snap_refresh_due_to_global_hold_forever",
),
pytest.param(
[
SubpResult(
stdout=(
'{"refresh": {"hold":'
' "2024-07-08T15:38:20-06:00"}}'
),
stderr=None,
),
SubpResult(stdout="All snaps up to date.", stderr=""),
],
"",
id="perform_snap_refresh_due_to_temporary_global_hold",
),
pytest.param(
[
SubpResult(
stdout="{}",
stderr=(
'error: snap "core" has no "refresh.hold" '
"configuration option"
),
),
SubpResult(stdout="All snaps up to date.", stderr=""),
],
"",
id="snap_refresh_performed_when_no_global_hold_is_set",
),
),
)
def test_package_command_avoids_snap_refresh_when_refresh_hold_is_forever(
self, subp_side_effect, expected_log, caplog, mocker
):
"""Do not call snap refresh when snap refresh.hold is forever.
This indicates an environment where snaps refreshes are not preferred
for whatever reason.
"""
m_snap_available = mocker.patch(
"cloudinit.distros.ubuntu.Snap.available",
return_value=True,
)
m_subp = mocker.patch(
"cloudinit.subp.subp",
side_effect=subp_side_effect,
)
m_apt_run_package_command = mocker.patch(
"cloudinit.distros.package_management.apt.Apt.run_package_command",
)
cls = fetch("ubuntu")
distro = cls("ubuntu", {}, None)
with caplog.at_level(logging.INFO):
distro.package_command("upgrade")
m_apt_run_package_command.assert_called_once_with("upgrade")
m_snap_available.assert_called_once()
expected_calls = [mocker.call(["snap", "get", "system", "-d"])]
if expected_log:
assert expected_log in caplog.text
else:
m_snap_upgrade_packges.assert_not_called()
expected_calls.append(mocker.call(["snap", "refresh"]))
assert m_subp.call_args_list == expected_calls
Loading