From 5ffce2e33b8f57287946dca7ef17ce02e6424fe4 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 1/17] Basic functionality for save_failed_tree plugin


---

diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
new file mode 100644
index 0000000..784bf20
--- /dev/null
+++ b/plugins/builder/save_failed_tree.py
@@ -0,0 +1,40 @@
+import fnmatch
+import os
+import tarfile
+import koji.tasks as tasks
+from __main__ import BuildRoot
+
+__all__ = ('SaveFailedTreeTask',)
+
+def omit_ccache(tarinfo):
+    if fnmatch.fnmatch(tarinfo.name, '*/tmp/krb5cc') or \
+       fnmatch.fnmatch(tarinfo.name, '*/etc/*.keytab'):
+        return None
+    else:
+        return tarinfo
+
+
+
+class SaveFailedTreeTask(tasks.BaseTaskHandler):
+    Methods = ['saveFailedTree']
+    _taskWeight = 3.0
+
+
+    def handler(self, taskID, full=False):
+        self.logger.debug("Starting saving buildroots for task %d [full=%s]" % (taskID, full))
+        tar_path = os.path.join(self.workdir, 'broots-task-%s.tar.gz' % taskID)
+        f = tarfile.open(tar_path, "w:gz")
+        for broot in self.session.listBuildroots(taskID=taskID):
+            broot = BuildRoot(self.session, self.options, broot['id'])
+            path = broot.rootdir()
+            if full:
+                self.logger.debug("Adding buildroot (full): %s" % path)
+            else:
+                path = os.path.join(path, 'builddir')
+                self.logger.debug("Adding buildroot: %s" % path)
+            f.add(path, filter=omit_ccache)
+        f.close()
+        self.logger.debug("Uploading %s to hub." % tar_path)
+        self.uploadFile(tar_path)
+        os.unlink(tar_path)
+        self.logger.debug("Finished saving buildroots for task %d" % taskID)
diff --git a/plugins/hub/save_failed_tree.py b/plugins/hub/save_failed_tree.py
new file mode 100644
index 0000000..9eccca6
--- /dev/null
+++ b/plugins/hub/save_failed_tree.py
@@ -0,0 +1,29 @@
+import koji
+from koji.plugin import export
+
+import sys
+sys.path.insert(0, '/usr/share/koji-hub/')
+import kojihub
+
+__all__ = ('saveFailedTree',)
+
+@export
+def saveFailedTree(taskID, full=False, **opts):
+    # let it raise errors
+    taskID = int(taskID)
+    full = bool(full)
+
+    task_info = kojihub.Task(taskID).getInfo()
+    if task_info['state'] != koji.TASK_STATES['FAILED']:
+        return 'Task %s has not failed.' % taskID
+    elif task_info['method'] != 'buildArch':
+        # TODO: allowed tasks could be defined in plugin hub config
+        return 'Only buildArch tasks can upload buildroot (Task %(id)s is %(method)s).' % task_info
+    # owner?
+    # permissions?
+
+    args = koji.encode_args(taskID, full, **opts)
+    taskopts = {
+        'assign': task_info['host_id'],
+    }
+    return kojihub.make_task('saveFailedTree', args, **taskopts)

From 86c1127009787961fcaa4b176cbc0752e6310d43 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 2/17] Basic docs for save_failed_tree plugin


---

diff --git a/docs/source/index.rst b/docs/source/index.rst
index 6de3165..9a6539e 100644
--- a/docs/source/index.rst
+++ b/docs/source/index.rst
@@ -31,6 +31,7 @@ Contents
     server_bootstrap
     server_howto
     using_the_koji_build_system
+    plugins
     writing_a_plugin
     writing_koji_code
     content_generators
diff --git a/docs/source/plugins.rst b/docs/source/plugins.rst
new file mode 100644
index 0000000..432941b
--- /dev/null
+++ b/docs/source/plugins.rst
@@ -0,0 +1,56 @@
+=======
+Plugins
+=======
+
+Following plugins are available in default koji installation.
+
+Runroot
+=======
+
+Plugin for running any command in buildroot.
+
+Save Failed Tree Plugin
+=======================
+
+In some cases developers want to investigate exact environment in which their
+build failed. Reconstructing this environment via mock needn't end with
+exactly same structure (due to builder settings, etc.). In such case this
+plugin can be used to retrieve tarball with complete mock tree.
+
+.. warning::
+  For security reasons, currently all ``/tmp/krb5cc*`` and ``/etc/*.keytab``
+  files are removed from tarball. If we found some other dangerous pieces,
+  they can be added to this blacklist.
+
+Special task method is created for achieving this which is called
+``SaveFailedTree``. This task can be created via CLI:
+``koji save-failed-tree <taskID>``. Additional options are:
+
+.. option:: --full
+
+   directs koji to create tarball with complete tree.
+
+.. option:: --nowait
+
+   exit immediately after creating task
+
+.. option:: --quiet
+
+   don't print any information to output
+
+After task finishes, one can find the tarball on relevant task web page (URL
+will be printed to stdout until ``--quiet`` is used.
+
+Currently plugin allow to save trees only for ``buildArch`` tasks and anybody
+is allowed to create this type of task (and download tarball).
+
+.. warning::
+  Don't forget that this type of task can generate huge amount of data, so use
+  it wisely.
+
+TODO
+----
+ * Make allowed task types configurable on hub
+ * Restricted access (original owner, special permission, ...)
+ * Separate volume/directory on hub
+ * garbage collector + policy for retaining generated tarballs

From c3607435adacc7894452da3b350f0bc08d0a5a94 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 3/17] CLI for save_failed_tree


---

diff --git a/cli/koji b/cli/koji
index 6eb48f3..ca9a8a6 100755
--- a/cli/koji
+++ b/cli/koji
@@ -7202,6 +7202,52 @@ def handle_runroot(options, session, args):
         sys.exit(1)
     return
 
+def handle_save_failed_tree(options, session, args):
+    "Create tarball with whole buildtree"
+    usage = _("usage: %prog save-failed-tree [options] taskID")
+    usage += _("\n(Specify the --help global option for a list of other help options)")
+    parser = OptionParser(usage=usage)
+    parser.disable_interspersed_args()
+    parser.add_option("-f", "--full", action="store_true", default=False,
+            help=_("Download whole tree, if not specified, only builddir will be downloaded"))
+    parser.add_option("--quiet", action="store_true",
+                      help=_("Do not print the task information"), default=options.quiet)
+    parser.add_option("--nowait", action="store_true",
+                      help=_("Don't wait on build"))
+
+    (opts, args) = parser.parse_args(args)
+
+    if len(args) != 1:
+        parser.error(_("List exactly one taskID"))
+
+    try:
+        taskID = int(args[0])
+    except ValueError:
+        parser.error(_("Task ID must be an integer."))
+
+    activate_session(session)
+    try:
+        task_id = session.saveFailedTree(taskID, opts.full)
+    except koji.GenericError as e:
+        if 'Invalid method' in str(e):
+            print "* The save_failed_tree plugin appears to not be installed" \
+                  " on the koji hub.  Please contact the administrator."
+        raise
+
+    if type(task_id) != int:
+        print 'Error: %s' % task_id
+        return
+
+    if not opts.quiet:
+        print "Created task:", task_id
+        print "Task info: %s/taskinfo?taskID=%s" % (options.weburl, task_id)
+
+    if opts.nowait:
+        return
+    else:
+        session.logout()
+        watch_tasks(session, [task_id], quiet=opts.quiet)
+
 
 def handle_help(options, session, args):
     "[info] List available commands"
diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index 784bf20..223e821 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -6,6 +6,7 @@ from __main__ import BuildRoot
 
 __all__ = ('SaveFailedTreeTask',)
 
+
 def omit_ccache(tarinfo):
     if fnmatch.fnmatch(tarinfo.name, '*/tmp/krb5cc') or \
        fnmatch.fnmatch(tarinfo.name, '*/etc/*.keytab'):
@@ -14,12 +15,10 @@ def omit_ccache(tarinfo):
         return tarinfo
 
 
-
 class SaveFailedTreeTask(tasks.BaseTaskHandler):
     Methods = ['saveFailedTree']
     _taskWeight = 3.0
 
-
     def handler(self, taskID, full=False):
         self.logger.debug("Starting saving buildroots for task %d [full=%s]" % (taskID, full))
         tar_path = os.path.join(self.workdir, 'broots-task-%s.tar.gz' % taskID)
diff --git a/plugins/hub/save_failed_tree.py b/plugins/hub/save_failed_tree.py
index 9eccca6..7f98602 100644
--- a/plugins/hub/save_failed_tree.py
+++ b/plugins/hub/save_failed_tree.py
@@ -1,14 +1,19 @@
+import sys
 import koji
 from koji.plugin import export
 
-import sys
 sys.path.insert(0, '/usr/share/koji-hub/')
 import kojihub
 
 __all__ = ('saveFailedTree',)
 
+
 @export
 def saveFailedTree(taskID, full=False, **opts):
+    '''xmlrpc method for creating saveFailedTree task. If arguments are
+    invalid, error message is returned. Otherwise task id of newly created
+    task is returned.'''
+
     # let it raise errors
     taskID = int(taskID)
     full = bool(full)
diff --git a/tests/test_cli/data/list-commands.txt b/tests/test_cli/data/list-commands.txt
index 7a4d8ce..6249a73 100644
--- a/tests/test_cli/data/list-commands.txt
+++ b/tests/test_cli/data/list-commands.txt
@@ -118,6 +118,7 @@ miscellaneous commands:
         call                      Execute an arbitrary XML-RPC call
         import-comps              Import group/package information from a comps file
         moshimoshi                Introduce yourself
+        save-failed-tree          Create tarball with whole buildtree
 
 monitor commands:
         wait-repo                 Wait for a repo to be regenerated
diff --git a/tests/test_cli/test_save_failed_tree.py b/tests/test_cli/test_save_failed_tree.py
new file mode 100644
index 0000000..90488fe
--- /dev/null
+++ b/tests/test_cli/test_save_failed_tree.py
@@ -0,0 +1,128 @@
+import StringIO
+import unittest
+import koji
+import mock
+
+import loadcli
+cli = loadcli.cli
+
+
+class TestSaveFailedTree(unittest.TestCase):
+    def setUp(self):
+        self.options = mock.MagicMock()
+        self.session = mock.MagicMock()
+        self.args = mock.MagicMock()
+        self.original_parser = cli.OptionParser
+        cli.OptionParser = mock.MagicMock()
+        self.parser = cli.OptionParser.return_value
+        cli.options = self.options  # globals!!!
+
+    def tearDown(self):
+        cli.OptionParser = self.original_parser
+
+    # Show long diffs in error output...
+    maxDiff = None
+
+    @mock.patch('koji_cli.activate_session')
+    def test_handle_save_failed_tree_simple(self, activate_session_mock):
+        # koji save-failed-tree 123456
+        task_id = 123456
+        arguments = [task_id]
+        options = mock.MagicMock()
+        options.full = False
+        options.nowait = True
+        self.parser.parse_args.return_value = [options, arguments]
+        self.session.getAPIVersion.return_value = koji.API_VERSION
+
+        # Mock out the xmlrpc server
+        self.session.saveFailedTree.return_value = 123
+
+        # Run it and check immediate output
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+
+        # Finally, assert that things were called as we expected.
+        activate_session_mock.assert_called_once_with(self.session)
+        self.session.saveFailedTree.assert_called_once_with(task_id, options.full)
+
+    @mock.patch('koji_cli.activate_session')
+    def test_handle_save_failed_tree_full(self, activate_session_mock):
+        # koji save-failed-tree 123456 --full
+        task_id = 123456
+        arguments = [task_id]
+        options = mock.MagicMock()
+        options.full = True
+        options.nowait = True
+        self.parser.parse_args.return_value = [options, arguments]
+        self.session.getAPIVersion.return_value = koji.API_VERSION
+
+        # Mock out the xmlrpc server
+        self.session.saveFailedTree.return_value = 123
+
+        # Run it and check immediate output
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+
+        # Finally, assert that things were called as we expected.
+        activate_session_mock.assert_called_once_with(self.session)
+        self.session.saveFailedTree.assert_called_once_with(task_id, options.full)
+
+    @mock.patch('koji_cli.activate_session')
+    @mock.patch('koji_cli.watch_tasks')
+    def test_handle_save_failed_tree_wait(self, watch_tasks_mock, activate_session_mock):
+        # koji save-failed-tree 123456 --full
+        task_id = 123456
+        arguments = [task_id]
+        options = mock.MagicMock()
+        options.full = True
+        options.nowait = False
+        options.quiet = False
+        self.parser.parse_args.return_value = [options, arguments]
+        self.session.getAPIVersion.return_value = koji.API_VERSION
+
+        # Mock out the xmlrpc server
+        spawned_id = 123
+        self.session.saveFailedTree.return_value = spawned_id
+
+        # Run it and check immediate output
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+
+        # Finally, assert that things were called as we expected.
+        self.session.saveFailedTree.assert_called_once_with(task_id, options.full)
+        activate_session_mock.assert_called_once_with(self.session)
+        self.session.logout.assert_called_once_with()
+        watch_tasks_mock.assert_called_once_with(self.session, [spawned_id],
+                                                 quiet=options.quiet)
+
+    @mock.patch('sys.stdout', new_callable=StringIO.StringIO)
+    @mock.patch('koji_cli.activate_session')
+    @mock.patch('koji_cli.watch_tasks')
+    def test_handle_save_failed_tree_errors(self, watch_tasks_mock, activate_session_mock, stdout):
+        # koji save-failed-tree 123 456
+        arguments = [123, 456]
+        options = mock.MagicMock()
+        self.parser.parse_args.return_value = [options, arguments]
+        self.parser.error.side_effect = Exception()
+        self.session.getAPIVersion.return_value = koji.API_VERSION
+
+        self.assertRaises(Exception, cli.handle_save_failed_tree,
+                          self.options, self.session, self.args)
+
+        arguments = ["text"]
+        self.parser.parse_args.return_value = [options, arguments]
+        self.assertRaises(Exception, cli.handle_save_failed_tree, self.options,
+                          self.session, self.args)
+
+        # plugin not installed
+        arguments = [123]
+        self.parser.parse_args.return_value = [options, arguments]
+        self.session.saveFailedTree.side_effect = koji.GenericError("Invalid method")
+        self.assertRaises(koji.GenericError, cli.handle_save_failed_tree,
+                          self.options, self.session, self.args)
+
+        # something wrong happened in task
+        stdout.seek(0)
+        stdout.truncate()
+        self.session.saveFailedTree.return_value = 'xyz'
+        self.session.saveFailedTree.side_effect = None
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+        actual = stdout.getvalue()
+        self.assertEqual(actual, 'Error: xyz\n')

From 54bcd8cfa248b889285b62b32325d99915443724 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 4/17] Define tasks via config file


---

diff --git a/docs/source/plugins.rst b/docs/source/plugins.rst
index 432941b..b3d98ad 100644
--- a/docs/source/plugins.rst
+++ b/docs/source/plugins.rst
@@ -41,7 +41,10 @@ Special task method is created for achieving this which is called
 After task finishes, one can find the tarball on relevant task web page (URL
 will be printed to stdout until ``--quiet`` is used.
 
-Currently plugin allow to save trees only for ``buildArch`` tasks and anybody
+Plugin allow to save trees only for tasks defined in config
+``/etc/koji-hub/plugins/save_failed_tree.conf``. Option
+``allowed_methods`` contains list of comma-delimited names of tasks. Default
+configuration contains line: ``allowed_methods = buildArch``. Anybody
 is allowed to create this type of task (and download tarball).
 
 .. warning::
diff --git a/plugins/hub/save_failed_tree.conf b/plugins/hub/save_failed_tree.conf
new file mode 100644
index 0000000..af31ad7
--- /dev/null
+++ b/plugins/hub/save_failed_tree.conf
@@ -0,0 +1,7 @@
+# config file for the Koji save-failed-trees plugin
+
+[permissions]
+# task methods for whose can be triggered buildroot export
+# * can be used to allow everything. In such case it must be only component
+# on line. Otherwise multiple values are delimited by comma.
+allowed_methods = buildArch
diff --git a/plugins/hub/save_failed_tree.py b/plugins/hub/save_failed_tree.py
index 7f98602..02f6838 100644
--- a/plugins/hub/save_failed_tree.py
+++ b/plugins/hub/save_failed_tree.py
@@ -1,4 +1,5 @@
 import sys
+import ConfigParser
 import koji
 from koji.plugin import export
 
@@ -7,23 +8,36 @@ import kojihub
 
 __all__ = ('saveFailedTree',)
 
+CONFIG_FILE = '/etc/koji-hub/plugins/save_failed_tree.conf'
+config = None
+allowed_methods = None
+
 
 @export
 def saveFailedTree(taskID, full=False, **opts):
     '''xmlrpc method for creating saveFailedTree task. If arguments are
     invalid, error message is returned. Otherwise task id of newly created
     task is returned.'''
+    global config, allowed_methods
 
     # let it raise errors
     taskID = int(taskID)
     full = bool(full)
 
+    # read configuration only once
+    if config is None:
+        config = ConfigParser.SafeConfigParser()
+        config.read(CONFIG_FILE)
+        allowed_methods = config.get('permissions', 'allowed_methods').split()
+        if len(allowed_methods) == 1 and allowed_methods[0] == '*':
+            allowed_methods = '*'
+
     task_info = kojihub.Task(taskID).getInfo()
     if task_info['state'] != koji.TASK_STATES['FAILED']:
-        return 'Task %s has not failed.' % taskID
-    elif task_info['method'] != 'buildArch':
-        # TODO: allowed tasks could be defined in plugin hub config
-        return 'Only buildArch tasks can upload buildroot (Task %(id)s is %(method)s).' % task_info
+        return 'Task %s has not failed. Only failed tasks can upload their buildroots.' % taskID
+    elif allowed_methods != '*' and task_info['method'] not in allowed_methods:
+        return 'Only %s tasks can upload their buildroots (Task %s is %s).' % \
+               (', '.join(allowed_methods), task_info['id'], task_info['method'])
     # owner?
     # permissions?
 

From e1ac33793e3be92558c392cf0c7e889d94ce062c Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 5/17] permissions


---

diff --git a/cli/koji b/cli/koji
index ca9a8a6..859758f 100755
--- a/cli/koji
+++ b/cli/koji
@@ -7229,15 +7229,22 @@ def handle_save_failed_tree(options, session, args):
     try:
         task_id = session.saveFailedTree(taskID, opts.full)
     except koji.GenericError as e:
-        if 'Invalid method' in str(e):
-            print "* The save_failed_tree plugin appears to not be installed" \
-                  " on the koji hub.  Please contact the administrator."
-        raise
-
-    if type(task_id) != int:
-        print 'Error: %s' % task_id
-        return
-
+        m = str(e)
+        if 'Only failed tasks can upload their buildroots.' in m:
+            print _("Only failed tasks can upload their buildroots.")
+        elif 'tasks can upload their buildroots (Task' in m:
+            print _("Task of this type has disabled support for uploading" \
+                    " buildroot. (configurable on hub)")
+        elif 'Invalid method' in m:
+            print _("* The save_failed_tree plugin appears to not be installed" \
+                    " on the koji hub.  Please contact the administrator.")
+        if logger.isEnabledFor(logging.DEBUG):
+            tb_str = ''.join(traceback.format_exception(*sys.exc_info()))
+            logger.debug(tb_str)
+        return 1
+    except koji.ActionNotAllowed:
+        print _("Only task owner or admin can run this task.")
+        return 1
     if not opts.quiet:
         print "Created task:", task_id
         print "Task info: %s/taskinfo?taskID=%s" % (options.weburl, task_id)
diff --git a/plugins/hub/save_failed_tree.py b/plugins/hub/save_failed_tree.py
index 02f6838..c818632 100644
--- a/plugins/hub/save_failed_tree.py
+++ b/plugins/hub/save_failed_tree.py
@@ -1,6 +1,7 @@
 import sys
 import ConfigParser
 import koji
+from koji.context import context
 from koji.plugin import export
 
 sys.path.insert(0, '/usr/share/koji-hub/')
@@ -34,12 +35,12 @@ def saveFailedTree(taskID, full=False, **opts):
 
     task_info = kojihub.Task(taskID).getInfo()
     if task_info['state'] != koji.TASK_STATES['FAILED']:
-        return 'Task %s has not failed. Only failed tasks can upload their buildroots.' % taskID
+        raise koji.PreBuildError, 'Task %s has not failed. Only failed tasks can upload their buildroots.' % taskID
     elif allowed_methods != '*' and task_info['method'] not in allowed_methods:
-        return 'Only %s tasks can upload their buildroots (Task %s is %s).' % \
+        raise koji.PreBuildError, 'Only %s tasks can upload their buildroots (Task %s is %s).' % \
                (', '.join(allowed_methods), task_info['id'], task_info['method'])
-    # owner?
-    # permissions?
+    elif task_info["owner"] != context.session.user_id and not context.session.assertPerm('admin'):
+        raise koji.ActionNotAllowed, "only owner of failed task or 'admin' can run this task"
 
     args = koji.encode_args(taskID, full, **opts)
     taskopts = {
diff --git a/tests/test_cli/test_save_failed_tree.py b/tests/test_cli/test_save_failed_tree.py
index 90488fe..b2ad7df 100644
--- a/tests/test_cli/test_save_failed_tree.py
+++ b/tests/test_cli/test_save_failed_tree.py
@@ -110,19 +110,28 @@ class TestSaveFailedTree(unittest.TestCase):
         self.parser.parse_args.return_value = [options, arguments]
         self.assertRaises(Exception, cli.handle_save_failed_tree, self.options,
                           self.session, self.args)
+        cli.logger = mock.MagicMock()
 
         # plugin not installed
         arguments = [123]
         self.parser.parse_args.return_value = [options, arguments]
         self.session.saveFailedTree.side_effect = koji.GenericError("Invalid method")
-        self.assertRaises(koji.GenericError, cli.handle_save_failed_tree,
-                          self.options, self.session, self.args)
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+        actual = stdout.getvalue()
+        self.assertTrue('The save_failed_tree plugin appears to not be installed' in actual)
+
+        # Task which is not FAILED
+        stdout.seek(0)
+        stdout.truncate()
+        self.session.saveFailedTree.side_effect = koji.PreBuildError('Only failed tasks can upload their buildroots.')
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+        actual = stdout.getvalue()
+        self.assertTrue('Only failed tasks can upload their buildroots.' in actual)
 
-        # something wrong happened in task
+        # Disabled/unsupported task
         stdout.seek(0)
         stdout.truncate()
-        self.session.saveFailedTree.return_value = 'xyz'
-        self.session.saveFailedTree.side_effect = None
+        self.session.saveFailedTree.side_effect = koji.PreBuildError('tasks can upload their buildroots (Task')
         cli.handle_save_failed_tree(self.options, self.session, self.args)
         actual = stdout.getvalue()
-        self.assertEqual(actual, 'Error: xyz\n')
+        self.assertTrue('Task of this type has disabled support for uploading' in actual)

From 6f76fc88f9270d4db3900267c556ac08d188b2b5 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 6/17] make stripped paths configurable


---

diff --git a/docs/source/plugins.rst b/docs/source/plugins.rst
index b3d98ad..b3c9572 100644
--- a/docs/source/plugins.rst
+++ b/docs/source/plugins.rst
@@ -17,6 +17,17 @@ build failed. Reconstructing this environment via mock needn't end with
 exactly same structure (due to builder settings, etc.). In such case this
 plugin can be used to retrieve tarball with complete mock tree.
 
+Additional feature is that some paths from buildroot can be left out from
+tarball. Feature can be configured via
+`/etc/kojid/plugins/save_failed_tree.conf` file. Currently only field
+filters.paths is used and it consists of globs (standard python's fnmatch is
+used) separated by ':'.
+
+.. code-block:: ini
+
+  [filters]
+  paths = /etc/*.keytab:/tmp/secret_data
+
 .. warning::
   For security reasons, currently all ``/tmp/krb5cc*`` and ``/etc/*.keytab``
   files are removed from tarball. If we found some other dangerous pieces,
diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index 223e821..4b82b9a 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -1,19 +1,30 @@
 import fnmatch
 import os
 import tarfile
+import ConfigParser
 import koji.tasks as tasks
 from __main__ import BuildRoot
 
 __all__ = ('SaveFailedTreeTask',)
 
+CONFIG_FILE = '/etc/kojid/plugins/save_failed_tree.conf'
+config = None
 
-def omit_ccache(tarinfo):
-    if fnmatch.fnmatch(tarinfo.name, '*/tmp/krb5cc') or \
-       fnmatch.fnmatch(tarinfo.name, '*/etc/*.keytab'):
+def omit_paths(tarinfo):
+    if any([fnmatch.fnmatch(tarinfo.name, f) for f in config['path_filters']]):
         return None
     else:
         return tarinfo
 
+def read_config():
+    global config
+    cp = ConfigParser.SafeConfigParser()
+    cp.read(CONFIG_FILE)
+    config = {
+        'path_filters': [],
+    }
+    if cp.has_option('filters', 'paths'):
+        config['path_filters'] = cp.get('filters', 'paths').split(':')
 
 class SaveFailedTreeTask(tasks.BaseTaskHandler):
     Methods = ['saveFailedTree']
@@ -21,6 +32,7 @@ class SaveFailedTreeTask(tasks.BaseTaskHandler):
 
     def handler(self, taskID, full=False):
         self.logger.debug("Starting saving buildroots for task %d [full=%s]" % (taskID, full))
+        read_config()
         tar_path = os.path.join(self.workdir, 'broots-task-%s.tar.gz' % taskID)
         f = tarfile.open(tar_path, "w:gz")
         for broot in self.session.listBuildroots(taskID=taskID):
@@ -31,7 +43,7 @@ class SaveFailedTreeTask(tasks.BaseTaskHandler):
             else:
                 path = os.path.join(path, 'builddir')
                 self.logger.debug("Adding buildroot: %s" % path)
-            f.add(path, filter=omit_ccache)
+            f.add(path, filter=omit_paths)
         f.close()
         self.logger.debug("Uploading %s to hub." % tar_path)
         self.uploadFile(tar_path)

From eee4ace19a0b6287470301cb9e0acb09b74bac20 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 7/17] removing already done TODOs


---

diff --git a/docs/source/plugins.rst b/docs/source/plugins.rst
index b3c9572..d3c29f0 100644
--- a/docs/source/plugins.rst
+++ b/docs/source/plugins.rst
@@ -64,7 +64,5 @@ is allowed to create this type of task (and download tarball).
 
 TODO
 ----
- * Make allowed task types configurable on hub
- * Restricted access (original owner, special permission, ...)
  * Separate volume/directory on hub
  * garbage collector + policy for retaining generated tarballs

From 087a0aa33966463fe411ba0eb3c48f55bdb8c337 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 8/17] config file


---

diff --git a/plugins/builder/save_failed_tree.conf b/plugins/builder/save_failed_tree.conf
new file mode 100644
index 0000000..5e16d4c
--- /dev/null
+++ b/plugins/builder/save_failed_tree.conf
@@ -0,0 +1,2 @@
+[filters]
+paths = */tmp/krb5cc:*/etc/*.keytab

From 06c5257d4caee6819046916b4f3a775eb1165871 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:40:17 +0000
Subject: [PATCH 9/17] use print function everywhere


---

diff --git a/cli/koji b/cli/koji
index 859758f..88c5c22 100755
--- a/cli/koji
+++ b/cli/koji
@@ -7231,23 +7231,23 @@ def handle_save_failed_tree(options, session, args):
     except koji.GenericError as e:
         m = str(e)
         if 'Only failed tasks can upload their buildroots.' in m:
-            print _("Only failed tasks can upload their buildroots.")
+            print(_("Only failed tasks can upload their buildroots."))
         elif 'tasks can upload their buildroots (Task' in m:
-            print _("Task of this type has disabled support for uploading" \
-                    " buildroot. (configurable on hub)")
+            print(_("Task of this type has disabled support for uploading" \
+                    " buildroot. (configurable on hub)"))
         elif 'Invalid method' in m:
-            print _("* The save_failed_tree plugin appears to not be installed" \
-                    " on the koji hub.  Please contact the administrator.")
+            print(_("* The save_failed_tree plugin appears to not be installed" \
+                    " on the koji hub.  Please contact the administrator."))
         if logger.isEnabledFor(logging.DEBUG):
             tb_str = ''.join(traceback.format_exception(*sys.exc_info()))
             logger.debug(tb_str)
         return 1
     except koji.ActionNotAllowed:
-        print _("Only task owner or admin can run this task.")
+        print(_("Only task owner or admin can run this task."))
         return 1
     if not opts.quiet:
-        print "Created task:", task_id
-        print "Task info: %s/taskinfo?taskID=%s" % (options.weburl, task_id)
+        print("Created task:", task_id)
+        print("Task info: %s/taskinfo?taskID=%s" % (options.weburl, task_id))
 
     if opts.nowait:
         return

From 003cc8ec8c0272ede7812518b7e889a0b85f665f Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:41:42 +0000
Subject: [PATCH 10/17] utilize multiple volume support


---

diff --git a/plugins/builder/save_failed_tree.conf b/plugins/builder/save_failed_tree.conf
index 5e16d4c..84df846 100644
--- a/plugins/builder/save_failed_tree.conf
+++ b/plugins/builder/save_failed_tree.conf
@@ -1,2 +1,5 @@
+[global]
+volume = DEFAULT
+
 [filters]
 paths = */tmp/krb5cc:*/etc/*.keytab
diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index 4b82b9a..d1b618a 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -22,9 +22,12 @@ def read_config():
     cp.read(CONFIG_FILE)
     config = {
         'path_filters': [],
+        'volume': None,
     }
     if cp.has_option('filters', 'paths'):
         config['path_filters'] = cp.get('filters', 'paths').split(':')
+    if cp.has_option('general', 'volume'):
+        config['volume'] = cp.get('general', 'volume').strip()
 
 class SaveFailedTreeTask(tasks.BaseTaskHandler):
     Methods = ['saveFailedTree']
@@ -46,6 +49,6 @@ class SaveFailedTreeTask(tasks.BaseTaskHandler):
             f.add(path, filter=omit_paths)
         f.close()
         self.logger.debug("Uploading %s to hub." % tar_path)
-        self.uploadFile(tar_path)
+        self.uploadFile(tar_path, volume=config['volume'])
         os.unlink(tar_path)
         self.logger.debug("Finished saving buildroots for task %d" % taskID)

From e97c2d0599fbbff2d9dc9dc53d555f7b3cc23812 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:48:13 +0000
Subject: [PATCH 11/17] remove unnecessary code


---

diff --git a/cli/koji b/cli/koji
index 88c5c22..545d5be 100755
--- a/cli/koji
+++ b/cli/koji
@@ -7207,7 +7207,6 @@ def handle_save_failed_tree(options, session, args):
     usage = _("usage: %prog save-failed-tree [options] taskID")
     usage += _("\n(Specify the --help global option for a list of other help options)")
     parser = OptionParser(usage=usage)
-    parser.disable_interspersed_args()
     parser.add_option("-f", "--full", action="store_true", default=False,
             help=_("Download whole tree, if not specified, only builddir will be downloaded"))
     parser.add_option("--quiet", action="store_true",

From 18e9921361a8bcfd690d7eacf4c4b9f7c38e51dd Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 12:48:39 +0000
Subject: [PATCH 12/17] handle unknown exception in main


---

diff --git a/cli/koji b/cli/koji
index 545d5be..5d782ad 100755
--- a/cli/koji
+++ b/cli/koji
@@ -7237,9 +7237,8 @@ def handle_save_failed_tree(options, session, args):
         elif 'Invalid method' in m:
             print(_("* The save_failed_tree plugin appears to not be installed" \
                     " on the koji hub.  Please contact the administrator."))
-        if logger.isEnabledFor(logging.DEBUG):
-            tb_str = ''.join(traceback.format_exception(*sys.exc_info()))
-            logger.debug(tb_str)
+        else:
+            raise
         return 1
     except koji.ActionNotAllowed:
         print(_("Only task owner or admin can run this task."))

From a4179a97e5c5200cfc433792a61fe8617323374f Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 14:04:40 +0000
Subject: [PATCH 13/17] Fail if host is disabled


---

diff --git a/plugins/hub/save_failed_tree.py b/plugins/hub/save_failed_tree.py
index c818632..46ee865 100644
--- a/plugins/hub/save_failed_tree.py
+++ b/plugins/hub/save_failed_tree.py
@@ -16,9 +16,10 @@ allowed_methods = None
 
 @export
 def saveFailedTree(taskID, full=False, **opts):
-    '''xmlrpc method for creating saveFailedTree task. If arguments are
-    invalid, error message is returned. Otherwise task id of newly created
-    task is returned.'''
+    """Create saveFailedTree task
+
+    If arguments are invalid, error message is returned. Otherwise task id of
+    newly created task is returned."""
     global config, allowed_methods
 
     # let it raise errors
@@ -35,12 +36,14 @@ def saveFailedTree(taskID, full=False, **opts):
 
     task_info = kojihub.Task(taskID).getInfo()
     if task_info['state'] != koji.TASK_STATES['FAILED']:
-        raise koji.PreBuildError, 'Task %s has not failed. Only failed tasks can upload their buildroots.' % taskID
+        raise koji.PreBuildError("Task %s has not failed. Only failed tasks can upload their buildroots." % taskID)
     elif allowed_methods != '*' and task_info['method'] not in allowed_methods:
-        raise koji.PreBuildError, 'Only %s tasks can upload their buildroots (Task %s is %s).' % \
-               (', '.join(allowed_methods), task_info['id'], task_info['method'])
+        raise koji.PreBuildError("Only %s tasks can upload their buildroots (Task %s is %s)." % \
+               (', '.join(allowed_methods), task_info['id'], task_info['method']))
     elif task_info["owner"] != context.session.user_id and not context.session.assertPerm('admin'):
-        raise koji.ActionNotAllowed, "only owner of failed task or 'admin' can run this task"
+        raise koji.ActionNotAllowed("Only owner of failed task or 'admin' can run this task.")
+    elif not kojihub.get_host(task_info['host_id'])['enabled']:
+        raise koji.PreBuildError("Host is disabled.")
 
     args = koji.encode_args(taskID, full, **opts)
     taskopts = {

From 5078a93b45193b05810c9c8814d46cc39b4f3510 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 14:33:37 +0000
Subject: [PATCH 14/17] check that correct builder is used


---

diff --git a/Makefile b/Makefile
index 9a0bd34..bc8767f 100644
--- a/Makefile
+++ b/Makefile
@@ -66,7 +66,7 @@ git-clean:
 
 test:
 	coverage erase
-	PYTHONPATH=hub/.:plugins/hub/. nosetests --with-coverage --cover-package .
+	PYTHONPATH=hub/.:plugins/hub/.:plugins/builder/. nosetests --with-coverage --cover-package .
 	coverage html
 	@echo Coverage report in htmlcov/index.html
 
diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index d1b618a..f5fec8a 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -2,6 +2,8 @@ import fnmatch
 import os
 import tarfile
 import ConfigParser
+
+import koji
 import koji.tasks as tasks
 from __main__ import BuildRoot
 
@@ -38,7 +40,10 @@ class SaveFailedTreeTask(tasks.BaseTaskHandler):
         read_config()
         tar_path = os.path.join(self.workdir, 'broots-task-%s.tar.gz' % taskID)
         f = tarfile.open(tar_path, "w:gz")
+        host_id = self.session.host.getHost()['id']
         for broot in self.session.listBuildroots(taskID=taskID):
+            if broot['host_id'] != host_id:
+                raise koji.GenericError("Task is run on wrong builder.")
             broot = BuildRoot(self.session, self.options, broot['id'])
             path = broot.rootdir()
             if full:
diff --git a/tests/test_plugins/test_save_failed_tree_builder.py b/tests/test_plugins/test_save_failed_tree_builder.py
new file mode 100644
index 0000000..76dedf9
--- /dev/null
+++ b/tests/test_plugins/test_save_failed_tree_builder.py
@@ -0,0 +1,113 @@
+import mock
+import os
+import sys
+import unittest
+
+# alter pythonpath to not load hub plugin
+sys.path = [os.path.join(os.path.dirname(__file__), '../../plugins/builder')] + sys.path
+#raise(Exception(sys.path))
+
+import koji
+# inject builder data
+from tests.test_builder.loadkojid import kojid
+import __main__
+__main__.BuildRoot = kojid.BuildRoot
+
+from save_failed_tree import SaveFailedTreeTask
+
+class TestSaveFailedTree(unittest.TestCase):
+    def setUp(self):
+        self.session = mock.MagicMock()
+        self.session.host.getHost.return_value = {'id': 1}
+        options = mock.MagicMock()
+        options.workdir = '/tmp/nonexistentdirectory'
+        options.mockdir = '/tmp/mockdir'
+        options.name = 'name'
+        self.t = SaveFailedTreeTask(123, 'saveFailedTree', {}, self.session, options)
+
+    @mock.patch('os.unlink')
+    @mock.patch('tarfile.open')
+    def testNonExistentTask(self, tarfile, os_unlink):
+        # empty tarball
+        tfile = mock.MagicMock(name='tfile')
+        tarfile.return_value = tfile
+
+        self.t.handler(1)
+
+        tarfile.assert_called_once_with(
+            '/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz',
+            'w:gz'
+        )
+        tfile.add.assert_not_called()
+        tfile.close.assert_called_once_with()
+        os_unlink.assert_called_once_with('/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz')
+
+    @mock.patch('os.unlink')
+    @mock.patch('tarfile.open')
+    def testCorrect(self, tarfile, os_unlink):
+        def getBuildroot(bid):
+            tmp = {
+                'tag_name': 'tag_name',
+                'repo_id': 'repo_id',
+            }
+            if bid == 1:
+                tmp['task_id'] = 1000
+                tmp['arch'] = 'x86_64'
+                tmp['tag_id'] = 5000
+            elif bid == 2:
+                tmp['task_id'] = 1001
+                tmp['arch'] = 'i386'
+                tmp['tag_id'] = 5001
+            return tmp
+
+        self.session.getBuildroot.side_effect = getBuildroot
+        tfile = mock.MagicMock(name='tfile')
+        tfile.add = mock.MagicMock()
+        tarfile.return_value = tfile
+        # simplified return values, only id should be used
+        self.session.listBuildroots.return_value = [{'id': 1, 'host_id': 1}, {'id': 2, 'host_id': 1}]
+
+        self.t.handler(1)
+
+        tarfile.assert_called_once_with(
+            '/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz',
+            'w:gz'
+        )
+        self.assertEqual(tfile.add.call_args_list[0][0][0], '/tmp/mockdir/tag_name-1-repo_id/root/builddir')
+        self.assertEqual(tfile.add.call_args_list[1][0][0], '/tmp/mockdir/tag_name-2-repo_id/root/builddir')
+        self.assertEqual(len(tfile.add.call_args_list), 2)
+        tfile.close.assert_called_once_with()
+        os_unlink.assert_called_once_with('/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz')
+
+    @mock.patch('os.unlink')
+    @mock.patch('tarfile.open')
+    def testWrongBuilder(self, tarfile, os_unlink):
+        def getBuildroot(bid):
+            tmp = {
+                'tag_name': 'tag_name',
+                'repo_id': 'repo_id',
+            }
+            if bid == 1:
+                tmp['task_id'] = 1000
+                tmp['arch'] = 'x86_64'
+                tmp['tag_id'] = 5000
+            elif bid == 2:
+                tmp['task_id'] = 1001
+                tmp['arch'] = 'i386'
+                tmp['tag_id'] = 5001
+            return tmp
+
+        self.session.getBuildroot.side_effect = getBuildroot
+        tfile = mock.MagicMock(name='tfile')
+        tarfile.return_value = tfile
+        # simplified return values, only id should be used
+        self.session.listBuildroots.return_value = [{'id': 1, 'host_id': 2}, {'id': 2, 'host_id': 2}]
+
+        with self.assertRaises(koji.GenericError):
+            self.t.handler(1)
+
+    def testFull(self):
+        pass
+
+    def testFailUpload(self):
+        pass

From d72a5b4f53d76ccc6a3e931eb02233a0586d1a17 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 28 2017 14:33:39 +0000
Subject: [PATCH 15/17] change config separator to whitespace


---

diff --git a/docs/source/plugins.rst b/docs/source/plugins.rst
index d3c29f0..e6e775d 100644
--- a/docs/source/plugins.rst
+++ b/docs/source/plugins.rst
@@ -21,12 +21,12 @@ Additional feature is that some paths from buildroot can be left out from
 tarball. Feature can be configured via
 `/etc/kojid/plugins/save_failed_tree.conf` file. Currently only field
 filters.paths is used and it consists of globs (standard python's fnmatch is
-used) separated by ':'.
+used) separated by whitespaces.
 
 .. code-block:: ini
 
   [filters]
-  paths = /etc/*.keytab:/tmp/secret_data
+  paths = /etc/*.keytab /tmp/secret_data
 
 .. warning::
   For security reasons, currently all ``/tmp/krb5cc*`` and ``/etc/*.keytab``
diff --git a/plugins/builder/save_failed_tree.conf b/plugins/builder/save_failed_tree.conf
index 84df846..66274c5 100644
--- a/plugins/builder/save_failed_tree.conf
+++ b/plugins/builder/save_failed_tree.conf
@@ -2,4 +2,4 @@
 volume = DEFAULT
 
 [filters]
-paths = */tmp/krb5cc:*/etc/*.keytab
+paths = */tmp/krb5cc */etc/*.keytab
diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index f5fec8a..1a22db6 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -27,7 +27,7 @@ def read_config():
         'volume': None,
     }
     if cp.has_option('filters', 'paths'):
-        config['path_filters'] = cp.get('filters', 'paths').split(':')
+        config['path_filters'] = cp.get('filters', 'paths').split()
     if cp.has_option('general', 'volume'):
         config['volume'] = cp.get('general', 'volume').strip()
 

From dda05bafe0d550799ba0a331b6f00610080b35c6 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Mar 30 2017 08:22:35 +0000
Subject: [PATCH 16/17] refactor


---

diff --git a/cli/koji b/cli/koji
index 5d782ad..5a234fd 100755
--- a/cli/koji
+++ b/cli/koji
@@ -7202,50 +7202,63 @@ def handle_runroot(options, session, args):
         sys.exit(1)
     return
 
+
 def handle_save_failed_tree(options, session, args):
     "Create tarball with whole buildtree"
-    usage = _("usage: %prog save-failed-tree [options] taskID")
+    usage = _("usage: %prog save-failed-tree [options] ID")
     usage += _("\n(Specify the --help global option for a list of other help options)")
     parser = OptionParser(usage=usage)
     parser.add_option("-f", "--full", action="store_true", default=False,
             help=_("Download whole tree, if not specified, only builddir will be downloaded"))
-    parser.add_option("--quiet", action="store_true",
-                      help=_("Do not print the task information"), default=options.quiet)
+    parser.add_option("-t", "--task", action="store_const", dest="mode",
+            const="task", default="task",
+            help=_("Treat ID as a task ID (the default)"))
+    parser.add_option("-r", "--buildroot", action="store_const", dest="mode",
+            const="buildroot",
+            help=_("Treat ID as a buildroot ID"))
+    parser.add_option("--quiet", action="store_true", default=options.quiet,
+                      help=_("Do not print the task information"))
     parser.add_option("--nowait", action="store_true",
                       help=_("Don't wait on build"))
 
     (opts, args) = parser.parse_args(args)
 
     if len(args) != 1:
-        parser.error(_("List exactly one taskID"))
+        parser.error(_("List exactly one task or buildroot ID"))
 
     try:
-        taskID = int(args[0])
+        id_val = int(args[0])
     except ValueError:
-        parser.error(_("Task ID must be an integer."))
+        parser.error(_("ID must be an integer"))
 
     activate_session(session)
+
+    if opts.mode == "buildroot":
+        br_id = id_val
+    else:
+        brs = [b['id'] for b in session.listBuildroots(taskID=id_val)]
+        if not brs:
+            print(_("No buildroots for task %s") % id_val)
+            return 1
+        br_id = max(brs)
+        if len(brs) > 1:
+            print(_("Multiple buildroots for task. Choosing last one (%s)") % br_id)
+
     try:
-        task_id = session.saveFailedTree(taskID, opts.full)
+        task_id = session.saveFailedTree(br_id, opts.full)
     except koji.GenericError as e:
         m = str(e)
-        if 'Only failed tasks can upload their buildroots.' in m:
-            print(_("Only failed tasks can upload their buildroots."))
-        elif 'tasks can upload their buildroots (Task' in m:
-            print(_("Task of this type has disabled support for uploading" \
-                    " buildroot. (configurable on hub)"))
-        elif 'Invalid method' in m:
-            print(_("* The save_failed_tree plugin appears to not be installed" \
-                    " on the koji hub.  Please contact the administrator."))
-        else:
-            raise
-        return 1
-    except koji.ActionNotAllowed:
-        print(_("Only task owner or admin can run this task."))
-        return 1
+        if 'Invalid method' in m:
+            print(_("* The save_failed_tree plugin appears to not be "
+                    "installed on the koji hub.  Please contact the "
+                    "administrator."))
+            return 1
+        raise
+
     if not opts.quiet:
-        print("Created task:", task_id)
-        print("Task info: %s/taskinfo?taskID=%s" % (options.weburl, task_id))
+        print(_("Created task %s for buildroot %s") % (task_id, br_id))
+        print("Task info: %s/taskinfo?taskID=%s"
+                % (options.weburl, task_id))
 
     if opts.nowait:
         return
diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index 1a22db6..d9b325c 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -12,12 +12,14 @@ __all__ = ('SaveFailedTreeTask',)
 CONFIG_FILE = '/etc/kojid/plugins/save_failed_tree.conf'
 config = None
 
+
 def omit_paths(tarinfo):
     if any([fnmatch.fnmatch(tarinfo.name, f) for f in config['path_filters']]):
         return None
     else:
         return tarinfo
 
+
 def read_config():
     global config
     cp = ConfigParser.SafeConfigParser()
@@ -31,29 +33,37 @@ def read_config():
     if cp.has_option('general', 'volume'):
         config['volume'] = cp.get('general', 'volume').strip()
 
+
 class SaveFailedTreeTask(tasks.BaseTaskHandler):
     Methods = ['saveFailedTree']
     _taskWeight = 3.0
 
-    def handler(self, taskID, full=False):
-        self.logger.debug("Starting saving buildroots for task %d [full=%s]" % (taskID, full))
+    def handler(self, buildrootID, full=False):
+        self.logger.debug("Saving buildroot %d [full=%s]", buildrootID, full)
         read_config()
-        tar_path = os.path.join(self.workdir, 'broots-task-%s.tar.gz' % taskID)
-        f = tarfile.open(tar_path, "w:gz")
+
+        brinfo = self.session.getBuildroot(buildrootID)
         host_id = self.session.host.getHost()['id']
-        for broot in self.session.listBuildroots(taskID=taskID):
-            if broot['host_id'] != host_id:
-                raise koji.GenericError("Task is run on wrong builder.")
-            broot = BuildRoot(self.session, self.options, broot['id'])
-            path = broot.rootdir()
-            if full:
-                self.logger.debug("Adding buildroot (full): %s" % path)
-            else:
-                path = os.path.join(path, 'builddir')
-                self.logger.debug("Adding buildroot: %s" % path)
-            f.add(path, filter=omit_paths)
+        if brinfo['host_id'] != host_id:
+            raise koji.GenericError("Task is run on wrong builder")
+        broot = BuildRoot(self.session, self.options, brinfo['id'])
+        path = broot.rootdir()
+
+        if full:
+            self.logger.debug("Adding buildroot (full): %s" % path)
+        else:
+            path = os.path.join(path, 'builddir')
+            self.logger.debug("Adding buildroot: %s" % path)
+        if not os.path.exists(path):
+            raise koji.GenericError("Buildroot directory is missing: %s" % path)
+
+        tar_path = os.path.join(self.workdir, 'broot-%s.tar.gz' % buildrootID)
+        self.logger.debug("Creating buildroot archive %s", tar_path)
+        f = tarfile.open(tar_path, "w:gz")
+        f.add(path, filter=omit_paths)
         f.close()
-        self.logger.debug("Uploading %s to hub." % tar_path)
+
+        self.logger.debug("Uploading %s to hub", tar_path)
         self.uploadFile(tar_path, volume=config['volume'])
         os.unlink(tar_path)
-        self.logger.debug("Finished saving buildroots for task %d" % taskID)
+        self.logger.debug("Finished saving buildroot %s", buildrootID)
diff --git a/plugins/hub/save_failed_tree.py b/plugins/hub/save_failed_tree.py
index 46ee865..3a98128 100644
--- a/plugins/hub/save_failed_tree.py
+++ b/plugins/hub/save_failed_tree.py
@@ -15,7 +15,7 @@ allowed_methods = None
 
 
 @export
-def saveFailedTree(taskID, full=False, **opts):
+def saveFailedTree(buildrootID, full=False, **opts):
     """Create saveFailedTree task
 
     If arguments are invalid, error message is returned. Otherwise task id of
@@ -23,7 +23,7 @@ def saveFailedTree(taskID, full=False, **opts):
     global config, allowed_methods
 
     # let it raise errors
-    taskID = int(taskID)
+    buildrootID = int(buildrootID)
     full = bool(full)
 
     # read configuration only once
@@ -34,6 +34,8 @@ def saveFailedTree(taskID, full=False, **opts):
         if len(allowed_methods) == 1 and allowed_methods[0] == '*':
             allowed_methods = '*'
 
+    brinfo = kojihub.get_buildroot(buildrootID, strict=True)
+    taskID = brinfo['task_id']
     task_info = kojihub.Task(taskID).getInfo()
     if task_info['state'] != koji.TASK_STATES['FAILED']:
         raise koji.PreBuildError("Task %s has not failed. Only failed tasks can upload their buildroots." % taskID)
@@ -45,8 +47,8 @@ def saveFailedTree(taskID, full=False, **opts):
     elif not kojihub.get_host(task_info['host_id'])['enabled']:
         raise koji.PreBuildError("Host is disabled.")
 
-    args = koji.encode_args(taskID, full, **opts)
+    args = koji.encode_args(buildrootID, full, **opts)
     taskopts = {
-        'assign': task_info['host_id'],
+        'assign': brinfo['host_id'],
     }
     return kojihub.make_task('saveFailedTree', args, **taskopts)

From 74db04d5a82e77ef9e78e6643d8de6d5067faaa1 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
Date: Mar 30 2017 09:24:41 +0000
Subject: [PATCH 17/17] updated tests


---

diff --git a/plugins/builder/save_failed_tree.py b/plugins/builder/save_failed_tree.py
index d9b325c..138572f 100644
--- a/plugins/builder/save_failed_tree.py
+++ b/plugins/builder/save_failed_tree.py
@@ -43,6 +43,8 @@ class SaveFailedTreeTask(tasks.BaseTaskHandler):
         read_config()
 
         brinfo = self.session.getBuildroot(buildrootID)
+        if brinfo is None:
+            raise koji.GenericError("Nonexistent buildroot: %s" % buildrootID)
         host_id = self.session.host.getHost()['id']
         if brinfo['host_id'] != host_id:
             raise koji.GenericError("Task is run on wrong builder")
diff --git a/tests/test_cli/test_save_failed_tree.py b/tests/test_cli/test_save_failed_tree.py
index b2ad7df..b8acf12 100644
--- a/tests/test_cli/test_save_failed_tree.py
+++ b/tests/test_cli/test_save_failed_tree.py
@@ -27,12 +27,14 @@ class TestSaveFailedTree(unittest.TestCase):
     def test_handle_save_failed_tree_simple(self, activate_session_mock):
         # koji save-failed-tree 123456
         task_id = 123456
+        broot_id = 321
         arguments = [task_id]
         options = mock.MagicMock()
         options.full = False
         options.nowait = True
         self.parser.parse_args.return_value = [options, arguments]
         self.session.getAPIVersion.return_value = koji.API_VERSION
+        self.session.listBuildroots.return_value = [{'id': 321}]
 
         # Mock out the xmlrpc server
         self.session.saveFailedTree.return_value = 123
@@ -42,18 +44,46 @@ class TestSaveFailedTree(unittest.TestCase):
 
         # Finally, assert that things were called as we expected.
         activate_session_mock.assert_called_once_with(self.session)
-        self.session.saveFailedTree.assert_called_once_with(task_id, options.full)
+        self.session.listBuildroots.assert_called_once_with(taskID=task_id)
+        self.session.saveFailedTree.assert_called_once_with(broot_id, options.full)
+
+    @mock.patch('koji_cli.activate_session')
+    def test_handle_save_failed_tree_buildroots(self, activate_session_mock):
+        # koji save-failed-tree --buildroot 123456
+        broot_id = 321
+        arguments = [broot_id]
+        options = mock.MagicMock()
+        options.full = False
+        options.nowait = True
+        options.mode = "buildroot"
+        self.parser.parse_args.return_value = [options, arguments]
+        self.session.getAPIVersion.return_value = koji.API_VERSION
+        self.session.listBuildroots.return_value = [{'id': 321}]
+
+        # Mock out the xmlrpc server
+        self.session.saveFailedTree.return_value = 123
+
+        # Run it and check immediate output
+        cli.handle_save_failed_tree(self.options, self.session, self.args)
+
+        # Finally, assert that things were called as we expected.
+        activate_session_mock.assert_called_once_with(self.session)
+        self.session.listBuildroots.assert_not_called()
+        self.session.saveFailedTree.assert_called_once_with(broot_id, options.full)
+
 
     @mock.patch('koji_cli.activate_session')
     def test_handle_save_failed_tree_full(self, activate_session_mock):
         # koji save-failed-tree 123456 --full
         task_id = 123456
+        broot_id = 321
         arguments = [task_id]
         options = mock.MagicMock()
         options.full = True
         options.nowait = True
         self.parser.parse_args.return_value = [options, arguments]
         self.session.getAPIVersion.return_value = koji.API_VERSION
+        self.session.listBuildroots.return_value = [{'id': 321}]
 
         # Mock out the xmlrpc server
         self.session.saveFailedTree.return_value = 123
@@ -63,13 +93,15 @@ class TestSaveFailedTree(unittest.TestCase):
 
         # Finally, assert that things were called as we expected.
         activate_session_mock.assert_called_once_with(self.session)
-        self.session.saveFailedTree.assert_called_once_with(task_id, options.full)
+        self.session.listBuildroots.assert_called_once_with(taskID=task_id)
+        self.session.saveFailedTree.assert_called_once_with(broot_id, options.full)
 
     @mock.patch('koji_cli.activate_session')
     @mock.patch('koji_cli.watch_tasks')
     def test_handle_save_failed_tree_wait(self, watch_tasks_mock, activate_session_mock):
         # koji save-failed-tree 123456 --full
         task_id = 123456
+        broot_id = 321
         arguments = [task_id]
         options = mock.MagicMock()
         options.full = True
@@ -77,6 +109,7 @@ class TestSaveFailedTree(unittest.TestCase):
         options.quiet = False
         self.parser.parse_args.return_value = [options, arguments]
         self.session.getAPIVersion.return_value = koji.API_VERSION
+        self.session.listBuildroots.return_value = [{'id': 321}]
 
         # Mock out the xmlrpc server
         spawned_id = 123
@@ -86,7 +119,8 @@ class TestSaveFailedTree(unittest.TestCase):
         cli.handle_save_failed_tree(self.options, self.session, self.args)
 
         # Finally, assert that things were called as we expected.
-        self.session.saveFailedTree.assert_called_once_with(task_id, options.full)
+        self.session.listBuildroots.assert_called_once_with(taskID=task_id)
+        self.session.saveFailedTree.assert_called_once_with(broot_id, options.full)
         activate_session_mock.assert_called_once_with(self.session)
         self.session.logout.assert_called_once_with()
         watch_tasks_mock.assert_called_once_with(self.session, [spawned_id],
@@ -102,6 +136,7 @@ class TestSaveFailedTree(unittest.TestCase):
         self.parser.parse_args.return_value = [options, arguments]
         self.parser.error.side_effect = Exception()
         self.session.getAPIVersion.return_value = koji.API_VERSION
+        self.session.listBuildroots.return_value = [{'id': 321}]
 
         self.assertRaises(Exception, cli.handle_save_failed_tree,
                           self.options, self.session, self.args)
@@ -120,18 +155,9 @@ class TestSaveFailedTree(unittest.TestCase):
         actual = stdout.getvalue()
         self.assertTrue('The save_failed_tree plugin appears to not be installed' in actual)
 
-        # Task which is not FAILED
-        stdout.seek(0)
-        stdout.truncate()
-        self.session.saveFailedTree.side_effect = koji.PreBuildError('Only failed tasks can upload their buildroots.')
-        cli.handle_save_failed_tree(self.options, self.session, self.args)
-        actual = stdout.getvalue()
-        self.assertTrue('Only failed tasks can upload their buildroots.' in actual)
-
-        # Disabled/unsupported task
-        stdout.seek(0)
-        stdout.truncate()
-        self.session.saveFailedTree.side_effect = koji.PreBuildError('tasks can upload their buildroots (Task')
-        cli.handle_save_failed_tree(self.options, self.session, self.args)
-        actual = stdout.getvalue()
-        self.assertTrue('Task of this type has disabled support for uploading' in actual)
+        # Task which is not FAILED, disabled in config, wrong owner
+        self.session.saveFailedTree.side_effect = koji.PreBuildError('placeholder')
+        with self.assertRaises(koji.PreBuildError) as cm:
+            cli.handle_save_failed_tree(self.options, self.session, self.args)
+        e = cm.exception
+        self.assertEqual(e, self.session.saveFailedTree.side_effect)
diff --git a/tests/test_plugins/test_save_failed_tree_builder.py b/tests/test_plugins/test_save_failed_tree_builder.py
index 76dedf9..56c92a5 100644
--- a/tests/test_plugins/test_save_failed_tree_builder.py
+++ b/tests/test_plugins/test_save_failed_tree_builder.py
@@ -27,34 +27,37 @@ class TestSaveFailedTree(unittest.TestCase):
 
     @mock.patch('os.unlink')
     @mock.patch('tarfile.open')
-    def testNonExistentTask(self, tarfile, os_unlink):
-        # empty tarball
+    def testNonExistentBuildroot(self, tarfile, os_unlink):
         tfile = mock.MagicMock(name='tfile')
         tarfile.return_value = tfile
+        self.session.getBuildroot.return_value = None
 
-        self.t.handler(1)
+        with self.assertRaises(koji.GenericError) as cm:
+            self.t.handler(1)
+        self.assertTrue('Nonexistent buildroot' in str(cm.exception))
 
-        tarfile.assert_called_once_with(
-            '/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz',
-            'w:gz'
-        )
+        tarfile.assert_not_called()
         tfile.add.assert_not_called()
-        tfile.close.assert_called_once_with()
-        os_unlink.assert_called_once_with('/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz')
+        tfile.close.assert_not_called()
+        os_unlink.assert_not_called()
 
+    @mock.patch('os.path.exists')
     @mock.patch('os.unlink')
     @mock.patch('tarfile.open')
-    def testCorrect(self, tarfile, os_unlink):
+    def testCorrect(self, tarfile, os_unlink, os_exists):
         def getBuildroot(bid):
             tmp = {
                 'tag_name': 'tag_name',
                 'repo_id': 'repo_id',
+                'host_id': 1,
             }
             if bid == 1:
+                tmp['id'] = 1
                 tmp['task_id'] = 1000
                 tmp['arch'] = 'x86_64'
                 tmp['tag_id'] = 5000
             elif bid == 2:
+                tmp['id'] = 2
                 tmp['task_id'] = 1001
                 tmp['arch'] = 'i386'
                 tmp['tag_id'] = 5001
@@ -64,20 +67,19 @@ class TestSaveFailedTree(unittest.TestCase):
         tfile = mock.MagicMock(name='tfile')
         tfile.add = mock.MagicMock()
         tarfile.return_value = tfile
-        # simplified return values, only id should be used
-        self.session.listBuildroots.return_value = [{'id': 1, 'host_id': 1}, {'id': 2, 'host_id': 1}]
+        os_exists.return_value = True
 
         self.t.handler(1)
 
         tarfile.assert_called_once_with(
-            '/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz',
+            '/tmp/nonexistentdirectory/tasks/123/123/broot-1.tar.gz',
             'w:gz'
         )
+
+        tfile.add.assert_called_once()
         self.assertEqual(tfile.add.call_args_list[0][0][0], '/tmp/mockdir/tag_name-1-repo_id/root/builddir')
-        self.assertEqual(tfile.add.call_args_list[1][0][0], '/tmp/mockdir/tag_name-2-repo_id/root/builddir')
-        self.assertEqual(len(tfile.add.call_args_list), 2)
         tfile.close.assert_called_once_with()
-        os_unlink.assert_called_once_with('/tmp/nonexistentdirectory/tasks/123/123/broots-task-1.tar.gz')
+        os_unlink.assert_called_once_with('/tmp/nonexistentdirectory/tasks/123/123/broot-1.tar.gz')
 
     @mock.patch('os.unlink')
     @mock.patch('tarfile.open')
@@ -86,12 +88,15 @@ class TestSaveFailedTree(unittest.TestCase):
             tmp = {
                 'tag_name': 'tag_name',
                 'repo_id': 'repo_id',
+                'host_id': 2000,
             }
             if bid == 1:
+                tmp['id'] = 1
                 tmp['task_id'] = 1000
                 tmp['arch'] = 'x86_64'
                 tmp['tag_id'] = 5000
             elif bid == 2:
+                tmp['id'] = 2
                 tmp['task_id'] = 1001
                 tmp['arch'] = 'i386'
                 tmp['tag_id'] = 5001
@@ -100,8 +105,6 @@ class TestSaveFailedTree(unittest.TestCase):
         self.session.getBuildroot.side_effect = getBuildroot
         tfile = mock.MagicMock(name='tfile')
         tarfile.return_value = tfile
-        # simplified return values, only id should be used
-        self.session.listBuildroots.return_value = [{'id': 1, 'host_id': 2}, {'id': 2, 'host_id': 2}]
 
         with self.assertRaises(koji.GenericError):
             self.t.handler(1)

