uniter: integrate filter type

uniter: integrate filter type

By tweaking the filter slightly, to provide more carefully-tailored events,
the uniter Modes have been radically simplified.

Note that state.Unit was previously mis-specified, and is now fixed: it's
fine (in fact, potentially necessary) to resolve errors when the unit is
Dying.

Also note that there are roughly 50% more full Uniter tests than the
previous branch, but they run in roughly the same amount of time. Win!
But! This is actually a significant slowdown, because I fixed a bunch of
happy-path 500ms waits in the uniter tests. It's true that this variant does
more unnecessary work in the service of simpler Mode code, and I haven't been
able to find any obvious hotspots (apart from the pre-existing one) so if
reviewers would bear subtle performance implications in mind I would be most
grateful :).

https://code.launchpad.net/~fwereade/juju-core/uniter-use-filter/+merge/127159

Requires: https://code.launchpad.net/~fwereade/juju-core/uniter-filter-type/+merge/127142

(do not edit description out of merge proposal)

[fwereade, 2012-09-30 20:04:18]

Please take a look.

[rog, 2012-10-01 11:02:58]

LGTM - very nice improvements.

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

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:159: return nil, fmt.Errorf("insane uniter state: %#v",
s)
"insane uniter state; expected Abide, got %#v", s) ?
nice to say what we're expecting to see. same applies
for similar error messages below.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:286: func modeContext(name string, next *Mode, err
*error) func() {
as discussed online, most/all of this could be moved into the main loop.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:325: if e := u.unit.ClearResolved(); e != nil {
do we actually want the ClearResolved error to override the rf error? if we do,
then perhaps a comment saying why might be appropriate here.

[niemeyer, 2012-10-01 22:47:33]

That's a great CL. Probably ready after this round:

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

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:165: // Execute an initial config-changed hook regardless
of state.
s/regardless of state//; We only got here after quite a bit of state-checking.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:171: case <-u.configEvents():
It'd be good to have a comment explaining why we have the logic here in that
shape. I suspect the reasoning is that we might have a config event either way,
and we want to catch it and discard it, but it's not obvious why we:

1) Say we want an event above

2) Receive the event

3) Ignore it completely

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:177: // Watch for everything else (including further
config changes).
d

It's not watching everything else. It's watching the relevant channels for the
context, as usual.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:286: func modeContext(name string, next *Mode, err
*error) func() {
On 2012/10/01 11:02:58, rog wrote:
> as discussed online, most/all of this could be moved into the main loop.

+1 on turning this into straightforward code. Some of that shouldn't go to the
main loop, though, as it's magically manipulating the contract of the modes.
More details below.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:292: if *next == nil {
This seems to obscure the logic without benefit. When we want ModeContinue, we
should return ModeContinue.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:296: *next, *err = ModeHookError, nil
Same idea. The state machine is being hidden behind the magic. If you want to
avoid typing if err == foo too often, it's trivial to make this with forward
logic by having:

    return runHook(...)

Where this function returns (next Mode, err error).

This may sound silly, but a person reading this code will understand what it is
doing, while a person that reads "return nil, runHook(...)", has to dig what the
heck is going on when we return nil, and why does it not stop running, and why
in certain cases the error doesn't actually stop the state machine.

This is all avoided if we keep the convention simple and strong:

1) Errors stop the state machine

2) First parameter has the next mode

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:150: case <-u.unitDying():
This double check is a bit unexpected. We have dying checks in the modes. If
there was a race and the unit started dying just after we last checked, we may
see the race race 1 nanosecond later, when the unit gets into the "default"
branch below.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:309: // operations. It does nothing when rm is
ResolvedNone; otherwise, it calls
The handling of resolved still feels very convoluted for how trivial the meat of
these functions is.

Rather than attempting to agree on something we're both happy with right now, I
suggest inlining the real functionality in the modes themselves for now. There
are only two occurrences, and they're not even that similar to each other. We
can easily factor it out once we have more time and a plan.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:314: if rm == state.ResolvedNone {
This seems to be consistently ignored. Why are we even sending it in the filter
channel?

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:323: panic(fmt.Errorf("unhandled resolved mode %q", rf))
Ditto. If we can't handle, we shouldn't send. Panicking will mean we can't
introduce a mode without exploding existing units. Better keeping it an error
state instead.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:302: // Reaction to entity deaths.
Nice testing.

[fwereade, 2012-10-02 07:01:35]

Thanks again. Nothing controversial, I think :).

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

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:165: // Execute an initial config-changed hook regardless
of state.
On 2012/10/01 22:47:33, niemeyer wrote:
> s/regardless of state//; We only got here after quite a bit of state-checking.

SGTM

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:171: case <-u.configEvents():
On 2012/10/01 22:47:33, niemeyer wrote:
> It'd be good to have a comment explaining why we have the logic here in that
> shape. I suspect the reasoning is that we might have a config event either
way,
> and we want to catch it and discard it, but it's not obvious why we:
> 
> 1) Say we want an event above
> 
> 2) Receive the event
> 
> 3) Ignore it completely

The rationale is that we want to run one config-changed event here, and only run
further ones in the upcoming loop when they correspond to a different config. So
we prime the watcher to deliver a guaranteed event here, and can then rest
assured that it won't send any more until it gets one newer than the one it sent
in this loop, where we handled it.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:177: // Watch for everything else (including further
config changes).
On 2012/10/01 22:47:33, niemeyer wrote:
> d
> 
> It's not watching everything else. It's watching the relevant channels for the
> context, as usual.

Ha, yes.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:296: *next, *err = ModeHookError, nil
On 2012/10/01 22:47:33, niemeyer wrote:
> Same idea. The state machine is being hidden behind the magic. If you want to
> avoid typing if err == foo too often, it's trivial to make this with forward
> logic by having:
> 
>     return runHook(...)
> 
> Where this function returns (next Mode, err error).
> 
> This may sound silly, but a person reading this code will understand what it
is
> doing, while a person that reads "return nil, runHook(...)", has to dig what
the
> heck is going on when we return nil, and why does it not stop running, and why
> in certain cases the error doesn't actually stop the state machine.
> 
> This is all avoided if we keep the convention simple and strong:
> 
> 1) Errors stop the state machine
> 
> 2) First parameter has the next mode

Good points all. Will drop the ModeContinue bits entirely; would anyone object
to putting the errHookFailed handling explicitly in he main loop?

[niemeyer, 2012-10-02 14:12:58]

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

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:296: *next, *err = ModeHookError, nil
On 2012/10/02 07:01:35, fwereade wrote:
> On 2012/10/01 22:47:33, niemeyer wrote:
> > This is all avoided if we keep the convention simple and strong:
> > 
> > 1) Errors stop the state machine
> > 
> > 2) First parameter has the next mode
> 
> Good points all. Will drop the ModeContinue bits entirely; would anyone object
> to putting the errHookFailed handling explicitly in he main loop?

I'd much prefer having it close to the logic that handles the error instead, and
holding the conventions mentioned above true. There's nothing special about this
error as far as the state machine is concerned.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:1: package uniter
Please note that there are comments in this file too.

[fwereade, 2012-10-02 20:22:07]

Responses to comments I missed before. Hopefully nothing too controversial?

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:1: package uniter
On 2012/10/02 14:12:59, niemeyer wrote:
> Please note that there are comments in this file too.
Ha, I remember reading them, not sure how I missed them last pass. Thanks.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:150: case <-u.unitDying():
On 2012/10/01 22:47:33, niemeyer wrote:
> This double check is a bit unexpected. We have dying checks in the modes. If
> there was a race and the unit started dying just after we last checked, we may
> see the race race 1 nanosecond later, when the unit gets into the "default"
> branch below.

OK, ModeAbide has a unitDying check, but the others don't; in ModeConflicted and
ModeHookError, there's no check and (because of this) no difference between the
Alive and Dying loops (neither of which requires a unitDying check).

In ModeAbide, there is an entirely harmless race between a charm upgrade and a
dying unit, but I see no outcomes that are not also possible in a non-racy
scenario. We either upgrade or we don't, and the object here is *not* to make a
last-nanosecond decision: it's to make that decision in the first place, along
with all the other decisions that follow.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:309: // operations. It does nothing when rm is
ResolvedNone; otherwise, it calls
On 2012/10/01 22:47:33, niemeyer wrote:
> The handling of resolved still feels very convoluted for how trivial the meat
of
> these functions is.
> 
> Rather than attempting to agree on something we're both happy with right now,
I
> suggest inlining the real functionality in the modes themselves for now. There
> are only two occurrences, and they're not even that similar to each other. We
> can easily factor it out once we have more time and a plan.

OK, SGTM, as long as you can live with the unitDying check in getUpgrade --
that's what causes there to be only two uses of this.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:314: if rm == state.ResolvedNone {
On 2012/10/01 22:47:33, niemeyer wrote:
> This seems to be consistently ignored. Why are we even sending it in the
filter
> channel?

It seemed like a bad fit with the "want" model, but after live discussion i
think it could work nicely. Cheers.

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:323: panic(fmt.Errorf("unhandled resolved mode %q", rf))
On 2012/10/01 22:47:33, niemeyer wrote:
> Ditto. If we can't handle, we shouldn't send. Panicking will mean we can't
> introduce a mode without exploding existing units. Better keeping it an error
> state instead.

SGTM

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:325: if e := u.unit.ClearResolved(); e != nil {
On 2012/10/01 11:02:58, rog wrote:
> do we actually want the ClearResolved error to override the rf error? if we
do,
> then perhaps a comment saying why might be appropriate here.

Hmm. yeah, I will re-examine this with a clearer head tomorrow.

[niemeyer, 2012-10-02 22:44:34]

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/2001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:150: case <-u.unitDying():
On 2012/10/02 20:22:07, fwereade wrote:
> On 2012/10/01 22:47:33, niemeyer wrote:
> > This double check is a bit unexpected. We have dying checks in the modes. If
> > there was a race and the unit started dying just after we last checked, we
may
> > see the race race 1 nanosecond later, when the unit gets into the "default"
> > branch below.
> 
> OK, ModeAbide has a unitDying check, but the others don't; in ModeConflicted
and
> ModeHookError, there's no check and (because of this) no difference between
the
> Alive and Dying loops (neither of which requires a unitDying check).
> 
> In ModeAbide, there is an entirely harmless race between a charm upgrade and a
> dying unit, but I see no outcomes that are not also possible in a non-racy
> scenario. We either upgrade or we don't, and the object here is *not* to make
a
> last-nanosecond decision: it's to make that decision in the first place, along
> with all the other decisions that follow.

Understood, but this doesn't look like the right place for this decision. If we
shouldn't notify about new charms when the unit is dying, why are we doing so in
the first place?

[fwereade, 2012-10-04 13:31:04]

Please take a look.

[fwereade, 2012-10-04 13:34:32]

It crosses my mind, incidentally, that all these charm events are actually
upgrade events. I will fix that before submitting unless anyone objects.

[fwereade, 2012-10-04 13:38:21]

Please take a look.

[niemeyer, 2012-10-04 17:48:11]

Some awesome progress here.

A few comments:

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/filter.go#new...
worker/uniter/filter.go:64: unitChanged := func() error {
As we talked online, this is awesome. Let's just try to make these into actual
methods.

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/filter.go#new...
worker/uniter/filter.go:140: // Consume the initial config change so that we can
send a config
It's not clear to me how that changes anything below. We'll still be subject to
the pseudo-random order of selects in terms of what will happen first.

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

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:304: return ModeUpgrading(sch), nil
Upgrading?

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/uniter.go#new...
worker/uniter/uniter.go:79: log.Printf("unit %q started pinger", u.unit)
I'd prefer to have a single message logged at the top that says:

    "unit %q started"

Everything being done here is a consequence of that, and will be executed unless
we see an error that will be reported.

[fwereade, 2012-10-04 21:56:35]

Please take a look.

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/filter.go#new...
worker/uniter/filter.go:64: unitChanged := func() error {
On 2012/10/04 17:48:12, niemeyer wrote:
> As we talked online, this is awesome. Let's just try to make these into actual
> methods.

Done.

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/filter.go#new...
worker/uniter/filter.go:140: // Consume the initial config change so that we can
send a config
On 2012/10/04 17:48:12, niemeyer wrote:
> It's not clear to me how that changes anything below. We'll still be subject
to
> the pseudo-random order of selects in terms of what will happen first.

Discussed online; replacement comment should be clearer.

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

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/modes.go#newc...
worker/uniter/modes.go:304: return ModeUpgrading(sch), nil
On 2012/10/04 17:48:12, niemeyer wrote:
> Upgrading?

The only way to get into ModeConflicted is via ModeUpgrading -- installing just
pulls into a fresh dir and is unlikely to encounter conflicts ;).

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6588053/diff/14001/worker/uniter/uniter.go#new...
worker/uniter/uniter.go:79: log.Printf("unit %q started pinger", u.unit)
On 2012/10/04 17:48:12, niemeyer wrote:
> I'd prefer to have a single message logged at the top that says:
> 
>     "unit %q started"
> 
> Everything being done here is a consequence of that, and will be executed
unless
> we see an error that will be reported.

Done.

[niemeyer, 2012-10-04 22:39:56]

LGTM, with a few last suggestions for your consideration:

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:34: // The want* chans are used to indicate that the
filter should sent
s/sent/send/

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:44: rm          state.ResolvedMode
Can we have this as "resolved"?

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:48: gotForce    bool
I'm finding slightly hard to read the code involving those variable names as
intended. I suggest something like this instead:

var upgradeRequested serviceCharm
var upgradeAvailable serviceCharm

type serviceCharm struct {
    url   *charm.URL
    force bool
}

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:170: // event to be sent explicitly).
This is alluding to the problem without explaining it. This describes the issue
more directly:

// Consume first event so that this sequence doesn't happen:
//
// 1) WantConfigEvent called; channel unblocked
// 2) Config event sent
// 3) Initial config event received; channel unblocked
// 4) Config event sent again, for the same configuration

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:221: // On request, ensure that if an event is available
it will be resent.
// Handle explicit requests for events.

(this is not necessarily about *resending*)

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:296: log.Debugf("filter: no new charm event")
This doesn't sound right. There's a path above that unblocks the channel and we
get here even then.

[fwereade, 2012-10-05 00:00:19]

*** Submitted:

uniter: integrate filter type

By tweaking the filter slightly, to provide more carefully-tailored events,
the uniter Modes have been radically simplified.

Note that state.Unit was previously mis-specified, and is now fixed: it's
fine (in fact, potentially necessary) to resolve errors when the unit is
Dying.

Also note that there are roughly 50% more full Uniter tests than the
previous branch, but they run in roughly the same amount of time. Win!
But! This is actually a significant slowdown, because I fixed a bunch of
happy-path 500ms waits in the uniter tests. It's true that this variant does
more unnecessary work in the service of simpler Mode code, and I haven't been
able to find any obvious hotspots (apart from the pre-existing one) so if
reviewers would bear subtle performance implications in mind I would be most
grateful :).

R=rog, niemeyer
CC=
https://codereview.appspot.com/6588053

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:34: // The want* chans are used to indicate that the
filter should sent
On 2012/10/04 22:39:56, niemeyer wrote:
> s/sent/send/

Done.

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:44: rm          state.ResolvedMode
On 2012/10/04 22:39:56, niemeyer wrote:
> Can we have this as "resolved"?

Done.

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:48: gotForce    bool
On 2012/10/04 22:39:56, niemeyer wrote:
> I'm finding slightly hard to read the code involving those variable names as
> intended. I suggest something like this instead:
> 
> var upgradeRequested serviceCharm
> var upgradeAvailable serviceCharm
> 
> type serviceCharm struct {
>     url   *charm.URL
>     force bool
> }

Done.

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:170: // event to be sent explicitly).
On 2012/10/04 22:39:56, niemeyer wrote:
> This is alluding to the problem without explaining it. This describes the
issue
> more directly:
> 
> // Consume first event so that this sequence doesn't happen:
> //
> // 1) WantConfigEvent called; channel unblocked
> // 2) Config event sent
> // 3) Initial config event received; channel unblocked
> // 4) Config event sent again, for the same configuration

Done.

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:221: // On request, ensure that if an event is available
it will be resent.
On 2012/10/04 22:39:56, niemeyer wrote:
> // Handle explicit requests for events.
> 
> (this is not necessarily about *resending*)

Done.

https://codereview.appspot.com/6588053/diff/17001/worker/uniter/filter.go#new...
worker/uniter/filter.go:296: log.Debugf("filter: no new charm event")
On 2012/10/04 22:39:56, niemeyer wrote:
> This doesn't sound right. There's a path above that unblocks the channel and
we
> get here even then.

Done.