state: store units inside their respective services

state: store units inside their respective services

Making this change simplifies a lot of code - not need
to keep a separate sequence numbering system for units,
for example.

https://code.launchpad.net/~rogpeppe/juju/go-state-units-under-service/+merge/108341

(do not edit description out of merge proposal)

[rog, 2012-06-01 13:47:32]

Please take a look.

[rog, 2012-06-01 18:39:59]

Please take a look.

[fwereade, 2012-06-04 07:29:19]

LGTM modulo below.

https://codereview.appspot.com/6247066/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode120
state/service.go:120: return zkRemoveTree(s.st.zk, unit.zkPath())
Not relevant to this CL... but do we actually want to do this? The unit agent
will surely still be running; I'm not sure it'll handle this all that well...

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode228
state/service.go:228: return s.zkPath() + "/config"
I strongly approve of all the changes like this one :).

https://codereview.appspot.com/6247066/diff/7001/state/topology.go
File state/topology.go (left):

https://codereview.appspot.com/6247066/diff/7001/state/topology.go#oldcode283
state/topology.go:283: // sequence number will be increased monotonically for
each service.
Update comment

https://codereview.appspot.com/6247066/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode77
state/unit.go:77: func mkUnitKey(serviceKey string, unitId int) string {
What does "mk" signify?

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode86
state/unit.go:86: return "", fmt.Errorf("invalid unit key %q (1) %s", unitKey,
debug.Callers(0, 10))
Is this the proposed implementation or an oversight?

[niemeyer, 2012-06-06 20:15:09]

Very nice change. LGTM, assuming you're happy with the adaptations below.
Otherwise, please reply accordingly and repropose.

Please note you'll have to propose+submit this to the new juju-core project,
since the old trunk is now gone (sorry!). You can do this by creating a new
branch from the new project as you'd usually do, merging from this work onto it,
and then just proposing as usual.

https://codereview.appspot.com/6247066/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/machine.go#newcode88
state/machine.go:88: // keyId returns the id corresponding to the given machine
or unit id.
s/keyId/keySequence/ or s/keyId/keySeq/

This is not an identifier. "unit-0000001" is the identifier, and we use the term
"key" there, after much debate, precisely to avoid the confusion with the idea
of an "id" which is public. We can easily avoid the confusion still by calling
the sequence number what it really is (it comes from the idea of sequence from
ZooKeeper).

As a special case, we do call the *machine* sequence number an identifier,
because that's how we handle it across the board (machine.Id(), etc).

https://codereview.appspot.com/6247066/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode13
state/service.go:13: pathPkg "path"
stdpath

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode84
state/service.go:84: kprefix = kprefix[:strings.LastIndex(kprefix, "-")+1]
There's no reason to go over the trouble here. It's already assuming knowledge
about how makeUnitKey works (who said it could skip 'til the last dash like
that?) so it may as well be a lot more clear and say "/units/unit-" + s.key +
"-".

https://codereview.appspot.com/6247066/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode77
state/unit.go:77: func mkUnitKey(serviceKey string, unitId int) string {
On 2012/06/04 07:29:19, fwereade wrote:
> What does "mk" signify?

I assume "make", but I'd also prefer the word itself rather than saving two
chars. Even more when we have serviceKeyForUnitKey below. :-)

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode86
state/unit.go:86: return "", fmt.Errorf("invalid unit key %q (1) %s", unitKey,
debug.Callers(0, 10))
On 2012/06/04 07:29:19, fwereade wrote:
> Is this the proposed implementation or an oversight?

Yeah, looks like an oversight. We need something closer to the error message of
makeUnitKey.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode518
state/unit.go:518: func parseUnitName(name string) (serviceName string,
serviceId int, err error) {
s/serviceId/serviceSeq/

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode523
state/unit.go:523: id, err := strconv.ParseInt(parts[1], 10, 0)
s/id/seq/

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode527
state/unit.go:527: return parts[0], int(id), nil
s/id/seq/

[niemeyer, 2012-06-06 20:16:16]

I have marked the branch in Launchpad as Rejected, but only because of the move
to juju-core. The above review is still valid and is what counts.

[rog, 2012-06-07 12:07:29]

submitted in branch from juju-core

https://codereview.appspot.com/6247066/diff/7001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/machine.go#newcode88
state/machine.go:88: // keyId returns the id corresponding to the given machine
or unit id.
On 2012/06/06 20:15:09, niemeyer wrote:
> s/keyId/keySequence/ or s/keyId/keySeq/
> 
> This is not an identifier. "unit-0000001" is the identifier, and we use the
term
> "key" there, after much debate, precisely to avoid the confusion with the idea
> of an "id" which is public. We can easily avoid the confusion still by calling
> the sequence number what it really is (it comes from the idea of sequence from
> ZooKeeper).
> 
> As a special case, we do call the *machine* sequence number an identifier,
> because that's how we handle it across the board (machine.Id(), etc).

Done.

https://codereview.appspot.com/6247066/diff/7001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode84
state/service.go:84: kprefix = kprefix[:strings.LastIndex(kprefix, "-")+1]
On 2012/06/06 20:15:09, niemeyer wrote:
> There's no reason to go over the trouble here. It's already assuming knowledge
> about how makeUnitKey works (who said it could skip 'til the last dash like
> that?) so it may as well be a lot more clear and say "/units/unit-" + s.key +
> "-".

it's not *quite* as simple as that (makeUnitKey strips off the initial
"service-" prefix) but done anyway.

https://codereview.appspot.com/6247066/diff/7001/state/service.go#newcode120
state/service.go:120: return zkRemoveTree(s.st.zk, unit.zkPath())
On 2012/06/04 07:29:19, fwereade wrote:
> Not relevant to this CL... but do we actually want to do this? The unit agent
> will surely still be running; I'm not sure it'll handle this all that well...

that's an interesting point (although, as you say, not relevant to this CL).
what *should* we do here? wait until the unit agent has gone away? leave the
directory around and GC it later? or perhaps the best approach *is* to delete
the directory and let the unit agent detect that.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode77
state/unit.go:77: func mkUnitKey(serviceKey string, unitId int) string {
On 2012/06/06 20:15:09, niemeyer wrote:
> On 2012/06/04 07:29:19, fwereade wrote:
> > What does "mk" signify?
> 
> I assume "make", but I'd also prefer the word itself rather than saving two
> chars. Even more when we have serviceKeyForUnitKey below. :-)

done. old C proclivities creeping in there... :-)

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode86
state/unit.go:86: return "", fmt.Errorf("invalid unit key %q (1) %s", unitKey,
debug.Callers(0, 10))
On 2012/06/06 20:15:09, niemeyer wrote:
> On 2012/06/04 07:29:19, fwereade wrote:
> > Is this the proposed implementation or an oversight?
> 
> Yeah, looks like an oversight. We need something closer to the error message
of
> makeUnitKey.

oops. done.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode518
state/unit.go:518: func parseUnitName(name string) (serviceName string,
serviceId int, err error) {
On 2012/06/06 20:15:09, niemeyer wrote:
> s/serviceId/serviceSeq/

Done.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode523
state/unit.go:523: id, err := strconv.ParseInt(parts[1], 10, 0)
On 2012/06/06 20:15:09, niemeyer wrote:
> s/id/seq/

Done.

https://codereview.appspot.com/6247066/diff/7001/state/unit.go#newcode527
state/unit.go:527: return parts[0], int(id), nil
On 2012/06/06 20:15:09, niemeyer wrote:
> s/id/seq/

Done.

[fwereade, 2012-06-07 15:20:57]

On 2012/06/07 12:07:29, rog wrote:
> On 2012/06/04 07:29:19, fwereade wrote:
> > Not relevant to this CL... but do we actually want to do this? The unit
agent
> > will surely still be running; I'm not sure it'll handle this all that
well...
> 
> that's an interesting point (although, as you say, not relevant to this CL).
> what *should* we do here? wait until the unit agent has gone away? leave the
> directory around and GC it later? or perhaps the best approach *is* to delete
> the directory and let the unit agent detect that.

Offhand, I think I'd favour a dying/dead approach... eg this sets "dying", which
is handled by the UA (which cleans itself up, and sets "dead"); MA can watch for
"dead" on assigned units, tidy them up, and finally delete the state once we're
sure it's unused. Not sure though.