Hello Iain,

sorry for the delay!

| From 7f37957794d3b65e8d2e236d6af7dafcb2dc79e9 Mon Sep 17 00:00:00 2001
| From: Iain Lane <email address hidden>
| Date: Fri, 24 Feb 2017 12:41:32 +0000
| Subject: [PATCH] Add a debug-fail hook and implement it for
|  autopkgtest-virt-ssh
| 
| At the minute, this is mainly so that the nova script can have its
| failure information (`nova console-log') propagated up to the output, so
| that in the case of kernel panics or other random failures we get useful
| output that the driver of autopkgtest (e.g. autopkgtest-cloud) can look
| at.
| 
| https://bugs.launchpad.net/auto-package-testing/+bug/1630578
| ---
|  lib/VirtSubproc.py        |  6 ++++++
|  lib/adt_testbed.py        |  1 +
|  virt/autopkgtest-virt-ssh | 36 ++++++++++++++++++++++++++++--------
|  3 files changed, 35 insertions(+), 8 deletions(-)
| 
| diff --git a/lib/VirtSubproc.py b/lib/VirtSubproc.py
| index d76a861..e4845fa 100644
| --- a/lib/VirtSubproc.py
| +++ b/lib/VirtSubproc.py
| @@ -699,6 +699,12 @@ def prepare():
|          os.kill(os.getpid(), sig)
|      sethandlers(handler)
|  
| +def cmd_log_debug_fail(c, ce):
| +    cmdnumargs(c, ce)
| +    try:
| +        adtlog.info(caller.hook_debug_fail())
| +    except AttributeError:
| +        pass
|  

Needs two blank lines between functions (tests/pycodestyle will fail
otherwise). Can you please rename this s/log/auxverb/ to more clearly say what
it's for?

| --- a/virt/autopkgtest-virt-ssh
| +++ b/virt/autopkgtest-virt-ssh
| @@ -163,7 +163,7 @@ def parse_args():
|          capabilities += args.capability
|  
|  
| -def execute_setup_script(command, fail_ok=False):
| +def execute_setup_script(command, fail_ok=False, print_stderr=True):
|      '''Run the --setup-script, if given.
|  
|      Arguments passed after -- to the main program are passed verbatim to the
| @@ -173,10 +173,13 @@ def execute_setup_script(command, fail_ok=False):
|  
|      :param command: Command to execute. The command must match a function in
|                      the ssh script
| -    :param fail_ok: If True, failures will not cause bombing, and this function
| -                    returns the exit code instead.
| +    :param fail_ok: If True, failures will not cause bombing
| +    :return: A tuple (stdout, stderr, return code). stdout and stderr may be
| +             None, for example if the script fails and fail_ok is True.

Nitpick: Other functions like VirtSubproc.execute_timeout() return (rc, out, err)
so for consistency it would be nice to use the same order.

|      '''
|      global sshconfig, args
| +    out = None
| +    err = None
|  
|      if args.setup_script:
|          fpath = args.setup_script
| @@ -194,14 +197,23 @@ def execute_setup_script(command, fail_ok=False):
|              cmd += sshconfig['extraopts'].split(' ')
|  
|          adtlog.debug('Executing setup script: %s' % ' '.join(cmd))
| -        (status, out, err) = VirtSubproc.execute_timeout(
| -            None, 1800, cmd, stdout=subprocess.PIPE)
| +        (status, o, e) = VirtSubproc.execute_timeout(
| +            None,
| +            1800,
| +            cmd,
| +            stdout=subprocess.PIPE,
| +            stderr=subprocess.PIPE)
| +        out = o
| +        err = e

Why the extra o and e varibles? This looks confusing. Just directly use
"(status, out, err) = " as before?

| +        if print_stderr:
| +            # Keep outputting the error on stderr as well as capturing it
| +            sys.stderr.write(e)

This will cause delayed stderr messages when things are hanging and autopkgtest
runs interactively, but setting up a complex "tee" pipelining here isn't worth
the effort, so good enough.

|  def hook_open():
|      host_setup('open')
| @@ -440,7 +460,7 @@ def hook_wait_reboot():
|      global sshcmd
|  
|      if args.setup_script:
| -        rc = execute_setup_script('wait-reboot', fail_ok=True)
| +        (out, err, rc) = execute_setup_script('wait-reboot', fail_ok=True)

Nitpick: Use "rc = execute..()[0]" or use "_" as variable name for unused
components, i. e. "(rc, _, _) = execute..()".

Did you run the tests/autopkgtest SshRunner{No,With}Script tests with that?

Thanks!

Martin