Dump cloud-init-output.log on failed bootstrap

Dump cloud-init-output.log on failed bootstrap

Currently if bootstrap (or manual provisioning)
fails, we don't get a useful error message, just
"exit status 1" (because the SSH script failed).
This change is to dump the contents of the
cloud-init-output.log file to stderr when the SSH
script fails.

Fixes lp:1279710

https://code.launchpad.net/~axwalk/juju-core/lp1279710-cloudinitoutput-on-error/+merge/211016

(do not edit description out of merge proposal)

[axw, 2014-03-14 10:34:26]

Please take a look.

[rog, 2014-03-14 12:11:38]

LGTM assuming you've tested it live, with a few superficial suggestions below.

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:409: func DumpLogOnError(mcfg *MachineConfig)
string {
This isn't really that deeply connected to MachineConfig,
or environs, and it's perhaps reasonable for the callers to know about
the cloudinit file, so perhaps this might be better in
utils, say, as:

// DumpFileOnErrorScript returns a script that
// prints the contents of the given file to
// standard error if any commands following it exit with an error.
// The returned script is newline terminated.
func DumpFileOnErrorScript(filename string) string

then the callers can use utils.DumpFileOnErrorScript(mcfg.CloudInitOutputLog)

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:418: trap "dump_cloudinit_log" EXIT
i didn't know that EXIT works rather than 0, but as both dash and bash seem to
support it, i guess it's ok.

The quotes are unnecessary here BTW.

https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:319: script = fmt.Sprintf("rm -f %s\n",
utils.ShQuote(mcfg.CloudInitOutputLog)) + script
rather than doing thing backwards, i'd be tempted to build the script up in
order:

var buf bytes.Buffer
// Always remove the cloud-init-output.log file first, if it exists.
fmt.Fprintf(&buf, "rm -f %s\n", ...)
buf.WriteString(cloudinit.DumpLogOnError(mcfg))
configScript, err := sshinit.ConfigureScript(cloudcfg)
...
buf.WriteString(configScript)
return buf.String(), nil

[axw, 2014-03-17 03:21:31]

Please take a look.

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:409: func DumpLogOnError(mcfg *MachineConfig)
string {
On 2014/03/14 12:11:39, rog wrote:
> This isn't really that deeply connected to MachineConfig,
> or environs, and it's perhaps reasonable for the callers to know about
> the cloudinit file, so perhaps this might be better in
> utils, say, as:
> 
> // DumpFileOnErrorScript returns a script that
> // prints the contents of the given file to
> // standard error if any commands following it exit with an error.
> // The returned script is newline terminated.
> func DumpFileOnErrorScript(filename string) string
> 
> then the callers can use utils.DumpFileOnErrorScript(mcfg.CloudInitOutputLog)

Done, though I'm starting a new package: utils/shell. We can move ShQuote and
other shell related things in there over time.

https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.g...
environs/cloudinit/cloudinit.go:418: trap "dump_cloudinit_log" EXIT
On 2014/03/14 12:11:39, rog wrote:
> i didn't know that EXIT works rather than 0, but as both dash and bash seem to
> support it, i guess it's ok.
> 
> The quotes are unnecessary here BTW.

Thanks, removed

https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go...
environs/manual/provisioner.go:319: script = fmt.Sprintf("rm -f %s\n",
utils.ShQuote(mcfg.CloudInitOutputLog)) + script
On 2014/03/14 12:11:39, rog wrote:
> rather than doing thing backwards, i'd be tempted to build the script up in
> order:
> 
> var buf bytes.Buffer
> // Always remove the cloud-init-output.log file first, if it exists.
> fmt.Fprintf(&buf, "rm -f %s\n", ...)
> buf.WriteString(cloudinit.DumpLogOnError(mcfg))
> configScript, err := sshinit.ConfigureScript(cloudcfg)
> ...
> buf.WriteString(configScript)
> return buf.String(), nil

Done.

[rog, 2014-03-17 08:22:38]

On 2014/03/17 03:21:31, axw wrote:
> Please take a look.
> 
> https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.go
> File environs/cloudinit/cloudinit.go (right):
> 
>
https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.g...
> environs/cloudinit/cloudinit.go:409: func DumpLogOnError(mcfg *MachineConfig)
> string {
> On 2014/03/14 12:11:39, rog wrote:
> > This isn't really that deeply connected to MachineConfig,
> > or environs, and it's perhaps reasonable for the callers to know about
> > the cloudinit file, so perhaps this might be better in
> > utils, say, as:
> > 
> > // DumpFileOnErrorScript returns a script that
> > // prints the contents of the given file to
> > // standard error if any commands following it exit with an error.
> > // The returned script is newline terminated.
> > func DumpFileOnErrorScript(filename string) string
> > 
> > then the callers can use
utils.DumpFileOnErrorScript(mcfg.CloudInitOutputLog)
> 
> Done, though I'm starting a new package: utils/shell. We can move ShQuote and
> other shell related things in there over time.
> 
>
https://codereview.appspot.com/75990043/diff/1/environs/cloudinit/cloudinit.g...
> environs/cloudinit/cloudinit.go:418: trap "dump_cloudinit_log" EXIT
> On 2014/03/14 12:11:39, rog wrote:
> > i didn't know that EXIT works rather than 0, but as both dash and bash seem
to
> > support it, i guess it's ok.
> > 
> > The quotes are unnecessary here BTW.
> 
> Thanks, removed
> 
> https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go
> File environs/manual/provisioner.go (right):
> 
>
https://codereview.appspot.com/75990043/diff/1/environs/manual/provisioner.go...
> environs/manual/provisioner.go:319: script = fmt.Sprintf("rm -f %s\n",
> utils.ShQuote(mcfg.CloudInitOutputLog)) + script
> On 2014/03/14 12:11:39, rog wrote:
> > rather than doing thing backwards, i'd be tempted to build the script up in
> > order:
> > 
> > var buf bytes.Buffer
> > // Always remove the cloud-init-output.log file first, if it exists.
> > fmt.Fprintf(&buf, "rm -f %s\n", ...)
> > buf.WriteString(cloudinit.DumpLogOnError(mcfg))
> > configScript, err := sshinit.ConfigureScript(cloudcfg)
> > ...
> > buf.WriteString(configScript)
> > return buf.String(), nil
> 
> Done.

LGTM