apt: include apt preferences (e.g. pinning) when selecting packages #78327

phemmer · 2022-07-21T18:41:15Z

SUMMARY

Factor in apt's preferences files when determining candidate packages to install.

Fixes #77969

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

apt

ADDITIONAL INFORMATION

s-hertel

Thanks for the quick fix.

Would you be able to complete this with tests? Feel free to modify those that are here if it's convenient: https://github.com/ansible/ansible/pull/78319/files.

phemmer · 2022-07-22T04:05:40Z

Sure, question then: What do we expect the behavior to be if someone has pinned version 1.0, and they do >=1.1? Should it install 1.1, or should it install nothing?

bcoca · 2022-07-22T13:05:39Z

it should 'fail' and note the conflict so user can either override the pinning or remove the version specified

ansibot · 2022-07-25T03:19:50Z

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/apt.py:715:36: E712: comparison to True should be 'if cond is not True:' or 'if not cond:'

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/apt.py:715:15: singleton-comparison: Comparison 'version_installable != True' should be 'version_installable is not True' if checking for the singleton value True, or 'not version_installable' if testing for falsiness

click here for bot help

noonedeadpunk

From what I see, ansible now is not providing version to that apt when it's not required. And pinning usecases I tried seems working now as well.

noonedeadpunk · 2022-07-25T07:58:17Z

lib/ansible/modules/apt.py

-        if (not installed and not only_upgrade) or (installed and not installed_version) or (upgrade and version_installable):
-            if version_installable or version:
-                pkg_list.append("'%s=%s'" % (name, version_installable or version))
+        if (not installed and not only_upgrade) or (installed and not installed_version and version_installable) or (upgrade and version_installable):


I'm not sure I read this logic properly, or well, fully see all cases when version will be specified for the package installation (either because of my skills or overall code readability), so having some comment about intended behavior would be useful here IMO.

phemmer · 2022-07-25T12:33:23Z

From what I see, ansible now is not providing version to that apt when it's not required.

No, it's the opposite. It's now passing the version more. There were a few corner cases where it wasn't passing the version.

noonedeadpunk · 2022-07-25T12:45:53Z

No, it's the opposite. It's now passing the version more. There were a few corner cases where it wasn't passing the version.

well, even with debug ansible not showing actual command that is ran, but according to output I'd say that version is not provided here:

TASK [package] ****************************************************************************************************************************************************************************************************************************************************************
task path: /home/ubuntu/openstack-ansible/test.yml:3
Running ansible.legacy.apt
Using module file /opt/ansible-runtime/lib/python3.8/site-packages/ansible/modules/apt.py
Pipelining is enabled.
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: root
<127.0.0.1> EXEC /bin/sh -c '/opt/ansible-runtime/bin/python3 && sleep 0'
changed: [localhost] => {
    "cache_update_time": 1658707710,
    "cache_updated": false,
    "changed": true,
    "diff": {},
    "invocation": {
        "module_args": {
            "allow_change_held_packages": false,
            "allow_downgrade": false,
            "allow_unauthenticated": false,
            "autoclean": false,
            "autoremove": false,
            "cache_valid_time": 0,
            "clean": false,
            "deb": null,
            "default_release": null,
            "dpkg_options": "force-confdef,force-confold",
            "fail_on_autoremove": false,
            "force": false,
            "force_apt_get": false,
            "install_recommends": null,
            "lock_timeout": 60,
            "name": "erlang-base",
            "only_upgrade": false,
            "package": [
                "erlang-base"
            ],
            "policy_rc_d": null,
            "purge": false,
            "state": "present",
            "update_cache": null,
            "update_cache_retries": 5,
            "update_cache_retry_max_delay": 12,
            "upgrade": null
        }
    },
    "stderr": "",
    "stderr_lines": [],
    "stdout": "Reading package lists...\nBuilding dependency tree...\nReading state information...\nSuggested packages:\n  erlang-tools erlang erlang-manpages erlang-doc\nRecommended packages:\n  libsctp1 erlang-crypto erlang-syntax-tools\nThe following NEW packages will be installed:\n  erlang-base\n0 upgraded, 1 newly installed, 0 to remove and 101 not upgraded.\nNeed to get 0 B/8063 kB of archives.\nAfter this operation, 14.4 MB of additional disk space will be used.\nSelecting previously unselected package erlang-base.\r\n(Reading database ... \r(Reading database ... 5%\r(Reading database ... 10%\r(Reading database ... 15%\r(Reading database ... 20%\r(Reading database ... 25%\r(Reading database ... 30%\r(Reading database ... 35%\r(Reading database ... 40%\r(Reading database ... 45%\r(Reading database ... 50%\r(Reading database ... 55%\r(Reading database ... 60%\r(Reading database ... 65%\r(Reading database ... 70%\r(Reading database ... 75%\r(Reading database ... 80%\r(Reading database ... 85%\r(Reading database ... 90%\r(Reading database ... 95%\r(Reading database ... 100%\r(Reading database ... 105039 files and directories currently installed.)\r\nPreparing to unpack .../erlang-base_1%3a22.2.7+dfsg-1_amd64.deb ...\r\nUnpacking erlang-base (1:22.2.7+dfsg-1) ...\r\nSetting up erlang-base (1:22.2.7+dfsg-1) ...\r\nCreated symlink /etc/systemd/system/multi-user.target.wants/epmd.service → /lib/systemd/system/epmd.service.\r\nCreated symlink /etc/systemd/system/sockets.target.wants/epmd.socket → /lib/systemd/system/epmd.socket.\r\nSearching for services which depend on erlang and should be started... none found.\r\nProcessing triggers for man-db (2.9.1-1) ...\r\n",
    "stdout_lines": [
        "Reading package lists...",
        "Building dependency tree...",
        "Reading state information...",
        "Suggested packages:",
        "  erlang-tools erlang erlang-manpages erlang-doc",
        "Recommended packages:",
        "  libsctp1 erlang-crypto erlang-syntax-tools",
        "The following NEW packages will be installed:",
        "  erlang-base",
        "0 upgraded, 1 newly installed, 0 to remove and 101 not upgraded.",
        "Need to get 0 B/8063 kB of archives.",
        "After this operation, 14.4 MB of additional disk space will be used.",
        "Selecting previously unselected package erlang-base.",
        "(Reading database ... ",
        "(Reading database ... 5%",
        "(Reading database ... 10%",
        "(Reading database ... 15%",
        "(Reading database ... 20%",
        "(Reading database ... 25%",
        "(Reading database ... 30%",
        "(Reading database ... 35%",
        "(Reading database ... 40%",
        "(Reading database ... 45%",
        "(Reading database ... 50%",
        "(Reading database ... 55%",
        "(Reading database ... 60%",
        "(Reading database ... 65%",
        "(Reading database ... 70%",
        "(Reading database ... 75%",
        "(Reading database ... 80%",
        "(Reading database ... 85%",
        "(Reading database ... 90%",
        "(Reading database ... 95%",
        "(Reading database ... 100%",
        "(Reading database ... 105039 files and directories currently installed.)",
        "Preparing to unpack .../erlang-base_1%3a22.2.7+dfsg-1_amd64.deb ...",
        "Unpacking erlang-base (1:22.2.7+dfsg-1) ...",
        "Setting up erlang-base (1:22.2.7+dfsg-1) ...",
        "Created symlink /etc/systemd/system/multi-user.target.wants/epmd.service → /lib/systemd/system/epmd.service.",
        "Created symlink /etc/systemd/system/sockets.target.wants/epmd.socket → /lib/systemd/system/epmd.socket.",
        "Searching for services which depend on erlang and should be started... none found.",
        "Processing triggers for man-db (2.9.1-1) ..."
    ]
}

But why explicitly add version to the package, when operator haven't asked for it? What motivation is behind that and why not just trust system dependency resolution?
I won't even touch part of the real use-case for this feature one more time, as I personally fail to see it's value because pinning is applied only during runtime. But IMO, if you as an operator haven't asked to do smth (like provide version for the package), then let system decide and not inject it. It's more common usecase not to specify version to the package, and I believe it should execute faster as well, as some part of the code with dependency resolution can be just skipped?

phemmer · 2022-07-25T13:20:02Z

@bcoca
it should 'fail' and note the conflict so user can either override the pinning or remove the version specified

Ok, doing this required a little more change than just the original 2 lines. However it also fixed a few cases where if you asked the module to install a version, but no version matched, it would silently do nothing (no change, but also no error).

Note that the way I addressed this was though this: https://github.com/ansible/ansible/pull/78327/files#diff-821fcd0036eed7308025fa9e825de90fb23fd6ffe6d86e82db56802c5fddc0f7R715-R718. However for virtual packages where we can't figure out what real package to install, and where we pass apt-get a package name without version specifier, it's still possible to get a 0 upgraded, 0 newly installed result where we have no change and no error. It might be prudent to convert 0 upgraded, 0 newly installed into an error. However this is not new behavior, and it would seem has been this way for ages. So could also just leave it.

bcoca · 2022-07-25T13:33:23Z

@noonedeadpunk debug will log the actual commands to the log of the remote system (syslog/journal)

@phemmer I'm ok with splitting that into it's own PR

phemmer · 2022-07-25T13:41:07Z

No, it's the opposite. It's now passing the version more. There were a few corner cases where it wasn't passing the version.

well, even with debug ansible not showing actual command that is ran, but according to output I'd say that version is not provided here:

That's a virtual package. When the module can't figure out what real package is to satisfy a virtual package, it stops trying and lets apt-get figure it out.

But why explicitly add version to the package, when operator haven't asked for it?

It doesn't matter if the operator has asked for it. The operator asked for a package, why should it matter if ansible figures out the version or if apt-get figures out the version?

What motivation is behind that and why not just trust system dependency resolution?

Because ansible needs to know what the version candidates are so that it can properly do change detection. Once ansible knows the version, there is no harm in using the version. Yes we could add code to explicitly strip the version off when the user didn't specify it, but there is no point to doing so.

It's more common usecase not to specify version to the package, and I believe it should execute faster as well, as some part of the code with dependency resolution can be just skipped?

Because as mentioned, ansible needs to figure out whether it should do something before actually doing it. In the process of checking whether there is a package that can be installed, it also obtains the version information. So determining the version and passing it to apt-get does not add additional overhead.

s-hertel · 2022-07-25T13:42:21Z

@noonedeadpunk The dependency resolution would need to happen regardless - if no version is specified, the module checks for upgrades. This just also identifies which upgrade - which, you're right, is not strictly necessary if no version or release is specified since the outcome should be identical. But this seems simpler to me than special-casing no version + no release. It also has the perk of allowing check mode + diff mode to reflect what would be installed.

noonedeadpunk · 2022-07-25T13:47:59Z

debug will log the actual commands to the log of the remote system (syslog/journal)

yeah, eventually it's in history.log and command had version specified: Commandline: /usr/bin/apt-get -y -o Dpkg::Options::=--force-confdef -o Dpkg::Options::=--force-confold install erlang-base=1:22.2.7+dfsg-1

Ok, thanks for explanation.
I believe you're right, just setting version feels a bit fragile for me at least and feels like having more maintenance cost. But since it's needed and done anyway, likely it doesn't matter indeed, as long as it works.

phemmer · 2022-07-25T13:55:28Z

Hold on merging. I think I see an issue around the use of only_upgrade. Will add a test case.

phemmer · 2022-07-25T18:03:05Z

Hold on merging. I think I see an issue around the use of only_upgrade. Will add a test case.

Was actually no issue. So good to go.

s-hertel

Changes still lgtm

lib/ansible/modules/apt.py

changelogs/fragments/77969-apt-preferences.yml

Fixes ansible#77969

s-hertel · 2022-08-05T14:49:17Z

rebuild_merge

…nsible#78327) Fixes ansible#77969 (cherry picked from commit 04e8927)

phemmer mentioned this pull request Jul 21, 2022

apt: Installing packages ignores pinning preferences #77969

Closed

1 task

mattclay removed the needs_triage Needs a first human triage before being processed. label Jul 21, 2022

s-hertel mentioned this pull request Jul 21, 2022

apt - fix installing preferred candidate if no version is specified #78319

Closed

s-hertel approved these changes Jul 21, 2022

View reviewed changes

phemmer force-pushed the apt-fix-preferences branch from 676f62d to c15eb79 Compare July 25, 2022 03:04

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jul 25, 2022

phemmer force-pushed the apt-fix-preferences branch from c15eb79 to 0a1ebee Compare July 25, 2022 03:39

ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 25, 2022

phemmer force-pushed the apt-fix-preferences branch from 0a1ebee to f9fd32f Compare July 25, 2022 04:04

noonedeadpunk reviewed Jul 25, 2022

View reviewed changes

phemmer force-pushed the apt-fix-preferences branch from f9fd32f to eb802dc Compare July 25, 2022 12:45

phemmer force-pushed the apt-fix-preferences branch from eb802dc to 7c38a51 Compare July 25, 2022 12:52

phemmer force-pushed the apt-fix-preferences branch from 7c38a51 to 50e4ecb Compare July 25, 2022 13:33

noonedeadpunk approved these changes Jul 25, 2022

View reviewed changes

phemmer force-pushed the apt-fix-preferences branch from 50e4ecb to 2ad5d68 Compare July 25, 2022 17:43

s-hertel reviewed Jul 25, 2022

View reviewed changes

lib/ansible/modules/apt.py Show resolved Hide resolved

changelogs/fragments/77969-apt-preferences.yml Outdated Show resolved Hide resolved

apt: include apt preferences (e.g. pinning) when selecting packages

d616279

Fixes ansible#77969

phemmer force-pushed the apt-fix-preferences branch from 2ad5d68 to d616279 Compare July 26, 2022 02:19

s-hertel approved these changes Jul 26, 2022

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 3, 2022

ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 5, 2022

ansibot merged commit 04e8927 into ansible:devel Aug 5, 2022

s-hertel pushed a commit to s-hertel/ansible that referenced this pull request Aug 5, 2022

apt: include apt preferences (e.g. pinning) when selecting packages (a…

ae8fd49

…nsible#78327) Fixes ansible#77969 (cherry picked from commit 04e8927)

s-hertel mentioned this pull request Aug 5, 2022

[2.13] apt: include apt preferences (e.g. pinning) when selecting packages #78462

Merged

ansible locked and limited conversation to collaborators Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apt: include apt preferences (e.g. pinning) when selecting packages #78327

apt: include apt preferences (e.g. pinning) when selecting packages #78327

phemmer commented Jul 21, 2022

s-hertel left a comment

phemmer commented Jul 22, 2022

bcoca commented Jul 22, 2022

ansibot commented Jul 25, 2022

noonedeadpunk left a comment

noonedeadpunk Jul 25, 2022

phemmer commented Jul 25, 2022

noonedeadpunk commented Jul 25, 2022 •

edited

Loading

phemmer commented Jul 25, 2022

bcoca commented Jul 25, 2022

phemmer commented Jul 25, 2022

s-hertel commented Jul 25, 2022

noonedeadpunk commented Jul 25, 2022

phemmer commented Jul 25, 2022

phemmer commented Jul 25, 2022

s-hertel left a comment

s-hertel commented Aug 5, 2022

apt: include apt preferences (e.g. pinning) when selecting packages #78327

apt: include apt preferences (e.g. pinning) when selecting packages #78327

Conversation

phemmer commented Jul 21, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

s-hertel left a comment

Choose a reason for hiding this comment

phemmer commented Jul 22, 2022

bcoca commented Jul 22, 2022

ansibot commented Jul 25, 2022

noonedeadpunk left a comment

Choose a reason for hiding this comment

noonedeadpunk Jul 25, 2022

Choose a reason for hiding this comment

phemmer commented Jul 25, 2022

noonedeadpunk commented Jul 25, 2022 • edited Loading

phemmer commented Jul 25, 2022

bcoca commented Jul 25, 2022

phemmer commented Jul 25, 2022

s-hertel commented Jul 25, 2022

noonedeadpunk commented Jul 25, 2022

phemmer commented Jul 25, 2022

phemmer commented Jul 25, 2022

s-hertel left a comment

Choose a reason for hiding this comment

s-hertel commented Aug 5, 2022

noonedeadpunk commented Jul 25, 2022 •

edited

Loading