From 3d8d03a131f8d8d5671f4d2b5096a8acc31f8754 Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: Jul 14 2017 12:48:33 +0000 Subject: Merge branch 'develop' --- diff --git a/libtaskotron.spec b/libtaskotron.spec index 02e5295..475562b 100644 --- a/libtaskotron.spec +++ b/libtaskotron.spec @@ -1,6 +1,6 @@ Name: libtaskotron # NOTE: if you update version, *make sure* to also update `libtaskotron/__init__.py` -Version: 0.4.23 +Version: 0.4.24 Release: 1%{?dist} Summary: Taskotron Support Library @@ -179,6 +179,9 @@ install -d %{buildroot}/%{_sharedstatedir}/taskotron/images %{python2_sitelib}/libtaskotron/ext/disposable/* %changelog +* Fri Jul 14 2017 Kamil Páral - 0.4.24-1 +- do not use --cacheonly for dnf operations + * Wed Jul 12 2017 Kamil Páral - 0.4.23-1 - fix python2-koji dep on F27+ - fix broken test suite diff --git a/libtaskotron/__init__.py b/libtaskotron/__init__.py index 364f8fb..762fb6f 100644 --- a/libtaskotron/__init__.py +++ b/libtaskotron/__init__.py @@ -4,4 +4,4 @@ # See the LICENSE file for more details on Licensing from __future__ import absolute_import -__version__ = '0.4.23' +__version__ = '0.4.24' diff --git a/libtaskotron/ext/fedora/rpm_utils.py b/libtaskotron/ext/fedora/rpm_utils.py index 5d3252a..971d33f 100644 --- a/libtaskotron/ext/fedora/rpm_utils.py +++ b/libtaskotron/ext/fedora/rpm_utils.py @@ -108,10 +108,6 @@ def cmpNEVR(nevr1, nevr2): def install(pkgs): '''Install packages from system repositories using DNF. Either root or sudo access required. - Note: This is first tested with ``--cacheonly`` to avoid downloading metadata and speed up the - process. If that command fails for whatever reason, we run it again, this time without - ``--cacheonly``. - :param pkgs: packages to be installed, e.g. ``['pidgin']``, or any other argument supported by ``dnf install`` command :type pkgs: list of str @@ -131,73 +127,21 @@ def install(pkgs): cmd = ['dnf', '--assumeyes', 'install'] cmd.extend(pkgs) - if dnf_cache_available(): - cmd.insert(1, '--cacheonly') - else: - log.warn('No DNF cache available, metadata will be need to be downloaded. If your DNF ' - "cache doesn't persist across task executions (e.g. on a disposable VM), " - 'consider creating the cache in your base system image to speed up execution.') - if not os_utils.is_root(): # we must have sudo at this point, don't test again needlessly cmd = ['sudo', '--non-interactive'] + cmd - while True: # we need to call the command twice, if the first run with --cacheonly fails - log.debug('Running: %s', ' '.join([pipes.quote(c) for c in cmd])) - try: - output = subprocess.check_output(cmd, stderr=subprocess.STDOUT) - except subprocess.CalledProcessError as e: - if '--cacheonly' in cmd: # maybe just outdated cache - log.debug(u'✘ Package installation failed, DNF output was:\n%s', e.output.rstrip()) - log.debug("Trying again with forced metadata refresh...") - cmd = list(cmd) # make a copy to allow unit tests to intercept both calls easily - cmd.remove('--cacheonly') - cmd.append('--refresh') - continue - - log.error(u'✘ Package installation failed. We tried to install following packages:\n%s' - '\nDNF returned exit code %d and output:\n%s', pkglist, e.returncode, - e.output.rstrip()) - raise exc.TaskotronError("Unable to install packages: %s" % pkglist) - else: - log.debug(u'✔ Package installation completed successfully. DNF output was:\n%s', - output.rstrip()) - return - - # safeguard against infinite loop, we should never reach this code - log.critical("Infinite loop detected in 'install()'") - assert False, "Infinite loop detected in 'install()'" - return - - -def dnf_cache_available(): - '''Determine whether DNF cache is available. - - If it is, we can run read-only actions solely from the cache, and try to use it even for - ``install`` action (provided the repos haven't changed much). This way we avoid contacting - DNF repos and downloading new metadata if they changed. - - This tests the system cache (if running as root or with sudo access) or the user cache - (otherwise). Since DNF has no direct way of testing this, we simply run an action that should - always pass (``dnf repolist``) with ``--cacheonly`` option and look at the exit code. - - :return: bool whether DNF cache is currently available and can be used (system or user cache, - depending on your current admin privileges) - ''' - cmd = ['dnf', '--cacheonly', 'repolist'] - - if not os_utils.is_root() and os_utils.has_sudo(): - cmd = ['sudo', '--non-interactive'] + cmd - - log.debug('Deciding whether DNF cache is available. Running: %s', ' '.join(cmd)) + log.debug('Running: %s', ' '.join([pipes.quote(c) for c in cmd])) try: - subprocess.check_output(cmd, stderr=subprocess.STDOUT) + output = subprocess.check_output(cmd, stderr=subprocess.STDOUT) except subprocess.CalledProcessError as e: - log.debug(u'✘ DNF cache is not available. Received exit code %d and output:\n%s', - e.returncode, e.output.rstrip()) - return False + log.error(u'✘ Package installation failed. We tried to install following packages:\n%s' + '\nDNF returned exit code %d and output:\n%s', pkglist, e.returncode, + e.output.rstrip()) + raise exc.TaskotronError("Unable to install packages: %s" % pkglist) else: - log.debug(u'✔ DNF cache is available.') - return True + log.debug(u'✔ Package installation completed successfully. DNF output was:\n%s', + output.rstrip()) + return def get_dist_tag(rpmstr): diff --git a/libtaskotron/minion.py b/libtaskotron/minion.py index 057b1b2..21f2ac6 100644 --- a/libtaskotron/minion.py +++ b/libtaskotron/minion.py @@ -61,28 +61,15 @@ class BaseMinion(object): log.info('Examining the minion and installing libtaskotron...') self.taskdir = config.get_config().client_taskdir - # create initial DNF cache (but only if needed) - # uses the same logic as in libtaskotron.ext.fedora.rpm_utils.dnf_cache_available() - self.ssh.cmd('dnf --cacheonly repolist || dnf makecache') - # add additional repos to minion repos = ['--add-repo %s' % pipes.quote(repo) for repo in config.get_config().minion_repos] if repos: self.ssh.cmd('dnf config-manager %s' % ' '.join(repos)) - # create DNF cache for taskotron repos - # this hard-coding is very suboptimal, but it's hard to find a better way how to refresh - # only taskotron repos (unless we require their names listed in configuration) - excluded = ['fedora', 'updates', 'updates-testing'] - exclude_cmd = ' '.join(['--disablerepo %s' % pipes.quote(repo) for repo in excluded]) - self.ssh.cmd('dnf makecache %s' % exclude_cmd) - # install libtaskotron # FIXME this should only need libtaskotron-core T651. libtaskotron-fedora should be # installed as a check dep. - # TODO: remove tar dep once libtaskotron 0.4.16 has been released for long enough - # (tar is here just for backward compatibility, see D965) - self.ssh.install_pkgs(['libtaskotron-core', 'libtaskotron-fedora', 'tar']) + self.ssh.install_pkgs(['libtaskotron-core', 'libtaskotron-fedora']) log.info('Copying task files onto the minion...') diff --git a/libtaskotron/remote_exec.py b/libtaskotron/remote_exec.py index a14bac8..95d44b0 100644 --- a/libtaskotron/remote_exec.py +++ b/libtaskotron/remote_exec.py @@ -161,22 +161,21 @@ class ParamikoWrapper(object): def install_pkgs(self, pkgs): '''Install packages via dnf. - First tries to install packages using ``dnf --cacheonly --best``. Running from cache - improves performance, while ``--best`` ensures updating to the latest version. If this - doesn't work for some reason, we try again with refreshed cache, and if that still - doesn't work, we drop ``--best`` to allow for broken deps of latest packages, if there + First tries to install packages using ``dnf --best``, which ensures + updating to the latest version. If this doesn't work, we drop + ``--best`` to allow for broken deps of latest packages, in case there are older packages with working deps. - :param list pkgs: A list of packages to be installed (supports any argument that - ``dnf install`` accepts) - :raise TaskotronRemoteError: If the command has non-zero return code or times out (see - :meth:`cmd`). + :param list pkgs: A list of packages to be installed (supports any + argument that ``dnf install`` accepts) + :raise TaskotronRemoteError: If the command has non-zero return code or + times out (see :meth:`cmd`). ''' pkgs_esc = [pipes.quote(pkg) for pkg in pkgs] log.info('Installing %d packages on remote host...', len(pkgs)) try: - self.cmd('dnf --assumeyes --cacheonly --best install %s' % ' '.join(pkgs_esc)) + self.cmd('dnf --assumeyes --best install %s' % ' '.join(pkgs_esc)) except exc.TaskotronRemoteProcessError as e: log.debug('Installation failed: %s', e) log.debug('Trying again with forced metadata refresh...') diff --git a/testing/test_remote_exec.py b/testing/test_remote_exec.py index 38dabb2..5b951bf 100644 --- a/testing/test_remote_exec.py +++ b/testing/test_remote_exec.py @@ -255,13 +255,11 @@ class TestRemoteExec(object): assert self.remote.cmd.call_count == 2 cmd1 = self.remote.cmd.call_args_list[0][0][0] - words1 = ['dnf', 'install', '--assumeyes', '--cacheonly', '--best'] + pkgs + words1 = ['dnf', 'install', '--assumeyes', '--best'] + pkgs assert all([word in cmd1 for word in words1]) cmd2 = self.remote.cmd.call_args_list[1][0][0] words2 = ['dnf', 'install', '--assumeyes', '--refresh', '--best'] + pkgs - not_words2 = ['--cacheonly'] assert all([word in cmd2 for word in words2]) - assert all([word not in cmd2 for word in not_words2]) def test_install_pkgs_try_three_times(self): '''If the first two executions fail, modify dnf options and try again''' @@ -274,7 +272,7 @@ class TestRemoteExec(object): # first two cmdline args were tested in test_install_pkgs_try_twice() cmd3 = self.remote.cmd.call_args_list[2][0][0] words3 = ['dnf', 'install', '--assumeyes'] + pkgs - not_words3 = ['--cacheonly', '--refresh', '--best'] + not_words3 = ['--refresh', '--best'] assert all([word in cmd3 for word in words3]) assert all([word not in cmd3 for word in not_words3]) diff --git a/testing/test_rpm_utils.py b/testing/test_rpm_utils.py index 074e96f..edbaee3 100644 --- a/testing/test_rpm_utils.py +++ b/testing/test_rpm_utils.py @@ -8,9 +8,8 @@ import pytest import subprocess -from libtaskotron.ext.fedora.rpm_utils import (cmpNEVR, dnf_cache_available, get_dist_tag, install, +from libtaskotron.ext.fedora.rpm_utils import (cmpNEVR, get_dist_tag, install, rpmformat) -from libtaskotron.ext.fedora import rpm_utils from libtaskotron import exceptions as exc from libtaskotron import os_utils @@ -142,8 +141,6 @@ class TestInstall(object): monkeypatch.setattr(os_utils, 'is_root', self.mock_is_root) self.mock_has_sudo = mock.Mock(return_value=True) monkeypatch.setattr(os_utils, 'has_sudo', self.mock_has_sudo) - self.mock_dnf_cache = mock.Mock(return_value=True) - monkeypatch.setattr(rpm_utils, 'dnf_cache_available', self.mock_dnf_cache) self.mock_check_output = mock.Mock(return_value='') monkeypatch.setattr(subprocess, 'check_output', self.mock_check_output) @@ -189,84 +186,6 @@ class TestInstall(object): call_args = self.mock_check_output.call_args[0][0] assert all([pkg in call_args for pkg in pkgs]) - def test_run_once(self): - '''The command should be executed only once if it passes''' - install(['foo']) - assert self.mock_check_output.call_count == 1 - - def test_run_once_fail(self): - '''If there's no cache and install fails, it should not try again''' - self.mock_dnf_cache.return_value = False - self.mock_check_output.side_effect = self.err - - with pytest.raises(exc.TaskotronError) as excinfo: - install(['foo']) - - assert self.mock_check_output.call_count == 1 - # direct comparison here, because isinstance() would also accept subclasses we throw for - # different issues (missing permissions) - assert type(excinfo.value) is exc.TaskotronError - - def test_run_twice_on_cache_fail(self): - '''If cache is used and install fails, it should run again without cache''' - self.mock_check_output.side_effect = self.err - with pytest.raises(exc.TaskotronError): - install(['foo']) - - assert self.mock_check_output.call_count == 2 - assert '--cacheonly' in self.mock_check_output.call_args_list[0][0][0] - assert '--cacheonly' not in self.mock_check_output.call_args_list[1][0][0] - - def test_run_twice_second_ok(self): - '''If installation is tried twice and second one succeeds, the whole operation should - succeed.''' - self.mock_check_output.side_effect = (self.err, mock.DEFAULT) - install(['foo']) - assert self.mock_check_output.call_count == 2 - - -@pytest.mark.usefixtures('setup') -class TestDnfCacheAvailable(object): - '''Test rpm_utils.dnf_cache_available()''' - - @pytest.fixture - def setup(self, monkeypatch): - self.mock_is_root = mock.Mock(return_value=True) - monkeypatch.setattr(os_utils, 'is_root', self.mock_is_root) - self.mock_has_sudo = mock.Mock(return_value=True) - monkeypatch.setattr(os_utils, 'has_sudo', self.mock_has_sudo) - - self.mock_check_output = mock.Mock(return_value='') - monkeypatch.setattr(subprocess, 'check_output', self.mock_check_output) - self.err = subprocess.CalledProcessError(1, 'cmd', output='') - - def test_cache_exists(self): - assert dnf_cache_available() is True - - def test_cache_exists_user(self): - self.mock_is_root.return_value = False - self.mock_has_sudo.return_value = False - assert dnf_cache_available() is True - - def test_cache_doesnt_exist(self): - self.mock_check_output.side_effect = self.err - assert dnf_cache_available() is False - - def test_cache_doesnt_exist_user(self): - self.mock_is_root.return_value = False - self.mock_has_sudo.return_value = False - self.mock_check_output.side_effect = self.err - assert dnf_cache_available() is False - - def test_add_sudo(self): - self.mock_is_root.return_value = False - dnf_cache_available() - assert self.mock_check_output.call_args[0][0].index('sudo') == 0 - - def test_dont_add_sudo(self): - dnf_cache_available() - assert 'sudo' not in self.mock_check_output.call_args[0][0] - class TestGetDistTag(object): def test_nvrs(self):