implement charm upgrades

implement charm upgrades



https://code.launchpad.net/~fwereade/juju-core/uniter-deploy-upgrades/+merge/122846

Requires: https://code.launchpad.net/~fwereade/juju-core/uniter-use-deployer/+merge/122838

(do not edit description out of merge proposal)

[fwereade, 2012-09-05 11:01:59]

Please take a look.

[niemeyer, 2012-09-11 13:12:34]

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

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode83
worker/uniter/modes.go:83: log.Printf("resuming charm upgrade")
I suggest preserving the logic of checking for op.Op, and panicking when
unknown.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode114
worker/uniter/modes.go:114: // * charm upgrade requests (not implemented)
Is this done now?

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode245
worker/uniter/modes.go:245: // upgrade-charm hook if no other hook is in an
error state.
Doesn't look like this mode checks the error state.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode260
worker/uniter/modes.go:260: // upgrading a charm: either by manually resolving
errors and then setting
s/: /. This may be done/

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode262
worker/uniter/modes.go:262: func ModeConflicted(sch *state.Charm) Mode {
We talked about making this generic. Just for awareness, is this down the
pipeline, or not yet done?

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode285
worker/uniter/modes.go:285: case rm, ok := <-resolvedw.Changes():
"mode" would be more descriptive than "rm"

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode292
worker/uniter/modes.go:292: err := u.charm.Snapshotf("user resolved upgrade
conflict")
"Upgrade conflict resolved."

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode294
worker/uniter/modes.go:294: if err == nil {
; e != nil && err == nil {

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode115
worker/uniter/uniter.go:115: // These operations can be interrupted to perform
an upgrade; if hook
This is a bit confusing. op.Op == Upgrade *is* an upgrade, how can we interrupt
an upgrade to perform an upgrade and then restore the previous upgrade?

[fwereade, 2012-09-11 13:22:11]

Thanks, excellent comments, I'll get onto fixing them now.

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

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode83
worker/uniter/modes.go:83: log.Printf("resuming charm upgrade")
On 2012/09/11 13:12:34, niemeyer wrote:
> I suggest preserving the logic of checking for op.Op, and panicking when
> unknown.

Fair enough, I'm constantly torn between the noise and the certainty of these
sorts of check, and have half-evolved a heuristic that says "if it's checked in
*this* package, don't go overboard". But then I'm usually happier when I get
paranoid, so...

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode114
worker/uniter/modes.go:114: // * charm upgrade requests (not implemented)
On 2012/09/11 13:12:34, niemeyer wrote:
> Is this done now?

Whoops, yeah.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode245
worker/uniter/modes.go:245: // upgrade-charm hook if no other hook is in an
error state.
On 2012/09/11 13:12:34, niemeyer wrote:
> Doesn't look like this mode checks the error state.

I don't think it should -- is there a state in which an upgrade is unreasonable?

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode260
worker/uniter/modes.go:260: // upgrading a charm: either by manually resolving
errors and then setting
On 2012/09/11 13:12:34, niemeyer wrote:
> s/: /. This may be done/

Better, thanks.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode262
worker/uniter/modes.go:262: func ModeConflicted(sch *state.Charm) Mode {
On 2012/09/11 13:12:34, niemeyer wrote:
> We talked about making this generic. Just for awareness, is this down the
> pipeline, or not yet done?

Not yet done, I'm afraid, I felt it deserved its own CL and I was wary of
implementing it on top of what still feels like a slightly uncertain pipeline.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode285
worker/uniter/modes.go:285: case rm, ok := <-resolvedw.Changes():
On 2012/09/11 13:12:34, niemeyer wrote:
> "mode" would be more descriptive than "rm"

Hmm, mode means something different in this context, I'd prefer to keep it if
you can bear it ;).

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode292
worker/uniter/modes.go:292: err := u.charm.Snapshotf("user resolved upgrade
conflict")
On 2012/09/11 13:12:34, niemeyer wrote:
> "Upgrade conflict resolved."
Thanks, better.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode294
worker/uniter/modes.go:294: if err == nil {
On 2012/09/11 13:12:34, niemeyer wrote:
> ; e != nil && err == nil {

Good idea

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode115
worker/uniter/uniter.go:115: // These operations can be interrupted to perform
an upgrade; if hook
On 2012/09/11 13:12:34, niemeyer wrote:
> This is a bit confusing. op.Op == Upgrade *is* an upgrade, how can we
interrupt
> an upgrade to perform an upgrade and then restore the previous upgrade?

I should expand on that -- the idea is that we could be resuming, or
force-replacing, an upgrade operation, but that in either case we also need to
preserve any previously-preserved hook error. I'll try to reword it.

[niemeyer, 2012-09-11 13:40:17]

LGTM

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

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode245
worker/uniter/modes.go:245: // upgrade-charm hook if no other hook is in an
error state.
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > Doesn't look like this mode checks the error state.
> 
> I don't think it should -- is there a state in which an upgrade is
unreasonable?

The comment just sounds a bit misleading. Perhaps:

// ModeUpgrading is responsible for upgrading the charm and for running
// the upgrade-charm hook. This mode must not be transitioned to while
// the unit is in an error state.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode285
worker/uniter/modes.go:285: case rm, ok := <-resolvedw.Changes():
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > "mode" would be more descriptive than "rm"
> 
> Hmm, mode means something different in this context, I'd prefer to keep it if
> you can bear it ;).

Definitely.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode115
worker/uniter/uniter.go:115: // These operations can be interrupted to perform
an upgrade; if hook
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > This is a bit confusing. op.Op == Upgrade *is* an upgrade, how can we
> interrupt
> > an upgrade to perform an upgrade and then restore the previous upgrade?
> 
> I should expand on that -- the idea is that we could be resuming, or
> force-replacing, an upgrade operation, but that in either case we also need to
> preserve any previously-preserved hook error. I'll try to reword it.

Appreciated, thanks!

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode155
worker/uniter/uniter.go:155: }[reason],
Btw, that's very pythonic. :-) It took me a moment to figure what was going on.
This would do the same in about the same space, and be more readable and
surprisingly efficient (despite it not mattering here):

hi := &hook.Info{}
switch reason {
case Install:
    hi.Kind = hook.Install
case Upgrade:
    hi.Kind = hook.Upgrade
}

[niemeyer, 2012-09-11 13:40:17]

LGTM

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

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode245
worker/uniter/modes.go:245: // upgrade-charm hook if no other hook is in an
error state.
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > Doesn't look like this mode checks the error state.
> 
> I don't think it should -- is there a state in which an upgrade is
unreasonable?

The comment just sounds a bit misleading. Perhaps:

// ModeUpgrading is responsible for upgrading the charm and for running
// the upgrade-charm hook. This mode must not be transitioned to while
// the unit is in an error state.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode285
worker/uniter/modes.go:285: case rm, ok := <-resolvedw.Changes():
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > "mode" would be more descriptive than "rm"
> 
> Hmm, mode means something different in this context, I'd prefer to keep it if
> you can bear it ;).

Definitely.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode115
worker/uniter/uniter.go:115: // These operations can be interrupted to perform
an upgrade; if hook
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > This is a bit confusing. op.Op == Upgrade *is* an upgrade, how can we
> interrupt
> > an upgrade to perform an upgrade and then restore the previous upgrade?
> 
> I should expand on that -- the idea is that we could be resuming, or
> force-replacing, an upgrade operation, but that in either case we also need to
> preserve any previously-preserved hook error. I'll try to reword it.

Appreciated, thanks!

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode155
worker/uniter/uniter.go:155: }[reason],
Btw, that's very pythonic. :-) It took me a moment to figure what was going on.
This would do the same in about the same space, and be more readable and
surprisingly efficient (despite it not mattering here):

hi := &hook.Info{}
switch reason {
case Install:
    hi.Kind = hook.Install
case Upgrade:
    hi.Kind = hook.Upgrade
}

[fwereade, 2012-09-13 09:35:53]

*** Submitted:

implement charm upgrades

R=niemeyer
CC=
https://codereview.appspot.com/6503072

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

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode83
worker/uniter/modes.go:83: log.Printf("resuming charm upgrade")
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > I suggest preserving the logic of checking for op.Op, and panicking when
> > unknown.
> 
> Fair enough, I'm constantly torn between the noise and the certainty of these
> sorts of check, and have half-evolved a heuristic that says "if it's checked
in
> *this* package, don't go overboard". But then I'm usually happier when I get
> paranoid, so...

Done.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode245
worker/uniter/modes.go:245: // upgrade-charm hook if no other hook is in an
error state.
On 2012/09/11 13:40:17, niemeyer wrote:
> On 2012/09/11 13:22:11, fwereade wrote:
> > On 2012/09/11 13:12:34, niemeyer wrote:
> > > Doesn't look like this mode checks the error state.
> > 
> > I don't think it should -- is there a state in which an upgrade is
> unreasonable?
> 
> The comment just sounds a bit misleading. Perhaps:
> 
> // ModeUpgrading is responsible for upgrading the charm and for running
> // the upgrade-charm hook. This mode must not be transitioned to while
> // the unit is in an error state.

Dropped all mention of errors, as discussed on IRC.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode260
worker/uniter/modes.go:260: // upgrading a charm: either by manually resolving
errors and then setting
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > s/: /. This may be done/
> 
> Better, thanks.

Done.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode260
worker/uniter/modes.go:260: // upgrading a charm: either by manually resolving
errors and then setting
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > s/: /. This may be done/
> 
> Better, thanks.

Done.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode292
worker/uniter/modes.go:292: err := u.charm.Snapshotf("user resolved upgrade
conflict")
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > "Upgrade conflict resolved."
> Thanks, better.

Done; also made the other Snapshotfs use full sentences :).

https://codereview.appspot.com/6503072/diff/1/worker/uniter/modes.go#newcode294
worker/uniter/modes.go:294: if err == nil {
On 2012/09/11 13:22:11, fwereade wrote:
> On 2012/09/11 13:12:34, niemeyer wrote:
> > ; e != nil && err == nil {
> 
> Good idea

Done.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode115
worker/uniter/uniter.go:115: // These operations can be interrupted to perform
an upgrade; if hook
On 2012/09/11 13:40:17, niemeyer wrote:
> On 2012/09/11 13:22:11, fwereade wrote:
> > On 2012/09/11 13:12:34, niemeyer wrote:
> > > This is a bit confusing. op.Op == Upgrade *is* an upgrade, how can we
> > interrupt
> > > an upgrade to perform an upgrade and then restore the previous upgrade?
> > 
> > I should expand on that -- the idea is that we could be resuming, or
> > force-replacing, an upgrade operation, but that in either case we also need
to
> > preserve any previously-preserved hook error. I'll try to reword it.
> 
> Appreciated, thanks!

Done.

https://codereview.appspot.com/6503072/diff/1/worker/uniter/uniter.go#newcode155
worker/uniter/uniter.go:155: }[reason],
On 2012/09/11 13:40:17, niemeyer wrote:
> Btw, that's very pythonic. :-) It took me a moment to figure what was going
on.
> This would do the same in about the same space, and be more readable and
> surprisingly efficient (despite it not mattering here):
> 
> hi := &hook.Info{}
> switch reason {
> case Install:
>     hi.Kind = hook.Install
> case Upgrade:
>     hi.Kind = hook.Upgrade
> }

Haha, I used to think that word was entirely complimentary ;p. Done.