Machine agent runs upgrade operations

Machine agent runs upgrade operations

When the machine agent starts, it runs whatever upgrade
operations are relevant according to the agents's jobs.
The upgrades need to complete before any of the workers
are started.

Changes to machine agent tests were needed because now
a machine needs to be properly provisioned for the agent to
start up, since the access to the state api will fail unless
a machine is provisioned.

Also included are some drive by fixes to some syslog and 
version tests from other reviews.

https://code.launchpad.net/~wallyworld/juju-core/machine-agent-run-upgrades/+merge/208060

Requires: https://code.launchpad.net/~wallyworld/juju-core/delete-rsyslog-upgrader/+merge/208058

(do not edit description out of merge proposal)

[wallyworld, 2014-02-25 04:21:41]

Please take a look.

[rog, 2014-02-25 09:02:15]

Looks great, thanks, with some suggestions below and the syslog templates fixed.

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/machine.go#newcode111
cmd/jujud/machine.go:111: func (a *MachineAgent) startWorkerAfterUpgrade(runner
worker.Runner, name string, start func() (worker.Worker, error)) {
Could we move this code to later on in the file, please,
and order it  so that the code continues to read from top to bottom?

I think that adds significantly to the accessibility of the code.

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/upgrade_test.go#newc...
cmd/jujud/upgrade_test.go:65: go func() { c.Check(a.Run(nil), gc.IsNil) }()
This check is not guaranteed to execute before the test
completes. Since Stop is guaranteed to return the same error as
Run, we could just omit the check. Alternatively, we could do something like:

done := make(chan error)
go func() {
    done <- a.Run(nil)
}()
defer func() {
    c.Check(a.Stop(), gc.IsNil)
    c.Check(<-done, gc.IsNil)
}()

All that said though, I see that this pattern is common
in the test code - perhaps better to leave as is and
fix all the cases at once. I don't think it's actively
harmful as is.

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf.go
File log/syslog/testing/syslogconf.go (right):

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:28: $template
JujuLogFormat{{namespace}},"%syslogtag:{{offset}}:$%%msg:::sp-if-no-1st-sp%%msg:::drop-last-lf%\n"
With the template change to use structs rather
than functions, this should be:

$template
JujuLogFormat{{.Namespace}},"%syslogtag:{{.Offset}}:$%%msg:::sp-if-no-1st-sp%%msg:
    ::drop-last-lf%\n"

(and similar changes whereever we're now using fields instead
of functions).

I'm a bit surprised this change passed the tests - we should really
have something that checks the output, even if it involves cut
and paste when the template changes.

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:54: Offset:     6 + len(namespace),
Do you know what the magic "6" represents here? It would be nicer to use
len("somestring") so we know
what the offset is representing.

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:67: $InputFileTag
juju{{namespace}}-{{machine}}:
s/machine/.Machine/

s/namespace/.Namespace/

etc

https://codereview.appspot.com/68250046/diff/1/upgrades/upgrade.go
File upgrades/upgrade.go (right):

https://codereview.appspot.com/68250046/diff/1/upgrades/upgrade.go#newcode132
upgrades/upgrade.go:132: if targetVersion.Compare(from) < 1 {
s/< 1/<= 0/

please. Then the "less than or equal" comparison actually uses
the "<=" operator (x.Compare(y) OP 0 === x OP y
when OP is a comparison operator)

https://codereview.appspot.com/68250046/diff/1/upgrades/upgrade.go#newcode174
upgrades/upgrade.go:174: }
logger.Infof("All upgrade steps completed successfully")

?

[wallyworld, 2014-02-26 04:23:35]

Please take a look.

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/machine.go#newcode111
cmd/jujud/machine.go:111: func (a *MachineAgent) startWorkerAfterUpgrade(runner
worker.Runner, name string, start func() (worker.Worker, error)) {
On 2014/02/25 09:02:15, rog wrote:
> Could we move this code to later on in the file, please,
> and order it  so that the code continues to read from top to bottom?
> 
> I think that adds significantly to the accessibility of the code.

Done.

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/68250046/diff/1/cmd/jujud/upgrade_test.go#newc...
cmd/jujud/upgrade_test.go:65: go func() { c.Check(a.Run(nil), gc.IsNil) }()
On 2014/02/25 09:02:15, rog wrote:
> This check is not guaranteed to execute before the test
> completes. Since Stop is guaranteed to return the same error as
> Run, we could just omit the check. Alternatively, we could do something like:
> 
> done := make(chan error)
> go func() {
>     done <- a.Run(nil)
> }()
> defer func() {
>     c.Check(a.Stop(), gc.IsNil)
>     c.Check(<-done, gc.IsNil)
> }()
> 
> All that said though, I see that this pattern is common
> in the test code - perhaps better to leave as is and
> fix all the cases at once. I don't think it's actively
> harmful as is.

Yeah, I copied what I saw in other places. I'd think best to fix everything in
one go.

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf.go
File log/syslog/testing/syslogconf.go (right):

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:28: $template
JujuLogFormat{{namespace}},"%syslogtag:{{offset}}:$%%msg:::sp-if-no-1st-sp%%msg:::drop-last-lf%\n"
On 2014/02/25 09:02:15, rog wrote:
> With the template change to use structs rather
> than functions, this should be:
> 
> $template
>
JujuLogFormat{{.Namespace}},"%syslogtag:{{.Offset}}:$%%msg:::sp-if-no-1st-sp%%msg:
>     ::drop-last-lf%\n"
> 
> (and similar changes whereever we're now using fields instead
> of functions).
> 
> I'm a bit surprised this change passed the tests - we should really
> have something that checks the output, even if it involves cut
> and paste when the template changes.

Because this branch is part of a pipe, and the previous pipe was added later and
removed the rsyslog upgrade code (and tests), the jujud and upgrade tests
passed. So I forgot that this code would not be used in that bit anymore. I
didn't run the entire suite yet which would have included other syslog tests.

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:54: Offset:     6 + len(namespace),
On 2014/02/25 09:02:15, rog wrote:
> Do you know what the magic "6" represents here? It would be nicer to use
> len("somestring") so we know
> what the offset is representing.

len("juju-") + 1

https://codereview.appspot.com/68250046/diff/1/log/syslog/testing/syslogconf....
log/syslog/testing/syslogconf.go:67: $InputFileTag
juju{{namespace}}-{{machine}}:
On 2014/02/25 09:02:15, rog wrote:
> s/machine/.Machine/
> 
> s/namespace/.Namespace/
> 
> etc

Done.

https://codereview.appspot.com/68250046/diff/1/upgrades/upgrade.go
File upgrades/upgrade.go (right):

https://codereview.appspot.com/68250046/diff/1/upgrades/upgrade.go#newcode132
upgrades/upgrade.go:132: if targetVersion.Compare(from) < 1 {
On 2014/02/25 09:02:15, rog wrote:
> s/< 1/<= 0/
> 
> please. Then the "less than or equal" comparison actually uses
> the "<=" operator (x.Compare(y) OP 0 === x OP y
> when OP is a comparison operator)

Done.

https://codereview.appspot.com/68250046/diff/1/upgrades/upgrade.go#newcode174
upgrades/upgrade.go:174: }
On 2014/02/25 09:02:15, rog wrote:
> logger.Infof("All upgrade steps completed successfully")
> 
> ?

Done.

[thumper, 2014-02-27 01:59:20]

I can see that you have addressed Rog's comments, although I'd also like to know
why the templating change didn't break any tests.  Can you check to see that
there is test coverage there, and why it didn't break?

LGTM with test confirmation.

[wallyworld, 2014-02-27 02:03:02]

On 2014/02/27 01:59:20, thumper wrote:
> I can see that you have addressed Rog's comments, although I'd also like to
know
> why the templating change didn't break any tests.  Can you check to see that
> there is test coverage there, and why it didn't break?
> 
> LGTM with test confirmation.

It didn't break any jujud tests because the upstream branch removed the rsyslog
upgrader and this was done after the template changes. The syslog tests did fail
but I didn't run them as I forgot that removing the rsyslog upgrader would cause
the templating stuff not to be executed.