cmd/jujud: implement uniter upgrade

cmd/jujud: implement uniter upgrade



https://code.launchpad.net/~rogpeppe/juju-core/064-uniter-upgrade/+merge/126714

(do not edit description out of merge proposal)

[rog, 2012-09-27 16:06:12]

Please take a look.

[niemeyer, 2012-09-27 17:07:53]

LGTM with trivials, but please wait for William.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode57
cmd/jujud/unit.go:57: log.Printf("exiting to upgrade to %v from %q",
tools.Binary, tools.URL)
("exiting to upgrade from %v to %v (%q)", version.Current, tools.Binary,
tools.URL)

This means we can tell what the jump was.. will be useful when we stumble upon
bugs.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode62
cmd/jujud/unit.go:62: }
log.Printf("uniter error: %v", err)

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode67
cmd/jujud/unit.go:67: log.Printf("rerunning uniter after error: %v", err)
The original logging with "rerunning uniter" still sounds correct. We want to
see the error logged above, no matter which branch of the select we get here.

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/modes.go#newco...
worker/uniter/modes.go:60: return nil, fmt.Errorf("cannot read charm state: %v",
err)
The StateFile itself should say what's wrong, and it already does in some
situations.

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:45: func NewUniter(st *state.State, name string, dataDir
string) *Uniter {
Please sync up with William on these changes.

[fwereade, 2012-09-28 08:32:19]

Record of live discussion, plus a couple of unrelated points.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode53
cmd/jujud/unit.go:53: unit, err := a.runOnce()
As discussed, the unit belongs only with the UpgradedError, and shouldn't really
be allowed out to complicate things here.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode83
cmd/jujud/unit.go:83: }
As discussed live, please use ErrDead (and use that as the only reason for a nil
return, and add a "normal exit 0" stanza to the upstart conf). And don't return
the unit :).

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode85
cmd/jujud/unit.go:85: uniter.NewUniter(st, unit.Name(), a.Conf.DataDir),
I'm OK with the Uniter change though :).

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/modes.go#newco...
worker/uniter/modes.go:60: return nil, fmt.Errorf("cannot read charm state: %v",
err)
On 2012/09/27 17:07:53, niemeyer wrote:
> The StateFile itself should say what's wrong, and it already does in some
> situations.

I agree -- if the error from StateFile was insufficiently helpful, please fix
StateFile rather than just 1 of the N clients :).

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:45: func NewUniter(st *state.State, name string, dataDir
string) *Uniter {
On 2012/09/27 17:07:53, niemeyer wrote:
> Please sync up with William on these changes.

After discussion, I'm ok with these :).

[rog, 2012-09-28 12:06:32]

Please take a look.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode53
cmd/jujud/unit.go:53: unit, err := a.runOnce()
On 2012/09/28 08:32:19, fwereade wrote:
> As discussed, the unit belongs only with the UpgradedError, and shouldn't
really
> be allowed out to complicate things here.

Done.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode57
cmd/jujud/unit.go:57: log.Printf("exiting to upgrade to %v from %q",
tools.Binary, tools.URL)
On 2012/09/27 17:07:53, niemeyer wrote:
> ("exiting to upgrade from %v to %v (%q)", version.Current, tools.Binary,
> tools.URL)
> 
> This means we can tell what the jump was.. will be useful when we stumble upon
> bugs.

done. the upgraded error now includes this information, and is now returned from
Run and will be printed before exit.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode62
cmd/jujud/unit.go:62: }
On 2012/09/27 17:07:53, niemeyer wrote:
> log.Printf("uniter error: %v", err)

done (omitting the "error" which should be self evident from the way we phrase
error messages)

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode67
cmd/jujud/unit.go:67: log.Printf("rerunning uniter after error: %v", err)
On 2012/09/27 17:07:53, niemeyer wrote:
> The original logging with "rerunning uniter" still sounds correct. We want to
> see the error logged above, no matter which branch of the select we get here.

Done.

https://codereview.appspot.com/6561063/diff/3007/cmd/jujud/unit.go#newcode83
cmd/jujud/unit.go:83: }
On 2012/09/28 08:32:19, fwereade wrote:
> As discussed live, please use ErrDead (and use that as the only reason for a
nil
> return, and add a "normal exit 0" stanza to the upstart conf). And don't
return
> the unit :).

Done.

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6561063/diff/3007/worker/uniter/modes.go#newco...
worker/uniter/modes.go:60: return nil, fmt.Errorf("cannot read charm state: %v",
err)
On 2012/09/28 08:32:19, fwereade wrote:
> On 2012/09/27 17:07:53, niemeyer wrote:
> > The StateFile itself should say what's wrong, and it already does in some
> > situations.
> 
> I agree -- if the error from StateFile was insufficiently helpful, please fix
> StateFile rather than just 1 of the N clients :).

ok, will leave for another CL.

[fwereade, 2012-09-28 12:40:51]

LGTM

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

https://codereview.appspot.com/6561063/diff/28/cmd/jujud/machine.go#newcode65
cmd/jujud/machine.go:65: log.Printf("restarting machiner after error: %v", err)
Please make the logging as in the unit agent.

[rog, 2012-09-28 12:53:51]

Please take a look.

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

https://codereview.appspot.com/6561063/diff/28/cmd/jujud/machine.go#newcode65
cmd/jujud/machine.go:65: log.Printf("restarting machiner after error: %v", err)
On 2012/09/28 12:40:51, fwereade wrote:
> Please make the logging as in the unit agent.

Done.

[rog, 2012-09-28 14:20:15]

Please take a look.

[niemeyer, 2012-09-28 14:39:34]

Thank you very much for rolling back the additions.

LGTM, with trivials:

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go#newcode60
cmd/jujud/unit.go:60: log.Printf("uniter: %v", err)
Let's please preserve the original:

    log.Printf("uniter error: %v")

"uniter: nil" doesn't mean much.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode35
cmd/jujud/upgrade.go:35: Tools     *state.Tools
NewTools would fit nicely here.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode40
cmd/jujud/upgrade.go:40: return "must restart: agent has been upgraded"

"an agent upgrade is available"

Strictly speaking, by the time we get the error the agent hasn't been upgraded
yet.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode44
cmd/jujud/upgrade.go:44: func (e *UpgradeReadyError) Upgrade() error {
This is great. I just suggest naming it as "ChangeAgentTools" so that we can
mentally link the call sites to the underlying implementation.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode178
cmd/jujud/upgrade.go:178: return u.newError(currentTools, tools)
s/newError/upgradeReady/?

https://codereview.appspot.com/6561063/diff/5008/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6561063/diff/5008/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:138: fmt.Sprintf("machine-%d", cfg.MachineId),
How's that related to the other changes in the branch?

It doesn't feel like we want this since we have a single one of these per
machine, and this means people have to use wildcards to work with the init
script.

https://codereview.appspot.com/6561063/diff/5008/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6561063/diff/5008/state/machine.go#newcode46
state/machine.go:46: // PathKeys returned by any other entities from the same
state.
s/PathKeys/PathKey values/

As a good convention, when we name methods/functions/types, they should be the
real ones.

[rog, 2012-09-28 15:28:58]

*** Submitted:

cmd/jujud: implement uniter upgrade

R=niemeyer, fwereade
CC=
https://codereview.appspot.com/6561063

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go
File cmd/jujud/unit.go (right):

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/unit.go#newcode60
cmd/jujud/unit.go:60: log.Printf("uniter: %v", err)
On 2012/09/28 14:39:34, niemeyer wrote:
> Let's please preserve the original:
> 
>     log.Printf("uniter error: %v")
> 
> "uniter: nil" doesn't mean much.

Changed to show a better error when it's nil, as discussed on line.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode35
cmd/jujud/upgrade.go:35: Tools     *state.Tools
On 2012/09/28 14:39:34, niemeyer wrote:
> NewTools would fit nicely here.

Done.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode40
cmd/jujud/upgrade.go:40: return "must restart: agent has been upgraded"
On 2012/09/28 14:39:34, niemeyer wrote:
> 
> "an agent upgrade is available"
> 
> Strictly speaking, by the time we get the error the agent hasn't been upgraded
> yet.

Done.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode44
cmd/jujud/upgrade.go:44: func (e *UpgradeReadyError) Upgrade() error {
On 2012/09/28 14:39:34, niemeyer wrote:
> This is great. I just suggest naming it as "ChangeAgentTools" so that we can
> mentally link the call sites to the underlying implementation.

Done.

https://codereview.appspot.com/6561063/diff/5008/cmd/jujud/upgrade.go#newcode178
cmd/jujud/upgrade.go:178: return u.newError(currentTools, tools)
On 2012/09/28 14:39:34, niemeyer wrote:
> s/newError/upgradeReady/?

Done.

https://codereview.appspot.com/6561063/diff/5008/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6561063/diff/5008/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:138: fmt.Sprintf("machine-%d", cfg.MachineId),
On 2012/09/28 14:39:34, niemeyer wrote:
> How's that related to the other changes in the branch?
> 
> It doesn't feel like we want this since we have a single one of these per
> machine, and this means people have to use wildcards to work with the init
> script.

left alone, after discussion.

https://codereview.appspot.com/6561063/diff/5008/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6561063/diff/5008/state/machine.go#newcode46
state/machine.go:46: // PathKeys returned by any other entities from the same
state.
On 2012/09/28 14:39:34, niemeyer wrote:
> s/PathKeys/PathKey values/
> 
> As a good convention, when we name methods/functions/types, they should be the
> real ones.

done. i changed the one it was copied from too.