Comment 12 for bug 1630578

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