IT LIVES (er, the Uniter, that is)

IT LIVES (er, the Uniter, that is)

Featuring:

  * installation (not really resumable, except by luck)
  * install, start, config-changed hooks
  * error resolution of same, and resuming in the right place
  * response to further service config changes when in ModeStarted
  * slightly too many testing types, that will be used imminently for
    followup branches, and which I am therefore reluctant to delete now

Not Featuring:

  * upgrades/conflict resolution
  * relations
  * anything to do with lifecycles

https://code.launchpad.net/~fwereade/juju-core/it-lives/+merge/121021

Requires: https://code.launchpad.net/~fwereade/juju-core/dumb-charm-manager/+merge/121007

(do not edit description out of merge proposal)

[fwereade, 2012-08-23 15:22:44]

Please take a look.

[niemeyer, 2012-08-23 22:27:37]

This is superb. Thanks a lot for the hard work, certainly paid off.

Some very superficial comments:

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

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode18
worker/uniter/modes.go:18: // ModeInit determines the Mode in which to start a
new Uniter.
// ModeInit is the initial Uniter mode.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode19
worker/uniter/modes.go:19: func ModeInit(u *Uniter) (mode Mode, err error) {
s/mode/next/

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode50
worker/uniter/modes.go:50: // state. Potentially-inconsistent state will be
synchronized.
That last sentence is a bit hard to grasp. Extending it might help a casual
reader (or ourselves in a few weeks) understand why we're in an error state if
status == *started*, or if status == *succeeded*, we're *recovering*.

Also, can we call this something like hookStateMode?

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode81
worker/uniter/modes.go:81: if err != nil && err != errHookFailed {
What happens on errHookFailed? Looks like we're missing a mode: Started,
Succeeded, Committed.. and? Doesn't feel intuitive to leave it "Started"
forever. Or perhaps renaming Started to Pending or similar to fit its role more
properly?

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode190
worker/uniter/modes.go:190: if e := s.Stop(); e != nil && err == nil {
s/err/*err/

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

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode61
worker/uniter/uniter.go:61: }
hookPath := filepath.Join(path, "state", "hook")
charmDir := filepath.Join(path, "charm")
stateDir := filepath.Join(path, "state")

The same three lines, faster and cleaner.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode87
worker/uniter/uniter.go:87: u.tomb.Kill(u.pinger.Stop())
The logic above is a bit suspect. If it's not dying, kill? Why is it ignoring
the error if it is dying?

How about:

u.tomb.Kill(err)
u.tomb.Kill(u.pinger.Stop())

?

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode163
worker/uniter/uniter.go:163: if err := u.charm.WriteStatus(charm.Installed,
nil); err != nil {
The concept here seems a bit unexpected. We're persisting the idea that the
charm was installed despite the fact it wasn't? Isn't it more intuitive to
persist that information once we do know that the charm was actually installed?

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode168
worker/uniter/uniter.go:168: srv, err := server.NewServer(getCmd, socketPath)
I suggest moving that to before the checkpoints done above. Anything between the
checkpoint and the running of the hook increases the window for things to go
wrong.

Also, these state writing methods should be fsyncing.

[fwereade, 2012-08-24 13:14:14]

Please take a look.

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

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode18
worker/uniter/modes.go:18: // ModeInit determines the Mode in which to start a
new Uniter.
On 2012/08/23 22:27:38, niemeyer wrote:
> // ModeInit is the initial Uniter mode.

Done.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode19
worker/uniter/modes.go:19: func ModeInit(u *Uniter) (mode Mode, err error) {
On 2012/08/23 22:27:38, niemeyer wrote:
> s/mode/next/

Done.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode50
worker/uniter/modes.go:50: // state. Potentially-inconsistent state will be
synchronized.
On 2012/08/23 22:27:38, niemeyer wrote:
> That last sentence is a bit hard to grasp. Extending it might help a casual
> reader (or ourselves in a few weeks) understand why we're in an error state if
> status == *started*, or if status == *succeeded*, we're *recovering*.
> 
> Also, can we call this something like hookStateMode?

SGTM. Done.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode81
worker/uniter/modes.go:81: if err != nil && err != errHookFailed {
On 2012/08/23 22:27:38, niemeyer wrote:
> What happens on errHookFailed? Looks like we're missing a mode: Started,
> Succeeded, Committed.. and? Doesn't feel intuitive to leave it "Started"
> forever. Or perhaps renaming Started to Pending or similar to fit its role
more
> properly?

Hmm. Not really so keen on "pending", that implies to me that it hasn't started.
Changed to Running, Committing, Complete, which should be a little clearer.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/modes.go#newcode190
worker/uniter/modes.go:190: if e := s.Stop(); e != nil && err == nil {
On 2012/08/23 22:27:38, niemeyer wrote:
> s/err/*err/

Oof. Thank you.

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

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode61
worker/uniter/uniter.go:61: }
On 2012/08/23 22:27:38, niemeyer wrote:
> hookPath := filepath.Join(path, "state", "hook")
> charmDir := filepath.Join(path, "charm")
> stateDir := filepath.Join(path, "state")
> 
> The same three lines, faster and cleaner.

Done.

/hangs-head-in-shame

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode87
worker/uniter/uniter.go:87: u.tomb.Kill(u.pinger.Stop())
On 2012/08/23 22:27:38, niemeyer wrote:
> The logic above is a bit suspect. If it's not dying, kill? Why is it ignoring
> the error if it is dying?
> 
> How about:
> 
> u.tomb.Kill(err)
> u.tomb.Kill(u.pinger.Stop())
> 
> ?

Ha, yes; and I should return nil instead of ErrDying in the modes, which
actually seems sensible -- Dying is in this instance not an error at all. Done.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode163
worker/uniter/uniter.go:163: if err := u.charm.WriteStatus(charm.Installed,
nil); err != nil {
On 2012/08/23 22:27:38, niemeyer wrote:
> The concept here seems a bit unexpected. We're persisting the idea that the
> charm was installed despite the fact it wasn't? Isn't it more intuitive to
> persist that information once we do know that the charm was actually
installed?

No, we're persisting the fact that the charm really is deployed at this point --
we only get here after completing the charm change. The point of this is to make
the changing-charm and running-hook states overlap: only once the hook state is
written can we make the charm state accurate again.

I have an idea about queueing hooks that might be interesting, I'll look into it
after I've proposed this set of changes.

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode168
worker/uniter/uniter.go:168: srv, err := server.NewServer(getCmd, socketPath)
On 2012/08/23 22:27:38, niemeyer wrote:
> I suggest moving that to before the checkpoints done above. Anything between
the
> checkpoint and the running of the hook increases the window for things to go
> wrong.

SGTM.

> Also, these state writing methods should be fsyncing.

Good point. Done.

[fwereade, 2012-08-24 13:52:44]

Please take a look.

[niemeyer, 2012-08-27 17:24:26]

This is looking great, thank you.

LGTM with just a few trivials:

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

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode87
worker/uniter/uniter.go:87: u.tomb.Kill(u.pinger.Stop())
On 2012/08/24 13:14:14, fwereade wrote:
> On 2012/08/23 22:27:38, niemeyer wrote:
> > The logic above is a bit suspect. If it's not dying, kill? Why is it
ignoring
> > the error if it is dying?
> > 
> > How about:
> > 
> > u.tomb.Kill(err)
> > u.tomb.Kill(u.pinger.Stop())
> > 
> > ?
> 
> Ha, yes; and I should return nil instead of ErrDying in the modes, which
> actually seems sensible -- Dying is in this instance not an error at all.
Done.

Not as a coincidence, ErrDying should work fine in those cases, and I suggest
keeping it. Returning a bad mode with err == nil will be surprising.

https://codereview.appspot.com/6482053/diff/7001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6482053/diff/7001/juju/testing/conn.go#newcode33
juju/testing/conn.go:33: // Reset returns environment state to that which
existed at the start of
This feels pretty suspect. A bit premature to say that as I haven't seen actual
use cases thus far in the review, but it feels like a huge hint that a test
could actually be split in two.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/charm/charm.go
File worker/uniter/charm/charm.go (right):

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/charm/charm.go...
worker/uniter/charm/charm.go:97: var surl *string
s/*//

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/charm/charm.go...
worker/uniter/charm/charm.go:104: return charm.ParseURL(*surl)
s/*//

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/hook/hook.go#n...
worker/uniter/hook/hook.go:95: // Pending indicates that execution of the hook
has started. A hook
s/has started/is pending/ :-)

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

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:49: func ModeContinue(u *Uniter) (next Mode, err error) {
Great idea.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:68: if err := u.runHook(hs.Info); err != nil {
Having this here is quite nice.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:125: return nil, nil
ErrDying was more sensible here. Having wrong result data with err == nil is
error prone.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:167: return nil, nil
Ditto.

[fwereade, 2012-08-28 09:23:41]

*** Submitted:

IT LIVES (er, the Uniter, that is)

Featuring:

  * installation (not really resumable, except by luck)
  * install, start, config-changed hooks
  * error resolution of same, and resuming in the right place
  * response to further service config changes when in ModeStarted
  * slightly too many testing types, that will be used imminently for
    followup branches, and which I am therefore reluctant to delete now

Not Featuring:

  * upgrades/conflict resolution
  * relations
  * anything to do with lifecycles

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

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

https://codereview.appspot.com/6482053/diff/1/worker/uniter/uniter.go#newcode87
worker/uniter/uniter.go:87: u.tomb.Kill(u.pinger.Stop())
On 2012/08/27 17:24:26, niemeyer wrote:
> On 2012/08/24 13:14:14, fwereade wrote:
> > On 2012/08/23 22:27:38, niemeyer wrote:
> > > The logic above is a bit suspect. If it's not dying, kill? Why is it
> ignoring
> > > the error if it is dying?
> > > 
> > > How about:
> > > 
> > > u.tomb.Kill(err)
> > > u.tomb.Kill(u.pinger.Stop())
> > > 
> > > ?
> > 
> > Ha, yes; and I should return nil instead of ErrDying in the modes, which
> > actually seems sensible -- Dying is in this instance not an error at all.
> Done.
> 
> Not as a coincidence, ErrDying should work fine in those cases, and I suggest
> keeping it. Returning a bad mode with err == nil will be surprising. 

As discussed, the issue is the collision between ErrorContextf and ErrDying.

https://codereview.appspot.com/6482053/diff/7001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6482053/diff/7001/juju/testing/conn.go#newcode33
juju/testing/conn.go:33: // Reset returns environment state to that which
existed at the start of
On 2012/08/27 17:24:26, niemeyer wrote:
> This feels pretty suspect. A bit premature to say that as I haven't seen
actual
> use cases thus far in the review, but it feels like a huge hint that a test
> could actually be split in two.

As discussed, this is to let me write the uniter tests as tables but start each
one with a fresh environment.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/charm/charm.go
File worker/uniter/charm/charm.go (right):

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/charm/charm.go...
worker/uniter/charm/charm.go:97: var surl *string
On 2012/08/27 17:24:26, niemeyer wrote:
> s/*//
Huh, what was I thinking :/. Done.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/charm/charm.go...
worker/uniter/charm/charm.go:104: return charm.ParseURL(*surl)
On 2012/08/27 17:24:26, niemeyer wrote:
> s/*//

Done.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/hook/hook.go#n...
worker/uniter/hook/hook.go:95: // Pending indicates that execution of the hook
has started. A hook
On 2012/08/27 17:24:26, niemeyer wrote:
> s/has started/is pending/ :-)

Done.

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

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:125: return nil, nil
On 2012/08/27 17:24:26, niemeyer wrote:
> ErrDying was more sensible here. Having wrong result data with err == nil is
> error prone.

Done.

https://codereview.appspot.com/6482053/diff/7001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:167: return nil, nil
On 2012/08/27 17:24:26, niemeyer wrote:
> Ditto.

Done.