dnf/dnf5: address issues discovered in 2.16 yum tests (#83659)

Fixes #85554

lib/ansible/modules/dnf.py

@@ -208,6 +208,8 @@ options:
         packages to install (because dependencies between the downgraded
         package and others can cause changes to the packages which were
         in the earlier transaction).
+      - Since this feature is not provided by C(dnf) itself but by M(ansible.builtin.dnf) module,
+        using this in combination with wildcard characters in O(name) may result in an unexpected results.
     type: bool
     default: "no"
     version_added: "2.7"
@@ -701,72 +703,56 @@ class DnfModule(YumDnf):
         self.module.exit_json(msg="", results=results)
 
     def _is_installed(self, pkg):
-        installed_query = dnf.subject.Subject(pkg).get_best_query(sack=self.base.sack).installed()
-        if dnf.util.is_glob_pattern(pkg):
-            available_query = dnf.subject.Subject(pkg).get_best_query(sack=self.base.sack).available()
-            return not (
-                {p.name for p in available_query} - {p.name for p in installed_query}
-            )
-        else:
-            return bool(installed_query)
+        return bool(dnf.subject.Subject(pkg).get_best_query(sack=self.base.sack).installed())
 
     def _is_newer_version_installed(self, pkg_spec):
+        # expects a versioned package spec
         try:
             if isinstance(pkg_spec, dnf.package.Package):
                 installed = sorted(self.base.sack.query().installed().filter(name=pkg_spec.name, arch=pkg_spec.arch))[-1]
                 return installed.evr_gt(pkg_spec)
             else:
-                available = dnf.subject.Subject(pkg_spec).get_best_query(sack=self.base.sack).available()
-                installed = self.base.sack.query().installed().filter(name=available[0].name)
-                for arch in sorted(set(p.arch for p in installed)):  # select only from already-installed arches for this case
-                    installed_pkg = sorted(installed.filter(arch=arch))[-1]
-                    try:
-                        available_pkg = sorted(available.filter(arch=arch))[-1]
-                    except IndexError:
-                        continue  # nothing currently available for this arch; keep going
-                    if installed_pkg.evr_gt(available_pkg):
-                        return True
-                return False
+                solution = dnf.subject.Subject(pkg_spec).get_best_solution(self.base.sack)
+                q = solution["query"]
+                if not q or not solution['nevra'] or solution['nevra'].has_just_name():
+                    return False
+                installed = self.base.sack.query().installed().filter(name=solution['nevra'].name)
+                if not installed:
+                    return False
+                return installed[0].evr_gt(q[0])
         except IndexError:
             return False
 
     def _mark_package_install(self, pkg_spec, upgrade=False):
         """Mark the package for install."""
-        is_newer_version_installed = self._is_newer_version_installed(pkg_spec)
-        is_installed = self._is_installed(pkg_spec)
         msg = ''
         try:
-            if is_newer_version_installed:
+            if dnf.util.is_glob_pattern(pkg_spec):
+                # Special case for package specs that contain glob characters.
+                # For these we skip `is_installed` and `is_newer_version_installed` tests that allow for the
+                # allow_downgrade feature and pass the package specs to dnf.
+                # Since allow_downgrade is not available in dnf and while it is relatively easy to implement it for
+                # package specs that evaluate to a single package, trying to mimic what would the dnf machinery do
+                # for glob package specs and then filtering those for allow_downgrade appears to always
+                # result in naive/inferior solution.
+                # NOTE this has historically never worked even before https://github.com/ansible/ansible/pull/82725
+                # where our (buggy) custom code ignored wildcards for the installed checks.
+                # TODO reasearch how feasible it is to implement the above
+                if upgrade:
+                    # for upgrade we pass the spec to both upgrade and install, to satisfy both available and installed
+                    # packages evaluated from the glob spec
+                    try:
+                        self.base.upgrade(pkg_spec)
+                    except dnf.exceptions.PackagesNotInstalledError:
+                        pass
+                self.base.install(pkg_spec, strict=self.base.conf.strict)
+            elif self._is_newer_version_installed(pkg_spec):
                 if self.allow_downgrade:
-                    # dnf only does allow_downgrade, we have to handle this ourselves
-                    # because it allows a possibility for non-idempotent transactions
-                    # on a system's package set (pending the yum repo has many old
-                    # NVRs indexed)
-                    if upgrade:
-                        if is_installed:  # Case 1
-                            # TODO: Is this case reachable?
-                            #
-                            # _is_installed() demands a name (*not* NVR) or else is always False
-                            # (wildcards are treated literally).
-                            #
-                            # Meanwhile, _is_newer_version_installed() demands something versioned
-                            # or else is always false.
-                            #
-                            # I fail to see how they can both be true at the same time for any
-                            # given pkg_spec. -re
-                            self.base.upgrade(pkg_spec)
-                        else:  # Case 2
-                            self.base.install(pkg_spec, strict=self.base.conf.strict)
-                    else:  # Case 3
-                        self.base.install(pkg_spec, strict=self.base.conf.strict)
-                else:  # Case 4, Nothing to do, report back
-                    pass
-            elif is_installed:  # A potentially older (or same) version is installed
-                if upgrade:  # Case 5
+                    self.base.install(pkg_spec, strict=self.base.conf.strict)
+            elif self._is_installed(pkg_spec):
+                if upgrade:
                     self.base.upgrade(pkg_spec)
-                else:  # Case 6, Nothing to do, report back
-                    pass
-            else:  # Case 7, The package is not installed, simply install it
+            else:
                 self.base.install(pkg_spec, strict=self.base.conf.strict)
         except dnf.exceptions.MarkingError as e:
             msg = "No package {0} available.".format(pkg_spec)

lib/ansible/modules/dnf5.py

@@ -178,6 +178,8 @@ options:
         packages to install (because dependencies between the downgraded
         package and others can cause changes to the packages which were
         in the earlier transaction).
+      - Since this feature is not provided by C(dnf5) itself but by M(ansible.builtin.dnf5) module,
+        using this in combination with wildcard characters in O(name) may result in an unexpected results.
     type: bool
     default: "no"
   download_only:
@@ -362,7 +364,7 @@ libdnf5 = None
 LIBDNF5_ERRORS = RuntimeError
 
 
-def is_installed(base, spec):
+def get_resolve_spec_settings():
     settings = libdnf5.base.ResolveSpecSettings()
     try:
         settings.set_group_with_name(True)
@@ -388,47 +390,34 @@ def is_installed(base, spec):
         settings.group_with_name = True
         settings.with_binaries = False
         settings.with_provides = False
+    return settings
+
+
+def is_installed(base, spec):
+    settings = get_resolve_spec_settings()
 
     installed_query = libdnf5.rpm.PackageQuery(base)
     installed_query.filter_installed()
     match, nevra = installed_query.resolve_pkg_spec(spec, settings, True)
-
-    # FIXME use `is_glob_pattern` function when available:
-    # https://github.com/rpm-software-management/dnf5/issues/1563
-    glob_patterns = set("*[?")
-    if any(set(char) & glob_patterns for char in spec):
-        available_query = libdnf5.rpm.PackageQuery(base)
-        available_query.filter_available()
-        available_query.resolve_pkg_spec(spec, settings, True)
-
-        return not (
-            {p.get_name() for p in available_query} - {p.get_name() for p in installed_query}
-        )
-    else:
-        return match
+    return match
 
 
 def is_newer_version_installed(base, spec):
-    # FIXME investigate whether this function can be replaced by dnf5's allow_downgrade option
+    # expects a versioned package spec
     if "/" in spec:
         spec = spec.split("/")[-1]
         if spec.endswith(".rpm"):
             spec = spec[:-4]
 
-    try:
-        spec_nevra = next(iter(libdnf5.rpm.Nevra.parse(spec)))
-    except LIBDNF5_ERRORS:
-        return False
-    except StopIteration:
-        return False
-
-    spec_version = spec_nevra.get_version()
-    if not spec_version:
+    settings = get_resolve_spec_settings()
+    match, spec_nevra = libdnf5.rpm.PackageQuery(base).resolve_pkg_spec(spec, settings, True)
+    if not match or spec_nevra.has_just_name():
         return False
+    spec_name = spec_nevra.get_name()
 
     installed = libdnf5.rpm.PackageQuery(base)
     installed.filter_installed()
-    installed.filter_name([spec_nevra.get_name()])
+    installed.filter_name([spec_name])
     installed.filter_latest_evr()
     try:
         installed_package = list(installed)[-1]
@@ -436,8 +425,8 @@ def is_newer_version_installed(base, spec):
         return False
 
     target = libdnf5.rpm.PackageQuery(base)
-    target.filter_name([spec_nevra.get_name()])
-    target.filter_version([spec_version])
+    target.filter_name([spec_name])
+    target.filter_version([spec_nevra.get_version()])
     spec_release = spec_nevra.get_release()
     if spec_release:
         target.filter_release([spec_release])
@@ -719,8 +708,26 @@ class Dnf5Module(YumDnf):
             goal.add_rpm_upgrade(settings)
         elif self.state in {"installed", "present", "latest"}:
             upgrade = self.state == "latest"
+            # FIXME use `is_glob_pattern` function when available:
+            # https://github.com/rpm-software-management/dnf5/issues/1563
+            glob_patterns = set("*[?")
             for spec in self.names:
-                if is_newer_version_installed(base, spec):
+                if any(set(char) & glob_patterns for char in spec):
+                    # Special case for package specs that contain glob characters.
+                    # For these we skip `is_installed` and `is_newer_version_installed` tests that allow for the
+                    # allow_downgrade feature and pass the package specs to dnf.
+                    # Since allow_downgrade is not available in dnf and while it is relatively easy to implement it for
+                    # package specs that evaluate to a single package, trying to mimic what would the dnf machinery do
+                    # for glob package specs and then filtering those for allow_downgrade appears to always
+                    # result in naive/inferior solution.
+                    # TODO reasearch how feasible it is to implement the above
+                    if upgrade:
+                        # for upgrade we pass the spec to both upgrade and install, to satisfy both available and installed
+                        # packages evaluated from the glob spec
+                        goal.add_upgrade(spec, settings)
+                    if not self.update_only:
+                        goal.add_install(spec, settings)
+                elif is_newer_version_installed(base, spec):
                     if self.allow_downgrade:
                         goal.add_install(spec, settings)
                 elif is_installed(base, spec):

test/integration/targets/dnf/tasks/repo.yml

@@ -630,6 +630,87 @@
         - provides-package
         - provided-package
 
+# https://github.com/ansible/ansible/issues/45250
+- block:
+    - name: Install dinginessentail-1.0, dinginessentail-olive-1.0, landsidescalping-1.0
+      dnf:
+        name: "dinginessentail-1.0,dinginessentail-olive-1.0,landsidescalping-1.0"
+        state: present
+
+    - name: Upgrade dinginessentail*
+      dnf:
+        name: dinginessentail*
+        state: latest
+      register: dnf_result
+
+    - name: Check dinginessentail with rpm
+      shell: rpm -q dinginessentail
+      register: rpm_result
+
+    - name: Verify update of dinginessentail
+      assert:
+        that:
+            - "rpm_result.stdout.startswith('dinginessentail-1.1-1')"
+
+    - name: Check dinginessentail-olive with rpm
+      shell: rpm -q dinginessentail-olive
+      register: rpm_result
+
+    - name: Verify update of dinginessentail-olive
+      assert:
+        that:
+            - "rpm_result.stdout.startswith('dinginessentail-olive-1.1-1')"
+
+    - name: Check landsidescalping with rpm
+      shell: rpm -q landsidescalping
+      register: rpm_result
+
+    - name: Verify landsidescalping did NOT get updated
+      assert:
+        that:
+            - "rpm_result.stdout.startswith('landsidescalping-1.0-1')"
+
+    - name: Verify yum module outputs
+      assert:
+        that:
+            - "dnf_result is changed"
+            - "'msg' in dnf_result"
+            - "'rc' in dnf_result"
+            - "'results' in dnf_result"
+  always:
+    - name: Clean up
+      dnf:
+        name: dinginessentail,dinginessentail-olive,landsidescalping
+        state: absent
+
+- name: test allow_downgrade
+  block:
+    - dnf:
+        name: dinginessentail-1.1
+        state: present
+
+    - dnf:
+        name: dinginessentail-1.0
+        state: present
+        allow_downgrade: true
+    - dnf:
+        name: dinginessentail-1.1
+        state: present
+
+    - dnf:
+        name: dinginessentail-1.0
+        state: present
+        allow_downgrade: false
+      register: r
+
+    - assert:
+        that:
+          - r is not changed
+  always:
+    - dnf:
+        name: dinginessentail
+        state: absent
+
 - name: Test failures occured during loading repositories are properly handled
   vars:
     repo_name: test-non-existing-gpgkey-file
@@ -657,3 +738,15 @@
     - file:
         name: /etc/yum.repos.d/{{ repo_name }}.repo
         state: absent
+
+
+- name: Attempt to install a package with invalid name
+  dnf:
+    name: invalid[package_name]
+  register: r
+  ignore_errors: true
+
+- assert:
+    that:
+      - r is failed
+      - r.msg is contains("Failed to install some of the specified packages")