From 6ccbcc22e4c19183a7c90f3312cdf3fa788920a9 Mon Sep 17 00:00:00 2001
From: Lukas Brabec <lbrabec@redhat.com>
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 <kparal@redhat.com>
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 <kparal@redhat.com>
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 <kparal@redhat.com>
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 <kparal@redhat.com>
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 <kparal@redhat.com>
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 <kparal@redhat.com>
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))

