Merge lp:~fwereade/juju-core/prepare-leave-scope into lp:~go-bot/juju-core/trunk

[William Reade]

state: RelationUnit.PrepareLeaveScope

Added a mechanism whereby units can report imminent departure from a
relation, such that related ones see them disappear immediately and
reconfigure to ignore them *before* they actually disappear for good.

https://codereview.appspot.com/12841044/

state: RelationUnit.PrepareLeaveScope

Added a mechanism whereby units can report imminent departure from a
relation, such that related ones see them disappear immediately and
reconfigure to ignore them *before* they actually disappear for good.

Exactly when the mechanism should be invoked remains open; it's clear
that it's a good idea for peers and providers, but I'm not sure it's
sensible for providers to see the departure of requirers until it actually
happens (because providers may have set up creds for the specific
requirers, and it'd be unhelpful to cut off their access before they're
ready).

Thoughts?

https://codereview.appspot.com/12841044/

[William Reade (fwereade)]

Reviewers: mp+181065_code.launchpad.net,

Message:
Please take a look.

Description:
state: RelationUnit.PrepareLeaveScope

Added a mechanism whereby units can report imminent departure from a
relation, such that related ones see them disappear immediately and
reconfigure to ignore them *before* they actually disappear for good.

Exactly when the mechanism should be invoked remains open; it's clear
that it's a good idea for peers and providers, but I'm not sure it's
sensible for providers to see the departure of requirers until it
actually
happens (because providers may have set up creds for the specific
requirers, and it'd be unhelpful to cut off their access before they're
ready).

Thoughts?

https://code.launchpad.net/~fwereade/juju-core/prepare-leave-scope/+merge/181065

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12841044/

Affected files:
   A [revision details]
   M state/relationunit.go
   M state/relationunit_test.go
   M state/watcher.go

[Ian Booth (wallyworld)]

The code looks sane. I must confess I don't fully grok the higher level
use cases around how the functionality will be consumed. LGTM

https://codereview.appspot.com/12841044/diff/1/state/relationunit_test.go
File state/relationunit_test.go (right):

https://codereview.appspot.com/12841044/diff/1/state/relationunit_test.go#newcode662
state/relationunit_test.go:662: c.Assert(prr.rru0.InScope(), gc.Equals,
true)
Since it's one line, could we also test that rru1 is still in scope,
just because?

https://codereview.appspot.com/12841044/

[John A Meinel (jameinel)]

Can we get a final decision on whether we want to finish this off and land it, or reject it so it gets out of the review queue?

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/12841044/

[Martin Packman (gz)]

LGTM. Doc changes needed anywhere?

https://codereview.appspot.com/12841044/

=== modified file 'state/relationunit.go'
--- state/relationunit.go	2014-04-17 12:47:50 +0000
+++ state/relationunit.go	2014-05-14 15:04:58 +0000
@@ -121,7 +121,7 @@
 		C:      ru.st.relationScopes.Name,
 		Id:     ruKey,
 		Assert: txn.DocMissing,
-		Insert: relationScopeDoc{ruKey},
+		Insert: relationScopeDoc{Key: ruKey},
 	})
 
 	// * If the unit should have a subordinate, and does not, create it.
@@ -223,6 +223,27 @@
 	}}, lDoc.Id, nil
 }
 
+// PrepareLeaveScope causes the unit to be reported as departed by watchers,
+// but does not *actually* leave the scope, to avoid triggering relation
+// cleanup.
+func (ru *RelationUnit) PrepareLeaveScope() error {
+	key, err := ru.key(ru.unit.Name())
+	if err != nil {
+		return err
+	}
+	if count, err := ru.st.relationScopes.FindId(key).Count(); err != nil {
+		return err
+	} else if count == 0 {
+		return nil
+	}
+	ops := []txn.Op{{
+		C:      ru.st.relationScopes.Name,
+		Id:     key,
+		Update: bson.D{{"$set", bson.D{{"departing", true}}}},
+	}}
+	return ru.st.runTransaction(ops)
+}
+
 // LeaveScope signals that the unit has left its scope in the relation.
 // After the unit has left its relation scope, it is no longer a member
 // of the relation; if the relation is dying when its last member unit
@@ -376,10 +397,15 @@
 // relationScopeDoc represents a unit which is in a relation scope.
 // The relation, container, role, and unit are all encoded in the key.
 type relationScopeDoc struct {
-	Key string `bson:"_id"`
+	Key       string `bson:"_id"`
+	Departing bool
 }
 
 func (d *relationScopeDoc) unitName() string {
-	parts := strings.Split(d.Key, "#")
+	return unitNameFromScopeKey(d.Key)
+}
+
+func unitNameFromScopeKey(key string) string {
+	parts := strings.Split(key, "#")
 	return parts[len(parts)-1]
 }

=== modified file 'state/relationunit_test.go'
--- state/relationunit_test.go	2014-04-14 12:36:13 +0000
+++ state/relationunit_test.go	2014-05-14 15:04:58 +0000
@@ -621,6 +621,85 @@
 	assertNotInScope(c, prr.rru0)
 }
 
+func (s *RelationUnitSuite) TestCoalesceWatchScope(c *gc.C) {
+	pr := NewPeerRelation(c, s.State)
+
+	// Test empty initial event.
+	w0 := pr.ru0.WatchScope()
+	defer testing.AssertStop(c, w0)
+	s.assertScopeChange(c, w0, nil, nil)
+	s.assertNoScopeChange(c, w0)
+
+	// ru1 and ru2 enter; check changes observed together.
+	err := pr.ru1.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+	err = pr.ru2.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+	s.assertScopeChange(c, w0, []string{"riak/1", "riak/2"}, nil)
+	s.assertNoScopeChange(c, w0)
+
+	// ru1 leaves and re-enters; check no change observed.
+	err = pr.ru1.LeaveScope()
+	c.Assert(err, gc.IsNil)
+	err = pr.ru1.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+	s.assertNoScopeChange(c, w0)
+
+	// ru1 and ru2 leave; check changes observed together.
+	err = pr.ru1.LeaveScope()
+	c.Assert(err, gc.IsNil)
+	err = pr.ru2.LeaveScope()
+	c.Assert(err, gc.IsNil)
+	s.assertScopeChange(c, w0, nil, []string{"riak/1", "riak/2"})
+	s.assertNoScopeChange(c, w0)
+}
+
+func (s *RelationUnitSuite) TestPrepareLeaveScope(c *gc.C) {
+	prr := NewProReqRelation(c, &s.ConnSuite, charm.ScopeGlobal)
+
+	// Test empty initial event.
+	w0 := prr.pru0.WatchScope()
+	defer testing.AssertStop(c, w0)
+	s.assertScopeChange(c, w0, nil, nil)
+	s.assertNoScopeChange(c, w0)
+
+	// rru0 and rru1 enter; check changes.
+	err := prr.rru0.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+	err = prr.rru1.EnterScope(nil)
+	c.Assert(err, gc.IsNil)
+	s.assertScopeChange(c, w0, []string{"wordpress/0", "wordpress/1"}, nil)
+	s.assertNoScopeChange(c, w0)
+
+	// rru0 notifies that it will leave soon; it's reported as departed by the
+	// watcher, but InScope remains accurate.
+	err = prr.rru0.PrepareLeaveScope()
+	c.Assert(err, gc.IsNil)
+	s.assertScopeChange(c, w0, nil, []string{"wordpress/0"})
+	s.assertNoScopeChange(c, w0)
+	assertInScope(c, prr.rru0)
+
+	// rru1 leaves, and the relation is destroyed; it's not removed, because
+	// rru0 keeps it alive until it really leaves scope.
+	err = prr.rru1.LeaveScope()
+	c.Assert(err, gc.IsNil)
+	s.assertScopeChange(c, w0, nil, []string{"wordpress/1"})
+	s.assertNoScopeChange(c, w0)
+	err = prr.rel.Destroy()
+	c.Assert(err, gc.IsNil)
+	err = prr.rel.Refresh()
+	c.Assert(err, gc.IsNil)
+
+	// rru0 really leaves; the relation is cleaned up.
+	err = prr.rru0.LeaveScope()
+	c.Assert(err, gc.IsNil)
+	err = prr.rel.Destroy()
+	c.Assert(err, gc.IsNil)
+	s.assertNoScopeChange(c, w0)
+	err = prr.rel.Refresh()
+	c.Assert(err, jc.Satisfies, errors.IsNotFound)
+}
+
 func (s *RelationUnitSuite) assertScopeChange(c *gc.C, w *state.RelationScopeWatcher, entered, left []string) {
 	s.State.StartSync()
 	select {

=== modified file 'state/watcher.go'
--- state/watcher.go	2014-04-30 23:18:40 +0000
+++ state/watcher.go	2014-05-14 15:04:58 +0000
@@ -448,14 +448,54 @@
 	return w.out
 }
 
-// RelationScopeWatcher observes changes to the set of units
-// in a particular relation scope.
-type RelationScopeWatcher struct {
-	commonWatcher
-	prefix     string
-	ignore     string
-	knownUnits set.Strings
-	out        chan *RelationScopeChange
+// scopeInfo holds a RelationScopeWatcher's last-delivered state, and any
+// known but undelivered changes thereto.
+type scopeInfo struct {
+	base map[string]bool
+	diff map[string]bool
+}
+
+func (info *scopeInfo) add(name string) {
+	if info.base[name] {
+		delete(info.diff, name)
+	} else {
+		info.diff[name] = true
+	}
+}
+
+func (info *scopeInfo) remove(name string) {
+	if info.base[name] {
+		info.diff[name] = false
+	} else {
+		delete(info.diff, name)
+	}
+}
+
+func (info *scopeInfo) commit() {
+	for name, change := range info.diff {
+		if change {
+			info.base[name] = true
+		} else {
+			delete(info.base, name)
+		}
+	}
+	info.diff = map[string]bool{}
+}
+
+func (info *scopeInfo) hasChanges() bool {
+	return len(info.diff) > 0
+}
+
+func (info *scopeInfo) changes() *RelationScopeChange {
+	ch := &RelationScopeChange{}
+	for name, change := range info.diff {
+		if change {
+			ch.Entered = append(ch.Entered, name)
+		} else {
+			ch.Left = append(ch.Left, name)
+		}
+	}
+	return ch
 }
 
 var _ Watcher = (*RelationScopeWatcher)(nil)
@@ -467,6 +507,15 @@
 	Left    []string
 }
 
+// RelationScopeWatcher observes changes to the set of units
+// in a particular relation scope.
+type RelationScopeWatcher struct {
+	commonWatcher
+	prefix string
+	ignore string
+	out    chan *RelationScopeChange
+}
+
 func newRelationScopeWatcher(st *State, scope, ignore string) *RelationScopeWatcher {
 	w := &RelationScopeWatcher{
 		commonWatcher: commonWatcher{st: st},
@@ -489,58 +538,75 @@
 	return w.out
 }
 
-func (changes *RelationScopeChange) isEmpty() bool {
-	return len(changes.Entered)+len(changes.Left) == 0
-}
-
-func (w *RelationScopeWatcher) mergeChange(changes *RelationScopeChange, ch watcher.Change) (err error) {
-	doc := &relationScopeDoc{ch.Id.(string)}
-	if !strings.HasPrefix(doc.Key, w.prefix) {
-		return nil
-	}
-	name := doc.unitName()
-	if name == w.ignore {
-		return nil
-	}
-	if ch.Revno == -1 {
-		if w.knownUnits.Contains(name) {
-			changes.Left = append(changes.Left, name)
-			w.knownUnits.Remove(name)
-		}
-		return nil
-	}
-	if !w.knownUnits.Contains(name) {
-		changes.Entered = append(changes.Entered, name)
-		w.knownUnits.Add(name)
-	}
-	return nil
-}
-
-func (w *RelationScopeWatcher) getInitialEvent() (initial *RelationScopeChange, err error) {
-	changes := &RelationScopeChange{}
+// initialInfo returns an uncommitted scopeInfo with the current set of units.
+func (w *RelationScopeWatcher) initialInfo() (info *scopeInfo, err error) {
 	docs := []relationScopeDoc{}
-	sel := bson.D{{"_id", bson.D{{"$regex", "^" + w.prefix}}}}
-	err = w.st.relationScopes.Find(sel).All(&docs)
-	if err != nil {
+	sel := bson.D{
+		{"_id", bson.D{{"$regex", "^" + w.prefix}}},
+		{"departing", bson.D{{"$ne", true}}},
+	}
+	if err = w.st.relationScopes.Find(sel).All(&docs); err != nil {
 		return nil, err
 	}
+	info = &scopeInfo{
+		base: map[string]bool{},
+		diff: map[string]bool{},
+	}
 	for _, doc := range docs {
 		if name := doc.unitName(); name != w.ignore {
-			changes.Entered = append(changes.Entered, name)
-			w.knownUnits.Add(name)
-		}
-	}
-	return changes, nil
+			info.add(name)
+		}
+	}
+	return info, nil
+}
+
+// mergeChanges updates info with the contents of the changes in ids. False
+// values are always treated as removed; true values cause the associated
+// document to be read, and whether it's treated as added or removed depends
+// on the value of the document's Departing field.
+func (w *RelationScopeWatcher) mergeChanges(info *scopeInfo, ids map[interface{}]bool) (err error) {
+	existIds := []string{}
+	for id_, exists := range ids {
+		id, ok := id_.(string)
+		if !ok {
+			logger.Warningf("ignoring bad relation scope id: %#v", id_)
+			continue
+		}
+		if exists {
+			existIds = append(existIds, id)
+		} else {
+			doc := &relationScopeDoc{Key: id}
+			info.remove(doc.unitName())
+		}
+	}
+	docs := []relationScopeDoc{}
+	sel := bson.D{{"_id", bson.D{{"$in", existIds}}}}
+	if err := w.st.relationScopes.Find(sel).All(&docs); err != nil {
+		return err
+	}
+	for _, doc := range docs {
+		name := doc.unitName()
+		if doc.Departing {
+			info.remove(name)
+		} else if name != w.ignore {
+			info.add(name)
+		}
+	}
+	return nil
 }
 
 func (w *RelationScopeWatcher) loop() error {
-	ch := make(chan watcher.Change)
-	w.st.watcher.WatchCollection(w.st.relationScopes.Name, ch)
-	defer w.st.watcher.UnwatchCollection(w.st.relationScopes.Name, ch)
-	changes, err := w.getInitialEvent()
+	in := make(chan watcher.Change)
+	filter := func(key interface{}) bool {
+		return strings.HasPrefix(key.(string), w.prefix)
+	}
+	w.st.watcher.WatchCollectionWithFilter(w.st.relationScopes.Name, in, filter)
+	defer w.st.watcher.UnwatchCollection(w.st.relationScopes.Name, in)
+	info, err := w.initialInfo()
 	if err != nil {
 		return err
 	}
+	sent := false
 	out := w.out
 	for {
 		select {
@@ -548,15 +614,22 @@
 			return stateWatcherDeadError(w.st.watcher.Err())
 		case <-w.tomb.Dying():
 			return tomb.ErrDying
-		case c := <-ch:
-			if err := w.mergeChange(changes, c); err != nil {
+		case ch := <-in:
+			latest, ok := collect(ch, in, w.tomb.Dying())
+			if !ok {
+				return tomb.ErrDying
+			}
+			if err := w.mergeChanges(info, latest); err != nil {
 				return err
 			}
-			if !changes.isEmpty() {
+			if info.hasChanges() {
 				out = w.out
+			} else if sent {
+				out = nil
 			}
-		case out <- changes:
-			changes = &RelationScopeChange{}
+		case out <- info.changes():
+			info.commit()
+			sent = true
 			out = nil
 		}
 	}
@@ -607,6 +680,15 @@
 	return len(changes.Changed)+len(changes.Departed) == 0
 }
 
+func setRelationUnitChangeVersion(changes *params.RelationUnitsChange, key string, revno int64) {
+	name := unitNameFromScopeKey(key)
+	settings := params.UnitSettings{Version: revno}
+	if changes.Changed == nil {
+		changes.Changed = map[string]params.UnitSettings{}
+	}
+	changes.Changed[name] = settings
+}
+
 // mergeSettings reads the relation settings node for the unit with the
 // supplied id, and sets a value in the Changed field keyed on the unit's
 // name. It returns the mgo/txn revision number of the settings node.
@@ -615,13 +697,7 @@
 	if err != nil {
 		return -1, err
 	}
-	name := (&relationScopeDoc{key}).unitName()
-	settings := params.UnitSettings{Version: node.txnRevno}
-	if changes.Changed == nil {
-		changes.Changed = map[string]params.UnitSettings{name: settings}
-	} else {
-		changes.Changed[name] = settings
-	}
+	setRelationUnitChangeVersion(changes, key, node.txnRevno)
 	return node.txnRevno, nil
 }
 
@@ -696,9 +772,11 @@
 				out = nil
 			}
 		case c := <-w.updates:
-			if _, err = w.mergeSettings(&changes, c.Id.(string)); err != nil {
-				return err
+			id, ok := c.Id.(string)
+			if !ok {
+				logger.Warningf("ignoring bad relation scope id: %#v", c.Id)
 			}
+			setRelationUnitChangeVersion(&changes, id, c.Revno)
 			out = w.out
 		case out <- changes:
 			sentInitial = true