An actual Uniter type, would you believe?

An actual Uniter type, would you believe?

It turns out that the unit agent state machine is involved enough that
trying to implement it piecemeal is confusing and unhelpful; but,
thankfully, it is also simple enough that I can implement it as one big
lump and feel reasonably confident that my reviewers won't hate me too
much...

Apart from a couple of small changes to the state API (one of which fixes
lp:1020318, one of which possibly implies a new bug but is possibly just
related to lp:1033852), this change implements the non-relation parts of
the uniter. That is to say that it fixes lp:1037042, lp:1037043,
lp:1037044, lp:1037046, lp:1037047, lp:1037050, and lp:1037067, which
(should?) cover all aspects of installation, upgrade, error resolution,
and service config handling; leaving only the relation work and lifecycle
handling to go.

Note that the majority of the lines in this diff are purely down to
uniter_test.go, and most of that would have to be written to support
any of the interesting test cases; and, similarly, every part of uniter.go
is required to support even basic installation without hook error recovery.

https://code.launchpad.net/~fwereade/juju-core/real-live-uniter/+merge/120315

Requires: https://code.launchpad.net/~fwereade/juju-core/uniter-support-tweaks/+merge/120293

(do not edit description out of merge proposal)

[fwereade, 2012-08-20 02:27:31]

Please take a look.

[fwereade, 2012-08-20 14:36:00]

just a thought...

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

https://codereview.appspot.com/6460110/diff/1/worker/uniter/modes.go#newcode138
worker/uniter/modes.go:138: defer watcher.Stop(config, &u.tomb)
Hmm, that's not right, the Uniter doesn't expect that the tomb will be changed
by modes; they're just meant to respond to it. Need to convert all watcher.Stops
to error returns.

[rog, 2012-08-21 10:29:02]

a few largely superficial comments

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

https://codereview.appspot.com/6460110/diff/1/worker/uniter/modes.go#newcode39
worker/uniter/modes.go:39: } else if hs.Status == hook.StatusStarted {
i'd prefer this without the unnecessary else clauses. also applies elsewhere
where there are if{return}else... patterns.

the else makes the code more dense and makes me think that there might be
something subtle going on that i'm missing.

https://codereview.appspot.com/6460110/diff/1/worker/uniter/modes.go#newcode88
worker/uniter/modes.go:88: if !changedCharm {
invert the case and return here, saving a level of indentation?

https://codereview.appspot.com/6460110/diff/1/worker/uniter/modes.go#newcode138
worker/uniter/modes.go:138: defer watcher.Stop(config, &u.tomb)
On 2012/08/20 14:36:00, fwereade wrote:
> Hmm, that's not right, the Uniter doesn't expect that the tomb will be changed
> by modes; they're just meant to respond to it. Need to convert all
watcher.Stops
> to error returns.

you could of course change the central loop to check for u.tomb.Err() !=
ErrStillRunning.

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

https://codereview.appspot.com/6460110/diff/1/worker/uniter/uniter.go#newcode59
worker/uniter/uniter.go:59: return filepath.Join(append([]string{path},
parts...)...)
this seems a bit complex for what you need.
you could do: p := filepath.Join
then hook.NewStateFile(p(path, "state", "hook")) etc.

https://codereview.appspot.com/6460110/diff/1/worker/uniter/uniter.go#newcode81
worker/uniter/uniter.go:81: select {
i'm not sure why you've got this select here.
why not just u.tomb.Kill(err) ?

https://codereview.appspot.com/6460110/diff/1/worker/uniter/uniter.go#newcode121
worker/uniter/uniter.go:121: bun, err := u.bundles.Read(sch, &u.tomb)
i wonder about passing in the tomb here. all Read really needs is tomb.Dying. i
did something similar to this recently in cmd/jujud and decided that it was more
obvious to pass around a stop channel (of type <-chan struct{}) than the whole
tomb. then it's obvious that the tomb isn't being killed, for example.

https://codereview.appspot.com/6460110/diff/1/worker/uniter/uniter.go#newcode228
worker/uniter/uniter.go:228: path := filepath.Join(environs.VarDir, "units",
strings.Replace(name, "/", "-", 1))
i think i'd use Replace(..., -1); otherwise it's not obvious without looking
elsewhere that the resulting replaced string is free from slashes (maybe it's
deliberately not! in which case a comment is merited). the unit name convention
*may* change in the future - best not to have too many subtle dependencies on
its form, i think.