[PATCH OSSTEST v2 0/3] Misc standalone wrapper improvements

[PATCH OSSTEST v2 0/3] Misc standalone wrapper improvements

[Ian Campbell]

This combines 2/3 of the previous "Misc standalone wrapper improvements"
post (the third being "standalone: Rotate logs rather than clobbering them"
which is now in pretest) plus an extra patch "standalone: Make it possible
to pass options to run-test" which I posted v1 of separately.

Note that "cs-adjust-flight: Add job-status to report job stats" was
previously "standalone: Add get-job-status to pick status out of
standalone.db"

Ian.

[PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats

[Ian Campbell]

The return code of sg-run-job does not reflect the state of the job,
which is instead written to the database. For the benefit of running
tests in a loop until failure add a command to retrieve the status to
stdout.

Since this new op is not a change adjust the doc comment accordingly.

While there add a doc string for the undocumented "branch" subcommand
of cs-adjust-flight.

Add a get-job-status command to the standalone helper script.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Replaces "standalone: Add get-job-status to pick status out of
standalone.db" with a variant using cs-adjust-flight
---
 cs-adjust-flight | 23 +++++++++++++++++++++--
 standalone       | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/cs-adjust-flight b/cs-adjust-flight
index 834e2c8..4e071c8 100755
--- a/cs-adjust-flight
+++ b/cs-adjust-flight
@@ -3,9 +3,9 @@
 # destination flight must already exist
 #
 # args:
-#   <dst-flight> [<change> ...]
+#   <dst-flight> [<op> ...]
 #
-# <change>:
+# <op>:
 #   copy <flight>
 #   copy-jobs <flight> <job-spec>
 #   jobs-list <job-spec>
@@ -16,6 +16,8 @@
 #   runvar-perlop <job-spec> <var-spec> <perl-expr>
 #   recipe-set <job-spec> <new-value>
 #   intended-blessing <intended-blessing>
+#   branch <new-branch>
+#   job-status <job-spec>
 #
 # <foo-spec>:
 #   <foo-name>
@@ -341,6 +343,23 @@ sub change__branch {
     verbose "$dstflight branch set to $branch\n";
 }
 
+sub change__job_status {
+    die unless @changes >= 1;
+    my $jobs = shift @changes;
+
+    my $q = $dbh_tests->prepare(<<END);
+        SELECT status
+          FROM jobs
+         WHERE flight = ? AND job = ?
+END
+    for_jobs($dstflight, $jobs, sub {
+	my ($job) = @_;
+	$q->execute($dstflight, $job);
+	my ($s) = $q->fetchrow_array();
+	print "$job $s\n";
+    });
+}
+
 sub changes () {
     debug("CHANGES...\n");
 
diff --git a/standalone b/standalone
index f64462e..b362b38 100755
--- a/standalone
+++ b/standalone
@@ -35,6 +35,11 @@ Operations:
   hosts next time. Otherwise osstest will complain if you change the
   host(s) which a job is running on on successive runs.
 
+* get-job-status [cf] [JOB]
+
+  Prints the status (pass, fail, running, etc) of the given job to
+  stdout.
+
 Options:
 
 -c FILE, --config=FILE        Use FILE as configuration file
@@ -140,8 +145,9 @@ if [ $reuse -eq 0 ]; then
     read
 fi
 
-if [ ! -f standalone.db ] ; then
-    echo "No standalone.db? Run standalone-reset." >&2
+db="standalone.db"
+if [ ! -f $db ] ; then
+    echo "No $db? Run standalone-reset." >&2
     exit 1
 fi
 
@@ -199,6 +205,22 @@ with_logging() {
     fi
 }
 
+job_status() {
+    flight=$1; shift
+    job=$1; shift
+
+    status=$(OSSTEST_CONFIG=$config \
+		./cs-adjust-flight $flight job-status $job)
+
+    case "$status" in
+	$job\ *) echo ${status#$job };;
+	*)
+	    echo >&2 "ERROR: $status"
+	    exit 1
+	    ;;
+    esac
+}
+
 # other potential ops:
 # - run standalone reset
 
@@ -304,6 +326,20 @@ case $op in
 	OSSTEST_JOB=$job \
 	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
 	;;
+
+    get-job-status)
+	need_flight;
+
+	if [ $# -ne 1 ] ; then
+	    echo "get-job-status: Need job" >&2
+	    exit 1
+	fi
+
+	job=$1; shift
+
+	job_status $flight $job
+
+	;;
     *)
 	echo "Unknown op $op" ; exit 1 ;;
 esac
-- 
2.5.3

Re: [PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats"):
> Since this new op is not a change adjust the doc comment accordingly.

Good idea.  (I have already introduced another op which is not a
change: jobs-list.)

> While there add a doc string for the undocumented "branch" subcommand
> of cs-adjust-flight.

Clearly that should have been called branch-set.  Oh well.  (Feel free
to rename it if you feel like it.  I would do so in my tree but
conflicts.)

> +sub change__job_status {
> +    die unless @changes >= 1;
> +    my $jobs = shift @changes;
> +
> +    my $q = $dbh_tests->prepare(<<END);
> +        SELECT status
> +          FROM jobs
> +         WHERE flight = ? AND job = ?
> +END
> +    for_jobs($dstflight, $jobs, sub {
> +	my ($job) = @_;
> +	$q->execute($dstflight, $job);
> +	my ($s) = $q->fetchrow_array();
> +	print "$job $s\n";

 or die $!;

> +    });
> +}

This output format is awkward, isn't it ?

Would it be too horrible to omit `$job ' if the spec was a literal and
can therefore only match one item ?  You can test that with something like
    print "$job " or die $! if defined spec_re($jobs);

> +    case "$status" in
> +	$job\ *) echo ${status#$job };;

If you decide to keep `$job ' in the output, should read:

  +	"$job"\ *) echo ${status#$job };;

since otherwise wildcards in $job would be treated as an attempt to do
glob matching.

Or, better

  +	"$job "*) echo ${status#$job };;

which is a better idiom.

> +	*)
> +	    echo >&2 "ERROR: $status"
> +	    exit 1

Do we not have `fail' in scope ?  Also TBH I'm not sure you need to be
this careful anyway...

> +	if [ $# -ne 1 ] ; then
> +	    echo "get-job-status: Need job" >&2
> +	    exit 1

`fail' again ?

Ian.

Re: [PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats

[Ian Campbell]

On Mon, 2015-10-05 at 11:48 +0100, Ian Jackson wrote:
> > +    });
> > +}
> 
> This output format is awkward, isn't it ?
> 
> Would it be too horrible to omit `$job ' if the spec was a literal and
> can therefore only match one item ?  You can test that with something
> like
>     print "$job " or die $! if defined spec_re($jobs);

Yes I can make this work I think.

I wasn't too sure about having the output differ depending on the
parameters like this, but I think it probably is best.

> > +	*)
> > +	    echo >&2 "ERROR: $status"
> > +	    exit 1
> 
> Do we not have `fail' in scope ?

It doesn't look like it, the script doesn't source anything at all so
there's certainly no path to mgi-common. The rest of the script is already
doing explicit echo + exit. I could tack on a cleanup.

>   Also TBH I'm not sure you need to be
> this careful anyway...

Belt and braces ;-)

> > +	if [ $# -ne 1 ] ; then
> > +	    echo "get-job-status: Need job" >&2
> > +	    exit 1
> 
> `fail' again ?
> 
> Ian.

[PATCH OSSTEST v2 2/3] standalone: Check job status at end of run-job.

[Ian Campbell]

Check if the job passed and if not (so status is fail, broken, running
etc) then return an error.

This is convenient for scripting.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 standalone | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/standalone b/standalone
index b362b38..e4c16d7 100755
--- a/standalone
+++ b/standalone
@@ -308,6 +308,11 @@ case $op in
 	OSSTEST_HOST_REUSE=$reuse \
 	OSSTEST_SIMULATE=$dryrun \
 	    with_logging logs/$flight/$job.log ./sg-run-job $job
+
+	status=`job_status $flight $job`
+	echo "$flight.$job status = $status" >&2
+	if [ "x$status" != xpass ] ; then exit 1; fi
+
 	;;
     run-test)
 	need_flight; need_host
-- 
2.5.3

[PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test

[Ian Campbell]

Currently the remainder of the comnand line is passed after the host=
ident, which allows for other idents to be given, which isn't all that
useful in practice.

Instead arrange that any additional options up to a "--" marker are
passed before host= and anything after are passed after.

Since the options themselves have a leading -- this can confuse the
scripts own option parsing, meaning you may need more than one "--"
marker, the first to separate the standalone helper args from the ts
args and a second to separate from any ident optiopns.

./standalone run-test -h $HOST -- test-amd64-amd64-xl-xsm ts-host-install --rescue -- guest=debian

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 standalone | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/standalone b/standalone
index e4c16d7..3cbcb30 100755
--- a/standalone
+++ b/standalone
@@ -325,11 +325,18 @@ case $op in
 	job=$1; shift
 	ts=$1; shift
 
+	options=
+	for i in $@ ; do
+	    if [ x$i = x-- ] ; then shift; break ; fi
+	    options="$options $i"
+	    shift
+	done
+
 	OSSTEST_CONFIG=$config \
 	OSSTEST_FLIGHT=$flight \
 	OSSTEST_HOST_REUSE=$reuse \
 	OSSTEST_JOB=$job \
-	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
+	    with_logging logs/$flight/$job.$ts.log ./$ts $options $hosts $@
 	;;
 
     get-job-status)
-- 
2.5.3

Re: [PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test"):
> Currently the remainder of the comnand line is passed after the host=
> ident, which allows for other idents to be given, which isn't all that
> useful in practice.
...
> +	options=
> +	for i in $@ ; do
> +	    if [ x$i = x-- ] ; then shift; break ; fi
> +	    options="$options $i"
> +	    shift
> +	done

Please use an array variable so that it is possible to pass options
containing whitespace etc.  Sorry; I realise this is a very marginal
use case but it's definitely a good idea not to paint oneself into a
corner about this kind of thing.

Ian.

Re: [PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test

[Ian Campbell]

On Mon, 2015-10-05 at 11:50 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 3/3] standalone: Make it possible
> to pass options to run-test"):
> > Currently the remainder of the comnand line is passed after the host=
> > ident, which allows for other idents to be given, which isn't all that
> > useful in practice.
> ...
> > +	options=
> > +	for i in $@ ; do
> > +	    if [ x$i = x-- ] ; then shift; break ; fi
> > +	    options="$options $i"
> > +	    shift
> > +	done
> 
> Please use an array variable so that it is possible to pass options
> containing whitespace etc.  Sorry; I realise this is a very marginal
> use case but it's definitely a good idea not to paint oneself into a
> corner about this kind of thing.

Ack.