Repository: ansible/ansible
Pull request: dnf - properly handle idempotent removal of wildcard globs
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Fixes #62809

Signed-off-by: Adam Miller <admiller@redhat.com>
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
dnf



Problem statement:
dnf (and probly yum) wild-card match remove is not idempotence
##### SUMMARY
When using wild-card in package name to remove, the first run will remove the packages,
but the second run will fail with message `Failed to install some of the specified packages`

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME

- dnf

##### ANSIBLE VERSION
```
ansible 2.8.5
  config file = /home/rabin/src/ansible/workstation-setup/ansible.cfg
  configured module search path = ['/home/rabin/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.7.4 (default, Jul  9 2019, 16:32:37) [GCC 9.1.1 20190503 (Red Hat 9.1.1-1)]
```

##### CONFIGURATION

```
ANSIBLE_SSH_CONTROL_PATH(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = ./.ansible/ssh-%%C
CACHE_PLUGIN(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = jsonfile
CACHE_PLUGIN_CONNECTION(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = ./.ansible/facts_cache
CACHE_PLUGIN_TIMEOUT(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = 86400
DEFAULT_EXECUTABLE(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = /bin/bash
DEFAULT_GATHERING(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = smart
DEFAULT_HOST_LIST(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = ['/home/rabin/src/ansible/workstation-setup/inventory']
DEFAULT_REMOTE_PORT(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = 22
RETRY_FILES_SAVE_PATH(/home/rabin/src/ansible/workstation-setup/ansible.cfg) = /home/rabin/src/ansible/workstation-setup/.ansible/retry
```

##### OS / ENVIRONMENT

* Fedora 30

##### STEPS TO REPRODUCE

try to remove a package twice
or run the playbook twice (first run will work if there is a match, but secend run will fail)


```yaml
- dnf:
    name: lohit-*-fonts

- dnf:
    name: lohit-*-fonts
    state: absent

- dnf:
    name: lohit-*-fonts
    state: absent

```

##### EXPECTED RESULTS
Same as when you ask it to remove a spesific package
```yaml
- dnf:
    name: lohit-tamil-fonts

- dnf:
    name: lohit-tamil-fonts
    state: absent

- dnf:
    name: lohit-tamil-fonts
    state: absent
```


##### ACTUAL RESULTS
```
fatal: [localhost]: FAILED! => {"changed": false, "failures": ["lohit-*-fonts - no package matched: lohit-*-fonts"], "msg": "Failed to install some of the specified packages", "rc": 1, "results": []}
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/packaging/os/dnf.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/packaging/os/dnf.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @DJMuggs @berenddeschouwer @ignatenkobrain @maxamillion
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: dnf - properly handle idempotent removal of wildcard globs

Fixes #62809

Signed-off-by: Adam Miller <admiller@redhat.com>

Patch to review:
diff --git a/changelogs/fragments/62809-dnf-wildcard-absent-failure.yml b/changelogs/fragments/62809-dnf-wildcard-absent-failure.yml
new file mode 100644
index 00000000000000..a2a3db47465334
--- /dev/null
+++ b/changelogs/fragments/62809-dnf-wildcard-absent-failure.yml
@@ -0,0 +1,3 @@
+---
+minor_changes:
+  - dnf - Properly handle idempotent transactions with package name wildcard globs (https://github.com/ansible/ansible/issues/62809)
diff --git a/lib/ansible/modules/packaging/os/dnf.py b/lib/ansible/modules/packaging/os/dnf.py
index 55f569bc06cb95..eb03db5ec5faf2 100644
--- a/lib/ansible/modules/packaging/os/dnf.py
+++ b/lib/ansible/modules/packaging/os/dnf.py
@@ -334,16 +334,29 @@ def is_lockfile_pid_valid(self):
         # https://github.com/ansible/ansible/issues/57189
         return True
 
-    def _sanitize_dnf_error_msg(self, spec, error):
+    def _sanitize_dnf_error_msg_install(self, spec, error):
         """
         For unhandled dnf.exceptions.Error scenarios, there are certain error
-        messages we want to filter. Do that here.
+        messages we want to filter in an install scenario. Do that here.
         """
         if to_text("no package matched") in to_text(error):
             return "No package {0} available.".format(spec)
 
         return error
 
+    def _sanitize_dnf_error_msg_remove(self, spec, error):
+        """
+        For unhandled dnf.exceptions.Error scenarios, there are certain error
+        messages we want to ignore in a removal scenario as known benign
+        failures. Do that here.
+        """
+        if to_text("no package matched") in to_text(error):
+            return (False, "{0} is not installed".format(spec))
+
+        # Return value is tuple of:
+        #   ("Is this actually a failure?", "Error Message")
+        return (True, error)
+
     def _package_dict(self, package):
         """Return a dictionary of information for the package."""
         # NOTE: This no longer contains the 'dnfstate' field because it is
@@ -991,7 +1004,7 @@ def ensure(self):
                         install_result = self._mark_package_install(pkg_spec)
                         if install_result['failed']:
                             failure_response['msg'] += install_result['msg']
-                            failure_response['failures'].append(self._sanitize_dnf_error_msg(pkg_spec, install_result['failure']))
+                            failure_response['failures'].append(self._sanitize_dnf_error_msg_install(pkg_spec, install_result['failure']))
                         else:
                             response['results'].append(install_result['msg'])
 
@@ -1051,7 +1064,7 @@ def ensure(self):
                         install_result = self._mark_package_install(pkg_spec, upgrade=True)
                         if install_result['failed']:
                             failure_response['msg'] += install_result['msg']
-                            failure_response['failures'].append(self._sanitize_dnf_error_msg(pkg_spec, install_result['failure']))
+                            failure_response['failures'].append(self._sanitize_dnf_error_msg_install(pkg_spec, install_result['failure']))
                         else:
                             response['results'].append(install_result['msg'])
 
@@ -1104,7 +1117,11 @@ def ensure(self):
                         try:
                             self.base.remove(pkg_spec)
                         except dnf.exceptions.MarkingError as e:
-                            failure_response['failures'].append('{0} - {1}'.format(pkg_spec, to_native(e)))
+                            is_failure, handled_remove_error = self._sanitize_dnf_error_msg_remove(pkg_spec, to_native(e))
+                            if is_failure:
+                                failure_response['failures'].append('{0} - {1}'.format(pkg_spec, to_native(e)))
+                            else:
+                                response['results'].append(handled_remove_error)
                         continue
 
                     installed_pkg = list(map(str, installed.filter(name=pkg_spec).run()))
diff --git a/test/integration/targets/yum/tasks/yum.yml b/test/integration/targets/yum/tasks/yum.yml
index 800fe912891ef9..bf70882ed0530a 100644
--- a/test/integration/targets/yum/tasks/yum.yml
+++ b/test/integration/targets/yum/tasks/yum.yml
@@ -693,6 +693,40 @@
     yum_version: "{%- if item.yumstate == 'installed' -%}{{ item.version }}{%- else -%}{{ yum_version }}{%- endif -%}"
   with_items: "{{ yum_version.results }}"
 
+- name: Ensure double uninstall of wildcard globs works
+  block:
+  - name: "Install lohit-*-fonts"
+    yum:
+      name: "lohit-*-fonts"
+      state: present
+
+  - name: "Remove lohit-*-fonts (1st time)"
+    yum:
+      name: "lohit-*-fonts"
+      state: absent
+    register: remove_lohit_fonts_1
+
+  - name: "Verify lohit-*-fonts (1st time)"
+    assert:
+      that:
+        - "remove_lohit_fonts_1 is changed"
+        - "'msg' in remove_lohit_fonts_1"
+        - "'results' in remove_lohit_fonts_1"
+
+  - name: "Remove lohit-*-fonts (2nd time)"
+    yum:
+      name: "lohit-*-fonts"
+      state: absent
+    register: remove_lohit_fonts_2
+
+  - name: "Verify lohit-*-fonts (2nd time)"
+    assert:
+      that:
+        - "remove_lohit_fonts_2 is not changed"
+        - "'msg' in remove_lohit_fonts_2"
+        - "'results' in remove_lohit_fonts_2"
+        - "'lohit-*-fonts is not installed' in remove_lohit_fonts_2['results']"
+
 - block:
   - name: uninstall bc
     yum: name=bc state=removed


Code review comments:
diff_hunk: @@ -334,16 +334,29 @@ def is_lockfile_pid_valid(self):
         # https://github.com/ansible/ansible/issues/57189
         return True
 
-    def _sanitize_dnf_error_msg(self, spec, error):
+    def _sanitize_dnf_error_msg_install(self, spec, error):
         """
         For unhandled dnf.exceptions.Error scenarios, there are certain error
-        messages we want to filter. Do that here.
+        messages we want to filter in an install scenario. Do that here.
         """
         if to_text("no package matched") in to_text(error):
             return "No package {0} available.".format(spec)
 
         return error
 
+    def _sanitize_dnf_error_msg_remove(self, spec, error):
+        """
+        For unhandled dnf.exceptions.Error scenarios, there are certain error
+        messages we want to ignore in a removal scenario as known benign
+        failures. Do that here.
+        """
+        if to_text("no package matched") in to_text(error):
path: lib/ansible/modules/packaging/os/dnf.py
text: @user1:
```suggestion
        if 'no package matched' in to_native(error):
```