state: subordinates created in EnterScope

state: subordinates created in EnterScope

This renders Service.AddUnitSubordinateTo redundant... *except* in tests.
Removing it and fixing all the tests is a monstrous job that would result
in a suitably monstrous diff; a series of followups will resolve this
situation, but for the moment it's intractable.

https://code.launchpad.net/~fwereade/juju-core/state-ensure-subordinate/+merge/138742

Requires: https://code.launchpad.net/~fwereade/juju-core/dying-unit-enter-scope/+merge/137585

(do not edit description out of merge proposal)

[fwereade, 2012-12-07 14:41:56]

Please take a look.

[dave_cheney.net, 2012-12-10 03:14:13]

LGTM. The comments about err shadowing and ErrorContext may or may not be
relevant.

https://codereview.appspot.com/6906046/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode438
state/relation.go:438: return nil, err
err is shadowed here, will the ErrorContextf work ?

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode449
state/relation.go:449: } else if err != mgo.ErrNotFound {
same

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode476
state/relation.go:476: if alive, err := getAlive(ru.st.services, serviceName);
err != nil {
same

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode479
state/relation.go:479: return nil, fmt.Errorf("service %q is not alive",
serviceName)
same, but different

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode481
state/relation.go:481: if alive, err := getAlive(ru.st.units, principalName);
err != nil {
same

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode483
state/relation.go:483: } else if !alive {
same

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode486
state/relation.go:486: if alive, err := getAlive(ru.st.relations, relationKey);
err != nil {
same

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode491
state/relation.go:491: return nil, fmt.Errorf("inconsistent state")
same

https://codereview.appspot.com/6906046/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6906046/diff/1/state/service.go#newcode323
state/service.go:323: if err := s.st.runner.Run(ops, "", nil); err ==
txn.ErrAborted {
err is shadowed pretty much everywhere, that may or may not be a problem

[rog, 2012-12-11 11:21:36]

looks fine. just one or two queries and suggestions.

https://codereview.appspot.com/6906046/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6906046/diff/1/state/life.go#newcode84
state/life.go:84: func getAlive(coll *mgo.Collection, id interface{}) (bool,
error) {
s/getAlive/isAlive/ ?

https://codereview.appspot.com/6906046/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode427
state/relation.go:427: // the relation, or the subordinate service are not
Alive.
That's a bit of a mouthful, and it doesn't even say that it adds a subordinate
unit if necessary.

// EnsureSubordinate ensures that the unit
// has a subordinate if required, creating the subordinate
// unit if necessary.
//
// If the unit is not principal in a container-scoped relation;
// ErrNoSubordinateRequired is returned.
// Creating a subordinate unit will fail unless the unit,
// the relation and the subordinate service are all alive.

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode435
state/relation.go:435: defer trivial.ErrorContextf(&err, "cannot create
subordinate")
that's not strictly accurate - this can also be returned when finding if the
subordinate exists.

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode437
state/relation.go:437: if related, err :=
ru.relation.RelatedEndpoints(ru.endpoint.ServiceName); err != nil {
if you moved the assignment out of the if statement, you would not need to
predeclare serviceName.

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode438
state/relation.go:438: return nil, err
On 2012/12/10 03:14:13, dfc wrote:
> err is shadowed here, will the ErrorContextf work ?

yes, it will work fine.

https://codereview.appspot.com/6906046/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6906046/diff/1/state/service.go#newcode266
state/service.go:266: // assumed to be true, once AddUnitSubordinateTo hasbeen
removed.
s/hasbeen/has been/
no hasbeens around here, i hope! :-)

i don't understand the last sentence. what does it mean to "remove" a parameter?

https://codereview.appspot.com/6906046/diff/1/state/service.go#newcode323
state/service.go:323: if err := s.st.runner.Run(ops, "", nil); err ==
txn.ErrAborted {
On 2012/12/10 03:14:13, dfc wrote:
> err is shadowed pretty much everywhere, that may or may not be a problem

it only matters with bare return statements.

[fwereade, 2012-12-12 08:47:10]

Please take a look.

https://codereview.appspot.com/6906046/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6906046/diff/1/state/life.go#newcode84
state/life.go:84: func getAlive(coll *mgo.Collection, id interface{}) (bool,
error) {
On 2012/12/11 11:21:37, rog wrote:
> s/getAlive/isAlive/ ?

Name collision -- that was what I originally tried :). I think getAlive is a
sensible choice because it hits the network; but I'd be ok with
s/isAlive/assertAlive/ if you prefer, so we can use isAlive for this func.

https://codereview.appspot.com/6906046/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode427
state/relation.go:427: // the relation, or the subordinate service are not
Alive.
On 2012/12/11 11:21:37, rog wrote:
> That's a bit of a mouthful, and it doesn't even say that it adds a subordinate
> unit if necessary.
> 
> // EnsureSubordinate ensures that the unit
> // has a subordinate if required, creating the subordinate
> // unit if necessary.
> //
> // If the unit is not principal in a container-scoped relation;
> // ErrNoSubordinateRequired is returned.
> // Creating a subordinate unit will fail unless the unit,
> // the relation and the subordinate service are all alive.

Not quite convinced by that either-- I'll have another go.

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode435
state/relation.go:435: defer trivial.ErrorContextf(&err, "cannot create
subordinate")
On 2012/12/11 11:21:37, rog wrote:
> that's not strictly accurate - this can also be returned when finding if the
> subordinate exists.

Ha, good point, thanks.

https://codereview.appspot.com/6906046/diff/1/state/relation.go#newcode437
state/relation.go:437: if related, err :=
ru.relation.RelatedEndpoints(ru.endpoint.ServiceName); err != nil {
On 2012/12/11 11:21:37, rog wrote:
> if you moved the assignment out of the if statement, you would not need to
> predeclare serviceName.

Another good point, cheers :)

https://codereview.appspot.com/6906046/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6906046/diff/1/state/service.go#newcode266
state/service.go:266: // assumed to be true, once AddUnitSubordinateTo hasbeen
removed.
On 2012/12/11 11:21:37, rog wrote:
> s/hasbeen/has been/
> no hasbeens around here, i hope! :-)
> 
> i don't understand the last sentence. what does it mean to "remove" a
parameter?

It means to drop it from the signature, because the only reason it exists is to
avoid duplicating all this code for the use of AddUnitSubordinateTo. This icky
ad-hoc surgery around AUST is the bit that makes me really unhappy here...

Regardless, I think that bit should be moved to an internal comment. Cheers.

[niemeyer, 2012-12-13 14:37:26]

https://codereview.appspot.com/6906046/diff/1/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6906046/diff/1/state/life.go#newcode84
state/life.go:84: func getAlive(coll *mgo.Collection, id interface{}) (bool,
error) {
On 2012/12/12 08:47:10, fwereade wrote:
> On 2012/12/11 11:21:37, rog wrote:
> > s/getAlive/isAlive/ ?
> 
> Name collision -- that was what I originally tried :). I think getAlive is a
> sensible choice because it hits the network; but I'd be ok with
> s/isAlive/assertAlive/ if you prefer, so we can use isAlive for this func.

What about isAliveDoc and notDeadDoc?

https://codereview.appspot.com/6906046/diff/7001/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6906046/diff/7001/state/life.go#newcode86
state/life.go:86: if err := coll.FindId(id).One(&doc); err == mgo.ErrNotFound {
func isAlive(...) {bool error) {
    n, err := coll.Find(D{{"_id", id}, {"life", Alive}}).Count()
    return n == 1, err
}

Besides being simpler, this also avoids transferring the whole document over the
wire (we could use Select, but that's still better).

https://codereview.appspot.com/6906046/diff/7001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode423
state/relation.go:423: // ErrNoSubordinateRequired indicates that a relation
unit is not a principal
That seems slightly misleading. It's not that a subordinate is not required.
It's that it is *incorrect* to have a subordinate in situations where the errors
is returned. That was the main reason AddSubordinateUnit was deemed
inappropriate.

I suggest something like this instead:

var ErrInvalidSubordinate = errors.New("invalid context for the creation of a
subordinate unit")

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode430
state/relation.go:430: // required subordinate does not exist, it will be
created. It will fail to
That's repeating the first sentence.

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode431
state/relation.go:431: // create a subordinate if any of the principal, the
relation, or the subordinate
s/fail to create a subordinate/return an error/

This is relevant. If a subordinate exists, it will also fail to create a
subordinate, but that's not an error.

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode433
state/relation.go:433: func (ru *RelationUnit) EnsureSubordinate() (sub *Unit,
err error) {
We need this to be EnsureHasSubordinate or similar. It's a bit awkward to call
ru.EnsureSubordinate() when ru must necessarily be of a *principal* unit.

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode437
state/relation.go:437: if ru.endpoint.RelationScope != charm.ScopeContainer {
This can be ||ed above.

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode440
state/relation.go:440: defer trivial.ErrorContextf(&err, "cannot get or create
subordinate")
This should be either "cannot ensure subordinate", or we should have more
specific messages below. There are many error reasons below that are unrelated
to "get or create" (which is already a bit confusing by itself).

https://codereview.appspot.com/6906046/diff/7001/state/relation.go#newcode495
state/relation.go:495: return nil, fmt.Errorf("inconsistent state")
Nice logic above.

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

https://codereview.appspot.com/6906046/diff/7001/state/service.go#newcode267
state/service.go:267: // NOTE: strictSubordinates is a temporary parameter,
which allows us to use
Thanks for the upfront explanation. I was wondering about that above.

https://codereview.appspot.com/6906046/diff/7001/state/service.go#newcode276
state/service.go:276: return "", nil, fmt.Errorf("service is subordinate")
s/sub/a sub/, as below

https://codereview.appspot.com/6906046/diff/7001/state/service.go#newcode414
state/service.go:414: // (shouldn't this already be the case before the
principal is Dead?)
In theory both the subordinates and the principal may reach a Dead state. Do we
constrain the API anywhere in state to prevent that from happening?

https://codereview.appspot.com/6906046/diff/7001/state/service.go#newcode446
state/service.go:446: udoc := &unitDoc{}
What's wrong with the old logic? It looks significantly more
readable/maintainable.

[fwereade, 2012-12-14 15:39:02]

Please take a look.

https://codereview.appspot.com/6906046/diff/7001/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6906046/diff/7001/state/life.go#newcode86
state/life.go:86: if err := coll.FindId(id).One(&doc); err == mgo.ErrNotFound {
On 2012/12/13 14:37:27, niemeyer wrote:
> func isAlive(...) {bool error) {
>     n, err := coll.Find(D{{"_id", id}, {"life", Alive}}).Count()
>     return n == 1, err
> }
> 
> Besides being simpler, this also avoids transferring the whole document over
the
> wire (we could use Select, but that's still better).

Much nicer, thanks.

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

https://codereview.appspot.com/6906046/diff/7001/state/service.go#newcode414
state/service.go:414: // (shouldn't this already be the case before the
principal is Dead?)
On 2012/12/13 14:37:27, niemeyer wrote:
> In theory both the subordinates and the principal may reach a Dead state. Do
we
> constrain the API anywhere in state to prevent that from happening?

This'd be Unit.EnsureDead's job, but it doesn't do it.

https://codereview.appspot.com/6906046/diff/7001/state/service.go#newcode446
state/service.go:446: udoc := &unitDoc{}
On 2012/12/13 14:37:27, niemeyer wrote:
> What's wrong with the old logic? It looks significantly more
> readable/maintainable.

Nothing -- but it was a single-use method that I factored back into the single
caller... and then forgot to delete.

[niemeyer, 2012-12-14 16:19:14]

LGTM, just one detail that I was unclear about:

https://codereview.appspot.com/6906046/diff/15001/state/life.go
File state/life.go (right):

https://codereview.appspot.com/6906046/diff/15001/state/life.go#newcode84
state/life.go:84: func isAliveDoc(coll *mgo.Collection, id interface{}) (bool,
error) {
Sorry for the ambiguity. I actually meant to have isAlive in this function, and
isAliveDoc and notDeadDoc on the bson.D document variables.

[fwereade, 2012-12-14 16:57:31]

*** Submitted:

state: subordinates created in EnterScope

This renders Service.AddUnitSubordinateTo redundant... *except* in tests.
Removing it and fixing all the tests is a monstrous job that would result
in a suitably monstrous diff; a series of followups will resolve this
situation, but for the moment it's intractable.

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