launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/report-script-result into lp:maas.

Commit message:
Updated protocol for signaling commissioning-script results to the metadata API.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/report-script-result/+merge/140650

Discussed this with Raphaël.  There were some design mismatches in how a commissioning script (such as the lshw script) would report its output files and its return code to the signal() metadata API call.

The new design is as follows.  Many commissioning scripts may be run in the process of commissioning one node.  Each now produces at least one, or at most two output files (we'll see about additional files if we ever really need them): stdout and stderr.  And of course each script run returns a numeric Unix return code.  All of these outputs of a single script are now reported at once, in one signal() call.  The stderr file is omitted if there was no error output.  We still want the stdout file even if it's empty, because the storage of return values is associated with these files.  There has to be at least one.

Signalling the result of one script still has the status "WORKING," indicating that it is a kind of progress report.  We may still want to change that; it's a simple matter of changing that string in signal_result() in the shell code.  But there's also a new parameter: script_result.  This stores the script's numerical return value.

The old code reported return values individually per script it ran (in an uncomfortably loosely-structured way) and then gathered up and sent all the output files.  Nothing very firmly associated scripts with files, except stdout/stderr files have names predictably tied to the script.  In the new setup, that is still the case but stdout/stderr are the only files.

Now, about implementation.

I was honestly a little worried that this line would cause trouble:

    local files="--stdout=$stdout"
    if [ ... ]
    then
        files="$files --stderr=$stderr"
    fi

Now wouldn't $files expand to a single string that might contain spaces?  Turns out it doesn't.  I wrote a simple python script that accepted some of the same options, and defined a version of signal_result() in my shell that called this python script.  Then I ran some basic manual tests with that experimental setup, in lieu of unit tests.  It did indeed pass two --file=$x options, exactly as hoped.  What if a filename contains spaces?  Kibo help us.  Unix shell quoting always scares me.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/report-script-result/+merge/140650
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/report-script-result into lp:maas.

=== modified file 'src/metadataserver/commissioning/snippets/maas_signal.py'
--- src/metadataserver/commissioning/snippets/maas_signal.py	2012-12-18 12:31:47 +0000
+++ src/metadataserver/commissioning/snippets/maas_signal.py	2012-12-19 12:41:21 +0000
@@ -105,6 +105,9 @@
         help="Power type.", choices=POWER_TYPES, default=None)
     parser.add_argument("--power-parameters", dest='power_parms',
         help="Power parameters.", default=None)
+    parser.add_argument(
+        "--script-result", metavar="retval", type=int, dest='script_result',
+        help="Return code of a commissioning script.")
 
     parser.add_argument("status",
         help="Status", choices=VALID_STATUS, action='store')
@@ -132,7 +135,11 @@
     params = {
         "op": "signal",
         "status": args.status,
-        "error": args.message}
+        "error": args.message,
+        }
+
+    if args.script_result:
+        params['script_result'] = args.script_result
 
     for ent in args.posts:
         try:

=== modified file 'src/metadataserver/commissioning/user_data.template'
--- src/metadataserver/commissioning/user_data.template	2012-12-18 12:31:47 +0000
+++ src/metadataserver/commissioning/user_data.template	2012-12-19 12:41:21 +0000
@@ -77,10 +77,29 @@
    return 1
 }
 
+# Invoke the "signal()" API call to report progress.
+# Usage: signal <status> <message>
 signal() {
    maas-signal "--config=${CRED_CFG}" "$@"
 }
 
+# Report result of a commissioning script: output file, error output
+# file if there was any error output, and return code.
+# Usage: signal <return-value> <message> <stdout-file> <stderr-file>
+signal_result() {
+   local result=$1 message="$2" stdout="$3" stderr="$4"
+   local files="--file=$stdout"
+   if [ -f "$stderr" -a -s "$stderr" ]
+   then
+      files="$files --file=$stderr"
+   fi
+   maas-signal \
+      "--config=${CRED_CFG}" \
+      "--script-result=$result" \
+      $files \
+      WORKING "$message"
+}
+
 fail() {
    [ -z "$CRED_CFG" ] || signal FAILED "$1"
    echo "FAILED: $1" 1>&2;
@@ -153,10 +172,13 @@
    for script in "${SCRIPTS_D}/"*; do
       [ -f "$script" -a -f "$script" ] || continue
       name=${script##*/}
-      signal WORKING "starting ${script##*/} [$cur/$total]"
+      signal WORKING "starting ${name} [$cur/$total]"
       "$script" > "${OUT_D}/${name}.out" 2> "${OUT_D}/${name}.err"
       ret=$?
-      signal WORKING "finished $name [$cur/$total]: $ret"
+      signal_result \
+         "$ret" "finished $name [$cur/$total]: $ret" \
+         "${OUT_D}/${name}.out" \
+         "${OUT_D}/${name}.err"
       if [ $ret -eq 0 ]; then
           numpass=$(($numpass+1))
           failed="${failed} ${name}"
@@ -164,20 +186,13 @@
       cur=$(($cur+1))
    done
 
-   # Get a list of all files created, ignoring empty ones.
-   local fargs=""
-   for file in "${OUT_D}/"*; do
-      [ -f "$file" -a -s "$file" ] || continue
-      fargs="$fargs --file=${file##*/}"
-   done
-
    if [ $numpass -eq $total ]; then
       ( cd "${OUT_D}" &&
-         signal $fargs OK "finished [$numpass/$total]" )
+         signal OK "finished [$numpass/$total]" )
       return 0
    else
       ( cd "${OUT_D}" &&
-         signal $fargs OK "failed [$numpass/$total] ($failed)" )
+         signal OK "failed [$numpass/$total] ($failed)" )
       return $(($count-$numpass))
    fi