From 6ccbcc22e4c19183a7c90f3312cdf3fa788920a9 Mon Sep 17 00:00:00 2001 From: Lukas Brabec Date: May 10 2018 11:24:02 +0000 Subject: [PATCH 1/7] variables for env matching added Playbooks can now contain these variables for devise_environment function: - taskotron_match_host_distro - taskotron_match_host_release - taskotron_match_host_arch If not present, values default to False and default env/{distro,release,arch} is used instead. --- diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py index e1d6934..bbdcb6a 100644 --- a/libtaskotron/executor.py +++ b/libtaskotron/executor.py @@ -28,6 +28,34 @@ except ImportError as e: raise exc.TaskotronImportError(e) +class Playbook_Data(object): + '''Class holding parsed variables from playbook''' + def __init__(self, playbook={}): + self.match_host_distro = playbook.get('vars', {}).get( + 'taskotron_match_host_distro', False) + log.info("Variable taskotron_match_host_distro is %s", self.match_host_distro) + + self.match_host_release = playbook.get('vars', {}).get( + 'taskotron_match_host_release', False) + log.info("Variable taskotron_match_host_release is %s", self.match_host_release) + + self.match_host_arch = playbook.get('vars', {}).get( + 'taskotron_match_host_arch', False) + log.info("Variable taskotron_match_host_arch is %s", self.match_host_arch) + + if playbook.get('vars',{}).get('taskotron_generic_task', False): + self.generic = True + self.exec_tasks = 'tasks_generic.yml' + else: + self.generic = False + self.exec_tasks = 'tasks_sti.yml' + log.info('Variable taskotron_generic_task is: %s', self.generic) + + self.keepalive = playbook.get('vars', {}).get( + 'taskotron_keepalive_minutes', None) + log.info('Variable taskotron_keepalive_minutes is: %s', self.keepalive) + + class Executor(object): '''Executor executes given task in the format of Ansible playbook. Before actual execution, executor decides where the playbook will be executed @@ -45,15 +73,17 @@ class Executor(object): self.task_vm = None self.run_remotely = False - def _spawn_vm(self, uuid): + def _spawn_vm(self, uuid, playbook_data): '''Spawn a virtual machine using testcloud. :param str uuid: unicode string uuid for the task being executed + :param playbook_data: instance of Playbook_Data with parsed vars from + playbook :returns: str ip address of spawned vm ''' log.info('Spawning disposable client') - env = image_utils.devise_environment(self.arg_data) + env = image_utils.devise_environment(self.arg_data, playbook_data) self.task_vm = vm.TestCloudMachine(uuid) retries = config.get_config().spawn_vm_retries @@ -137,13 +167,15 @@ class Executor(object): e) raise exc.TaskotronPlaybookError(e) - def _run_playbook(self, test_playbook, ipaddr, root=True): + def _run_playbook(self, test_playbook, ipaddr, playbook_data, root=True): '''Run the ansible-playbook command to execute given playbook containing the task. :param str test_playbook: name of the playbook, relative to the task directory :param str ipaddr: IP address of the machine the task will be run on + :param playbook_data: instance of Playbook_Data with parsed vars from + playbook :param bool root: whether to run as ``root`` for local execution mode :return: a tuple of ``(str, bool)``. The first item is stream output of the ansible-playbook command (stdout and stderr merged together). @@ -174,25 +206,6 @@ class Executor(object): minion_repos = config.get_config().minion_repos - # load taskotron vars from playbook and compute variables - with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ - as playbook_file: - # playbook should contain at least one play as the file passed the - # syntax check - # TODO: should we look for 'taskotron_generic_task' in all plays, - # not just in the first? - playbook = yaml.load(playbook_file.read())[0] - if playbook.get('vars',{}).get('taskotron_generic_task', False): - generic = True - exec_tasks = 'tasks_generic.yml' - log.debug('Playbook %s is a generic task', test_playbook) - else: - generic = False - exec_tasks = 'tasks_sti.yml' - log.debug('Playbook %s is an STI task', test_playbook) - keepalive = playbook.get('vars', {}).get( - 'taskotron_keepalive_minutes', None) - # set up variables to load into playbooks. # separate them into the ones to forward, and the ones to use # internally @@ -213,7 +226,7 @@ class Executor(object): ansible_vars_internal = { 'artifacts_root': self.arg_data['artifactsdir'], 'client_taskdir': config.get_config().client_taskdir, - 'exec_tasks': exec_tasks, + 'exec_tasks': playbook_data.exec_tasks, 'minion_repos': minion_repos, 'taskdir': self.arg_data['taskdir'], 'test_playbook': test_playbook, @@ -223,8 +236,8 @@ class Executor(object): varsfile_internal = os.path.join(artifacts_subdir, 'taskotron', 'ansible_vars_internal.json') - if keepalive: - ansible_vars_internal['keepalive_minutes'] = keepalive + if playbook_data.keepalive: + ansible_vars_internal['keepalive_minutes'] = playbook_data.keepalive # figure out the ansible-playbook command cmd = [ @@ -271,7 +284,7 @@ class Executor(object): signal.signal(signal.SIGTERM, self._interrupt_handler) output, _ = os_utils.popen_rt(cmd, cwd=ansible_dir) - return (output, generic) + return (output, playbook_data.generic) except subprocess.CalledProcessError as e: log.error('ansible-playbook ended with %d return code', e.returncode) @@ -340,14 +353,19 @@ class Executor(object): failed = [] for test_playbook in test_playbooks: + with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ + as playbook_file: + playbook_yaml = yaml.load(playbook_file.read())[0] + playbook_data = Playbook_Data(playbook_yaml) + ipaddr = self._get_client_ipaddr() if ipaddr is None: - ipaddr = self._spawn_vm(self.arg_data['uuid']) + ipaddr = self._spawn_vm(self.arg_data['uuid'], playbook_data) log.info('Running playbook %s on machine: %s', test_playbook, ipaddr) try: - _, generic = self._run_playbook(test_playbook, ipaddr) + _, generic = self._run_playbook(test_playbook, ipaddr, playbook_data) if generic: self._report_results(test_playbook) except exc.TaskotronInterruptError as e: diff --git a/libtaskotron/ext/disposable/vm.py b/libtaskotron/ext/disposable/vm.py index a790fe6..9d443ff 100644 --- a/libtaskotron/ext/disposable/vm.py +++ b/libtaskotron/ext/disposable/vm.py @@ -62,11 +62,6 @@ class TestCloudMachine(object): if config.get_config().force_imageurl: img_url = config.get_config().imageurl else: - distro = distro or config.get_config().default_disposable_distro - release = release or config.get_config().default_disposable_release - flavor = flavor or config.get_config().default_disposable_flavor - arch = arch or config.get_config().default_disposable_arch - log.debug("Looking for image with DISTRO: %s, RELEASE: %s, FLAVOR: %s, ARCH: %s" % (distro, release, flavor, arch)) diff --git a/libtaskotron/image_utils.py b/libtaskotron/image_utils.py index 01f21b8..5608ce0 100644 --- a/libtaskotron/image_utils.py +++ b/libtaskotron/image_utils.py @@ -9,6 +9,7 @@ import re from libtaskotron import exceptions as exc from libtaskotron.logger import log +from libtaskotron import config try: from libtaskotron.ext.fedora import rpm_utils @@ -16,13 +17,14 @@ except ImportError as e: raise exc.TaskotronImportError(e) -def devise_environment(arg_data): +def devise_environment(arg_data, playbook_data): '''Takes an input item and type, and returns a required run-environment (i.e. distro, arch, fedora release, and base-image flavor), based on item and type. - :param dict formula: parsed formula file (or dict with equivalent structure) :param dict arg_data: parsed command-line arguments. item, type and arch are used in this method + :param bool match_host_arch: devise arch only when this param is True, otherwise + use default arch :return: dict containing distro, release, flavor, arch. Each either set, or None :raise TaskotronValueError: when environment can't be parsed from the formula ''' @@ -32,7 +34,7 @@ def devise_environment(arg_data): item = arg_data.get('item', None) item_type = arg_data.get('type', None) - if not env['distro']: + if playbook_data.match_host_distro: if item_type == 'koji_build': # FIXME: find a way to make this not Fedora-specific # For `xchat-2.8.8-21.fc20` disttag is `fc20` for example @@ -52,8 +54,10 @@ def devise_environment(arg_data): else: log.debug("Environment/distro can not be inferred from %r:%r. Using default.", item_type, item) + else: + log.debug('Variable taskotron_match_host_distro is set to False. Using default distro.') - if not env['release']: + if playbook_data.match_host_release: if item_type == 'koji_build': # FIXME: find a way to make this not Fedora-specific # Last two characters in rpm's disttag are the Fedora release. @@ -73,16 +77,25 @@ def devise_environment(arg_data): else: log.debug("Environment/release can not be inferred from %r:%r. Using default.", item_type, item) + else: + log.debug('Variable taskotron_match_host_release is set to False. Using default release.') if not env['flavor']: log.debug("Environment/flavor not specified. Using default") - if not env['arch']: + if playbook_data.match_host_arch: arch = arg_data.get('arch') if not arch or arch == 'noarch': log.warn("Environment/arch can not be inferred from %r. Using default.", arch) else: env['arch'] = arch log.debug("Environment/arch inferred from %r to %r", arch, env['arch']) + else: + log.debug('Variable taskotron_match_host_arch is set to False. Using default arch.') + + env['distro'] = env['distro'] or config.get_config().default_disposable_distro + env['release'] = env['release'] or config.get_config().default_disposable_release + env['flavor'] = env['flavor'] or config.get_config().default_disposable_flavor + env['arch'] = env['arch'] or config.get_config().default_disposable_arch return env diff --git a/testing/functest_executor.py b/testing/functest_executor.py index 9428dd7..c71a79b 100644 --- a/testing/functest_executor.py +++ b/testing/functest_executor.py @@ -8,6 +8,7 @@ import pytest import mock import signal +import yaml from libtaskotron import executor import libtaskotron.exceptions as exc @@ -71,7 +72,7 @@ class TestExecutor(): # the tradeoff here is that we need to run with root=False # (non-default) output, _ = self.executor._run_playbook(self.playbook_name, - self.ipaddr, root=False) + self.ipaddr, executor.Playbook_Data(yaml.load(PLAYBOOK)[0]), root=False) # FIXME: We currently have no idea whether the test playbook passed # or failed, because we ignore the inner playbook's exit status. So # we can't really verify here whether everything worked ok. diff --git a/testing/test_executor.py b/testing/test_executor.py index 8f75692..a5dfbfe 100644 --- a/testing/test_executor.py +++ b/testing/test_executor.py @@ -11,6 +11,7 @@ import signal import os import json import subprocess +import yaml from libtaskotron import executor import libtaskotron.exceptions as exc @@ -39,6 +40,7 @@ PLAYBOOK_STI=''' msg: This is a sample debug printout from an STI task ''' +PLAYBOOK_DATA=executor.Playbook_Data(yaml.load(PLAYBOOK)[0]) @pytest.mark.usefixtures('setup') class TestExecutor(): @@ -127,7 +129,7 @@ class TestExecutor(): monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) output, _ = self.executor._run_playbook(self.playbook_name, - self.ipaddr) + self.ipaddr, PLAYBOOK_DATA) # must return playbook output assert output == 'fake output' @@ -170,7 +172,8 @@ class TestExecutor(): monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) with pytest.raises(exc.TaskotronError): - self.executor._run_playbook(self.playbook_name, self.ipaddr) + self.executor._run_playbook(self.playbook_name, self.ipaddr, + PLAYBOOK_DATA) # must unmask signals even when playbook failed assert mock_signal.call_count == 4 # 2 signals masked, then reset @@ -190,7 +193,8 @@ class TestExecutor(): monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) with pytest.raises(exc.TaskotronInterruptError): - self.executor._run_playbook(self.playbook_name, self.ipaddr) + self.executor._run_playbook(self.playbook_name, self.ipaddr, + PLAYBOOK_DATA) # must unmask signals even when playbook failed assert mock_signal.call_count == 4 # 2 signals masked, then reset @@ -213,7 +217,8 @@ class TestExecutor(): monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) with pytest.raises(exc.TaskotronInterruptError): - self.executor._run_playbook(self.playbook_name, self.ipaddr) + self.executor._run_playbook(self.playbook_name, self.ipaddr, + PLAYBOOK_DATA) def test_execute_local(self, monkeypatch): '''Execution using local mode''' @@ -416,7 +421,7 @@ class TestExecutor(): vm_instance.ipaddr = '10.11.12.13' monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm) - ipaddr = self.executor._spawn_vm(None) + ipaddr = self.executor._spawn_vm(None, PLAYBOOK_DATA) assert ipaddr == '10.11.12.13' assert mock_vm_prepare.call_count == 2 @@ -428,6 +433,6 @@ class TestExecutor(): monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm) with pytest.raises(exc.TaskotronMinionError): - self.executor._spawn_vm(None) + self.executor._spawn_vm(None, PLAYBOOK_DATA) assert mock_vm_prepare.call_count == self.conf.spawn_vm_retries diff --git a/testing/test_image_utils.py b/testing/test_image_utils.py index 82b9bbb..f785dec 100644 --- a/testing/test_image_utils.py +++ b/testing/test_image_utils.py @@ -6,6 +6,8 @@ '''Unit tests for libtaskotron/image_utils.py''' from libtaskotron.image_utils import devise_environment +from libtaskotron.executor import Playbook_Data +from libtaskotron import config class TestDeviseEnvironment: @@ -15,12 +17,19 @@ class TestDeviseEnvironment: 'type': 'koji_build', 'arch': 'noarch', } + self.playbook_data = Playbook_Data({ + 'vars': { + 'taskotron_match_host_distro': True, + 'taskotron_match_host_release': True, + 'taskotron_match_host_arch': False + } + }) def test_koji_build(self): - env = devise_environment(self.arg_data) + env = devise_environment(self.arg_data, self.playbook_data) assert env['distro'] == 'fedora' assert env['release'] == '27' - assert env['arch'] == None + assert env['arch'] == config.get_config().default_disposable_arch def test_koji_tag(self): self.arg_data = { @@ -28,13 +37,38 @@ class TestDeviseEnvironment: 'type': 'koji_tag', 'arch': 'x86_64', } - env = devise_environment(self.arg_data) + self.playbook_data.match_host_arch = True + env = devise_environment(self.arg_data, self.playbook_data) assert env['distro'] == 'fedora' assert env['release'] == '27' assert env['arch'] == self.arg_data['arch'] def test_unknown_distro(self): self.arg_data['item'] = 'htop-2.0.2-4.el7' - env = devise_environment(self.arg_data) - assert env['distro'] == None - assert env['release'] == None + env = devise_environment(self.arg_data, self.playbook_data) + assert env['distro'] == config.get_config().default_disposable_distro + assert env['release'] == config.get_config().default_disposable_release + + def test_match_host_arch_true(self): + self.arg_data = { + 'item': 'htop-2.0.2-4.fc27', + 'type': 'koji_build', + 'arch': 'armhfp', + } + self.playbook_data.match_host_arch = True + env = devise_environment(self.arg_data, self.playbook_data) + assert env['distro'] == 'fedora' + assert env['release'] == '27' + assert env['arch'] == 'armhfp' + + def test_match_host_arch_false(self): + self.arg_data = { + 'item': 'htop-2.0.2-4.fc27', + 'type': 'koji_build', + 'arch': 'armhfp', + } + self.playbook_data.match_host_arch = False + env = devise_environment(self.arg_data, self.playbook_data) + assert env['distro'] == 'fedora' + assert env['release'] == '27' + assert env['arch'] == config.get_config().default_disposable_arch From a9af4ea75b6fa03a4480db64b6ddafa1ce33a067 Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: May 11 2018 12:16:30 +0000 Subject: [PATCH 2/7] rework lbrabec's patch It uses dict for data management now. I used the opportunity to restructure it a bit, all available variables are now documented in executor.py. The variables are saved into a file to debugging, including the new ones. There's a new method that takes care of parsing variables and populating the dictionary, keeping the logic in one place. The number of debug printouts was reduced where they seemed unnecessary. A bit unrelated change is removing root=bool support to run the playbook as a standard user. It was a non-complete feature (not hooked up to cmdline args), never used, very probably broken. I didn't want to spend time to hook it up properly to the updated workflow. We can add it back later if needed. Please note that testsuite is broken at the moment, will fix it when I get ack on the patch. --- diff --git a/data/ansible/runner.yml b/data/ansible/runner.yml index f07a3c4..ac3c3cb 100644 --- a/data/ansible/runner.yml +++ b/data/ansible/runner.yml @@ -7,28 +7,7 @@ remote_user: root gather_facts: no vars: - become_root: true # whether to run playbooks as root - exec_tasks: tasks_generic.yml # path to taskotron tasks playbook (generic or STI) - heartbeat_file: "{{artifacts_root}}/taskotron/heartbeat.log" # heartbeat will appear in this file - heartbeat_interval: 120 # add line to heartbeat_file every x seconds (2 minutes) - keepalive_minutes: 0 # how long should heartbeat process run (0 to disable) - local: false # running on local machine (overlord), no remote connection - # These variables are also available: - # artifacts - path to the playbook-specific artifacts directory (on overlord and minion) - # artifacts_root - path to the root artifacts directory (on overlord and minion) - # client_taskdir - path to directory with test suite (on minion) - # minion_repos - a list of repos (strings) to install on the minion - # taskdir - path to directory with test suite (on overlord) - # taskotron_arch - architecture of taskotron_item to be tested - # taskotron_item - item under test - # taskotron_item_type - item type under test - # taskotron_supported_arches - list of base architectures supported by Taskotron (e.g. 'armhfp') - # taskotron_supported_binary_arches - list of base+binary architectures supported by Taskotron (e.g. 'armhfp, armv7hl') - # test_playbook - path to playbook to execute inside client_taskdir - # (usually tests.yml) - # sti_inventory - path to inventory file to be used with STI tests (local - # inside client_taskdir or global) - # varsfile - name of the ansible vars file inside artifacts/taskotron/ to forward to task + # Available variables are listed and documented inside executor.py tasks: - name: Install required packages dnf: @@ -73,7 +52,7 @@ file: path: "{{ artifacts_root }}/taskotron" state: directory - when: keepalive_minutes|int > 0 + when: taskotron_keepalive_minutes|int > 0 delegate_to: localhost - name: Set up extra DNF repositories (minion_repos) @@ -95,10 +74,10 @@ - name: Start heartbeat command: > ./heartbeat.sh start {{ heartbeat_file }} {{ heartbeat_interval }} - {{ keepalive_minutes | int * 60 }} - async: "{{ keepalive_minutes | int * 60 + 60 }}" + {{ taskotron_keepalive_minutes | int * 60 }} + async: "{{ taskotron_keepalive_minutes | int * 60 + 60 }}" poll: 0 - when: keepalive_minutes|int > 0 + when: taskotron_keepalive_minutes|int > 0 delegate_to: localhost - name: Include either generic or STI execution tasks @@ -108,7 +87,7 @@ - name: Kill heartbeat command: ./heartbeat.sh stop {{ heartbeat_file }} - when: keepalive_minutes|int > 0 + when: taskotron_keepalive_minutes|int > 0 delegate_to: localhost tags: - failsafe diff --git a/data/ansible/tasks_generic.yml b/data/ansible/tasks_generic.yml index 9099dbc..f692bca 100644 --- a/data/ansible/tasks_generic.yml +++ b/data/ansible/tasks_generic.yml @@ -2,7 +2,7 @@ # minion. The available variables are described in runner.yml. - name: Run {{ test_playbook }} - become: '{{ become_root }}' + become: yes become_user: root shell: > ansible-playbook "{{ client_taskdir }}/{{ test_playbook }}" diff --git a/docs/source/writingtasks.rst b/docs/source/writingtasks.rst index bffd4ab..f6cdaae 100644 --- a/docs/source/writingtasks.rst +++ b/docs/source/writingtasks.rst @@ -182,5 +182,17 @@ task. All the variables need to be defined in the first play of each specified number of minutes (the keepalive time) and only then the standard timeout counter will be started. +``taskotron_match_host_arch`` + (bool) Set to ``True``, if your task needs to be executed on a machine of + the same architecture as provided by ``taskotron_arch``. + +``taskotron_match_host_distro`` + (bool) Set to ``True``, if your task needs to be executed on the same + distribution (e.g. Fedora, RHEL) as indicated by ``taskotron_item``. + +``taskotron_match_host_release`` + (bool) Set to ``True``, if your task needs to be executed on the same + distribution release version (e.g. Fedora 27) as indicated by + ``taskotron_item``. .. _ResultsDB: https://fedoraproject.org/wiki/ResultsDB diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py index bbdcb6a..cf07395 100644 --- a/libtaskotron/executor.py +++ b/libtaskotron/executor.py @@ -12,6 +12,7 @@ import yaml import fnmatch import signal import json +import copy from libtaskotron import config from libtaskotron import image_utils @@ -27,33 +28,68 @@ try: except ImportError as e: raise exc.TaskotronImportError(e) - -class Playbook_Data(object): - '''Class holding parsed variables from playbook''' - def __init__(self, playbook={}): - self.match_host_distro = playbook.get('vars', {}).get( - 'taskotron_match_host_distro', False) - log.info("Variable taskotron_match_host_distro is %s", self.match_host_distro) - - self.match_host_release = playbook.get('vars', {}).get( - 'taskotron_match_host_release', False) - log.info("Variable taskotron_match_host_release is %s", self.match_host_release) - - self.match_host_arch = playbook.get('vars', {}).get( - 'taskotron_match_host_arch', False) - log.info("Variable taskotron_match_host_arch is %s", self.match_host_arch) - - if playbook.get('vars',{}).get('taskotron_generic_task', False): - self.generic = True - self.exec_tasks = 'tasks_generic.yml' - else: - self.generic = False - self.exec_tasks = 'tasks_sti.yml' - log.info('Variable taskotron_generic_task is: %s', self.generic) - - self.keepalive = playbook.get('vars', {}).get( - 'taskotron_keepalive_minutes', None) - log.info('Variable taskotron_keepalive_minutes is: %s', self.keepalive) +#: list of all vars and their defaults used exposed in our runner playbook +_PLAYBOOK_VARS_TEMPLATE = { + # path to the root artifacts directory (on overlord and minion) + 'artifacts_root': None, + # path to the playbook-specific artifacts directory (on overlord and + # minion) + 'artifacts': None, + # path to directory with test suite (on minion) + 'client_taskdir': None, + # path to taskotron tasks playbook (generic or STI) + 'exec_tasks': None, + # heartbeat will appear in this file + 'heartbeat_file': None, + # add line to heartbeat_file every x seconds (2 minutes) + 'heartbeat_interval': 120, + # running on local machine (overlord), no remote connection + 'local': False, + # a list of repos (strings) to install on the minion + 'minion_repos': [], + # path to inventory file to be used with STI tests (local inside + # client_taskdir or global) + 'sti_inventory': None, + # path to directory with test suite (on overlord) + 'taskdir': None, + # architecture of taskotron_item to be tested + 'taskotron_arch': None, + # whether to run a generic or STI task + 'taskotron_generic_task': False, + # item under test + 'taskotron_item_type': None, + # item type under test + 'taskotron_item': None, + # how long should heartbeat process run (0 to disable) + 'taskotron_keepalive_minutes': 0, + # whether VM guest arch has to match taskotron_arch + 'taskotron_match_host_arch': False, + # whether VM guest distro has to match taskotron_item + 'taskotron_match_host_distro': False, + # whether VM guest release has to match taskotron_item + 'taskotron_match_host_release': False, + # list of base architectures supported by Taskotron (e.g. 'armhfp') + 'taskotron_supported_arches': None, + # list of base+binary architectures supported by Taskotron (e.g. 'armhfp, + # armv7hl') + 'taskotron_supported_binary_arches': None, + # path to playbook to execute inside client_taskdir (usually tests.yml) + 'test_playbook': None, + # name of the ansible vars file inside artifacts/taskotron/ to forward to + # task + 'varsfile': 'task_vars.json', +} + +#: list of all vars to be exposed in the task playbook +# if you adjust this, also adjust html documentation +_FORWARDED_VARS = [ + 'artifacts', + 'taskotron_arch', + 'taskotron_item_type', + 'taskotron_item', + 'taskotron_supported_arches', + 'taskotron_supported_binary_arches', +] class Executor(object): @@ -72,18 +108,19 @@ class Executor(object): self.arg_data = arg_data self.task_vm = None self.run_remotely = False + self.ipaddr = self._get_client_ipaddr() - def _spawn_vm(self, uuid, playbook_data): + def _spawn_vm(self, uuid, playbook_vars): '''Spawn a virtual machine using testcloud. :param str uuid: unicode string uuid for the task being executed - :param playbook_data: instance of Playbook_Data with parsed vars from - playbook + :param dict playbook_vars: a vars dict based on + :const:`_PLAYBOOK_VARS_TEMPLATE` :returns: str ip address of spawned vm ''' log.info('Spawning disposable client') - env = image_utils.devise_environment(self.arg_data, playbook_data) + env = image_utils.devise_environment(self.arg_data, playbook_vars) self.task_vm = vm.TestCloudMachine(uuid) retries = config.get_config().spawn_vm_retries @@ -167,84 +204,95 @@ class Executor(object): e) raise exc.TaskotronPlaybookError(e) - def _run_playbook(self, test_playbook, ipaddr, playbook_data, root=True): + def _create_playbook_vars(self, test_playbook): + vars_ = copy.deepcopy(_PLAYBOOK_VARS_TEMPLATE) + cfg = config.get_config() + + # load all provided taskotron_* vars first, so that they don't override + # out logic later on + with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ + as playbook_file: + playbook_yaml = yaml.safe_load(playbook_file.read()) + # we only consider variables in the first play + playbook_vars = playbook_yaml[0].get('vars', {}) + for var, val in playbook_vars.items(): + if var.startswith('taskotron_'): + vars_[var] = val + + # populate vars + vars_['taskotron_arch'] = self.arg_data['arch'] + vars_['taskotron_item'] = self.arg_data['item'] + vars_['taskotron_item_type'] = self.arg_data['type'] + vars_['taskotron_supported_arches'] = cfg.supported_arches + vars_['taskotron_supported_binary_arches'] = [binarch for arch in + cfg.supported_arches for binarch in arch_utils.Arches.binary[arch]] + vars_['artifacts'] = os.path.join(self.arg_data['artifactsdir'], + test_playbook) + vars_['artifacts_root'] = self.arg_data['artifactsdir'] + vars_['client_taskdir'] = cfg.client_taskdir + vars_['taskdir'] = self.arg_data['taskdir'] + vars_['test_playbook'] = test_playbook + vars_['minion_repos'] = cfg.minion_repos + vars_['local'] = not self.run_remotely + vars_['heartbeat_file'] = os.path.join(vars_['artifacts_root'], + 'taskotron', 'heartbeat.log') + if vars_['taskotron_generic_task']: + vars_['exec_tasks'] = 'tasks_generic.yml' + else: + vars_['exec_tasks'] = 'tasks_sti.yml' + if os.path.isfile(os.path.join(vars_['taskdir'], 'inventory')): + vars_['sti_inventory'] = os.path.join(vars_['client_taskdir'], + 'inventory') + else: + vars_['sti_inventory'] = '/usr/share/ansible/inventory' + + return vars_ + + def _run_playbook(self, test_playbook, ipaddr, playbook_vars): '''Run the ansible-playbook command to execute given playbook containing the task. :param str test_playbook: name of the playbook, relative to the task directory :param str ipaddr: IP address of the machine the task will be run on - :param playbook_data: instance of Playbook_Data with parsed vars from - playbook - :param bool root: whether to run as ``root`` for local execution mode - :return: a tuple of ``(str, bool)``. The first item is stream output of - the ansible-playbook command (stdout and stderr merged together). - The second item marks whether this playbook is a Taskotron generic - task (``True``) or a plain STI task (``False``). - :rtype: tuple + :param dict playbook_vars: vars dict based on + :const:`_PLAYBOOK_VARS_TEMPLATE` + :return: stream output of the ansible-playbook command (stdout and + stderr merged together) + :rtype: str :raise TaskotronPlaybookError: when the playbook is not syntactically correct ''' - # syntax check - self._check_playbook_syntax( - os.path.join(self.arg_data['taskdir'], test_playbook)) - - # compute variables - ansible_dir = os.path.join(config.get_config()._data_dir, 'ansible') - artifacts_subdir = os.path.join(self.arg_data['artifactsdir'], - test_playbook) + # save forwarded variables, so that task playbook can load them + fwdvars = {} + for fwdvar in _FORWARDED_VARS: + fwdvars[fwdvar] = playbook_vars[fwdvar] + file_utils.makedirs(os.path.join(playbook_vars['artifacts'], + 'taskotron')) + varsfile = os.path.join(playbook_vars['artifacts'], 'taskotron', + 'task_vars.json') + with open(varsfile, 'w') as vf: + vars_str = json.dumps(fwdvars, indent=2, sort_keys=True) + vf.write(vars_str) + log.debug('Saved task vars file %s with contents:\n%s', + varsfile, vars_str) - if os.path.isfile(os.path.join(self.arg_data['taskdir'], 'inventory')): - sti_inventory = os.path.join(config.get_config().client_taskdir, - 'inventory') - else: - sti_inventory = "/usr/share/ansible/inventory" - - supported_arches = config.get_config().supported_arches - supported_binary_arches = [binarch for arch in supported_arches - for binarch in arch_utils.Arches.binary[arch]] - - minion_repos = config.get_config().minion_repos - - # set up variables to load into playbooks. - # separate them into the ones to forward, and the ones to use - # internally - - # if you adjust forwarded variables, don't forget to adjust docs - # as well - ansible_vars = { - 'artifacts': artifacts_subdir, - 'taskotron_arch': self.arg_data['arch'], - 'taskotron_item': self.arg_data['item'], - 'taskotron_item_type': self.arg_data['type'], - 'taskotron_supported_arches': supported_arches, - 'taskotron_supported_binary_arches': supported_binary_arches, - } - varsfile = os.path.join(artifacts_subdir, 'taskotron', - 'ansible_vars.json') - - ansible_vars_internal = { - 'artifacts_root': self.arg_data['artifactsdir'], - 'client_taskdir': config.get_config().client_taskdir, - 'exec_tasks': playbook_data.exec_tasks, - 'minion_repos': minion_repos, - 'taskdir': self.arg_data['taskdir'], - 'test_playbook': test_playbook, - 'sti_inventory': sti_inventory, - 'varsfile': os.path.basename(varsfile), - } - varsfile_internal = os.path.join(artifacts_subdir, 'taskotron', - 'ansible_vars_internal.json') - - if playbook_data.keepalive: - ansible_vars_internal['keepalive_minutes'] = playbook_data.keepalive + # save also all runner playbook variables, for debugging + allvarsfile = os.path.join(playbook_vars['artifacts'], 'taskotron', + 'internal_vars.json') + with open(allvarsfile, 'w') as vf: + vars_str = json.dumps(playbook_vars, indent=2, sort_keys=True) + vf.write(vars_str) + log.debug('Saved internal ansible vars file %s with contents:\n%s', + allvarsfile, vars_str) + ansible_dir = os.path.join(config.get_config()._data_dir, 'ansible') # figure out the ansible-playbook command cmd = [ 'ansible-playbook', 'runner.yml', '--inventory=%s,' % ipaddr, # the ending comma is important - '--extra-vars=@%s' % varsfile, - '--extra-vars=@%s' % varsfile_internal, + '--extra-vars=@%s' % allvarsfile, + '--become', ] if self.run_remotely: @@ -252,30 +300,10 @@ class Executor(object): cmd.append('--private-key=%s' % self.arg_data['ssh_privkey']) else: cmd.append('--connection=local') - ansible_vars_internal['local'] = True - if root: - cmd.append('--become') - ansible_vars_internal['become_root'] = True - else: - ansible_vars_internal['become_root'] = False if self.arg_data['debug']: cmd.append('-vv') - # store the variables in json files, so that ansible can load them - file_utils.makedirs(os.path.join(artifacts_subdir, 'taskotron')) - with open(varsfile, 'w') as vf: - vars_str = json.dumps(ansible_vars, indent=2, sort_keys=True) - vf.write(vars_str) - log.debug('Saved ansible vars file %s with contents:\n%s', - varsfile, vars_str) - with open(varsfile_internal, 'w') as vf: - vars_str = json.dumps(ansible_vars_internal, indent=2, - sort_keys=True) - vf.write(vars_str) - log.debug('Saved internal ansible vars file %s with contents:\n%s', - varsfile_internal, vars_str) - log.debug('Running ansible playbook %s', ' '.join(cmd)) try: # during playbook execution, handle system signals asking us to @@ -284,7 +312,7 @@ class Executor(object): signal.signal(signal.SIGTERM, self._interrupt_handler) output, _ = os_utils.popen_rt(cmd, cwd=ansible_dir) - return (output, playbook_data.generic) + return output except subprocess.CalledProcessError as e: log.error('ansible-playbook ended with %d return code', e.returncode) @@ -353,20 +381,27 @@ class Executor(object): failed = [] for test_playbook in test_playbooks: - with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ - as playbook_file: - playbook_yaml = yaml.load(playbook_file.read())[0] - playbook_data = Playbook_Data(playbook_yaml) - - ipaddr = self._get_client_ipaddr() - if ipaddr is None: - ipaddr = self._spawn_vm(self.arg_data['uuid'], playbook_data) - - log.info('Running playbook %s on machine: %s', test_playbook, ipaddr) - try: - _, generic = self._run_playbook(test_playbook, ipaddr, playbook_data) - if generic: + # syntax check + self._check_playbook_syntax(os.path.join( + self.arg_data['taskdir'], test_playbook)) + + # compute variables + playbook_vars = self._create_playbook_vars(test_playbook) + + # spawn VM if needed + ipaddr = self.ipaddr + if ipaddr is None: + ipaddr = self._spawn_vm(self.arg_data['uuid'], + playbook_vars) + + # execute + log.info('Running playbook %s on machine: %s', test_playbook, + ipaddr) + self._run_playbook(test_playbook, ipaddr, playbook_vars) + + # report results + if playbook_vars['taskotron_generic_task']: self._report_results(test_playbook) except exc.TaskotronInterruptError as e: log.error('Caught system interrupt during execution of ' diff --git a/libtaskotron/image_utils.py b/libtaskotron/image_utils.py index 5608ce0..56be9c1 100644 --- a/libtaskotron/image_utils.py +++ b/libtaskotron/image_utils.py @@ -17,47 +17,43 @@ except ImportError as e: raise exc.TaskotronImportError(e) -def devise_environment(arg_data, playbook_data): - '''Takes an input item and type, and returns a required run-environment (i.e. - distro, arch, fedora release, and base-image flavor), based on item and type. - - :param dict arg_data: parsed command-line arguments. item, type and arch - are used in this method - :param bool match_host_arch: devise arch only when this param is True, otherwise - use default arch - :return: dict containing distro, release, flavor, arch. Each either set, or None - :raise TaskotronValueError: when environment can't be parsed from the formula +def devise_environment(arg_data, playbook_vars): + '''Takes an input item and type, and returns a required run-environment, + or a default one, if the task doesn't require anything specific. + + :param dict arg_data: parsed command-line arguments (item, type and arch + are used in this method) + :param dict playbook_vars: vars dict based on + :const:`executor._PLAYBOOK_VARS_TEMPLATE` + :return: dict containing ``distro``, ``release``, ``flavor`` and ``arch``. + Each either set, or ``None``. ''' env = {'distro': None, 'release': None, 'flavor': None, 'arch': None} - item = arg_data.get('item', None) - item_type = arg_data.get('type', None) + item = arg_data['item'] + item_type = arg_data['type'] + arch = arg_data['arch'] - if playbook_data.match_host_distro: + if playbook_vars['taskotron_match_host_distro']: if item_type == 'koji_build': # FIXME: find a way to make this not Fedora-specific # For `xchat-2.8.8-21.fc20` disttag is `fc20` for example try: distro = rpm_utils.get_dist_tag(item)[:2] - env['distro'] = {'fc': 'fedora'}.get(distro, None) + env['distro'] = {'fc': 'fedora'}.get(distro) except exc.TaskotronValueError: - env['distro'] = None + log.debug('Failed to parse distro from koji build %s, using ' + 'default', item) elif item_type == 'koji_tag': if re.match(r'^f[0-9]{2}-.*', item): env['distro'] = 'fedora' if env['distro']: - log.debug("Environment/distro overriden with ENVVAR from %r:%r to %r", - item_type, item, env['distro']) - else: - log.debug("Environment/distro can not be inferred from %r:%r. Using default.", - item_type, item) - else: - log.debug('Variable taskotron_match_host_distro is set to False. Using default distro.') - - if playbook_data.match_host_release: + log.debug('Forcing environment distro to %s', env['distro']) + + if playbook_vars['taskotron_match_host_release']: if item_type == 'koji_build': # FIXME: find a way to make this not Fedora-specific # Last two characters in rpm's disttag are the Fedora release. @@ -65,37 +61,26 @@ def devise_environment(arg_data, playbook_data): try: env['release'] = rpm_utils.get_dist_tag(item)[-2:] except exc.TaskotronValueError: - env['release'] = None + log.debug('Failed to parse release from koji build %s, using ' + 'default', item) elif item_type == 'koji_tag': if re.match(r'^f[0-9]{2}-.*', item): env['release'] = item[1:3] if env['release']: - log.debug("Environment/release inferred from %r:%r to %r", - item_type, item, env['release']) - else: - log.debug("Environment/release can not be inferred from %r:%r. Using default.", - item_type, item) - else: - log.debug('Variable taskotron_match_host_release is set to False. Using default release.') - - if not env['flavor']: - log.debug("Environment/flavor not specified. Using default") - - if playbook_data.match_host_arch: - arch = arg_data.get('arch') - if not arch or arch == 'noarch': - log.warn("Environment/arch can not be inferred from %r. Using default.", arch) - else: + log.debug('Forcing environment release to %s', env['release']) + + if playbook_vars['taskotron_match_host_arch']: + if arch != 'noarch': env['arch'] = arch - log.debug("Environment/arch inferred from %r to %r", arch, env['arch']) - else: - log.debug('Variable taskotron_match_host_arch is set to False. Using default arch.') + log.debug('Forcing environment arch to %s', env['arch']) env['distro'] = env['distro'] or config.get_config().default_disposable_distro env['release'] = env['release'] or config.get_config().default_disposable_release env['flavor'] = env['flavor'] or config.get_config().default_disposable_flavor env['arch'] = env['arch'] or config.get_config().default_disposable_arch + log.debug('Devised environment: %s', env) + return env From 02d7dce3d5da7ffbc730840f2f12b69bb0ae0990 Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: May 11 2018 12:42:40 +0000 Subject: [PATCH 3/7] move vars template to end of file, use varsfile default --- diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py index cf07395..16dbf4c 100644 --- a/libtaskotron/executor.py +++ b/libtaskotron/executor.py @@ -28,69 +28,6 @@ try: except ImportError as e: raise exc.TaskotronImportError(e) -#: list of all vars and their defaults used exposed in our runner playbook -_PLAYBOOK_VARS_TEMPLATE = { - # path to the root artifacts directory (on overlord and minion) - 'artifacts_root': None, - # path to the playbook-specific artifacts directory (on overlord and - # minion) - 'artifacts': None, - # path to directory with test suite (on minion) - 'client_taskdir': None, - # path to taskotron tasks playbook (generic or STI) - 'exec_tasks': None, - # heartbeat will appear in this file - 'heartbeat_file': None, - # add line to heartbeat_file every x seconds (2 minutes) - 'heartbeat_interval': 120, - # running on local machine (overlord), no remote connection - 'local': False, - # a list of repos (strings) to install on the minion - 'minion_repos': [], - # path to inventory file to be used with STI tests (local inside - # client_taskdir or global) - 'sti_inventory': None, - # path to directory with test suite (on overlord) - 'taskdir': None, - # architecture of taskotron_item to be tested - 'taskotron_arch': None, - # whether to run a generic or STI task - 'taskotron_generic_task': False, - # item under test - 'taskotron_item_type': None, - # item type under test - 'taskotron_item': None, - # how long should heartbeat process run (0 to disable) - 'taskotron_keepalive_minutes': 0, - # whether VM guest arch has to match taskotron_arch - 'taskotron_match_host_arch': False, - # whether VM guest distro has to match taskotron_item - 'taskotron_match_host_distro': False, - # whether VM guest release has to match taskotron_item - 'taskotron_match_host_release': False, - # list of base architectures supported by Taskotron (e.g. 'armhfp') - 'taskotron_supported_arches': None, - # list of base+binary architectures supported by Taskotron (e.g. 'armhfp, - # armv7hl') - 'taskotron_supported_binary_arches': None, - # path to playbook to execute inside client_taskdir (usually tests.yml) - 'test_playbook': None, - # name of the ansible vars file inside artifacts/taskotron/ to forward to - # task - 'varsfile': 'task_vars.json', -} - -#: list of all vars to be exposed in the task playbook -# if you adjust this, also adjust html documentation -_FORWARDED_VARS = [ - 'artifacts', - 'taskotron_arch', - 'taskotron_item_type', - 'taskotron_item', - 'taskotron_supported_arches', - 'taskotron_supported_binary_arches', -] - class Executor(object): '''Executor executes given task in the format of Ansible playbook. Before @@ -270,7 +207,7 @@ class Executor(object): file_utils.makedirs(os.path.join(playbook_vars['artifacts'], 'taskotron')) varsfile = os.path.join(playbook_vars['artifacts'], 'taskotron', - 'task_vars.json') + playbook_vars['varsfile']) with open(varsfile, 'w') as vf: vars_str = json.dumps(fwdvars, indent=2, sort_keys=True) vf.write(vars_str) @@ -425,3 +362,67 @@ class Executor(object): log.info('All playbooks finished successfully') return not failed + + +#: list of all vars and their defaults used exposed in our runner playbook +_PLAYBOOK_VARS_TEMPLATE = { + # path to the root artifacts directory (on overlord and minion) + 'artifacts_root': None, + # path to the playbook-specific artifacts directory (on overlord and + # minion) + 'artifacts': None, + # path to directory with test suite (on minion) + 'client_taskdir': None, + # path to taskotron tasks playbook (generic or STI) + 'exec_tasks': None, + # heartbeat will appear in this file + 'heartbeat_file': None, + # add line to heartbeat_file every x seconds (2 minutes) + 'heartbeat_interval': 120, + # running on local machine (overlord), no remote connection + 'local': False, + # a list of repos (strings) to install on the minion + 'minion_repos': [], + # path to inventory file to be used with STI tests (local inside + # client_taskdir or global) + 'sti_inventory': None, + # path to directory with test suite (on overlord) + 'taskdir': None, + # architecture of taskotron_item to be tested + 'taskotron_arch': None, + # whether to run a generic or STI task + 'taskotron_generic_task': False, + # item under test + 'taskotron_item_type': None, + # item type under test + 'taskotron_item': None, + # how long should heartbeat process run (0 to disable) + 'taskotron_keepalive_minutes': 0, + # whether VM guest arch has to match taskotron_arch + 'taskotron_match_host_arch': False, + # whether VM guest distro has to match taskotron_item + 'taskotron_match_host_distro': False, + # whether VM guest release has to match taskotron_item + 'taskotron_match_host_release': False, + # list of base architectures supported by Taskotron (e.g. 'armhfp') + 'taskotron_supported_arches': None, + # list of base+binary architectures supported by Taskotron (e.g. 'armhfp, + # armv7hl') + 'taskotron_supported_binary_arches': None, + # path to playbook to execute inside client_taskdir (usually tests.yml) + 'test_playbook': None, + # name of the ansible vars file inside artifacts/taskotron/ to forward to + # task + 'varsfile': 'task_vars.json', +} + +#: list of all vars to be exposed in the task playbook +# if you adjust this, also adjust html documentation +_FORWARDED_VARS = [ + 'artifacts', + 'taskotron_arch', + 'taskotron_item_type', + 'taskotron_item', + 'taskotron_supported_arches', + 'taskotron_supported_binary_arches', +] From 67557c4a640c3b9e49c81ad34a52fc2514e2e6be Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: May 15 2018 14:03:19 +0000 Subject: [PATCH 4/7] restructure per review comments --- diff --git a/data/ansible/tasks_sti.yml b/data/ansible/tasks_sti.yml index a044d98..8009264 100644 --- a/data/ansible/tasks_sti.yml +++ b/data/ansible/tasks_sti.yml @@ -22,7 +22,7 @@ - debug: var=acquired_subject - name: Run {{ test_playbook }} - become: '{{ become_root }}' + become: yes become_user: root # FIXME add context tags shell: > diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py index 16dbf4c..46b1a7f 100644 --- a/libtaskotron/executor.py +++ b/libtaskotron/executor.py @@ -12,7 +12,6 @@ import yaml import fnmatch import signal import json -import copy from libtaskotron import config from libtaskotron import image_utils @@ -41,6 +40,27 @@ class Executor(object): :ivar bool run_remotely: whether the task is run on a remote machine ''' + #: vars retrieved from tests.yml + ACCEPTED_VARS = [ + # if you adjust this, also adjust writingtasks.rst + 'taskotron_generic_task', + 'taskotron_keepalive_minutes', + 'taskotron_match_host_arch', + 'taskotron_match_host_distro', + 'taskotron_match_host_release', + ] + + #: list of all vars to be exposed in the task playbook + FORWARDED_VARS = [ + # if you adjust this, also adjust writingtasks.rst + 'artifacts', + 'taskotron_arch', + 'taskotron_item_type', + 'taskotron_item', + 'taskotron_supported_arches', + 'taskotron_supported_binary_arches', + ] + def __init__(self, arg_data): self.arg_data = arg_data self.task_vm = None @@ -142,41 +162,125 @@ class Executor(object): raise exc.TaskotronPlaybookError(e) def _create_playbook_vars(self, test_playbook): - vars_ = copy.deepcopy(_PLAYBOOK_VARS_TEMPLATE) + '''Create and return dictionary containing all variables to be used + with our ansible playbook. The dictionary contains these keys: + + artifacts_root + path to the root artifacts directory (on overlord and minion) + + artifacts + path to the playbook-specific artifacts directory (on overlord and + minion) + + client_taskdir + path to directory with test suite (on minion) + + exec_tasks + path to taskotron tasks playbook (generic or STI) + + heartbeat_file + heartbeat will appear in this file + + heartbeat_interval + add line to heartbeat_file every x seconds (2 minutes) + + local + running on local machine (overlord), no remote connection + + minion_repos + a list of repos (strings) to install on the minion + + sti_inventory + path to inventory file to be used with STI tests (local inside + client_taskdir or global) + + taskdir + path to directory with test suite (on overlord) + + taskotron_arch + architecture of taskotron_item to be tested + + taskotron_generic_task + whether to run a generic or STI task + + taskotron_item_type + item under test + + taskotron_item + item type under test + + taskotron_keepalive_minutes + how long should heartbeat process run (0 to disable) + + taskotron_match_host_arch + whether VM guest arch has to match taskotron_arch + + taskotron_match_host_distro + whether VM guest distro has to match taskotron_item + + taskotron_match_host_release + whether VM guest release has to match taskotron_item + + taskotron_supported_arches + list of base architectures supported by Taskotron (e.g. 'armhfp') + + taskotron_supported_binary_arches + list of base+binary architectures supported by Taskotron (e.g. 'armhfp, + armv7hl') + + test_playbook + path to playbook to execute inside client_taskdir (usually tests.yml) + + varsfile + name of the ansible vars file inside artifacts/taskotron/ to forward to + task + ''' + + vars_ = {} cfg = config.get_config() - # load all provided taskotron_* vars first, so that they don't override - # out logic later on + # default values + vars_['taskotron_generic_task'] = False + vars_['heartbeat_interval'] = 120 + vars_['taskotron_keepalive_minutes'] = 0 + vars_['taskotron_match_host_arch'] = False + vars_['taskotron_match_host_distro'] = False + vars_['taskotron_match_host_release'] = False + vars_['varsfile'] = 'task_vars.json' + + # load all allowed vars from tests.yml with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ as playbook_file: playbook_yaml = yaml.safe_load(playbook_file.read()) # we only consider variables in the first play playbook_vars = playbook_yaml[0].get('vars', {}) for var, val in playbook_vars.items(): - if var.startswith('taskotron_'): + if var in self.ACCEPTED_VARS: vars_[var] = val - # populate vars + # compute vars + vars_['artifacts'] = os.path.join(self.arg_data['artifactsdir'], + test_playbook) + vars_['artifacts_root'] = self.arg_data['artifactsdir'] + vars_['client_taskdir'] = cfg.client_taskdir + vars_['heartbeat_file'] = os.path.join(vars_['artifacts_root'], + 'taskotron', 'heartbeat.log') + vars_['local'] = not self.run_remotely + vars_['minion_repos'] = cfg.minion_repos + vars_['taskdir'] = self.arg_data['taskdir'] vars_['taskotron_arch'] = self.arg_data['arch'] vars_['taskotron_item'] = self.arg_data['item'] vars_['taskotron_item_type'] = self.arg_data['type'] vars_['taskotron_supported_arches'] = cfg.supported_arches vars_['taskotron_supported_binary_arches'] = [binarch for arch in cfg.supported_arches for binarch in arch_utils.Arches.binary[arch]] - vars_['artifacts'] = os.path.join(self.arg_data['artifactsdir'], - test_playbook) - vars_['artifacts_root'] = self.arg_data['artifactsdir'] - vars_['client_taskdir'] = cfg.client_taskdir - vars_['taskdir'] = self.arg_data['taskdir'] vars_['test_playbook'] = test_playbook - vars_['minion_repos'] = cfg.minion_repos - vars_['local'] = not self.run_remotely - vars_['heartbeat_file'] = os.path.join(vars_['artifacts_root'], - 'taskotron', 'heartbeat.log') + if vars_['taskotron_generic_task']: vars_['exec_tasks'] = 'tasks_generic.yml' else: vars_['exec_tasks'] = 'tasks_sti.yml' + if os.path.isfile(os.path.join(vars_['taskdir'], 'inventory')): vars_['sti_inventory'] = os.path.join(vars_['client_taskdir'], 'inventory') @@ -202,7 +306,7 @@ class Executor(object): ''' # save forwarded variables, so that task playbook can load them fwdvars = {} - for fwdvar in _FORWARDED_VARS: + for fwdvar in self.FORWARDED_VARS: fwdvars[fwdvar] = playbook_vars[fwdvar] file_utils.makedirs(os.path.join(playbook_vars['artifacts'], 'taskotron')) @@ -362,67 +466,3 @@ class Executor(object): log.info('All playbooks finished successfully') return not failed - - -#: list of all vars and their defaults used exposed in our runner playbook -_PLAYBOOK_VARS_TEMPLATE = { - # path to the root artifacts directory (on overlord and minion) - 'artifacts_root': None, - # path to the playbook-specific artifacts directory (on overlord and - # minion) - 'artifacts': None, - # path to directory with test suite (on minion) - 'client_taskdir': None, - # path to taskotron tasks playbook (generic or STI) - 'exec_tasks': None, - # heartbeat will appear in this file - 'heartbeat_file': None, - # add line to heartbeat_file every x seconds (2 minutes) - 'heartbeat_interval': 120, - # running on local machine (overlord), no remote connection - 'local': False, - # a list of repos (strings) to install on the minion - 'minion_repos': [], - # path to inventory file to be used with STI tests (local inside - # client_taskdir or global) - 'sti_inventory': None, - # path to directory with test suite (on overlord) - 'taskdir': None, - # architecture of taskotron_item to be tested - 'taskotron_arch': None, - # whether to run a generic or STI task - 'taskotron_generic_task': False, - # item under test - 'taskotron_item_type': None, - # item type under test - 'taskotron_item': None, - # how long should heartbeat process run (0 to disable) - 'taskotron_keepalive_minutes': 0, - # whether VM guest arch has to match taskotron_arch - 'taskotron_match_host_arch': False, - # whether VM guest distro has to match taskotron_item - 'taskotron_match_host_distro': False, - # whether VM guest release has to match taskotron_item - 'taskotron_match_host_release': False, - # list of base architectures supported by Taskotron (e.g. 'armhfp') - 'taskotron_supported_arches': None, - # list of base+binary architectures supported by Taskotron (e.g. 'armhfp, - # armv7hl') - 'taskotron_supported_binary_arches': None, - # path to playbook to execute inside client_taskdir (usually tests.yml) - 'test_playbook': None, - # name of the ansible vars file inside artifacts/taskotron/ to forward to - # task - 'varsfile': 'task_vars.json', -} - -#: list of all vars to be exposed in the task playbook -# if you adjust this, also adjust html documentation -_FORWARDED_VARS = [ - 'artifacts', - 'taskotron_arch', - 'taskotron_item_type', - 'taskotron_item', - 'taskotron_supported_arches', - 'taskotron_supported_binary_arches', -] From 85758c8418bae2f60aee4e43d1ccc41ff7fa7b40 Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: May 16 2018 02:12:38 +0000 Subject: [PATCH 5/7] moar changes --- diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py index 46b1a7f..4cb795e 100644 --- a/libtaskotron/executor.py +++ b/libtaskotron/executor.py @@ -225,15 +225,15 @@ class Executor(object): list of base architectures supported by Taskotron (e.g. 'armhfp') taskotron_supported_binary_arches - list of base+binary architectures supported by Taskotron (e.g. 'armhfp, - armv7hl') + list of base+binary architectures supported by Taskotron (e.g. + 'armhfp, armv7hl') test_playbook path to playbook to execute inside client_taskdir (usually tests.yml) varsfile - name of the ansible vars file inside artifacts/taskotron/ to forward to - task + name of the ansible vars file inside artifacts/taskotron/ to forward + to task ''' vars_ = {} diff --git a/libtaskotron/ext/disposable/vm.py b/libtaskotron/ext/disposable/vm.py index 9d443ff..494e795 100644 --- a/libtaskotron/ext/disposable/vm.py +++ b/libtaskotron/ext/disposable/vm.py @@ -46,7 +46,7 @@ class TestCloudMachine(object): #: hostname to use for spawned instance - based on username of current user self.hostname = 'taskotron-%s' % getpass.getuser() - def _prepare_image(self, distro=None, release=None, flavor=None, arch=None): + def _prepare_image(self, distro, release, flavor, arch): '''Use testcloud to prepare an image for local booting :param str distro: Distro to use in image discovery :param str release: Distro's release to use in image discovery diff --git a/libtaskotron/image_utils.py b/libtaskotron/image_utils.py index 56be9c1..289bd15 100644 --- a/libtaskotron/image_utils.py +++ b/libtaskotron/image_utils.py @@ -50,9 +50,6 @@ def devise_environment(arg_data, playbook_vars): if re.match(r'^f[0-9]{2}-.*', item): env['distro'] = 'fedora' - if env['distro']: - log.debug('Forcing environment distro to %s', env['distro']) - if playbook_vars['taskotron_match_host_release']: if item_type == 'koji_build': # FIXME: find a way to make this not Fedora-specific @@ -68,19 +65,17 @@ def devise_environment(arg_data, playbook_vars): if re.match(r'^f[0-9]{2}-.*', item): env['release'] = item[1:3] - if env['release']: - log.debug('Forcing environment release to %s', env['release']) - if playbook_vars['taskotron_match_host_arch']: if arch != 'noarch': env['arch'] = arch - log.debug('Forcing environment arch to %s', env['arch']) + + log.debug('Forced environment values: %s', env) env['distro'] = env['distro'] or config.get_config().default_disposable_distro env['release'] = env['release'] or config.get_config().default_disposable_release env['flavor'] = env['flavor'] or config.get_config().default_disposable_flavor env['arch'] = env['arch'] or config.get_config().default_disposable_arch - log.debug('Devised environment: %s', env) + log.debug('Environment to be used: %s', env) return env From ae81c3719ae5e196640bb267d558471973c7f58a Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: May 16 2018 13:55:56 +0000 Subject: [PATCH 6/7] add tests. Also add back option to run non-root, since tests need it --- diff --git a/data/ansible/runner.yml b/data/ansible/runner.yml index ac3c3cb..658b63c 100644 --- a/data/ansible/runner.yml +++ b/data/ansible/runner.yml @@ -7,7 +7,7 @@ remote_user: root gather_facts: no vars: - # Available variables are listed and documented inside executor.py + # Available variables are documented at executor.py:_create_playbook_vars() tasks: - name: Install required packages dnf: diff --git a/data/ansible/tasks_generic.yml b/data/ansible/tasks_generic.yml index f692bca..9099dbc 100644 --- a/data/ansible/tasks_generic.yml +++ b/data/ansible/tasks_generic.yml @@ -2,7 +2,7 @@ # minion. The available variables are described in runner.yml. - name: Run {{ test_playbook }} - become: yes + become: '{{ become_root }}' become_user: root shell: > ansible-playbook "{{ client_taskdir }}/{{ test_playbook }}" diff --git a/data/ansible/tasks_sti.yml b/data/ansible/tasks_sti.yml index 8009264..a044d98 100644 --- a/data/ansible/tasks_sti.yml +++ b/data/ansible/tasks_sti.yml @@ -22,7 +22,7 @@ - debug: var=acquired_subject - name: Run {{ test_playbook }} - become: yes + become: '{{ become_root }}' become_user: root # FIXME add context tags shell: > diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py index 4cb795e..158c18d 100644 --- a/libtaskotron/executor.py +++ b/libtaskotron/executor.py @@ -71,8 +71,8 @@ class Executor(object): '''Spawn a virtual machine using testcloud. :param str uuid: unicode string uuid for the task being executed - :param dict playbook_vars: a vars dict based on - :const:`_PLAYBOOK_VARS_TEMPLATE` + :param dict playbook_vars: a vars dict created by + :meth:`_create_playbook_vars` :returns: str ip address of spawned vm ''' log.info('Spawning disposable client') @@ -161,85 +161,96 @@ class Executor(object): e) raise exc.TaskotronPlaybookError(e) - def _create_playbook_vars(self, test_playbook): - '''Create and return dictionary containing all variables to be used - with our ansible playbook. The dictionary contains these keys: - - artifacts_root - path to the root artifacts directory (on overlord and minion) - - artifacts - path to the playbook-specific artifacts directory (on overlord and - minion) - - client_taskdir - path to directory with test suite (on minion) - - exec_tasks - path to taskotron tasks playbook (generic or STI) - - heartbeat_file - heartbeat will appear in this file - - heartbeat_interval - add line to heartbeat_file every x seconds (2 minutes) - - local - running on local machine (overlord), no remote connection - - minion_repos - a list of repos (strings) to install on the minion - - sti_inventory - path to inventory file to be used with STI tests (local inside - client_taskdir or global) - - taskdir - path to directory with test suite (on overlord) + def _load_playbook_vars(self, test_playbook): + '''Load accepted playbook vars from a playbook file and return it + as a dict. - taskotron_arch - architecture of taskotron_item to be tested - - taskotron_generic_task - whether to run a generic or STI task - - taskotron_item_type - item under test - - taskotron_item - item type under test - - taskotron_keepalive_minutes - how long should heartbeat process run (0 to disable) - - taskotron_match_host_arch - whether VM guest arch has to match taskotron_arch - - taskotron_match_host_distro - whether VM guest distro has to match taskotron_item - - taskotron_match_host_release - whether VM guest release has to match taskotron_item - - taskotron_supported_arches - list of base architectures supported by Taskotron (e.g. 'armhfp') - - taskotron_supported_binary_arches - list of base+binary architectures supported by Taskotron (e.g. - 'armhfp, armv7hl') + :param str test_playbook: name of the playbook, relative to the task + directory + :return: a dict with keyvals from the playbook which are allowed to be + loaded (see :attr:`ACCEPTED_VARS`). + ''' + vars_ = {} + with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ + as playbook_file: + playbook_yaml = yaml.safe_load(playbook_file.read()) + # we only consider variables in the first play + playbook_vars = playbook_yaml[0].get('vars', {}) + for var, val in playbook_vars.items(): + if var in self.ACCEPTED_VARS: + vars_[var] = val + return vars_ - test_playbook - path to playbook to execute inside client_taskdir (usually tests.yml) + def _create_playbook_vars(self, test_playbook): + '''Create and return dictionary containing all variables to be used + with our ansible playbook. - varsfile - name of the ansible vars file inside artifacts/taskotron/ to forward - to task + :param str test_playbook: name of the playbook, relative to the task + directory + :param bool root: whether to run playbooks as root. This is mainly + for testing purposes. + + :return: A dictionary containing these keys: + + artifacts_root + path to the root artifacts directory (on overlord and minion) + artifacts + path to the playbook-specific artifacts directory (on overlord + and minion) + become_root + whether to run playbooks as root + client_taskdir + path to directory with test suite (on minion) + exec_tasks + path to taskotron tasks playbook (generic or STI) + heartbeat_file + heartbeat will appear in this file + heartbeat_interval + add line to heartbeat_file every x seconds (2 minutes) + local + running on local machine (overlord), no remote connection + minion_repos + a list of repos (strings) to install on the minion + sti_inventory + path to inventory file to be used with STI tests (local inside + client_taskdir or global) + taskdir + path to directory with test suite (on overlord) + taskotron_arch + architecture of taskotron_item to be tested + taskotron_generic_task + whether to run a generic or STI task + taskotron_item_type + item under test + taskotron_item + item type under test + taskotron_keepalive_minutes + how long should heartbeat process run (0 to disable) + taskotron_match_host_arch + whether VM guest arch has to match taskotron_arch + taskotron_match_host_distro + whether VM guest distro has to match taskotron_item + taskotron_match_host_release + whether VM guest release has to match taskotron_item + taskotron_supported_arches + list of base architectures supported by Taskotron (e.g. + 'armhfp') + taskotron_supported_binary_arches + list of base+binary architectures supported by Taskotron (e.g. + 'armhfp, armv7hl') + test_playbook + path to playbook to execute inside client_taskdir (usually + tests.yml) + varsfile + name of the ansible vars file inside ``artifacts/taskotron/`` + to forward to task ''' vars_ = {} cfg = config.get_config() # default values + vars_['become_root'] = True vars_['taskotron_generic_task'] = False vars_['heartbeat_interval'] = 120 vars_['taskotron_keepalive_minutes'] = 0 @@ -249,14 +260,8 @@ class Executor(object): vars_['varsfile'] = 'task_vars.json' # load all allowed vars from tests.yml - with open(os.path.join(self.arg_data['taskdir'], test_playbook), 'r') \ - as playbook_file: - playbook_yaml = yaml.safe_load(playbook_file.read()) - # we only consider variables in the first play - playbook_vars = playbook_yaml[0].get('vars', {}) - for var, val in playbook_vars.items(): - if var in self.ACCEPTED_VARS: - vars_[var] = val + loaded_vars = self._load_playbook_vars(test_playbook) + vars_.update(loaded_vars) # compute vars vars_['artifacts'] = os.path.join(self.arg_data['artifactsdir'], @@ -296,8 +301,8 @@ class Executor(object): :param str test_playbook: name of the playbook, relative to the task directory :param str ipaddr: IP address of the machine the task will be run on - :param dict playbook_vars: vars dict based on - :const:`_PLAYBOOK_VARS_TEMPLATE` + :param dict playbook_vars: vars dict created by + :meth:`_create_playbook_vars` :return: stream output of the ansible-playbook command (stdout and stderr merged together) :rtype: str @@ -333,9 +338,11 @@ class Executor(object): 'ansible-playbook', 'runner.yml', '--inventory=%s,' % ipaddr, # the ending comma is important '--extra-vars=@%s' % allvarsfile, - '--become', ] + if playbook_vars['become_root']: + cmd.append('--become') + if self.run_remotely: if self.arg_data['ssh_privkey']: cmd.append('--private-key=%s' % self.arg_data['ssh_privkey']) diff --git a/libtaskotron/ext/disposable/vm.py b/libtaskotron/ext/disposable/vm.py index 494e795..74aaddf 100644 --- a/libtaskotron/ext/disposable/vm.py +++ b/libtaskotron/ext/disposable/vm.py @@ -120,7 +120,7 @@ class TestCloudMachine(object): "already defined".format(self.instancename)) return existing_instance - def prepare(self, distro=None, release=None, flavor=None, arch=None): + def prepare(self, distro, release, flavor, arch): '''Prepare a virtual machine for running tasks. :param str distro: Distro to use in image discovery :param str release: Distro's release to use in image discovery diff --git a/libtaskotron/image_utils.py b/libtaskotron/image_utils.py index 289bd15..20db606 100644 --- a/libtaskotron/image_utils.py +++ b/libtaskotron/image_utils.py @@ -23,8 +23,8 @@ def devise_environment(arg_data, playbook_vars): :param dict arg_data: parsed command-line arguments (item, type and arch are used in this method) - :param dict playbook_vars: vars dict based on - :const:`executor._PLAYBOOK_VARS_TEMPLATE` + :param dict playbook_vars: vars dict created by + :meth:`executor._create_playbook_vars` :return: dict containing ``distro``, ``release``, ``flavor`` and ``arch``. Each either set, or ``None``. ''' diff --git a/testing/functest_executor.py b/testing/functest_executor.py index c71a79b..279b38c 100644 --- a/testing/functest_executor.py +++ b/testing/functest_executor.py @@ -8,7 +8,6 @@ import pytest import mock import signal -import yaml from libtaskotron import executor import libtaskotron.exceptions as exc @@ -42,6 +41,9 @@ class TestExecutor(): 'type': 'koji_build', 'arch': 'noarch', 'debug': False, + 'local': True, + 'libvirt': False, + 'ssh': False, 'ssh_privkey': None, } self.playbook_name = 'tests.yml' @@ -49,6 +51,8 @@ class TestExecutor(): self.playbook.write(PLAYBOOK) self.ipaddr = '127.0.0.1' self.executor = executor.Executor(self.arg_data) + self.playbook_vars = self.executor._create_playbook_vars( + self.playbook_name) monkeypatch.setattr(config, '_config', None) self.conf = config.get_config() @@ -68,11 +72,11 @@ class TestExecutor(): '''Execute a very simple playbook whether everything works''' mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) + # it's non-default, but we can't run the test suite as root + self.playbook_vars['become_root'] = False - # the tradeoff here is that we need to run with root=False - # (non-default) - output, _ = self.executor._run_playbook(self.playbook_name, - self.ipaddr, executor.Playbook_Data(yaml.load(PLAYBOOK)[0]), root=False) + output = self.executor._run_playbook(self.playbook_name, + self.ipaddr, self.playbook_vars) # FIXME: We currently have no idea whether the test playbook passed # or failed, because we ignore the inner playbook's exit status. So # we can't really verify here whether everything worked ok. diff --git a/testing/test_executor.py b/testing/test_executor.py index a5dfbfe..527c68f 100644 --- a/testing/test_executor.py +++ b/testing/test_executor.py @@ -40,7 +40,6 @@ PLAYBOOK_STI=''' msg: This is a sample debug printout from an STI task ''' -PLAYBOOK_DATA=executor.Playbook_Data(yaml.load(PLAYBOOK)[0]) @pytest.mark.usefixtures('setup') class TestExecutor(): @@ -68,6 +67,8 @@ class TestExecutor(): self.playbook.write(PLAYBOOK) self.ipaddr = '127.0.0.1' self.executor = executor.Executor(self.arg_data) + self.playbook_vars = self.executor._create_playbook_vars( + self.playbook_name) monkeypatch.setattr(config, '_config', None) self.conf = config.get_config() @@ -120,31 +121,25 @@ class TestExecutor(): def test_run_playbook(self, monkeypatch): '''A standard invocation of ansible-playbook''' - mock_check_syntax = mock.Mock() - monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', - mock_check_syntax) mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) mock_popen = mock.Mock(return_value=('fake output', None)) monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) - output, _ = self.executor._run_playbook(self.playbook_name, - self.ipaddr, PLAYBOOK_DATA) + output = self.executor._run_playbook(self.playbook_name, + self.ipaddr, self.playbook_vars) # must return playbook output assert output == 'fake output' - # must check syntax - mock_check_syntax.assert_called_once() - # must mask signals assert mock_signal.call_count == 4 # 2 signals masked, then reset # must export ansible vars varsfile = os.path.join(self.artifactsdir.strpath, self.playbook_name, - 'taskotron', 'ansible_vars.json') + 'taskotron', 'task_vars.json') varsfile_internal = os.path.join(self.artifactsdir.strpath, - self.playbook_name, 'taskotron', 'ansible_vars_internal.json') + self.playbook_name, 'taskotron', 'internal_vars.json') for vf in [varsfile, varsfile_internal]: assert os.path.isfile(vf) with open(vf, 'r') as f: @@ -155,16 +150,12 @@ class TestExecutor(): cmd_args = mock_popen.call_args[0][0] assert cmd_args[0] == 'ansible-playbook' assert '--inventory={},'.format(self.ipaddr) in cmd_args - assert '--extra-vars=@{}'.format(varsfile) in cmd_args assert '--extra-vars=@{}'.format(varsfile_internal) in cmd_args assert '--become' in cmd_args assert '--connection=local' in cmd_args def test_run_playbook_failed(self, monkeypatch): '''Should raise when ansible-playbook fails''' - mock_check_syntax = mock.Mock() - monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', - mock_check_syntax) mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) cpe = subprocess.CalledProcessError(returncode=99, cmd='fake') @@ -172,8 +163,8 @@ class TestExecutor(): monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) with pytest.raises(exc.TaskotronError): - self.executor._run_playbook(self.playbook_name, self.ipaddr, - PLAYBOOK_DATA) + self.executor._run_playbook(self.playbook_name, self.ipaddr, + self.playbook_vars) # must unmask signals even when playbook failed assert mock_signal.call_count == 4 # 2 signals masked, then reset @@ -183,9 +174,6 @@ class TestExecutor(): def test_run_playbook_interrupted(self, monkeypatch): '''Should try failsafe stop when interrupted''' - mock_check_syntax = mock.Mock() - monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', - mock_check_syntax) mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) error = exc.TaskotronInterruptError(15, 'SIGTERM') @@ -193,8 +181,8 @@ class TestExecutor(): monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) with pytest.raises(exc.TaskotronInterruptError): - self.executor._run_playbook(self.playbook_name, self.ipaddr, - PLAYBOOK_DATA) + self.executor._run_playbook(self.playbook_name, self.ipaddr, + self.playbook_vars) # must unmask signals even when playbook failed assert mock_signal.call_count == 4 # 2 signals masked, then reset @@ -206,9 +194,6 @@ class TestExecutor(): def test_run_playbook_failsafe_error(self, monkeypatch): '''When failsafe stop gives an error, it should be ignored''' - mock_check_syntax = mock.Mock() - monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', - mock_check_syntax) mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) error = exc.TaskotronInterruptError(15, 'SIGTERM') @@ -218,10 +203,70 @@ class TestExecutor(): with pytest.raises(exc.TaskotronInterruptError): self.executor._run_playbook(self.playbook_name, self.ipaddr, - PLAYBOOK_DATA) + self.playbook_vars) + + def test_run_playbook_forwarded_vars(self, monkeypatch): + '''Only certain vars should get forwarded to task playbook, no other''' + mock_signal = mock.Mock() + monkeypatch.setattr(signal, 'signal', mock_signal) + mock_popen = mock.Mock(return_value=('fake output', None)) + monkeypatch.setattr(os_utils, 'popen_rt', mock_popen) + + self.executor._run_playbook(self.playbook_name, + self.ipaddr, self.playbook_vars) + varsfile = os.path.join(self.artifactsdir.strpath, self.playbook_name, + 'taskotron', 'task_vars.json') + + assert os.path.isfile(varsfile) + with open(varsfile, 'r') as vf: + vars_ = json.load(vf) + + for var in executor.Executor.FORWARDED_VARS: + assert var in vars_ + assert len(vars_) == len(executor.Executor.FORWARDED_VARS) + + def test_get_client_ipaddr_local(self): + '''Local execution''' + self.arg_data['local'] = True + self.arg_data['libvirt'] = False + self.arg_data['ssh'] = False + self.executor = executor.Executor(self.arg_data) + + ipaddr = self.executor._get_client_ipaddr() + + assert ipaddr == '127.0.0.1' + assert self.executor.run_remotely == False + + def test_get_client_ipaddr_libvirt(self): + '''Libvirt execution''' + self.arg_data['local'] = False + self.arg_data['libvirt'] = True + self.arg_data['ssh'] = False + self.executor = executor.Executor(self.arg_data) + + ipaddr = self.executor._get_client_ipaddr() + + assert ipaddr == None + assert self.executor.run_remotely == True + + def test_get_client_ipaddr_ssh(self): + '''Ssh execution''' + self.arg_data['local'] = False + self.arg_data['libvirt'] = False + self.arg_data['ssh'] = True + self.arg_data['machine'] = '127.0.0.2' + self.executor = executor.Executor(self.arg_data) + + ipaddr = self.executor._get_client_ipaddr() + + assert ipaddr == '127.0.0.2' + assert self.executor.run_remotely == True def test_execute_local(self, monkeypatch): '''Execution using local mode''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_spawn_vm = mock.Mock() monkeypatch.setattr(executor.Executor, '_spawn_vm', mock_spawn_vm) mock_run_playbook = mock.Mock(return_value=(None, True)) @@ -230,11 +275,13 @@ class TestExecutor(): mock_report_results = mock.Mock() monkeypatch.setattr(executor.Executor, '_report_results', mock_report_results) - assert self.executor.arg_data['local'] == True + self.executor.ipaddr = '127.0.0.1' + self.executor.run_remotely = False success = self.executor.execute() assert success == True + mock_check_syntax.assert_called_once() mock_spawn_vm.assert_not_called() mock_run_playbook.assert_called_once() assert mock_run_playbook.call_args[0][1] == '127.0.0.1' @@ -242,6 +289,9 @@ class TestExecutor(): def test_execute_ssh(self, monkeypatch): '''Execution using ssh mode''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_spawn_vm = mock.Mock() monkeypatch.setattr(executor.Executor, '_spawn_vm', mock_spawn_vm) mock_run_playbook = mock.Mock(return_value=(None, True)) @@ -250,13 +300,13 @@ class TestExecutor(): mock_report_results = mock.Mock() monkeypatch.setattr(executor.Executor, '_report_results', mock_report_results) - self.executor.arg_data['local'] = False - self.executor.arg_data['ssh'] = True - self.executor.arg_data['machine'] = '127.0.0.2' + self.executor.ipaddr = '127.0.0.2' + self.executor.run_remotely = True success = self.executor.execute() assert success == True + mock_check_syntax.assert_called_once() mock_spawn_vm.assert_not_called() mock_run_playbook.assert_called_once() assert mock_run_playbook.call_args[0][1] == '127.0.0.2' @@ -264,6 +314,9 @@ class TestExecutor(): def test_execute_libvirt(self, monkeypatch): '''Execution using libvirt mode''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_spawn_vm = mock.Mock(return_value='127.0.0.3') monkeypatch.setattr(executor.Executor, '_spawn_vm', mock_spawn_vm) mock_run_playbook = mock.Mock(return_value=(None, True)) @@ -274,12 +327,13 @@ class TestExecutor(): mock_report_results) mock_task_vm = mock.Mock() self.executor.task_vm = mock_task_vm - self.executor.arg_data['local'] = False - self.executor.arg_data['libvirt'] = True + self.executor.ipaddr = None + self.executor.run_remotely = True success = self.executor.execute() assert success == True + mock_check_syntax.assert_called_once() mock_spawn_vm.assert_called_once() mock_run_playbook.assert_called_once() assert mock_run_playbook.call_args[0][1] == '127.0.0.3' @@ -288,6 +342,9 @@ class TestExecutor(): def test_execute_no_playbooks(self, monkeypatch): '''Should raise when there are no playbooks''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_run_playbook = mock.Mock() monkeypatch.setattr(executor.Executor, '_run_playbook', mock_run_playbook) @@ -299,11 +356,15 @@ class TestExecutor(): with pytest.raises(exc.TaskotronError): self.executor.execute() + mock_check_syntax.assert_not_called() mock_run_playbook.assert_not_called() mock_report_results.assert_not_called() def test_execute_more_playbooks(self, monkeypatch): '''Should execute all found playbooks''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_run_playbook = mock.Mock(return_value=(None, True)) monkeypatch.setattr(executor.Executor, '_run_playbook', mock_run_playbook) @@ -315,6 +376,7 @@ class TestExecutor(): success = self.executor.execute() assert success == True + assert mock_check_syntax.call_count == 2 assert mock_run_playbook.call_count == 2 playbooks = [ mock_run_playbook.call_args_list[0][0][0], @@ -332,6 +394,9 @@ class TestExecutor(): def test_execute_error(self, monkeypatch): '''Should raise on playbook errors''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_run_playbook = mock.Mock(side_effect=exc.TaskotronError) monkeypatch.setattr(executor.Executor, '_run_playbook', mock_run_playbook) @@ -342,11 +407,15 @@ class TestExecutor(): success = self.executor.execute() assert success == False + mock_check_syntax.assert_called_once() mock_run_playbook.assert_called_once() mock_report_results.assert_not_called() def test_execute_more_playbooks_error(self, monkeypatch): '''Should execute all found playbooks even if some has error''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_run_playbook = mock.Mock(side_effect=[ exc.TaskotronError, (None, True)]) @@ -360,6 +429,7 @@ class TestExecutor(): success = self.executor.execute() assert success == False + assert mock_check_syntax.call_count == 2 assert mock_run_playbook.call_count == 2 playbooks = [ mock_run_playbook.call_args_list[0][0][0], @@ -376,6 +446,9 @@ class TestExecutor(): def test_execute_sti_no_report(self, monkeypatch): '''Shouldn't try to report results for STI tasks''' self.playbook.write(PLAYBOOK_STI) + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) mock_spawn_vm = mock.Mock() monkeypatch.setattr(executor.Executor, '_spawn_vm', mock_spawn_vm) mock_run_playbook = mock.Mock(return_value=(None, False)) @@ -388,6 +461,7 @@ class TestExecutor(): success = self.executor.execute() assert success == True + mock_check_syntax.assert_called_once() mock_spawn_vm.assert_not_called() mock_run_playbook.assert_called_once() assert mock_run_playbook.call_args[0][1] == '127.0.0.1' @@ -395,6 +469,9 @@ class TestExecutor(): def test_execute_interrupted(self, monkeypatch): '''Should halt execution and return when interrupted''' + mock_check_syntax = mock.Mock() + monkeypatch.setattr(executor.Executor, '_check_playbook_syntax', + mock_check_syntax) error = exc.TaskotronInterruptError(15, 'SIGTERM') mock_run_playbook = mock.Mock(side_effect=error) monkeypatch.setattr(executor.Executor, '_run_playbook', @@ -407,6 +484,7 @@ class TestExecutor(): success = self.executor.execute() assert success == False + mock_check_syntax.assert_called_once() mock_run_playbook.assert_called_once() mock_report_results.assert_not_called() @@ -421,7 +499,7 @@ class TestExecutor(): vm_instance.ipaddr = '10.11.12.13' monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm) - ipaddr = self.executor._spawn_vm(None, PLAYBOOK_DATA) + ipaddr = self.executor._spawn_vm(None, self.playbook_vars) assert ipaddr == '10.11.12.13' assert mock_vm_prepare.call_count == 2 @@ -433,6 +511,28 @@ class TestExecutor(): monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm) with pytest.raises(exc.TaskotronMinionError): - self.executor._spawn_vm(None, PLAYBOOK_DATA) + self.executor._spawn_vm(None, self.playbook_vars) assert mock_vm_prepare.call_count == self.conf.spawn_vm_retries + + def test_load_playbook_accepted_vars(self): + '''All accepted vars must be loaded and none other''' + playbook = yaml.safe_load(PLAYBOOK)[0] + playbook['vars'].clear() + # add all accepted + for var in executor.Executor.ACCEPTED_VARS: + playbook['vars'][var] = 'This is ' + var + # add unaccepted starting with taskotron_ + playbook['vars']['taskotron_unaccepted'] = 'ignore me' + # add unaccepted generic + playbook['vars']['unaccepted'] = 'ignore me' + self.playbook.remove() + self.playbook.write(yaml.safe_dump([playbook])) + + vars_ = self.executor._load_playbook_vars(self.playbook_name) + + for var in executor.Executor.ACCEPTED_VARS: + assert var in vars_ + assert 'taskotron_unaccepted' not in vars_ + assert 'unaccepted' not in vars_ + assert len(vars_) == len(executor.Executor.ACCEPTED_VARS) diff --git a/testing/test_image_utils.py b/testing/test_image_utils.py index f785dec..874dadc 100644 --- a/testing/test_image_utils.py +++ b/testing/test_image_utils.py @@ -5,70 +5,93 @@ '''Unit tests for libtaskotron/image_utils.py''' +import pytest + from libtaskotron.image_utils import devise_environment -from libtaskotron.executor import Playbook_Data from libtaskotron import config class TestDeviseEnvironment: def setup_method(self, method): self.arg_data = { - 'item': 'htop-2.0.2-4.fc27', + 'item': 'htop-2.0.2-4.fc20', 'type': 'koji_build', - 'arch': 'noarch', + 'arch': 'i686', + } + self.playbook_vars = { + 'taskotron_match_host_distro': True, + 'taskotron_match_host_release': True, + 'taskotron_match_host_arch': True, } - self.playbook_data = Playbook_Data({ - 'vars': { - 'taskotron_match_host_distro': True, - 'taskotron_match_host_release': True, - 'taskotron_match_host_arch': False - } - }) + self.cfg = config.get_config() def test_koji_build(self): - env = devise_environment(self.arg_data, self.playbook_data) + env = devise_environment(self.arg_data, self.playbook_vars) assert env['distro'] == 'fedora' - assert env['release'] == '27' - assert env['arch'] == config.get_config().default_disposable_arch + assert env['release'] == '20' + assert env['arch'] == self.arg_data['arch'] def test_koji_tag(self): self.arg_data = { - 'item': 'f27-updates-pending', + 'item': 'f20-updates-pending', 'type': 'koji_tag', - 'arch': 'x86_64', + 'arch': 'i686', } - self.playbook_data.match_host_arch = True - env = devise_environment(self.arg_data, self.playbook_data) + env = devise_environment(self.arg_data, self.playbook_vars) assert env['distro'] == 'fedora' - assert env['release'] == '27' + assert env['release'] == '20' assert env['arch'] == self.arg_data['arch'] def test_unknown_distro(self): self.arg_data['item'] = 'htop-2.0.2-4.el7' - env = devise_environment(self.arg_data, self.playbook_data) - assert env['distro'] == config.get_config().default_disposable_distro - assert env['release'] == config.get_config().default_disposable_release + env = devise_environment(self.arg_data, self.playbook_vars) + assert env['distro'] == self.cfg.default_disposable_distro + assert env['release'] == self.cfg.default_disposable_release + assert env['arch'] == self.arg_data['arch'] def test_match_host_arch_true(self): - self.arg_data = { - 'item': 'htop-2.0.2-4.fc27', - 'type': 'koji_build', - 'arch': 'armhfp', - } - self.playbook_data.match_host_arch = True - env = devise_environment(self.arg_data, self.playbook_data) + env = devise_environment(self.arg_data, self.playbook_vars) assert env['distro'] == 'fedora' - assert env['release'] == '27' - assert env['arch'] == 'armhfp' + assert env['release'] == '20' + assert env['arch'] == self.arg_data['arch'] def test_match_host_arch_false(self): - self.arg_data = { - 'item': 'htop-2.0.2-4.fc27', - 'type': 'koji_build', - 'arch': 'armhfp', - } - self.playbook_data.match_host_arch = False - env = devise_environment(self.arg_data, self.playbook_data) + self.playbook_vars['taskotron_match_host_arch'] = False + env = devise_environment(self.arg_data, self.playbook_vars) assert env['distro'] == 'fedora' - assert env['release'] == '27' - assert env['arch'] == config.get_config().default_disposable_arch + assert env['release'] == '20' + assert env['arch'] == self.cfg.default_disposable_arch + + def test_match_host_release_true(self): + env = devise_environment(self.arg_data, self.playbook_vars) + assert env['distro'] == 'fedora' + assert env['release'] == '20' + assert env['arch'] == self.arg_data['arch'] + + def test_match_host_release_false(self): + self.playbook_vars['taskotron_match_host_release'] = False + env = devise_environment(self.arg_data, self.playbook_vars) + assert env['distro'] == 'fedora' + assert env['release'] == self.cfg.default_disposable_release + assert env['arch'] == self.arg_data['arch'] + + @pytest.mark.parametrize('match_arch', [True, False]) + def test_noarch_to_default(self, match_arch): + self.arg_data['arch'] = 'noarch' + self.playbook_vars['taskotron_match_host_arch'] = match_arch + env = devise_environment(self.arg_data, self.playbook_vars) + assert env['distro'] == 'fedora' + assert env['release'] == '20' + assert env['arch'] == self.cfg.default_disposable_arch + + def test_no_disttag(self): + self.arg_data['item'] = 'htop-2.0.2-4' + env = devise_environment(self.arg_data, self.playbook_vars) + assert env['distro'] == 'fedora' + assert env['release'] == self.cfg.default_disposable_release + assert env['arch'] == self.arg_data['arch'] + + def test_flavor(self): + '''Flavor is always the default one''' + env = devise_environment(self.arg_data, self.playbook_vars) + assert env['flavor'] == self.cfg.default_disposable_flavor diff --git a/testing/test_vm.py b/testing/test_vm.py index bd01b24..f0e35b6 100644 --- a/testing/test_vm.py +++ b/testing/test_vm.py @@ -28,7 +28,8 @@ class TestvmImagePrepare(object): test_vm = vm.TestCloudMachine(self.ref_uuid) with pytest.raises(exc.TaskotronImageError): - test_vm._prepare_image() + test_vm._prepare_image(distro=None, release=None, flavor=None, + arch=None) def should_behave_on_success(self, monkeypatch): stub_image = MagicMock() @@ -38,7 +39,8 @@ class TestvmImagePrepare(object): test_vm = vm.TestCloudMachine(self.ref_uuid) - test_vm._prepare_image() + test_vm._prepare_image(distro=None, release=None, flavor=None, + arch=None) class TestvmInstancePrepare(object): From b1be2b4f186906b14a1e6bfa7357b7a8c1f2843b Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: May 16 2018 14:55:41 +0000 Subject: [PATCH 7/7] trivial tests improvements --- diff --git a/testing/functest_executor.py b/testing/functest_executor.py index 279b38c..a7c2a48 100644 --- a/testing/functest_executor.py +++ b/testing/functest_executor.py @@ -70,6 +70,7 @@ class TestExecutor(): def test_run_playbook_simple(self, monkeypatch): '''Execute a very simple playbook whether everything works''' + # don't override Ctrl+C during testing mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) # it's non-default, but we can't run the test suite as root diff --git a/testing/test_executor.py b/testing/test_executor.py index 527c68f..0bd483f 100644 --- a/testing/test_executor.py +++ b/testing/test_executor.py @@ -138,9 +138,9 @@ class TestExecutor(): # must export ansible vars varsfile = os.path.join(self.artifactsdir.strpath, self.playbook_name, 'taskotron', 'task_vars.json') - varsfile_internal = os.path.join(self.artifactsdir.strpath, + allvarsfile = os.path.join(self.artifactsdir.strpath, self.playbook_name, 'taskotron', 'internal_vars.json') - for vf in [varsfile, varsfile_internal]: + for vf in [varsfile, allvarsfile]: assert os.path.isfile(vf) with open(vf, 'r') as f: json.load(f) @@ -150,7 +150,7 @@ class TestExecutor(): cmd_args = mock_popen.call_args[0][0] assert cmd_args[0] == 'ansible-playbook' assert '--inventory={},'.format(self.ipaddr) in cmd_args - assert '--extra-vars=@{}'.format(varsfile_internal) in cmd_args + assert '--extra-vars=@{}'.format(allvarsfile) in cmd_args assert '--become' in cmd_args assert '--connection=local' in cmd_args @@ -194,6 +194,7 @@ class TestExecutor(): def test_run_playbook_failsafe_error(self, monkeypatch): '''When failsafe stop gives an error, it should be ignored''' + # don't override Ctrl+C during testing mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) error = exc.TaskotronInterruptError(15, 'SIGTERM') @@ -207,6 +208,7 @@ class TestExecutor(): def test_run_playbook_forwarded_vars(self, monkeypatch): '''Only certain vars should get forwarded to task playbook, no other''' + # don't override Ctrl+C during testing mock_signal = mock.Mock() monkeypatch.setattr(signal, 'signal', mock_signal) mock_popen = mock.Mock(return_value=('fake output', None))