state: EnterScope fixes

state: EnterScope fixes

EnterScope now takes a settings map, which is used to create or overwrite
the unit relation settings, if and only if a scope document does not
already exist. The overwriting improves the behaviour of units which leave
and then enter the same scope, by preventing stale settings from confusing
remote units.

In addition, RelationUnit no longer has an annoyingly hidden dependency on
the unit's private address; this makes it easier to create a subordinate,
and consequently makes the removal of AddUnitSubordinateTo more manageable.

https://code.launchpad.net/~fwereade/juju-core/untangle-enter-scope/+merge/142657

(do not edit description out of merge proposal)

[fwereade, 2013-01-10 10:10:40]

Please take a look.

[TheMue, 2013-01-11 16:03:04]

Looks nice, only one real comment now. But have to dig deeper into it.

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

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode308
state/relation.go:308: settingsChanged := func() (bool, error) { return false,
nil }
Tricky, but nice.

https://codereview.appspot.com/7058073/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7058073/diff/1/state/relation_test.go#newcode16
state/relation_test.go:16: var _ = Suite(&RelationSuite{})
Could is make sense to split this suite into two or more files? It is quite
large.

https://codereview.appspot.com/7058073/diff/1/state/relation_test.go#newcode352
state/relation_test.go:352: c.Assert(m["meme"], Equals, "foul-bachelor-frog")
Gene Hackman and Gene Simmons the same meme? ;)

[fwereade, 2013-01-14 07:55:18]

Thanks TheMue. Thoughts on the below?

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

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode308
state/relation.go:308: settingsChanged := func() (bool, error) { return false,
nil }
On 2013/01/11 16:03:04, TheMue wrote:
> Tricky, but nice.
Cheers :).

https://codereview.appspot.com/7058073/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7058073/diff/1/state/relation_test.go#newcode16
state/relation_test.go:16: var _ = Suite(&RelationSuite{})
On 2013/01/11 16:03:04, TheMue wrote:
> Could is make sense to split this suite into two or more files? It is quite
> large.

Yeah, it could definitely use some rationalization; I rather want to split
relation.go too, so RelationUnit can get its own file, but part of me is waiting
to fight the "right" name for RelationUnit, which I'm still not 100% happy with.
I'm also kinda tempted to trash OriginalRelationUnitSuite as well -- I think
it's now redundant, but the prospect of verifying that robustly is somewhat
daunting.

So, yeah, it's on my mind, but it's not a job for this CL, IMO, especially given
that there are changes in the code that would move.

https://codereview.appspot.com/7058073/diff/1/state/relation_test.go#newcode352
state/relation_test.go:352: c.Assert(m["meme"], Equals, "foul-bachelor-frog")
On 2013/01/11 16:03:04, TheMue wrote:
> Gene Hackman and Gene Simmons the same meme? ;)

Haha :)

[fwereade, 2013-01-14 14:49:51]

Hmm: I am starting to think that we should return ErrAlreadyInScope to make
explicit the fact that the settings are not written in that case. Opinions?

[rog, 2013-01-15 10:10:18]

nice. LGTM with the below remarks addressed.

i'm not sure what you were referring to with your ErrAlreadyInScope comment.

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

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode368
state/relation.go:368: t := fmt.Sprintf("cannot enter scope for unit %q in
relation %q", ru.unit, ru.relation) + ": %v"
rather than constructing a format string that can't be checked by govet, i think
it would be better to lose the %v and just not use Errorf, e.g. errors.New(t +
": concurrent settings change detected")

(or put the : in t)

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode377
state/relation.go:377: return fmt.Errorf(t, "inconsistent state")
"inconsistent state in EnterScope" ?

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode389
state/relation.go:389: } else if len(related) != 1 {
the else isn't necessary here.

https://codereview.appspot.com/7058073/diff/1/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/7058073/diff/1/state/settings.go#newcode263
state/settings.go:263: for k, _ := range s.disk {
s/, _//

[fwereade, 2013-01-15 14:06:45]

Please take a look.

[TheMue, 2013-01-15 14:39:20]

LGTM

https://codereview.appspot.com/7058073/diff/7002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/7058073/diff/7002/state/relation.go#newcode401
state/relation.go:401: func (ru *RelationUnit) subordinateOps() ([]txn.Op,
string, error) {
Better now, yes.

https://codereview.appspot.com/7058073/diff/7002/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/7058073/diff/7002/state/settings.go#newcode263
state/settings.go:263: for k, _ := range s.disk {
_ not needed.

https://codereview.appspot.com/7058073/diff/7002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7058073/diff/7002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:1342: timeout := time.After(5 * time.Second)
Had to read twice to get the idea of timeout and polling.

[fwereade, 2013-01-15 16:48:13]

*** Submitted:

state: EnterScope fixes

EnterScope now takes a settings map, which is used to create or overwrite
the unit relation settings, if and only if a scope document does not
already exist. The overwriting improves the behaviour of units which leave
and then enter the same scope, by preventing stale settings from confusing
remote units.

In addition, RelationUnit no longer has an annoyingly hidden dependency on
the unit's private address; this makes it easier to create a subordinate,
and consequently makes the removal of AddUnitSubordinateTo more manageable.

R=TheMue, rog
CC=
https://codereview.appspot.com/7058073

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

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode368
state/relation.go:368: t := fmt.Sprintf("cannot enter scope for unit %q in
relation %q", ru.unit, ru.relation) + ": %v"
On 2013/01/15 10:10:18, rog wrote:
> rather than constructing a format string that can't be checked by govet, i
think
> it would be better to lose the %v and just not use Errorf, e.g. errors.New(t +
> ": concurrent settings change detected")
> 
> (or put the : in t)

Done.

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode377
state/relation.go:377: return fmt.Errorf(t, "inconsistent state")
On 2013/01/15 10:10:18, rog wrote:
> "inconsistent state in EnterScope" ?

Done.

https://codereview.appspot.com/7058073/diff/1/state/relation.go#newcode389
state/relation.go:389: } else if len(related) != 1 {
On 2013/01/15 10:10:18, rog wrote:
> the else isn't necessary here.

Done.

https://codereview.appspot.com/7058073/diff/1/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/7058073/diff/1/state/settings.go#newcode263
state/settings.go:263: for k, _ := range s.disk {
On 2013/01/15 10:10:18, rog wrote:
> s/, _//

Done.

https://codereview.appspot.com/7058073/diff/7002/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/7058073/diff/7002/state/settings.go#newcode263
state/settings.go:263: for k, _ := range s.disk {
On 2013/01/15 14:39:21, TheMue wrote:
> _ not needed.

Done.

https://codereview.appspot.com/7058073/diff/7002/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7058073/diff/7002/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:1342: timeout := time.After(5 * time.Second)
On 2013/01/15 14:39:21, TheMue wrote:
> Had to read twice to get the idea of timeout and polling.

Absent suggestions for improvement, merging as-is.