Fix silent package upgrades in package install by vaibhavdaren · Pull Request #4937 · teemtee/tmt

vaibhavdaren · 2026-05-29T00:36:27Z

On RHEL and CentOS Stream, DNF ships with best=True by default. When tmt installed a set of packages and any one was missing, it sent the entire set to DNF via a shell-level presence check:

rpm -q --whatprovides pkg-a pkg-b pkg-c || dnf install -y pkg-a pkg-b pkg-c

If pkg-c was missing, the || fired and DNF received all three packages. With best=True, DNF silently upgraded pkg-a and pkg-b even though they were already installed. An unintended side effect that could break test environments.

Fix

Replace the shell-level check || install one-liner with a Python 2-stage process in PackageManager.install():

check_presence() — determine which packages are actually missing (per-package, Python level)
install() — pass only the missing subset to the package manager

This is now the single, consistent implementation for all package managers. No engine needs to handle check_first in its install script.

Previously, installing a set [A (installed, old), B (missing)] would inadvertently upgrade A (via best=True).
Now only B is passed to the package manager : A is untouched. This is intentional: check_first means "skip if present", not "upgrade if outdated".
Callers that need a guaranteed upgrade can pass check_first=False.

@bajertom edit: fixes #4909

gemini-code-assist

Code Review

This pull request refactors _construct_install_script in tmt/package_managers/dnf.py to avoid best=True upgrades when options.check_first is enabled by checking package presence with rpm before running the DNF install command. Feedback recommends guarding against empty installables and using the Command class instead of ShellScript to handle variable arguments safely and prevent execution errors.

vaibhavdaren · 2026-05-29T02:10:59Z

Unit Tests need to be fixed.

The set-level presence check (`rpm -q --whatprovides A B C || dnf install A B C`) fails as a unit if any single package is absent, sending the entire set to DNF. On RHEL and CentOS Stream where best=True is the default, already-installed packages in that set become upgrade candidates, silently landing guests at versions different from what `requires` declared. Replace the set-level guard with a per-package bash accumulator loop that identifies exactly which packages are missing, then passes only those to a single DNF invocation.

Replace the bash accumulator loop with a cleaner approach: call check_presence() first, install only missing packages, revert the engine to a plain unconditional install script. Update tests.

Use a CHECK_FIRST flag in INSTALL_TEMPLATE when set, the rendered script gates apt reinstall behind `dpkg-query --show` so reinstalling a missing package fails fast with a clear error rather than a confusing apt message. Also fix the non-filesystem-path reinstall path to escape installables consistently with install.

DnfEngine.check_presence now emits one command per package of the form `rpm -q --whatprovides PKG >&2 || echo PKG`, which echoes the bare package name to stdout when missing (instead of verbose rpm error messages). Bootc.check_presence still used the old zip-based parser that matched patterns like `no package provides X`, so missing packages were never recognised and falsely reported as present. Update Bootc.check_presence to treat each stdout line as the name of a missing package directly, consistent with how Dnf.check_presence works after the same refactor.

vaibhavdaren · 2026-06-05T08:59:01Z

/packit retest-failed

LecrisUT

Only some minor comments left, otherwise lgtm

LecrisUT · 2026-06-05T15:13:26Z

        return self.guest.run(
-            self.engine.reinstall(*installables, options=options).to_shell_command()
+            self.engine.reinstall(*to_reinstall, options=options).to_shell_command()
        )


It is the same as the base, no longer needed to be defined

The filtering logic is identical to the base, but the dispatch has to differ. GuestMock.execute runs commands inside the mock shell, while mock reinstall needs to run on the host via guest.run. Dropping the override would cause the base to call guest.execute(ShellScript('mock --pm-cmd reinstall ...') which fails because mock is not.

vaibhavdaren · 2026-06-05T20:51:57Z

Need to fix test failures

CI issues due to

>>> Usable URL not found
Failed to download metadata (metalink: "https://mirrors.fedoraproject.org/metalink?repo=eln-extras-1&arch=x86_64") for repository "eln-extras": Usable URL not found
Error: building at STEP "RUN <<EOF": while running runtime: exit status 1
make: *** [Makefile:316: images/test/tmt/container/test/fedora/eln:latest] Error 1
make: Leaving directory '/var/ARTIFACTS/work-extended-unit-testsit8qobnw/plans/features/extended-unit-tests/discover/default-0/tests'
:: [ 20:39:06 ] :: [ WARNING  ] :: rlWaitForCmd: Max number of test command invocations reached!
:: [ 20:39:06 ] :: [  FATAL   ] :: Unable to prepare the images
:: [ 20:39:06 ] :: [   FAIL   ] :: Unable to prepare the images (Assert: expected 0, got 1)
::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::
::   Duration: 166s
::   Assertions: 1 good, 1 bad
::   RESULT: WARN (Setup)

tmt-report-result: saving /Setup output /tmp/tmp.nRr5DdVGts into /var/ARTIFACTS/work-extended-unit-testsit8qobnw/plans/features/extended-unit-tests/execute/data/guest/default-0/tests/unit/with-system-packages/extended-1/data/Setup/output.txt
tmt-report-result: adding to the result file /var/ARTIFACTS/work-extended-unit-testsit8qobnw/plans/features/extended-unit-tests/execute/data/guest/default-0/tests/unit/with-system-packages/extended-1/data/tmt-report-results.yaml

Skip presence check when check_first is False, matching base class behavior.

vaibhavdaren · 2026-06-06T09:58:05Z

/packit retest-failed

LecrisUT

Some relevant failing tests, and still a lot to follow up after this, but otherwise looks fine as first step

LecrisUT · 2026-06-08T09:13:11Z

 {% endif %}

-{% if OPTIONS.check_first %}
+{% if CHECK_FIRST %}


This should need a default if not defined?

it's been fixed, so it can be resolved

vaibhavdaren · 2026-06-09T01:22:02Z

/packit build

tcornell-bus · 2026-06-09T18:14:25Z

Is this meant to be tied to an issue?

tcornell-bus · 2026-06-09T18:17:45Z

Could use a release note, please.

tcornell-bus · 2026-06-09T18:16:52Z

            for match in pattern.finditer(output):
                yield match.group(1)

+    def _check_first_filter(


since check_first = True by default, and this will change the behavior of all install operations. Is there a way for a user to set check_first = False easily on the commandline and through testing-farm?

The only effective change should be in the grouping of the check (which was previously in the engine script), and would be difficult to make that configurable (if not counterproductive). The check_first was never exposed to the user and it is instead indirectly set, e.g.

tmt/tmt/package_managers/dnf.py

Line 373 in b1751ea

options.check_first = False

But exposing this new interface, I think would be ok and in line with the discussion previously.

I am just concerned someone somewhere is unwittingly relying on the old behavior, and when it changes, it'll be an issue. Having it configurable right away should alleviate that.

Addressed in b3c52e1. Users can opt out via --no-check-first CLI flag, check-first: false in the plan config, or by setting TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST=0 as an environment variable.

bajertom · 2026-06-10T09:19:49Z

Is this meant to be tied to an issue?

I've added the issue. Same goes for #4938

LecrisUT · 2026-06-10T16:31:56Z

+        option='--check-first / --no-check-first',
+        is_flag=True,
+        show_default=True,
+        envvar='TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST',


These are created and handled automatically. Please no

LecrisUT · 2026-06-10T16:33:02Z

+        help="""
+            Check whether packages are already installed before attempting
+            to install them. Can also be set via the
+            ``TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST`` environment variable.


We do not document that these for each and other variable. It should be handled manually

LecrisUT · 2026-06-10T16:33:17Z

    )

+    check_first: bool = field(
+        default=configure_bool_constant(True, 'TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST'),


Suggested change

default=configure_bool_constant(True, 'TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST'),

default=True,

LecrisUT · 2026-06-10T16:36:52Z

+    The :ref:`/plugins/prepare/install` plugin no longer silently upgrades
+    already-installed packages. Only missing packages are passed to install,


We do not guarantee that. Would reverse these 2 sentences with the first one being a "should no longer silently upgrade pre-installed packages".

LecrisUT · 2026-06-10T16:39:20Z

+    and only present packages are passed to reinstall. The new ``check-first``
+    option (default: enabled) can be disabled via the ``--no-check-first``
+    CLI flag, the ``check-first: false`` plan config option, or the
+    ``TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST`` environment variable.


Do we have to document every such way to interact with it. Would just keep it at added check-first to the /plugins/prepare/install plugin and call it a day.

vaibhavdaren marked this pull request as draft May 29, 2026 00:36

gemini-code-assist Bot reviewed May 29, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py Outdated

vaibhavdaren changed the title ~~Fix silent package upgrades under DNF best=True~~ Fix silent package upgrades in package install May 29, 2026

LecrisUT reviewed May 29, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py

vaibhavdaren mentioned this pull request Jun 2, 2026

Discuss the design and agree on the package installation approach #4909

Open

vaibhavdaren added plugin | artifact Related to the `prepare/artifact` plugin. area | package managers Changes related to implementations of package managers labels Jun 2, 2026

vaibhavdaren added this to planning Jun 2, 2026

github-project-automation Bot moved this to backlog in planning Jun 2, 2026

vaibhavdaren moved this from backlog to review in planning Jun 2, 2026

vaibhavdaren added the ci | full test Pull request is ready for the full test execution label Jun 2, 2026

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch from 7f789d9 to 7e4f2d0 Compare June 2, 2026 12:31

vaibhavdaren marked this pull request as ready for review June 2, 2026 12:32

graphite-app Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py Outdated

vaibhavdaren marked this pull request as draft June 2, 2026 12:35

LecrisUT reviewed Jun 2, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py

Comment thread tmt/package_managers/dnf.py Outdated

vaibhavdaren self-assigned this Jun 2, 2026

vaibhavdaren requested a review from AthreyVinay June 2, 2026 16:38

vaibhavdaren marked this pull request as ready for review June 2, 2026 16:51

vaibhavdaren added this to the 1.75 milestone Jun 2, 2026

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch from f05316e to 0a2afcc Compare June 3, 2026 10:50

graphite-app Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py Outdated

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch from 0a2afcc to 27b8cc4 Compare June 3, 2026 11:27

AthreyVinay reviewed Jun 3, 2026

View reviewed changes

Comment thread tmt/package_managers/apt.py Outdated

graphite-app Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py Outdated

LecrisUT reviewed Jun 3, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py Outdated

vaibhavdaren added 3 commits June 4, 2026 09:04

Move DNF check_first to 2-stage Python process in Dnf.install()

52ad1c2

Replace the bash accumulator loop with a cleaner approach: call check_presence() first, install only missing packages, revert the engine to a plain unconditional install script. Update tests.

Address review comments: move check_first to PackageManager.install()

b22c9f5

vaibhavdaren added 2 commits June 5, 2026 13:23

therazix removed this from the 1.75 milestone Jun 5, 2026

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch from 01bc8c2 to 1798c59 Compare June 5, 2026 11:51

LecrisUT reviewed Jun 5, 2026

View reviewed changes

Comment thread tmt/package_managers/dnf.py Outdated

LecrisUT reviewed Jun 5, 2026

View reviewed changes

vaibhavdaren added 2 commits June 6, 2026 05:46

Fix RpmOstree.install to respect check_first=False

cc107d9

Skip presence check when check_first is False, matching base class behavior.

Pass CHECK_FIRST=False explicitly in AptEngine.install template calls

60a97d3

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch 2 times, most recently from 5e22fc6 to d9ec3a3 Compare June 6, 2026 11:30

expose checkfirst as a option

b3c52e1

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch from d9ec3a3 to fea9193 Compare June 7, 2026 18:54

graphite-app Bot reviewed Jun 7, 2026

View reviewed changes

Comment thread tmt/package_managers/rpm_ostree.py

LecrisUT approved these changes Jun 8, 2026

View reviewed changes

This was referenced Jun 8, 2026

Investigate whether RpmOstree.install override is still needed after per-package check refactor #4970

Open

Refactor _extra_dnf_options to avoid command string comparison #4971

Open

vaibhavdaren changed the base branch from main to fix/python3.15 June 8, 2026 23:23

vaibhavdaren force-pushed the vaibhav-per-package-install-check branch from 5aa7322 to 60a97d3 Compare June 8, 2026 23:27

vaibhavdaren changed the base branch from fix/python3.15 to main June 9, 2026 01:21

vaibhavdaren added this to the 1.76 milestone Jun 9, 2026

tcornell-bus reviewed Jun 9, 2026

View reviewed changes

vaibhavdaren mentioned this pull request Jun 9, 2026

enforce artifact repo origin via NEVRA substitution and reinstall phase #4976

Draft

LecrisUT reviewed Jun 10, 2026

View reviewed changes

	default=configure_bool_constant(True, 'TMT_PLUGIN_PREPARE_INSTALL_CHECK_FIRST'),
	default=True,

		The :ref:`/plugins/prepare/install` plugin no longer silently upgrades
		already-installed packages. Only missing packages are passed to install,

Conversation

vaibhavdaren commented May 29, 2026 • edited by bajertom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

vaibhavdaren commented May 29, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaibhavdaren commented Jun 5, 2026

Uh oh!

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vaibhavdaren commented Jun 5, 2026

Uh oh!

vaibhavdaren commented Jun 6, 2026

Uh oh!

Uh oh!

LecrisUT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vaibhavdaren commented Jun 9, 2026

Uh oh!

tcornell-bus commented Jun 9, 2026

Uh oh!

tcornell-bus commented Jun 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajertom commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vaibhavdaren commented May 29, 2026 •

edited by bajertom

Loading