state,worker: full lifecycle machines watcher

state,worker: full lifecycle machines watcher

MachinesWatcher now returns a list of ids that have had their
lifecycle changed in any way. It's up to the consumer to work
out details.

https://code.launchpad.net/~niemeyer/juju-core/full-lifecycle-machines-watcher/+merge/126825

Requires: https://code.launchpad.net/~themue/juju-core/go-firewaller-remove-tests-unit-service/+merge/126651

(do not edit description out of merge proposal)

[niemeyer, 2012-09-27 23:32:40]

Please take a look.

[TheMue, 2012-09-28 13:42:36]

LGTM, WatchPrincipalUnits() should later also be changed into unit names instead
of units.

[aram, 2012-09-28 13:51:00]

LGTM with the changes in the machines watcher test.

Someone who knows more about the firewaller and the provisioner should do a more
thorough review of those bits.

https://codereview.appspot.com/6566066/diff/1/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#oldcode372
state/state_test.go:372: func moreMachinesRequired(got *state.MachinesChange,
alive, dead []int) bool {
good riddance.

https://codereview.appspot.com/6566066/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode247
state/state_test.go:247: summary string
Great.

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode249
state/state_test.go:249: changes []int
I'd still prefer separate {alive,dying,dead} fields here, even though the
watcher returns a single []int because it makes it very easy in the function
below to check that when we get an event, the lifecycle of the entity is really
what we expect it to be.

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode312
state/state_test.go:312: "Report non-Dead machines that are removed",
s/non-Dead/Dead/?

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode417
state/state_test.go:417: c.Assert(got, DeepEquals, test.changes)
We should also check that the lifecycle of the entity is what we expect it to
be. See
https://codereview.appspot.com/6573064/diff/1/state/machine_test.go#newcode830

https://codereview.appspot.com/6566066/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6566066/diff/1/state/watcher.go#newcode174
state/watcher.go:174: type machineLifeDoc struct {
I pondered about this. If we use Select anyway, why don't we use the real
document, machineDoc in this case, and ignore the fields we don't care about?

https://codereview.appspot.com/6566066/diff/1/state/watcher.go#newcode223
state/watcher.go:223: return ids, nil
Interesting, so it's fine not to add it at the end, to preserve apparent order
of events?

[niemeyer, 2012-09-28 17:45:03]

*** Submitted:

state,worker: full lifecycle machines watcher

MachinesWatcher now returns a list of ids that have had their
lifecycle changed in any way. It's up to the consumer to work
out details.

R=TheMue, aram
CC=
https://codereview.appspot.com/6566066

https://codereview.appspot.com/6566066/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode249
state/state_test.go:249: changes []int
On 2012/09/28 13:51:00, aram wrote:
> I'd still prefer separate {alive,dying,dead} fields here, even though the
> watcher returns a single []int because it makes it very easy in the function
> below to check that when we get an event, the lifecycle of the entity is
really
> what we expect it to be.

As we talked online, that doesn't sound necessary. This would be testing that
the code within the test itself is doing what it claims to be doing. E.g. if we
EnsureDead, we don't have to test that the machine is indeed dead, or we'd be
cross-testing that EnsureDead works rather than verifying the behavior of the
watcher, which is the goal here.

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode312
state/state_test.go:312: "Report non-Dead machines that are removed",
On 2012/09/28 13:51:00, aram wrote:
> s/non-Dead/Dead/?

Thanks, I've clarified. It's actually "previously known".

https://codereview.appspot.com/6566066/diff/1/state/state_test.go#newcode417
state/state_test.go:417: c.Assert(got, DeepEquals, test.changes)
On 2012/09/28 13:51:00, aram wrote:
> We should also check that the lifecycle of the entity is what we expect it to
be.

As mentioned above, the test itself is the one enforcing the  lifecycle. This
would test the test, which at least here sounds unnecessary. We're verifying the
watcher behavior itself, given the changes performed by the test, which should
be working unless portions unrelated to the watcher are broken.

https://codereview.appspot.com/6566066/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6566066/diff/1/state/watcher.go#newcode174
state/watcher.go:174: type machineLifeDoc struct {
On 2012/09/28 13:51:00, aram wrote:
> I pondered about this. If we use Select anyway, why don't we use the real
> document, machineDoc in this case, and ignore the fields we don't care about?

I was on the fence on this one, but you're right. I've changed to use machineDoc
as suggested.

https://codereview.appspot.com/6566066/diff/1/state/watcher.go#newcode223
state/watcher.go:223: return ids, nil
On 2012/09/28 13:51:00, aram wrote:
> Interesting, so it's fine not to add it at the end, to preserve apparent order
> of events?

Yeah, it's certainly fine. We don't have guarantees of ordering from the
underlying watcher, and we don't offer those guarantees *within a single event*
upstream either.

Of course, it's not reasonable to report that a machine/unit/whatever is Alive
after it was found Dead.