Misc standalone wrapper improvements
v3 cover Misc standalone wrapper improvements
I've addressed the comments from last time which has resulted in 2 new patches, one to rename cs-adjust-flight's branch option to branch-set (and which then contains the docs strings fiddling previously in another patch) and another to switch to using fail.
Otherwise changes are described in the individual patches.
PATCH v3 1/5 cs-adjust-flight: `branch' command ought to be `branch-set'
Also add a doc string and since this op is not a change adjust the doc comment accordingly.
---
v3: New patch.
--- cs-adjust-flight | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
cs-adjust-flight +4 -3
@@ -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,7 @@ # runvar-perlop <job-spec> <var-spec> <perl-expr> # recipe-set <job-spec> <new-value> # intended-blessing <intended-blessing> +# branch-set <new-branch> # # <foo-spec>: # <foo-name>
@@ -330,7 +331,7 @@ sub change__intended_blessing { verbose "$dstflight blessing set to $blessing\n"; } -sub change__branch { +sub change__branch_set { die unless @changes >= 1; my $branch = shift @changes;
Signature · 1 line
2.5.3
Re: [PATCH OSSTEST 1/5] cs-adjust-flight: `branch' command ought to be `branch-set'
Ian Campbell writes ("[PATCH OSSTEST 1/5] cs-adjust-flight: branch' command ought to be branch-set'"):
Quoted 2 lines
Also add a doc string and since this op is not a change adjust the doc comment accordingly.
PATCH v3 2/5 cs-adjust-flight: Add job-status to report job stats
In reply to Ian CampbellThe 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.
Add a get-job-status command to the standalone helper script.
---
v3: Some incidental docs stuff moved to a more appropriate (new) patch. Only print the job if there might be multiple (i.e. the jobs argument is some sort of regex)
v2: Replaces "standalone: Add get-job-status to pick status out of standalone.db" with a variant using cs-adjust-flight
--- cs-adjust-flight | 19 +++++++++++++++++++ standalone | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 2 deletions(-)
cs-adjust-flight +19 -0
@@ -17,6 +17,7 @@ # recipe-set <job-spec> <new-value> # intended-blessing <intended-blessing> # branch-set <new-branch> +# job-status <job-spec> # # <foo-spec>: # <foo-name>
@@ -342,6 +343,24 @@ sub change__branch_set { 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 " or die $! if defined spec_re($jobs); + print "$s\n" or die $!; + }); +} + sub changes () { debug("CHANGES...\n");
standalone +31 -2
@@ -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,15 @@ with_logging() { fi } +job_status() { + flight=$1; shift + job=$1; shift + + status=$(OSSTEST_CONFIG=$config \ + ./cs-adjust-flight $flight job-status $job) + echo "$status" +} + # other potential ops: # - run standalone reset
@@ -304,6 +319,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
Signature · 1 line
2.5.3
Re: [PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats
Ian Campbell writes ("[PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats"):
Quoted 4 lines
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.
...
standalone with_logging() { · line 215
Quoted 8 lines
standalone +8
-0+job_status() { + flight=$1; shift + job=$1; shift + + status=$(OSSTEST_CONFIG=$config \ + ./cs-adjust-flight $flight job-status $job) + echo "$status" +}
This is rather odd. Why do you capture the value in a variable and then pass it to echo ? You could just let the job_status command print its output directly.
(Also I might quibble about unparsing the arguments to echo. Observe the output of (say) echo -n'. printf is often better. This is only relevant if $status might start with -' and if you want to keep the echo.)
Ian.
Re: [PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats
Quoted 7 lines · Ian Jackson
Ian Campbell writes ("[PATCH OSSTEST 2/5] cs-adjust-flight: Add job -status to report job stats"):
Quoted 4 lines
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.
...
Quoted 12 lines
standalone with_logging() { · line 215
Quoted 8 lines
standalone +8
-0+job_status() { + flight=$1; shift + job=$1; shift + + status=$(OSSTEST_CONFIG=$config \ + ./cs-adjust-flight $flight job-status $job) + echo "$status" +}This is rather odd. Why do you capture the value in a variable and then pass it to echo ? You could just let the
job_statuscommand print its output directly.
It was previously echo "${status#* }" (or something like that). I should have changed it.
Ian.
PATCH v3 3/5 standalone: Check job status at end of run-job.
In reply to Ian CampbellCheck if the job passed and if not (so status is fail, broken, running etc) then return an error.
This is convenient for scripting.
--- standalone | 5 +++++ 1 file changed, 5 insertions(+)
standalone +5 -0
@@ -301,6 +301,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
Signature · 1 line
2.5.3
PATCH v3 4/5 standalone: Make it possible to pass options to run-test
In reply to Ian CampbellCurrently 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
---
v3: Use an array var for correct whitespace handling.
--- standalone | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
standalone +8 -1
@@ -318,11 +318,18 @@ case $op in job=$1; shift ts=$1; shift + options=() + for i in $@ ; do + if [ x$i = x-- ] ; then shift; break ; fi + 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)
Signature · 1 line
2.5.3
Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test
Ian Campbell writes ("[PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
Quoted 6 lines
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.
...
standalone case $op in · line 332
Quoted 2 lines
standalone +1
-1- with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@ + with_logging logs/$flight/$job.$ts.log ./$ts ${options[@]} $hosts $@
You mean ... ./$ts "${options[@]}" $hosts ...
by analogy with "$@"
mariner:~> a=(a "1 2" b)
mariner:~> echo = ${a[@]}
= a 1 2 b
mariner:~> echo = "${a[@]}"
= a 1 2 b
mariner:~>
Ian.
Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test
Quoted 8 lines · Ian Jackson
Ian Campbell writes ("[PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
Quoted 6 lines
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.
Quoted 7 lines
...
Quoted 3 lines
(quoted diff) +1
-1- with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@ + with_logging logs/$flight/$job.$ts.log ./$ts ${options[@]}$hosts $@
You mean ... ./$ts "${options[@]}" $hosts ...
by analogy with "$@"
We (well, I, since I wrote that) don't use "$@" above but just the bare $@, which I copied. I suppose that one is wrong too?
Quoted 8 lines
mariner:~> a=(a "1 2" b)
mariner:~> echo = ${a[@]}
= a 1 2 b
mariner:~> echo = "${a[@]}"
= a 1 2 b
mariner:~>Ian.
Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test
Ian Campbell writes ("Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
Quoted 6 lines · Ian Jackson
Quoted 2 lines
You mean ... ./$ts "${options[@]}" $hosts ...
by analogy with "$@"We (well, I, since I wrote that) don't use "$@" above but just the bare $@, which I copied. I suppose that one is wrong too?
Yes, indeed. Sorry for not spotting that earlier.
Ian.
Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test
In reply to Ian CampbellQuoted 20 lines · Ian Campbell
Quoted 10 lines · Ian Jackson
Ian Campbell writes ("[PATCH OSSTEST 4/5] standalone: Make it possible to
pass options to run-test"):Quoted 7 lines
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.
Quoted 8 lines
...
Quoted 4 lines
(quoted diff) +1
-1- with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@ + with_logging logs/$flight/$job.$ts.log ./$ts${options[@]}
$hosts $@You mean ... ./$ts "${options[@]}" $hosts ...
by analogy with "$@"
Quoted 3 lines
We (well, I, since I wrote that) don't use "$@" above but just the bare $@,
which I copied. I suppose that one is wrong too?
I've decided it is and will insert a patch to fixup various unquoted uses of $@.
Also in this patch was
+ for i in $@ ; do
which is similarly wrong, I think.
Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test
Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
Quoted 3 lines
Also in this patch was
+ for i in $@ ; do
which is similarly wrong, I think.
I should have spotted that too...
Ian.
PATCH v3 5/5 standalone: Use fail() from mgi-common in most places
In reply to Ian CampbellFunctional change is simply to prepend "$0: ", to change the exit code for unknown operation and to slightly alter the error message when no arguments are given.
A few "exit 0" and "exit $rc" remain.
---
v3: New patch
--- standalone | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)
standalone +16 -26
@@ -58,10 +58,11 @@ Options: EOF } +. ./mgi-common + if [ $# -lt 1 ] ; then - echo "Need an operation" >&2 usage - exit 1 + fail "No arguments given" fi op=$1 ; shift
@@ -123,7 +124,7 @@ while true ; do --baseline)nobaseline=n; shift 1;; --help) usage; exit 0;; --) shift ; break ;; - *) echo "Internal error!" ; exit 1 ;; + *) fail "Internal error!" ;; esac done
@@ -147,34 +148,29 @@ fi db="standalone.db" if [ ! -f $db ] ; then - echo "No $db? Run standalone-reset." >&2 - exit 1 + fail "No $db? Run standalone-reset." fi if [ -z "$config" ] ; then - echo "No config specified." >&2 - exit 1 + fail "No config specified." fi IFS_saved=$IFS IFS=: for c in $config ; do if [ -z "$c" -o ! -r "$c" ] ; then - echo "Cannot read config $c." >&2 - exit 1 + fail "Cannot read config $c." fi done IFS=$IFS_saved need_flight() { if [ -z "$flight" ] ; then - echo "run-job: Need a flight" >&2 - exit 1 + fail "run-job: Need a flight" fi } need_host() { if [ "x$hosts" = x ] ; then - echo "run-job: Need a host" >&2 - exit 1 + fail "run-job: Need a host" fi }
@@ -228,8 +224,7 @@ case $op in need_flight if [ $# -lt 1 ] ; then - echo "make-flight: Need branch" >&2 - exit 1 + fail "make-flight: Need branch" fi branch=$1; shift
@@ -248,8 +243,7 @@ case $op in need_flight if [ $# -lt 1 ] ; then - echo "set-paths: Need job" >&2 - exit 1 + fail "set-paths: Need job" fi job=$1; shift
@@ -276,8 +270,7 @@ case $op in need_flight; if [ $# -lt 1 ] ; then - echo "run-job: Need job" >&2 - exit 1 + fail "run-job: Need job" fi job=$1; shift
@@ -288,8 +281,7 @@ case $op in need_flight; need_host if [ $# -lt 1 ] ; then - echo "run-job: Need job" >&2 - exit 1 + fail "run-job: Need job" fi job=$1; shift
@@ -311,8 +303,7 @@ case $op in need_flight; need_host if [ $# -lt 2 ] ; then - echo "run-test: Need job + test" >&2 - exit 1 + fail "run-test: Need job + test" fi job=$1; shift
@@ -336,8 +327,7 @@ case $op in need_flight; if [ $# -ne 1 ] ; then - echo "get-job-status: Need job" >&2 - exit 1 + fail "get-job-status: Need job" fi job=$1; shift
@@ -346,5 +336,5 @@ case $op in ;; *) - echo "Unknown op $op" ; exit 1 ;; + fail "Unknown op $op" ;; esac
Signature · 1 line
2.5.3
Re: [PATCH OSSTEST 5/5] standalone: Use fail() from mgi-common in most places
Ian Campbell writes ("[PATCH OSSTEST 5/5] standalone: Use fail() from mgi-common in most places"):
Quoted 7 lines
Functional change is simply to prepend "$0: ", to change the exit code for unknown operation and to slightly alter the error message when no arguments are given.
A few "exit 0" and "exit $rc" remain.
Signed-off-by Ian Campbell <[email protected]>