mstate: subordinate units.

mstate: subordinate units.

Subordinate units are automatically assigned to machines with their
principal units. A principal unit and its subordinates forms a unit
set. A machine can have multiple unit sets assigned to it.

https://code.launchpad.net/~aramh/juju-core/mstate-units-subordinate/+merge/113016

(do not edit description out of merge proposal)

[aram, 2012-07-02 10:57:19]

Please take a look.

[niemeyer, 2012-07-03 04:32:14]

https://codereview.appspot.com/6345056/diff/2001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/2001/mstate/unit.go#newcode30
mstate/unit.go:30: // its subsidiaries.
We can track the machine id of a principal in the principal itself, and we can
track the principal for the subordinate in the respective subordinate document.

By the way, before we get this in, can you please address (or respond to) the
lifecycle proposal on the current mstate package in a different CL, so we don't
increase the problem, and then address it in this branch as well and repropose?

[aram, 2012-07-03 20:11:51]

https://codereview.appspot.com/6345056/diff/2001/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/2001/mstate/unit.go#newcode30
mstate/unit.go:30: // its subsidiaries.
On 2012/07/03 04:32:14, niemeyer wrote:
> We can track the machine id of a principal in the principal itself, and we can
> track the principal for the subordinate in the respective subordinate
document.

Done.

> By the way, before we get this in, can you please address (or respond to) the
> lifecycle proposal on the current mstate package in a different CL, so we
don't
> increase the problem, and then address it in this branch as well and
repropose?

I've addressed the lifecycle proposal in a different branch:
https://codereview.appspot.com/6350068

It's just additions from this branch, so I think the changes can be reviewed
independently.

[aram, 2012-07-03 20:12:25]

Please take a look.

[niemeyer, 2012-07-03 21:11:13]

Looking nice. Just a few details:

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go#newcode99
mstate/service.go:99: return s.addUnit(name, name)
That's a bit unexpected, and I suspect you didn't expect it either, given that
the Units function logic is broken because of that (there will be duplicated
entries in the returned slice).

Is there any significant win of that approach? If not, I suggest going back to
the old behavior of having principal == "" when the unit is itself a principal.

https://codereview.appspot.com/6345056/diff/3008/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/state_test.go#newcode199
mstate/state_test.go:199: func (s *StateSuite) TestMachineUnits(c *C) {
I haven't been reviewing the tests in detail, as I'm assuming it's just a plain
copy of the equivalent state test, in the same file, with the same name, plus
any minors needed to s/zookeeper/mongo/. If that's ever not the case, please
mention it explicitly so that I can pay more attention.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode30
mstate/unit.go:30: unitDoc
This change is great, but let's use the convention of "doc unitDoc". Besides
being nice, it's necessary because the public fields of unitDoc will be visible
on Unit otherwise.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode33
mstate/unit.go:33: func newUnit(s *State, udoc *unitDoc) *Unit {
This can actually be:

func (s *State) newUnit

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode48
mstate/unit.go:48: return u.unitDoc.Name == u.unitDoc.Principal
That should also be reverted per previous comment.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode61
mstate/unit.go:61: err = u.st.units.Find(bson.D{{"_id",
u.unitDoc.Principal}}).One(&pudoc)
We can start to use the FindId method in those cases:

err = u.st.units.FindId(u.doc.Principal).One(&pudoc)

[aram, 2012-07-04 21:48:40]

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go#newcode99
mstate/service.go:99: return s.addUnit(name, name)
> That's a bit unexpected, and I suspect you didn't expect it either,
> given that the Units function logic is broken because of that (there
> will be duplicated entries in the returned slice).

I don't understand what you are telling me here. The Units function is
simpler because of this change, why any duplicated entries? The Units
function first queries for principals with a specific machine id, then
queries for all the machines, principals and subsidiaries, with that
principal field. This returns all the machines assigned to a specific
machines. The tests seem to validate this logic.

If we set principal to "", Units would be more complex because we would
need to merge the results from querying by machine (the principals)
with the results from querying by principal (the subsidiaries).

It seems to me a princial and its subsidiaries form a logical unit set
(even if we don't expose it) so it makes sense to have this logical
structure under a single key in the database.

I'm happy either way, the additional complexity in Units is minor,
I'm just trying to understand.

https://codereview.appspot.com/6345056/diff/3008/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/state_test.go#newcode199
mstate/state_test.go:199: func (s *StateSuite) TestMachineUnits(c *C) {
On 2012/07/03 21:11:13, niemeyer wrote:
> I haven't been reviewing the tests in detail, as I'm assuming it's just a
plain
> copy of the equivalent state test, in the same file, with the same name, plus
> any minors needed to s/zookeeper/mongo/. If that's ever not the case, please
> mention it explicitly so that I can pay more attention. 

That's mostly the case, in a couple of occasions I had to modify error string
assertions, but those cases have a TODO so we don't forget about them.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode30
mstate/unit.go:30: unitDoc
On 2012/07/03 21:11:13, niemeyer wrote:
> This change is great, but let's use the convention of "doc unitDoc". Besides
> being nice, it's necessary because the public fields of unitDoc will be
visible
> on Unit otherwise.

Of course, done.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode33
mstate/unit.go:33: func newUnit(s *State, udoc *unitDoc) *Unit {
On 2012/07/03 21:11:13, niemeyer wrote:
> This can actually be:
> 
> func (s *State) newUnit

Some convention we decided on?

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode61
mstate/unit.go:61: err = u.st.units.Find(bson.D{{"_id",
u.unitDoc.Principal}}).One(&pudoc)
On 2012/07/03 21:11:13, niemeyer wrote:
> We can start to use the FindId method in those cases:
> 
> err = u.st.units.FindId(u.doc.Principal).One(&pudoc)

Thanks, missed this useful method.

[aram, 2012-07-04 21:49:05]

Please take a look.

[niemeyer, 2012-07-06 11:03:43]

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go#newcode99
mstate/service.go:99: return s.addUnit(name, name)
On 2012/07/04 21:48:40, aram wrote:
> I'm happy either way, the additional complexity in Units is minor,
> I'm just trying to understand.

Ok, sorry, I was the one misunderstanding. Either way, I believe it's worse for
querying to have this change in place. How do we query for all the principal
units (not "subsidiaries", btw)?

https://codereview.appspot.com/6345056/diff/3008/mstate/state_test.go
File mstate/state_test.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/state_test.go#newcode199
mstate/state_test.go:199: func (s *StateSuite) TestMachineUnits(c *C) {
On 2012/07/04 21:48:40, aram wrote:
> On 2012/07/03 21:11:13, niemeyer wrote:
> > I haven't been reviewing the tests in detail, as I'm assuming it's just a
> plain
> > copy of the equivalent state test, in the same file, with the same name,
plus
> > any minors needed to s/zookeeper/mongo/. If that's ever not the case, please
> > mention it explicitly so that I can pay more attention. 
> 
> That's mostly the case, in a couple of occasions I had to modify error string
> assertions, but those cases have a TODO so we don't forget about them.

Okay, that's minor. Improving error string is also fine, but please do so on
both sides if possible.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode33
mstate/unit.go:33: func newUnit(s *State, udoc *unitDoc) *Unit {
On 2012/07/04 21:48:40, aram wrote:
> On 2012/07/03 21:11:13, niemeyer wrote:
> > This can actually be:
> > 
> > func (s *State) newUnit
> 
> Some convention we decided on?

No, it's a suggestion because s.newUnit(doc) reads nicely. I don't mind too much
either way. That said, if you're not using state as a receiver, please rename s
to st (yes, that's a convention we decided upon).

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode61
mstate/unit.go:61: err = u.st.units.Find(bson.D{{"_id",
u.unitDoc.Principal}}).One(&pudoc)
On 2012/07/04 21:48:40, aram wrote:
> Thanks, missed this useful method.

It's pretty new. I intend to introduce UpdateId, RemoveId, etc as well.

https://codereview.appspot.com/6345056/diff/10008/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/10008/mstate/unit.go#newcode48
mstate/unit.go:48: return u.doc.Name == u.doc.Principal
Still must be considered (to help me on re-reviewing).

[aram, 2012-07-08 22:26:59]

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go#newcode99
mstate/service.go:99: return s.addUnit(name, name)
> Ok, sorry, I was the one misunderstanding. Either way, I believe it's
> worse for querying to have this change in place. How do we query for
> all the principal units (not "subsidiaries", btw)?

To query only for all the principal units we query for docs that have
name (_id) equal to principal.

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/unit.go#newcode33
mstate/unit.go:33: func newUnit(s *State, udoc *unitDoc) *Unit {
On 2012/07/06 11:03:43, niemeyer wrote:
> On 2012/07/04 21:48:40, aram wrote:
> > On 2012/07/03 21:11:13, niemeyer wrote:
> > > This can actually be:
> > > 
> > > func (s *State) newUnit
> > 
> > Some convention we decided on?
> 
> No, it's a suggestion because s.newUnit(doc) reads nicely. I don't mind too
much
> either way. That said, if you're not using state as a receiver, please rename
s
> to st (yes, that's a convention we decided upon).

Done.

[aram, 2012-07-08 22:27:30]

Please take a look.

[niemeyer, 2012-07-09 23:22:16]

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go#newcode99
mstate/service.go:99: return s.addUnit(name, name)
On 2012/07/08 22:26:59, aram wrote:
> > Ok, sorry, I was the one misunderstanding. Either way, I believe it's
> > worse for querying to have this change in place. How do we query for2
> > all the principal units (not "subsidiaries", btw)?
> 
> To query only for all the principal units we query for docs that have
> name (_id) equal to principal.

This means a full scan. Please revert and fix logic accordingly so we can get
this CL in.

[aram, 2012-07-10 18:03:16]

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6345056/diff/3008/mstate/service.go#newcode99
mstate/service.go:99: return s.addUnit(name, name)
On 2012/07/09 23:22:17, niemeyer wrote:
> On 2012/07/08 22:26:59, aram wrote:
> > > Ok, sorry, I was the one misunderstanding. Either way, I believe it's
> > > worse for querying to have this change in place. How do we query for2
> > > all the principal units (not "subsidiaries", btw)?
> > 
> > To query only for all the principal units we query for docs that have
> > name (_id) equal to principal.
> 
> This means a full scan. Please revert and fix logic accordingly so we can get
> this CL in.

Done.

[aram, 2012-07-10 18:03:37]

Please take a look.

[niemeyer, 2012-07-10 20:28:08]

Thank you, LGTM.

[aram, 2012-07-10 23:44:34]

*** Submitted:

mstate: subordinate units.

Subordinate units are automatically assigned to machines with their
principal units. A principal unit and its subordinates forms a unit
set. A machine can have multiple unit sets assigned to it.

R=
CC=
https://codereview.appspot.com/6345056