Merge lp:~dimitern/juju-core/001-upgrade-charm-filter-restart-watchers into lp:~juju/juju-core/trunk

[Dimiter Naydenov]

uniter: WantAllRelationsEvents() filter

This is the first CL in preparation for the
implementation of upgrade-charm. Here, we'll
prepare the Uniter to handle upgrades better,
by restarting the relations watcher in the
filter, when requested with WantAllRelationsEvents().

https://codereview.appspot.com/7373046/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+149910_code.launchpad.net,

Message:
Please take a look.

Description:
uniter: restart filter relations watcher

This is the first CL in preparation for the
implementation of upgrade-charm. Here, we'll
prepare the Uniter to handle upgrades better,
by restarting the relations watcher in the
filter, when we detect an upgrade.

https://code.launchpad.net/~dimitern/juju-core/001-upgrade-charm-filter-restart-watchers/+merge/149910

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/service.go
   M worker/uniter/filter.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision:
<email address hidden>

Index: state/service.go
=== modified file 'state/service.go'
--- state/service.go 2013-02-20 13:27:51 +0000
+++ state/service.go 2013-02-21 19:02:04 +0000
@@ -267,6 +267,11 @@
  // SetCharm changes the charm for the service. New units will be started
with
  // this charm, and existing units will be upgraded to use it. If force is
true,
  // units will be upgraded even if they are in an error state.
+//
+// A sanity check is performed:
+// * The new charm must still implement the local endpoints of every
+// relation the service is in, otherwise return an error.
+//
  func (s *Service) SetCharm(ch *Charm, force bool) (err error) {
   ops := []txn.Op{{
    C: s.st.services.Name,

Index: worker/uniter/filter.go
=== modified file 'worker/uniter/filter.go'
--- worker/uniter/filter.go 2013-01-16 17:04:22 +0000
+++ worker/uniter/filter.go 2013-02-21 19:02:04 +0000
@@ -179,7 +179,8 @@
   configw := f.service.WatchConfig()
   defer watcher.Stop(configw, &f.tomb)
   relationsw := f.service.WatchRelations()
- defer watcher.Stop(relationsw, &f.tomb)
+ // relationsw can be recreated when sending upgrades
+ defer func() { watcher.Stop(relationsw, &f.tomb) }()

   // Config events cannot be meaningfully discarded until one is available;
   // once we receive the initial change, we unblock discard requests by
@@ -218,8 +219,11 @@
     discardConfig = f.discardConfig
    case ids, ok := <-relationsw.Changes():
     log.Debugf("filter: got relations change")
- if !ok {
- return watcher.MustErr(relationsw)
+ if !ok && relationsw.Err() == nil {
+ // It was stopped cleanly, so restart it
+ relationsw = f.service.WatchRelations()
+ } else {
+ return relationsw.Err()
     }
     f.relationsChanged(ids)

@@ -228,6 +232,10 @@
     log.Debugf("worker/uniter/filter: sent upgrade event")
     f.upgradeRequested.url = f.upgrade.URL()
     f.outUpgrade = nil
+ // To avoid missing any changes to relations, we need
+ // to restart the watcher after an upgrade is triggered
+ // (it'll get recreated above).
+ relationsw.Stop()
    case f.outResolved <- f.resolved:
     log.Debugf("worker/uniter/filter: sent resolved event")
     f.outResolved = nil

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/7373046/

[William Reade (fwereade)]

On reflection, as discussed online, too much magic. I recommend a
WantAllRelationsEvents method for clarity's sake.

https://codereview.appspot.com/7373046/diff/3001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/7373046/diff/3001/worker/uniter/filter.go#newcode226
worker/uniter/filter.go:226: return relationsw.Err()
I think this bit is equivalent to "if ok {return nil}". Don't you need:

if !ok {
   if err := relationsw.Err(); err != nil {
     return err
   }
   relationsw = f.service.WatchRelations()
}

https://codereview.appspot.com/7373046/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/7373046/diff/3001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/7373046/diff/3001/worker/uniter/filter.go#newcode226
worker/uniter/filter.go:226: return relationsw.Err()
On 2013/02/22 10:25:28, fwereade wrote:
> I think this bit is equivalent to "if ok {return nil}". Don't you
need:

> if !ok {
> if err := relationsw.Err(); err != nil {
> return err
> }
> relationsw = f.service.WatchRelations()
> }

As discussed, changed as it was.

https://codereview.appspot.com/7373046/

[William Reade (fwereade)]

LGTM, one trivial

https://codereview.appspot.com/7373046/diff/8001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/7373046/diff/8001/worker/uniter/filter.go#newcode144
worker/uniter/filter.go:144: // pending relation changes.
...should send an event for every known relation.

https://codereview.appspot.com/7373046/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/7373046/diff/8001/worker/uniter/filter.go
File worker/uniter/filter.go (right):

https://codereview.appspot.com/7373046/diff/8001/worker/uniter/filter.go#newcode144
worker/uniter/filter.go:144: // pending relation changes.
On 2013/02/22 11:25:33, fwereade wrote:
> ...should send an event for every known relation.

Done.

https://codereview.appspot.com/7373046/diff/8001/worker/uniter/filter.go#newcode268
worker/uniter/filter.go:268: // restart the relations watcher
On 2013/02/22 11:41:23, TheMue wrote:
> Maybe too pedantic, but like a correct style here: "Restart the
relations
> watcher.". See the "Send events ..." comment above.

Done.

https://codereview.appspot.com/7373046/

[Dimiter Naydenov (dimitern)]

*** Submitted:

uniter: WantAllRelationsEvents() filter

This is the first CL in preparation for the
implementation of upgrade-charm. Here, we'll
prepare the Uniter to handle upgrades better,
by restarting the relations watcher in the
filter, when requested with WantAllRelationsEvents().

R=fwereade, TheMue
CC=
https://codereview.appspot.com/7373046

https://codereview.appspot.com/7373046/

=== modified file 'worker/uniter/filter.go'
--- worker/uniter/filter.go	2013-01-16 17:04:22 +0000
+++ worker/uniter/filter.go	2013-02-22 14:13:22 +0000
@@ -36,8 +36,9 @@
 
 	// The want* chans are used to indicate that the filter should send
 	// events if it has them available.
-	wantUpgrade  chan serviceCharm
-	wantResolved chan struct{}
+	wantUpgrade      chan serviceCharm
+	wantResolved     chan struct{}
+	wantAllRelations chan struct{}
 
 	// discardConfig is used to indicate that any pending config event
 	// should be discarded.
@@ -59,19 +60,20 @@
 // supplied unit.
 func newFilter(st *state.State, unitName string) (*filter, error) {
 	f := &filter{
-		st:             st,
-		outUnitDying:   make(chan struct{}),
-		outConfig:      make(chan struct{}),
-		outConfigOn:    make(chan struct{}),
-		outUpgrade:     make(chan *state.Charm),
-		outUpgradeOn:   make(chan *state.Charm),
-		outResolved:    make(chan state.ResolvedMode),
-		outResolvedOn:  make(chan state.ResolvedMode),
-		outRelations:   make(chan []int),
-		outRelationsOn: make(chan []int),
-		wantResolved:   make(chan struct{}),
-		wantUpgrade:    make(chan serviceCharm),
-		discardConfig:  make(chan struct{}),
+		st:               st,
+		outUnitDying:     make(chan struct{}),
+		outConfig:        make(chan struct{}),
+		outConfigOn:      make(chan struct{}),
+		outUpgrade:       make(chan *state.Charm),
+		outUpgradeOn:     make(chan *state.Charm),
+		outResolved:      make(chan state.ResolvedMode),
+		outResolvedOn:    make(chan state.ResolvedMode),
+		outRelations:     make(chan []int),
+		outRelationsOn:   make(chan []int),
+		wantResolved:     make(chan struct{}),
+		wantAllRelations: make(chan struct{}),
+		wantUpgrade:      make(chan serviceCharm),
+		discardConfig:    make(chan struct{}),
 	}
 	go func() {
 		defer f.tomb.Done()
@@ -138,6 +140,15 @@
 	}
 }
 
+// WantAllRelationsEvents indicates that the filter should send an
+// event for every known relation.
+func (f *filter) WantAllRelationsEvents() {
+	select {
+	case <-f.tomb.Dying():
+	case f.wantAllRelations <- nothing:
+	}
+}
+
 // WantResolvedEvent indicates that the filter should send a resolved event
 // if one is available.
 func (f *filter) WantResolvedEvent() {
@@ -179,7 +190,8 @@
 	configw := f.service.WatchConfig()
 	defer watcher.Stop(configw, &f.tomb)
 	relationsw := f.service.WatchRelations()
-	defer watcher.Stop(relationsw, &f.tomb)
+	// relationsw can get restarted, so we need to bind its current value here
+	defer func() { watcher.Stop(relationsw, &f.tomb) }()
 
 	// Config events cannot be meaningfully discarded until one is available;
 	// once we receive the initial change, we unblock discard requests by
@@ -251,6 +263,11 @@
 			if f.resolved != state.ResolvedNone {
 				f.outResolved = f.outResolvedOn
 			}
+		case <-f.wantAllRelations:
+			log.Debugf("worker/uniter/filter: want all relations events")
+			// Restart the relations watcher.
+			watcher.Stop(relationsw, &f.tomb)
+			relationsw = f.service.WatchRelations()
 		case <-discardConfig:
 			log.Debugf("filter: discarded config event")
 			f.outConfig = nil

=== modified file 'worker/uniter/filter_test.go'
--- worker/uniter/filter_test.go	2013-01-25 18:33:16 +0000
+++ worker/uniter/filter_test.go	2013-02-22 14:13:22 +0000
@@ -327,6 +327,11 @@
 	assertChange([]int{0, 1})
 	assertNoChange()
 
+	// Request all relations events; no changes should happen
+	f.WantAllRelationsEvents()
+	assertChange([]int{0, 1})
+	assertNoChange()
+
 	// Add another relation, and change another's Life (by entering scope before
 	// Destroy, thereby setting the relation to Dying); check event.
 	s.addRelation(c)