[dnf] Fix skip_broken, add test coverage (v2) (#78158)

* [dnf] Fix skip_broken, add test coverage

Change:
- skip_broken was set in config but not actually used in calls to
  base.install()
- added a lot of test cases with specialized repo
- got rid of external (docker repo) nobest test cases since the
  specialized repo works well for those too
- Slight cleanup and adding comments in dnf module

Test Plan:
- ci_complete

Tickets:
- Fixes #73072

Original-author: Rick Elrod <rick@elrod.me>

* Use a better test for checking results list

ci_complete

Signed-off-by: Rick Elrod <rick@elrod.me>

Co-authored-by: Rick Elrod <rick@elrod.me>
pull/78207/head
LRitzdorf 4 years ago committed by GitHub
parent f70cc2fb7e
commit 6bcb494f83
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

@ -0,0 +1,4 @@
bugfixes:
- dnf - The ``skip_broken`` option is now used in installs (https://github.com/ansible/ansible/issues/73072).
- dnf - The ``nobest`` option now also works for ``state=latest``.
- dnf - Condense a few internal boolean returns.

@ -802,10 +802,7 @@ class DnfModule(YumDnf):
else:
package_spec['name'] = name
if installed.filter(**package_spec):
return True
else:
return False
return bool(installed.filter(**package_spec))
def _is_newer_version_installed(self, pkg_name):
candidate_pkg = self._packagename_dict(pkg_name)
@ -825,13 +822,8 @@ class DnfModule(YumDnf):
candidate_pkg['epoch'], candidate_pkg['version'], candidate_pkg['release'],
)
if evr_cmp == 1:
return True
else:
return False
return evr_cmp == 1
else:
return False
def _mark_package_install(self, pkg_spec, upgrade=False):
@ -846,21 +838,31 @@ class DnfModule(YumDnf):
# on a system's package set (pending the yum repo has many old
# NVRs indexed)
if upgrade:
if is_installed:
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:
self.base.install(pkg_spec)
else:
self.base.install(pkg_spec)
else: # Nothing to do, report back
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: # An potentially older (or same) version is installed
if upgrade:
elif is_installed: # A potentially older (or same) version is installed
if upgrade: # Case 5
self.base.upgrade(pkg_spec)
else: # Nothing to do, report back
else: # Case 6, Nothing to do, report back
pass
else: # The package is not installed, simply install it
self.base.install(pkg_spec)
else: # Case 7, The package is not installed, simply install it
self.base.install(pkg_spec, strict=self.base.conf.strict)
return {'failed': False, 'msg': '', 'failure': '', 'rc': 0}
@ -982,9 +984,9 @@ class DnfModule(YumDnf):
try:
if self._is_newer_version_installed(self._package_dict(pkg)['nevra']):
if self.allow_downgrade:
self.base.package_install(pkg)
self.base.package_install(pkg, strict=self.base.conf.strict)
else:
self.base.package_install(pkg)
self.base.package_install(pkg, strict=self.base.conf.strict)
except Exception as e:
self.module.fail_json(
msg="Error occurred attempting remote rpm operation: {0}".format(to_native(e)),
@ -1176,9 +1178,13 @@ class DnfModule(YumDnf):
response['results'].append("Packages providing %s not installed due to update_only specified" % spec)
else:
for pkg_spec in pkg_specs:
# best effort causes to install the latest package
# even if not previously installed
self.base.conf.best = True
# Previously we forced base.conf.best=True here.
# However in 2.11+ there is a self.nobest option, so defer to that.
# Note, however, that just because nobest isn't set, doesn't mean that
# base.conf.best is actually true. We only force it false in
# _configure_base(), we never set it to true, and it can default to false.
# Thus, we still need to explicitly set it here.
self.base.conf.best = not self.nobest
install_result = self._mark_package_install(pkg_spec, upgrade=True)
if install_result['failed']:
if install_result['msg']:
@ -1258,6 +1264,15 @@ class DnfModule(YumDnf):
self.base.autoremove()
try:
# NOTE for people who go down the rabbit hole of figuring out why
# resolve() throws DepsolveError here on dep conflict, but not when
# called from the CLI: It's controlled by conf.best. When best is
# set, Hawkey will fail the goal, and resolve() in dnf.base.Base
# will throw. Otherwise if it's not set, the update (install) will
# be (almost silently) removed from the goal, and Hawkey will report
# success. Note that in this case, similar to the CLI, skip_broken
# does nothing to help here, so we don't take it into account at
# all.
if not self.base.resolve(allow_erasing=self.allowerasing):
if failure_response['failures']:
failure_response['msg'] = 'Failed to install some of the specified packages'

@ -23,6 +23,10 @@
when: (ansible_distribution == 'Fedora' and ansible_distribution_major_version is version('23', '>=')) or
(ansible_distribution in ['RedHat', 'CentOS'] and ansible_distribution_major_version is version('8', '>='))
- include_tasks: skip_broken_and_nobest.yml
when: (ansible_distribution == 'Fedora' and ansible_distribution_major_version is version('23', '>=')) or
(ansible_distribution in ['RedHat', 'CentOS'] and ansible_distribution_major_version is version('8', '>='))
- include_tasks: filters_check_mode.yml
when: (ansible_distribution == 'Fedora' and ansible_distribution_major_version is version('23', '>=')) or
(ansible_distribution in ['RedHat', 'CentOS'] and ansible_distribution_major_version is version('8', '>='))
@ -65,19 +69,6 @@
when: (ansible_distribution == 'Fedora' and ansible_distribution_major_version is version('31', '>=')) or
(ansible_distribution in ['RedHat', 'CentOS'] and ansible_distribution_major_version is version('8', '>='))
# TODO: Construct our own instance where 'nobest' applies, so we can stop using
# a third-party repo to test this behavior.
#
# This fails due to conflicts on Fedora 34, but we can nuke this entirely once
# #74224 lands, because it covers nobest cases.
# Skipped in RHEL9 by changing the version test to == instead of >=
# due to missing RHEL9 docker-ce packages currently
- include_tasks: nobest.yml
when: (ansible_distribution == 'Fedora' and ansible_distribution_major_version is version('24', '>=') and
ansible_distribution_major_version is version('34', '!=')) or
(ansible_distribution in ['RedHat', 'CentOS'] and ansible_distribution_major_version is version('8', '=='))
- include_tasks: cacheonly.yml
when: (ansible_distribution == 'Fedora' and ansible_distribution_major_version is version('23', '>=')) or
(ansible_distribution in ['RedHat', 'CentOS'] and ansible_distribution_major_version is version('8', '>='))

@ -1,34 +0,0 @@
- name: Install dnf-plugins-core in order to use dnf config-manager
dnf:
name: dnf-plugins-core
state: present
- name: Add docker-ce repo (Only RedHat & CentOS)
shell: dnf config-manager --add-repo=https://download.docker.com/linux/centos/docker-ce.repo
when: (ansible_distribution in ['RedHat', 'CentOS'])
- name: Add docker-ce repo (Only Fedora)
shell: dnf config-manager --add-repo=https://download.docker.com/linux/fedora/docker-ce.repo
when: (ansible_distribution in ['Fedora'])
- name: Install docker using nobest option
dnf:
name: docker-ce
state: present
nobest: true
register: dnf_result
- name: Verify installation of docker-ce
assert:
that:
- not dnf_result is failed
- name: Cleanup packages
dnf:
name: docker-ce, dnf-plugins-core
state: absent
- name: Cleanup manually added repos
file:
name: "/etc/yum.repos.d/docker-ce.repo"
state: absent

@ -0,0 +1,318 @@
# Tests for skip_broken and allowerasing
# (and a bit of nobest because the test case is too good to pass up)
#
# There are a lot of fairly complex, corner cases we test here especially towards the end.
#
# The test repo is generated from the "skip-broken" repo in this repository:
# https://github.com/relrod/ansible-ci-contrived-yum-repos
#
# It is laid out like this:
#
# There are three packages, `broken-a`, `broken-b`, `broken-c`.
#
# * broken-a has three versions: 1.2.3, 1.2.3.4, 1.2.4, 2.0.0.
# * 1.2.3 and 1.2.4 have no dependencies
# * 1.2.3.4 and 2.0.0 both depend on a non-existent package (to break depsolving)
#
# * broken-b depends on broken-a-1.2.3
# * broken-c depends on broken-a-1.2.4
# * broken-d depends on broken-a (no version constraint)
#
# This allows us to test various upgrades, downgrades, and installs with broken dependencies.
# skip_broken should usually be successful in the upgrade/downgrade case, it will just do nothing.
#
# There is a nobest testcase or two thrown in, simply because this organization provides a very
# good test case for that behavior as well. For example, just installing "broken-a" with no version
# will try to install 2.0.0 which is broken. With nobest=true, it will fall back to 1.2.4. Similar
# for upgrading.
- block:
- name: Set up test yum repo
yum_repository:
name: skip-broken
description: ansible-test skip-broken test repo
baseurl: "{{ skip_broken_repo_baseurl }}"
gpgcheck: no
repo_gpgcheck: no
- name: Install two packages
dnf:
name:
- broken-a-1.2.3
- broken-b
# This will fail. We have broken-a-1.2.3, and broken-b with a strong
# dependency on it. broken-c has a strong dependency on broken-a-1.2.4.
# Since installing that would break broken-b, we get a conflict.
- name: Try installing a third package, intentionally broken
dnf:
name:
- broken-c
ignore_errors: true
register: dnf_fail
- assert:
that:
- dnf_fail is failed
- "'Depsolve Error' in dnf_fail.msg"
# skip_broken should still install nothing because the conflict is
# still an issue. But it should skip over the broken packages and not
# fail.
- name: Try installing it with skip_broken
dnf:
name:
- broken-c
skip_broken: true
register: skip_broken_res
- name: Assert that nothing got installed
assert:
that:
- skip_broken_res.msg == 'Nothing to do'
- skip_broken_res.rc == 0
- skip_broken_res.results == []
- name: Remove all test packages
dnf:
name:
- broken-*
state: absent
# broken-d depends on (unversioned) broken-a.
# broken-a-2.0.0 has a broken dependency that doesn't exist.
# skip_broken should cause us to skip our explicit broken-a-2.0.0
# and bring in broken-a-1.2.4 as a dep of broken-d.
- name: Ensure proper failure with explicit broken version
dnf:
name:
- broken-a-2.0.0
- broken-d
ignore_errors: true
register: dnf_fail
- name: Assert that nothing got installed
assert:
that:
- dnf_fail is failed
- "'Depsolve Error' in dnf_fail.msg"
- name: skip_broken with explicit version
dnf:
name:
- broken-a-2.0.0
- broken-d
skip_broken: true
register: skip_broken_res
- name: Assert that the right things got installed
assert:
that:
- skip_broken_res.rc == 0
- skip_broken_res.results|length == 2
- res.results|select("contains", "Installed: broken-a-1.2.4")|length > 0
- res.results|select("contains", "Installed: broken-d-1.2.5")|length > 0
- name: Remove all test packages
dnf:
name:
- broken-*
state: absent
# Walk the logic of _mark_package_install() here
# We need to use a full-ish NVR/wildcard. _is_newer_version_installed()
# will be false otherwise, no matter what. This might be a bug.
# Relatedly, the real "Case 1" in the code seemingly can't be reached:
# _is_newer_version_installed wants NVR, _is_installed wants name.
# Both can't be true at the same time given one pkg_spec. Thus, we start
# at "Case 2"
# prereq
- name: Install broken-a-1.2.4
dnf:
name:
- broken-a-1.2.4
state: present
# Case 2: newer version is installed, allow_downgrade is true,
# is_installed is false since we gave full NVR.
# "upgrade" to broken-a-1.2.3, allow_downgrade=true
- name: Do an "upgrade" to an older version of broken-a, allow_downgrade=true
dnf:
name:
- broken-a-1.2.3-1*
state: latest
allow_downgrade: true
check_mode: true
register: res
- assert:
that:
- res is changed
- res.results|select("contains", "Installed: broken-a-1.2.3")|length > 0
# Still case 2, but with broken package to test skip_broken
# skip_broken: false
- name: Do an "upgrade" to an older known broken version of broken-a, allow_downgrade=true, skip_broken=false
dnf:
name:
- broken-a-1.2.3.4-1*
state: latest
allow_downgrade: true
check_mode: true
ignore_errors: true
register: res
- assert:
that:
# 1.2.3.4 has non-existent dep. Fail without skip_broken.
- res is failed
- "'Depsolve Error' in res.msg"
# skip_broken: true
- name: Do an "upgrade" to an older known broken version of broken-a, allow_downgrade=true, skip_broken=true
dnf:
name:
- broken-a-1.2.3.4-1*
state: latest
allow_downgrade: true
skip_broken: true
check_mode: true
register: res
- assert:
that:
- res is not changed
- res.rc == 0
- res.msg == "Nothing to do"
# Case 3: newer version installed, allow_downgrade=true, but
# upgrade=false (i.e., state: present or installed)
- name: Install an older version of broken-a than currently installed
dnf:
name:
- broken-a-1.2.3-1*
state: present
allow_downgrade: true
check_mode: true
register: res
- assert:
that:
- res is changed
- res.results|select("contains", "Installed: broken-a-1.2.3")|length > 0
# Case 3 still, with broken package and skip_broken tests like above.
- name: Install an older known broken version of broken-a, allow_downgrade=true, skip_broken=false
dnf:
name:
- broken-a-1.2.3.4-1*
state: present
allow_downgrade: true
check_mode: true
ignore_errors: true
register: res
- assert:
that:
# 1.2.3.4 has non-existent dep. Fail without skip_broken.
- res is failed
- "'Depsolve Error' in res.msg"
# skip_broken: true
- name: Install an older known broken version of broken-a, allow_downgrade=true, skip_broken=true
dnf:
name:
- broken-a-1.2.3.4-1*
state: present
allow_downgrade: true
skip_broken: true
check_mode: true
register: res
- assert:
that:
- res is not changed
- res.rc == 0
- res.msg == "Nothing to do"
# Case 4: "upgrade" to broken-a-1.2.3, allow_downgrade=false
# is_newer_version_installed is true, allow_downgrade is false
- name: Do an "upgrade" to an older version of broken-a, allow_downgrade=false
dnf:
name:
- broken-a-1.2.3-1*
state: latest
allow_downgrade: false
check_mode: true
register: res
- assert:
that:
- res is not changed
- res.rc == 0
- res.msg == "Nothing to do"
# skip_broken doesn't apply to case 5 or 6 (older version installed).
# base.upgrade doesn't allow a strict= kwarg. However, nobest works here.
# Case 5: older version of package is installed, we specify name, no version
# otherwise we'd land in an earlier case. At this point, 1.2.4 is installed.
# broken-a-2.0.0 is available as an update but has a broken dependency.
- name: Update broken-a without nobest=true
dnf:
name:
- broken-a
state: latest
ignore_errors: true
register: dnf_fail
- assert:
that:
- dnf_fail is failed
- "'Depsolve Error' in dnf_fail.msg"
# With nobest: true, we will be "successful" but not actually perform
# any upgrade. That is, we are content not having the "best"/latest
# version.
- name: Update broken-a with nobest=true
dnf:
name:
- broken-a
state: latest
nobest: true
register: nobest
- assert:
that:
- nobest.rc == 0
- nobest.results == []
# Case 6: Current or older version already installed (no version specified
# in our pkg_spec) and we're requesting present, not latest.
#
# This isn't really relevant to skip_broken or nobest, but let's test it
# anyway since we're already walking the logic of the method.
- name: Install broken-a (even though it is already installed)
dnf:
name:
- broken-a
state: present
register: res
- assert:
that:
- res is not changed
# Case 7 is already tested quite extensively above in the earlier tests.
always:
- name: Remove test yum repo
yum_repository:
name: skip-broken
state: absent
- name: Remove all test packages installed
yum:
name:
- broken-*
state: absent

@ -2,3 +2,5 @@ dnf_log_files:
- /var/log/dnf.log
- /var/log/dnf.rpm.log
- /var/log/dnf.librepo.log
skip_broken_repo_baseurl: "https://ansible-ci-files.s3.amazonaws.com/test/integration/targets/dnf/skip-broken/RPMS/"

Loading…
Cancel
Save