state: automatically assign machines to subsidiary units from their principal units.

state: automatically assign machines to subsidiary units from their principal units.



https://code.launchpad.net/~rogpeppe/juju-core/assign-subordinate-units/+merge/109885

(do not edit description out of merge proposal)

[rog, 2012-06-12 17:08:43]

Please take a look.

[bcsaller, 2012-06-12 17:13:50]

https://codereview.appspot.com/6299073/diff/4001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6299073/diff/4001/state/topology.go#newcode402
state/topology.go:402: for _, svc := range t.topology.Services {
Subordinates can be added well after the principal is assigned to a machine and
won't see this call. Additionally the MA watches for machine assignments to take
deployment action which is not wanted/needed for subordinates (thats the
principal UA's job). A simple proxy for subordinate units to return their
containers machine id should be enough. 

What problem does this solve?

[rog, 2012-06-12 17:45:35]

https://codereview.appspot.com/6299073/diff/4001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6299073/diff/4001/state/topology.go#newcode402
state/topology.go:402: for _, svc := range t.topology.Services {
On 2012/06/12 17:13:50, bcsaller wrote:
> Subordinates can be added well after the principal is assigned to a machine
and
> won't see this call. Additionally the MA watches for machine assignments to
take
> deployment action which is not wanted/needed for subordinates (thats the
> principal UA's job).

i agree with this, thanks a lot for the feedback.
i now think this branch is misguided, and will reject it tomorrow unless someone
thinks otherwise.

[niemeyer, 2012-06-12 20:32:55]

https://codereview.appspot.com/6299073/diff/4001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6299073/diff/4001/state/topology.go#newcode402
state/topology.go:402: for _, svc := range t.topology.Services {
On 2012/06/12 17:45:35, rog wrote:
> On 2012/06/12 17:13:50, bcsaller wrote:
> > Subordinates can be added well after the principal is assigned to a machine
> and
> > won't see this call. Additionally the MA watches for machine assignments to
> take
> > deployment action which is not wanted/needed for subordinates (thats the
> > principal UA's job).
> 
> i agree with this, thanks a lot for the feedback.
> i now think this branch is misguided, and will reject it tomorrow unless
someone
> thinks otherwise.

Agreed. I suggest not fiddling with the machine information back and forth, and
instead having UnitsForMachine checking for subordinate units too.

Our high-level Unit implementation can have a sensible IsPrincipal
implementation that does not involve touching ZooKeeper further (we know if it's
a subordinate or not from topoUnit).

I suggest not having principal-specific methods at the high-level, or we'll end
up making the same kind of mistake we've made here, by handling only half of the
units by mistake in some cases.

[rog, 2012-06-13 15:55:12]

Please take a look.

[rog, 2012-06-13 16:05:56]

Please take a look.

[TheMue, 2012-06-13 16:58:42]

On 2012/06/13 16:05:56, rog wrote:
> Please take a look.

Mostly LGTM, only two little remarks.

[TheMue, 2012-06-13 16:58:56]

https://codereview.appspot.com/6299073/diff/1004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/service.go#newcode211
state/service.go:211: panic(err)
Mostly we return those errors. Why a panic() here?

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go#newcode1024
state/state_test.go:1024: bundle := testing.Charms.Bundle(c.MkDir(), "logging")
While addDummyCharm() is a method using the suite and the state of it
addLoggingCharm() is a function taking the state as an argument. Both are
several times called directly after each other. This is confusing.

This should be harmonized. A stand-alone function like here seems to be more
flexible. So addDummyCharm() should be changed to a function too.

[niemeyer, 2012-06-13 19:30:25]

LGTM

https://codereview.appspot.com/6299073/diff/1004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/service.go#newcode211
state/service.go:211: panic(err)
On 2012/06/13 16:58:56, TheMue wrote:
> Mostly we return those errors. Why a panic() here?

Agreed. This is an inconsistency in the topology, but we've been reporting these
inconsistencies as errors elsewhere.

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go#newcode370
state/state_test.go:370: subsidiaries []*state.Unit
s/subsidiaries/subordinates/

There are other occurrences of that in the CL.

https://codereview.appspot.com/6299073/diff/1004/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/topology.go#newcode352
state/topology.go:352: if unit.isPrincipal() {
Much more readable, thanks.

[rog, 2012-06-13 21:20:46]

https://codereview.appspot.com/6299073/diff/1004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/service.go#newcode211
state/service.go:211: panic(err)
On 2012/06/13 19:30:25, niemeyer wrote:
> On 2012/06/13 16:58:56, TheMue wrote:
> > Mostly we return those errors. Why a panic() here?
> 
> Agreed. This is an inconsistency in the topology, but we've been reporting
these
> inconsistencies as errors elsewhere.

If serviceAndUnit is returning an eror then there's something very wrong going
on, as we just got the key for the unit from the topology. It's would not be
inconsistency if this failed, it would be severe brokenness.

Perhaps I should add a comment as I did for a similar panic elsewhere.

[niemeyer, 2012-06-14 00:42:13]

https://codereview.appspot.com/6299073/diff/1004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/service.go#newcode211
state/service.go:211: panic(err)
On 2012/06/13 21:20:46, rog wrote:
> If serviceAndUnit is returning an eror then there's something very wrong going
> on, as we just got the key for the unit from the topology. It's would not be
> inconsistency if this failed, it would be severe brokenness.

The fact you deny even the fact that this is an inconsistency makes it sound
like an unnecessary rant coming. Frank politely made a suggestion that makes a
lot of sense to me:

- Such an inconsistency is not an error created in the local logic, and may
actually be caused by failures in an external system. Being resilient is a nice
idea in those cases.

- There's no good reason to explode and break the process down in this case.

- You're assuming the implementation of serviceAndUnit can't change. It's
blowing up based on an arbitrarily unknown error coming from a lower layer.
Sounds bad.


> Perhaps I should add a comment as I did for a similar panic elsewhere.

I would prefer to follow Frank's suggestion, in both panics.

[rog, 2012-06-14 07:55:30]

https://codereview.appspot.com/6299073/diff/1004/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/service.go#newcode211
state/service.go:211: panic(err)
On 2012/06/14 00:42:13, niemeyer wrote:
> - You're assuming the implementation of serviceAndUnit can't change. It's
> blowing up based on an arbitrarily unknown error coming from a lower layer.

that's a good argument.
done.

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go#newcode370
state/state_test.go:370: subsidiaries []*state.Unit
On 2012/06/13 19:30:25, niemeyer wrote:
> s/subsidiaries/subordinates/
> 
> There are other occurrences of that in the CL.

Done.

https://codereview.appspot.com/6299073/diff/1004/state/state_test.go#newcode1024
state/state_test.go:1024: bundle := testing.Charms.Bundle(c.MkDir(), "logging")
On 2012/06/13 16:58:56, TheMue wrote:
> While addDummyCharm() is a method using the suite and the state of it
> addLoggingCharm() is a function taking the state as an argument. Both are
> several times called directly after each other. This is confusing.
> 
> This should be harmonized. A stand-alone function like here seems to be more
> flexible. So addDummyCharm() should be changed to a function too.

they're a little different because addDummyCharm caches the charm in the
StateSuite - perhaps worth it for dummyCharm as it's used all over the place,
but not necessarily for loggingCharm as it's used very rarely.

if it's ok, i'll leave it as is currently, and suggest a more general solution
in an upcoming CL.

[rog, 2012-06-14 08:21:02]

*** Submitted:

state: automatically assign machines to subsidiary units from their principal
units.

R=bcsaller, niemeyer, TheMue
CC=
https://codereview.appspot.com/6299073