Misc standalone wrapper improvements
v2 cover Misc standalone wrapper improvements
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 v2 1/3 cs-adjust-flight: Add job-status to report job stats
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.
---
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(-)
cs-adjust-flight +21 -2
@@ -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");
standalone +38 -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,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
Signature · 1 line
2.5.3
Re: [PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats
Ian Campbell writes ("[PATCH OSSTEST v2 1/3] cs-adjust-flight: Add job-status to report job stats"):
Quoted 1 line
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.)
Quoted 2 lines
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.)
Quoted 4 lines
cs-adjust-flight +4
-0+sub change__job_status { + die unless @changes >= 1; + my $jobs = shift @changes; +
cs-adjust-flight sub change__branch { · line 359
Quoted 10 lines
cs-adjust-flight +10
-0+ 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 $!;
cs-adjust-flight sub change__branch { · line 361
Quoted 2 lines
cs-adjust-flight +2
-0+ }); +}
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);
standalone with_logging() { · line 216
Quoted 2 lines
standalone +2
-0+ 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.
standalone with_logging() { · line 219
Quoted 3 lines
standalone +3
-0+ *) + 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...
standalone case $op in · line 335
Quoted 3 lines
standalone +3
-0+ 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
Quoted 9 lines · Ian Jackson
cs-adjust-flight sub change__branch { · line 361
Quoted 2 lines
cs-adjust-flight +2
-0+ }); +}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.
Quoted 5 lines
standalone with_logging() { · line 219
Quoted 3 lines
standalone +3
-0+ *) + echo >&2 "ERROR: $status" + exit 1Do 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.
Quoted 2 lines
Also TBH I'm not sure you need to be
this careful anyway...
Belt and braces ;-)
Quoted 7 lines
standalone case $op in · line 335
Quoted 3 lines
standalone +3
-0+ if [ $# -ne 1 ] ; then + echo "get-job-status: Need job" >&2 + exit 1`fail' again ?
Ian.
PATCH v2 2/3 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
@@ -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
Signature · 1 line
2.5.3
PATCH v2 3/3 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
--- standalone | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
standalone +8 -1
@@ -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)
Signature · 1 line
2.5.3
Re: [PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test
Ian Campbell writes ("[PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test"):
Quoted 3 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.
...
standalone case $op in · line 333
Quoted 6 lines
standalone +6
-0+ 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
Quoted 6 lines · Ian Jackson
Ian Campbell writes ("[PATCH OSSTEST v2 3/3] standalone: Make it possible to pass options to run-test"):
Quoted 3 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.
...
Quoted 11 lines
standalone case $op in · line 333
Quoted 6 lines
standalone +6
-0+ options= + for i in $@ ; do + if [ x$i = x-- ] ; then shift; break ; fi + options="$options $i" + shift + donePlease 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.