Merge lp:~julian-edwards/maas/commissioning_error_check_bug_1239668 into lp:~maas-committers/maas/trunk

[Julian Edwards]

Use set -eu on commissioning user data template script, so it exits on errors.

[Gavin Panella (allenap)]

This is fine, but needs QA. There may be parts of the script that return non-zero and have therefore been ignored so far (and seemingly don't matter...) but that will crash the script with -eu.

review: Approve

[Julian Edwards (julian-edwards)]

On Tuesday 15 Oct 2013 08:17:58 you wrote:
> Review: Approve
>
> This is fine, but needs QA. There may be parts of the script that return
> non-zero and have therefore been ignored so far (and seemingly don't
> matter...) but that will crash the script with -eu.

I agree! Which is why I will be doing exactly that very soon :)

[Raphaël Badin (rvb)]

Doesn't this mean that, if script 3 out of 5 fails, script 4 and script 5 won't be run at all? Because that would be bad, we want all the scripts to run no matter what.

[Gavin Panella (allenap)]

> Doesn't this mean that, if script 3 out of 5 fails, script 4 and
> script 5 won't be run at all?  Because that would be bad, we want
> all the scripts to run no matter what.

You're right, it will:

      "$script" > "${OUT_D}/${name}.out" 2> "${OUT_D}/${name}.err"
      ret=$?

This makes me so angry, but I'm going to bite my tongue. The above can
be fixed with:

      ret=0 && "$script" > "${OUT_D}/${name}.out" 2> "${OUT_D}/${name}.err" || ret=$?

Untested, natch; this is shell script rodeo, yeehaw :)

On that topic, how about this from the same script:

      if [ $ret -eq 0 ]; then
          numpass=$(($numpass+1))
          failed="${failed} ${name}"
      fi

$numpass is incremented, but the list of failing scripts is also
updated to include the name of the script that *passed*.

   if [ $numpass -eq $total ]; then
      ( cd "${OUT_D}" &&
         signal OK "finished [$numpass/$total]" )
      return 0
   else
      ( cd "${OUT_D}" &&
         signal FAILED "failed [$numpass/$total] ($failed)" )
      return $(($count-$numpass))
   fi

Following on, that $failed variable will be added to the *failure*
report, which doesn't make much sense. This doesn't affect operation,
but it's misleading.

Also note the $count variable. It's not initialised anywhere, so its
use will crash this script with -u active.

I have no confidence in this script. We need to rewrite it with tests.

On that basis I'm voting against this branch because it'll break what
fragile semblance of working that we have right now and create a long
iteration loop (fix bug -> test -> land -> QA -> file bug *if we spot
it* -> repeat) which will leave MAAS broken for an indeterminate time.

review: Disapprove

[Julian Edwards (julian-edwards)]

Christ. From now on, please consider this to be a total ban on anyone landing [untested] shell script in MAAS.

Unmerged revisions

1705. By Julian Edwards on 2013-10-15

make user_data.template shell script set -eu (exit on errors and unset vars)

=== modified file 'etc/maas/templates/commissioning-user-data/user_data.template'
--- etc/maas/templates/commissioning-user-data/user_data.template	2013-10-07 21:35:59 +0000
+++ etc/maas/templates/commissioning-user-data/user_data.template	2013-10-15 06:01:11 +0000
@@ -8,6 +8,10 @@
 # For each, the main script calls home to maas with maas-signal, posting
 # the script's output as a separate file.
 #
+
+# Exit immediately on uncaught errors and unset variables.
+set -eu
+
 ####  IPMI setup  ######
 # If IPMI network settings have been configured statically, you can
 # make them DHCP. If 'true', the IPMI network source will be changed