Merge lp:~themue/pyjuju/go-state-unit-resolved-watcher into lp:pyjuju/go

[Frank Mueller]

Added ResolvedWatcher for Units.

It reads the resolved mode out of the node content if
it's created or changed.

https://codereview.appspot.com/6059047/

[Gustavo Niemeyer (niemeyer)]

This needs a pre-req branch when you're proposing. Look at the diffs.
It's piling up every single branch you've sent before.

https://codereview.appspot.com/6059047/

[William Reade (fwereade)]

Looks generally sensible, only showstopper is upgrade node content.

This isn't your fault -- I don't remember seeing this change discussed
on the list -- but it needs to be done all the same.

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

https://codereview.appspot.com/6059047/diff/1/state/state.go#newcode292
state/state.go:292: // A non-existing node is treaten as an empty node.
s/non-existing/nonexistent/
s/treaten/treated/

(I actually rather like "treaten", but... ;))

https://codereview.appspot.com/6059047/diff/1/state/unit.go
File state/unit.go (left):

https://codereview.appspot.com/6059047/diff/1/state/unit.go#oldcode453
state/unit.go:453:
This file is getting a bit unwieldy IMO. Could we move the Watcher types
into their own file?

https://codereview.appspot.com/6059047/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode287
state/unit.go:287: if err := validResolvedMode(mode, false); err != nil
{
I *think* acceptNone should be true here, shouldn't it?

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode469
state/unit.go:469: // parseResolveMode parses a given YAML for the
resolve mode
s/parseResolveMode/parseResolvedMode/

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472
state/unit.go:472: setting := &struct{ Retry ResolvedMode }{}
I have a slight preference for a non-anonymous struct
(ResolvedNodeContent, or something) here... we need to set the content
in the tests, and we will also need to do so from the CLI, and I'd
rather have a reasonable certainty that everything hitting this node is
using the same mechanism to do so.

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538
state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() {
Maybe "run" rather than "loop"?

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode552
state/unit.go:552: case w.changeChan <- change.Exists:
Existence isn't enough; there's a "force" upgrade mode now as well
(meaning "upgrade even if not actually running"), and we'll need to
handle that for feature parity.

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode600
state/unit.go:600: func (w *ResolvedWatcher) loop() {
As above, I prefer "run"; ymmv.

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode17
state/watch_test.go:17: return nil, false, false
How can we hit this path?

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode77
state/watch_test.go:77: case <-time.After(200 * time.Millisecond):
Why 200ms here and 100ms above?

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode119
state/watch_test.go:119: go func() {
Given that the events *can* coalesce, even if it's not very likely in
this instance, I think it would be better to interleave this stuff with
the tests.

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode164
state/watch_test.go:164: go func() {
As above.

https://codereview.appspot.com/6059047/diff/1/state/...

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/6059047/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472
state/unit.go:472: setting := &struct{ Retry ResolvedMode }{}
On 2012/04/19 08:29:50, fwereade wrote:
> I have a slight preference for a non-anonymous struct
(ResolvedNodeContent, or
> something) here... we need to set the content in the tests, and we
will also
> need to do so from the CLI, and I'd rather have a reasonable certainty
that
> everything hitting this node is using the same mechanism to do so.

orthogonal to this, declaring as a bare variable and using & in the
Unmarshal would read better IMHO,
var setting struct {
     Retry ResolvedMode
}
if err := goyaml.Unmarshal([]byte(yaml), &setting); err != nil {

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538
state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() {
On 2012/04/19 08:29:50, fwereade wrote:
> Maybe "run" rather than "loop"?

+1 ("loop" implies a particular implementation strategy which may not be
used).

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode17
state/watch_test.go:17: return nil, false, false
On 2012/04/19 08:29:50, fwereade wrote:
> How can we hit this path?

+1.
panic("not reached")

https://codereview.appspot.com/6059047/diff/1/state/watch_test.go#newcode119
state/watch_test.go:119: go func() {
On 2012/04/19 08:29:50, fwereade wrote:
> Given that the events *can* coalesce, even if it's not very likely in
this
> instance, I think it would be better to interleave this stuff with the
tests.

+1. especially as it makes the tests run slightly faster.

https://codereview.appspot.com/6059047/

[Gustavo Niemeyer (niemeyer)]

Still waiting for the pre-req to be sorted out, but a few
comments-on-comments.

https://codereview.appspot.com/6059047/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472
state/unit.go:472: setting := &struct{ Retry ResolvedMode }{}
On 2012/04/19 08:29:50, fwereade wrote:
> I have a slight preference for a non-anonymous struct
(ResolvedNodeContent, or
> something) here... we need to set the content in the tests, and we
will also
> need to do so from the CLI, and I'd rather have a reasonable certainty
that
> everything hitting this node is using the same mechanism to do so.

-1. The API from this package isn't about ZooKeeper nodes. It's about
services and machines.

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode538
state/unit.go:538: func (w *NeedsUpgradeWatcher) loop() {
On 2012/04/19 08:58:49, rog wrote:
> On 2012/04/19 08:29:50, fwereade wrote:
> > Maybe "run" rather than "loop"?

> +1 ("loop" implies a particular implementation strategy which may not
be used).

-1. This *is* a loop, and this isn't part of the API, which means it
*is* the implementation as well. I don't see the point.

https://codereview.appspot.com/6059047/

[William Reade (fwereade)]

https://codereview.appspot.com/6059047/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6059047/diff/1/state/unit.go#newcode472
state/unit.go:472: setting := &struct{ Retry ResolvedMode }{}
On 2012/04/19 09:11:55, niemeyer wrote:
> On 2012/04/19 08:29:50, fwereade wrote:
> > I have a slight preference for a non-anonymous struct
(ResolvedNodeContent, or
> > something) here... we need to set the content in the tests, and we
will also
> > need to do so from the CLI, and I'd rather have a reasonable
certainty that
> > everything hitting this node is using the same mechanism to do so.

> -1. The API from this package isn't about ZooKeeper nodes. It's about
services
> and machines.

It doesn't have to be public; how would you feel about
"resolvedNodeContent" instead? I'm really just trying to extend the
benefits of static typing to node content...

https://codereview.appspot.com/6059047/

[Gustavo Niemeyer (niemeyer)]

Frank, please have a look at the CL. It *still* contains unrelated
changes that are not supposed to be reviewed here.

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

https://codereview.appspot.com/6059047/diff/1/state/state.go#newcode292
state/state.go:292: // A non-existing node is treaten as an empty node.
On 2012/04/19 08:29:50, fwereade wrote:
> s/non-existing/nonexistent/

The correct form is actually non-existent, AFAIK:

http://oxforddictionaries.com/definition/non-existent

https://codereview.appspot.com/6059047/

[Gustavo Niemeyer (niemeyer)]

On 2012/04/24 10:01:51, TheMue wrote:
> A review comment of William for this proposal has been that the
handling of
> "needs upgrade" is outdated due to a change of the Python code. Do I
understand
> you right that you now want me to roll the changes back to the old
> implementation and create an extra change for it?

Oh, so an unrelated change to the needs-upgrade functionality was merged
in purposefully?

This is still the same topic we talked about back at the Rally in
Budapest: changes should be self-contained. Fixing the NeedsUpgrades
behavior is definitely not part of what this CL is supposed to be
covering. From its summary:

"""
Added ResolvedWatcher for Units.

It reads the resolved mode out of the node content if
it's created or changed.
"""

If you get a review comment from someone suggesting unrelated changes
and you agree with them, just put that in another branch and move on.

https://codereview.appspot.com/6059047/

[Gustavo Niemeyer (niemeyer)]

On 2012/04/24 12:10:21, TheMue wrote:
> Understood, I will try to be more thoughtful in future. So far to me
the whole
> watcher topic has been one task.

> How about this one here? I would like you to review this time both
changes at
> once as a final exception.

Sorry, but that was the same thing you said back in Budapest in that
same circumstance. It should be trivial to revert this, given that you
just added revisions on top of the original branch. Just follow
something like this:

bzr log # find the revision you started adding unrelated changes
cd ..
mv original-branch new-work
bzr branch -r <revision> new-work original-branch
cd original-branch
lbox propose

If done right, the above procedure should revert the changes to this CL
in a painless way.

https://codereview.appspot.com/6059047/

[Gustavo Niemeyer (niemeyer)]

I believe this became https://codereview.appspot.com/6120045/

https://codereview.appspot.com/6059047/

Unmerged revisions

118. By Frank Mueller on 2012-04-24

Merged missing parent changes.

117. By Frank Mueller on 2012-04-24

Done review changes after rollback and concentrate on the resolve watcher.

116. By Frank Mueller on 2012-04-18

Added ResolvedWatcher for Units.

=== modified file 'state/state_test.go'
--- state/state_test.go	2012-03-26 12:47:37 +0000
+++ state/state_test.go	2012-04-24 14:35:46 +0000
@@ -778,7 +778,7 @@
 	c.Assert(err, ErrorMatches, "no unused machine found")
 }
 
-func (s *StateSuite) TestGetSetClearUnitUpgrate(c *C) {
+func (s *StateSuite) TestGetSetClearUnitUpgrade(c *C) {
 	// Check that setting and clearing an upgrade flag on a unit works.
 	dummy, _ := addDummyCharm(c, s.st)
 	wordpress, err := s.st.AddService("wordpress", dummy)
@@ -786,38 +786,56 @@
 	unit, err := wordpress.AddUnit()
 	c.Assert(err, IsNil)
 
-	// Defaults to false.
-	upgrade, err := unit.NeedsUpgrade()
+	// Defaults to false and false.
+	needsUpgrade, err := unit.NeedsUpgrade()
 	c.Assert(err, IsNil)
-	c.Assert(upgrade, Equals, false)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{false, false})
 
 	// Can be set.
-	err = unit.SetNeedsUpgrade()
-	c.Assert(err, IsNil)
-	upgrade, err = unit.NeedsUpgrade()
-	c.Assert(err, IsNil)
-	c.Assert(upgrade, Equals, true)
+	err = unit.SetNeedsUpgrade(false)
+	c.Assert(err, IsNil)
+	needsUpgrade, err = unit.NeedsUpgrade()
+	c.Assert(err, IsNil)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{true, false})
 
 	// Can be set multiple times.
-	err = unit.SetNeedsUpgrade()
-	c.Assert(err, IsNil)
-	upgrade, err = unit.NeedsUpgrade()
-	c.Assert(err, IsNil)
-	c.Assert(upgrade, Equals, true)
+	err = unit.SetNeedsUpgrade(false)
+	c.Assert(err, IsNil)
+	needsUpgrade, err = unit.NeedsUpgrade()
+	c.Assert(err, IsNil)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{true, false})
 
 	// Can be cleared.
 	err = unit.ClearNeedsUpgrade()
 	c.Assert(err, IsNil)
-	upgrade, err = unit.NeedsUpgrade()
+	needsUpgrade, err = unit.NeedsUpgrade()
 	c.Assert(err, IsNil)
-	c.Assert(upgrade, Equals, false)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{false, false})
 
 	// Can be cleared multiple times
 	err = unit.ClearNeedsUpgrade()
 	c.Assert(err, IsNil)
-	upgrade, err = unit.NeedsUpgrade()
-	c.Assert(err, IsNil)
-	c.Assert(upgrade, Equals, false)
+	needsUpgrade, err = unit.NeedsUpgrade()
+	c.Assert(err, IsNil)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{false, false})
+
+	// Can be set forced.
+	err = unit.SetNeedsUpgrade(true)
+	c.Assert(err, IsNil)
+	needsUpgrade, err = unit.NeedsUpgrade()
+	c.Assert(err, IsNil)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{true, true})
+
+	// Can be set forced multiple times.
+	err = unit.SetNeedsUpgrade(true)
+	c.Assert(err, IsNil)
+	needsUpgrade, err = unit.NeedsUpgrade()
+	c.Assert(err, IsNil)
+	c.Assert(needsUpgrade, DeepEquals, &state.NeedsUpgrade{true, true})
+
+	// Can't be set multipe with different force flag. (TODO)
+	err = unit.SetNeedsUpgrade(false)
+	c.Assert(err, ErrorMatches, `unit upgrade already enabled: "wordpress/0"`)
 }
 
 func (s *StateSuite) TestGetSetClearResolved(c *C) {

=== modified file 'state/unit.go'
--- state/unit.go	2012-04-20 09:39:38 +0000
+++ state/unit.go	2012-04-24 14:35:46 +0000
@@ -28,6 +28,13 @@
 	ResolvedNoHooks    ResolvedMode = 1001
 )
 
+// NeedsUpgrade describes if a unit needs an
+// upgrade and if this is forced.
+type NeedsUpgrade struct {
+	Upgrade bool
+	Force   bool
+}
+
 // agentPingerPeriod defines the period of pinging the
 // ZooKeeper to signal that a unit agent is alive. It's
 // also used by machine.
@@ -233,23 +240,49 @@
 	return retryTopologyChange(u.st.zk, unassignUnit)
 }
 
-// NeedsUpgrade returns whether the unit needs an upgrade.
-func (u *Unit) NeedsUpgrade() (bool, error) {
-	stat, err := u.st.zk.Exists(u.zkNeedsUpgradePath())
+// NeedsUpgrade returns whether the unit needs an upgrade 
+// and if it is forced.
+func (u *Unit) NeedsUpgrade() (*NeedsUpgrade, error) {
+	yaml, _, err := u.st.zk.Get(u.zkNeedsUpgradePath())
+	if zookeeper.IsError(err, zookeeper.ZNONODE) {
+		return &NeedsUpgrade{false, false}, nil
+	}
 	if err != nil {
-		return false, err
-	}
-	return stat != nil, nil
+		return &NeedsUpgrade{false, false}, err
+	}
+	var setting struct {
+		Force bool
+	}
+	if err = goyaml.Unmarshal([]byte(yaml), &setting); err != nil {
+		return &NeedsUpgrade{false, false}, err
+	}
+	return &NeedsUpgrade{true, setting.Force}, nil
 }
 
-// SetNeedsUpgrade informs the unit that it should perform an upgrade.
-func (u *Unit) SetNeedsUpgrade() error {
-	_, err := u.st.zk.Create(u.zkNeedsUpgradePath(), "", 0, zkPermAll)
-	if zookeeper.IsError(err, zookeeper.ZNODEEXISTS) {
-		// Node already exists, so same state.
-		return nil
+// SetNeedsUpgrade informs the unit that it should perform 
+// a regular or forced upgrade.
+func (u *Unit) SetNeedsUpgrade(force bool) error {
+	setNeedsUpgrade := func(oldYaml string, stat *zookeeper.Stat) (string, error) {
+		var setting struct {
+			Force bool
+		}
+		if oldYaml == "" {
+			setting.Force = force
+			newYaml, err := goyaml.Marshal(setting)
+			if err != nil {
+				return "", err
+			}
+			return string(newYaml), nil
+		}
+		if err := goyaml.Unmarshal([]byte(oldYaml), &setting); err != nil {
+			return "", err
+		}
+		if setting.Force != force {
+			return "", fmt.Errorf("unit upgrade already enabled: %q", u.Name())
+		}
+		return oldYaml, nil
 	}
-	return err
+	return u.st.zk.RetryChange(u.zkNeedsUpgradePath(), 0, zkPermAll, setNeedsUpgrade)
 }
 
 // ClearNeedsUpgrade resets the upgrade notification. It is typically
@@ -284,7 +317,7 @@
 		return ResolvedNone, err
 	}
 	mode := setting.Retry
-	if err := validResolvedMode(mode); err != nil {
+	if err := validResolvedMode(mode, false); err != nil {
 		return ResolvedNone, err
 	}
 	return mode, nil
@@ -297,7 +330,7 @@
 // reexecute previous failed hooks or to continue as if they had 
 // succeeded before.
 func (u *Unit) SetResolved(mode ResolvedMode) error {
-	if err := validResolvedMode(mode); err != nil {
+	if err := validResolvedMode(mode, false); err != nil {
 		return err
 	}
 	setting := &struct{ Retry ResolvedMode }{mode}
@@ -322,6 +355,13 @@
 	return err
 }
 
+// WatchResolved returns a watcher that fires when the unit 
+// is marked as having had its problems resolved. See 
+// SetResolved for details.
+func (u *Unit) WatchResolved() *ResolvedWatcher {
+	return newResolvedWatcher(u.st, u.zkResolvedPath())
+}
+
 // OpenPort sets the policy of the port with protocol and number to be opened.
 func (u *Unit) OpenPort(protocol string, number int) error {
 	openPort := func(oldYaml string, stat *zookeeper.Stat) (string, error) {
@@ -459,9 +499,29 @@
 	return parts[0], int(sequenceNo), nil
 }
 
+// parseResolvedMode parses a given YAML for the resolve mode
+// and checks if it's valid.
+func parseResolvedMode(yaml string) (ResolvedMode, error) {
+	var setting struct {
+		Retry ResolvedMode
+	}
+	if err := goyaml.Unmarshal([]byte(yaml), &setting); err != nil {
+		return ResolvedNone, err
+	}
+	mode := setting.Retry
+	if err := validResolvedMode(mode, true); err != nil {
+		return ResolvedNone, err
+	}
+	return mode, nil
+}
+
 // validResolvedMode ensures that only valid values for the
-// resolved mode are used.
-func validResolvedMode(mode ResolvedMode) error {
+// resolved mode are used. If acceptNone is true also the
+// mode ResolvedNone is valid.
+func validResolvedMode(mode ResolvedMode, acceptNone bool) error {
+	if acceptNone && mode == ResolvedNone {
+		return nil
+	}
 	if mode != ResolvedRetryHooks && mode != ResolvedNoHooks {
 		return fmt.Errorf("invalid error resolution mode: %d", mode)
 	}
@@ -474,7 +534,7 @@
 	path       string
 	tomb       tomb.Tomb
 	watcher    *watcher.ContentWatcher
-	changeChan chan bool
+	changeChan chan NeedsUpgrade
 }
 
 // newNeedsUpgradeWatcher creates and starts a new resolved flag node 
@@ -483,7 +543,7 @@
 	w := &NeedsUpgradeWatcher{
 		st:         st,
 		path:       path,
-		changeChan: make(chan bool),
+		changeChan: make(chan NeedsUpgrade),
 		watcher:    watcher.NewContentWatcher(st.zk, path),
 	}
 	go w.loop()
@@ -493,7 +553,7 @@
 // Changes returns a channel that will receive notifications
 // about upgrades for the unit. Note that multiple changes
 // may be observed as a single event in the channel.
-func (w *NeedsUpgradeWatcher) Changes() <-chan bool {
+func (w *NeedsUpgradeWatcher) Changes() <-chan NeedsUpgrade {
 	return w.changeChan
 }
 
@@ -523,12 +583,91 @@
 				w.tomb.Kill(nil)
 				return
 			}
-			select {
-			case <-w.watcher.Dying():
-				return
-			case <-w.tomb.Dying():
-				return
-			case w.changeChan <- change.Exists:
+			var needsUpgrade NeedsUpgrade
+			if change.Exists {
+				needsUpgrade.Upgrade = true
+				var setting struct {
+					Force bool
+				}
+				if err := goyaml.Unmarshal([]byte(change.Content), &setting); err != nil {
+					w.tomb.Kill(err)
+					return
+				}
+				needsUpgrade.Force = setting.Force
+			}
+			select {
+			case <-w.watcher.Dying():
+				return
+			case <-w.tomb.Dying():
+				return
+			case w.changeChan <- needsUpgrade:
+			}
+		}
+	}
+}
+
+// ResolvedWatcher observes changes to a resolved flag node.
+type ResolvedWatcher struct {
+	st         *State
+	path       string
+	tomb       tomb.Tomb
+	watcher    *watcher.ContentWatcher
+	changeChan chan ResolvedMode
+}
+
+// newResolvedWatcher creates and starts a new resolved flag node 
+// watcher for the given path.
+func newResolvedWatcher(st *State, path string) *ResolvedWatcher {
+	w := &ResolvedWatcher{
+		st:         st,
+		path:       path,
+		changeChan: make(chan ResolvedMode),
+		watcher:    watcher.NewContentWatcher(st.zk, path),
+	}
+	go w.loop()
+	return w
+}
+
+// Changes returns a channel that will receive the new
+// resolved mode when a change is detected. Note that multiple
+// changes may be observed as a single event in the channel.
+func (w *ResolvedWatcher) Changes() <-chan ResolvedMode {
+	return w.changeChan
+}
+
+// Stop stops the watch and returns any error encountered
+// while watching. This method should always be called
+// before discarding the watcher.
+func (w *ResolvedWatcher) Stop() error {
+	w.tomb.Kill(nil)
+	if err := w.watcher.Stop(); err != nil {
+		w.tomb.Wait()
+		return err
+	}
+	return w.tomb.Wait()
+}
+
+// loop is the backend for watching the resolved flag node.
+func (w *ResolvedWatcher) loop() {
+	defer w.tomb.Done()
+	defer close(w.changeChan)
+
+	for {
+		select {
+		case <-w.tomb.Dying():
+			return
+		case change := <-w.watcher.Changes():
+			mode, err := parseResolvedMode(change.Content)
+			if err != nil {
+				w.tomb.Kill(err)
+				return
+			}
+			select {
+			case <-w.watcher.Dying():
+				return
+			case <-w.tomb.Dying():
+				return
+			case w.changeChan <- mode:
 			}
 		}
 	}

=== modified file 'state/watch_test.go'
--- state/watch_test.go	2012-04-19 15:23:59 +0000
+++ state/watch_test.go	2012-04-24 14:35:46 +0000
@@ -2,6 +2,7 @@
 
 import (
 	. "launchpad.net/gocheck"
+	"launchpad.net/juju/go/state"
 	"time"
 )
 
@@ -92,26 +93,26 @@
 
 	go func() {
 		time.Sleep(50 * time.Millisecond)
-		err = unit.SetNeedsUpgrade()
+		err = unit.SetNeedsUpgrade(false)
 		c.Assert(err, IsNil)
 		time.Sleep(50 * time.Millisecond)
 		err = unit.ClearNeedsUpgrade()
 		c.Assert(err, IsNil)
 		time.Sleep(50 * time.Millisecond)
-		err = unit.SetNeedsUpgrade()
+		err = unit.SetNeedsUpgrade(true)
 		c.Assert(err, IsNil)
 	}()
 
-	var expectedChanges = []bool{
-		true,
-		false,
-		true,
+	var expectedChanges = []state.NeedsUpgrade{
+		{true, false},
+		{false, false},
+		{true, true},
 	}
 	for _, want := range expectedChanges {
 		select {
 		case got, ok := <-needsUpgradeWatcher.Changes():
 			c.Assert(ok, Equals, true)
-			c.Assert(got, Equals, want)
+			c.Assert(got, DeepEquals, want)
 		case <-time.After(200 * time.Millisecond):
 			c.Fatalf("didn't get change: %#v", want)
 		}
@@ -126,3 +127,49 @@
 	err = needsUpgradeWatcher.Stop()
 	c.Assert(err, IsNil)
 }
+
+func (s *StateSuite) TestUnitWatchResolved(c *C) {
+	dummy, _ := addDummyCharm(c, s.st)
+	wordpress, err := s.st.AddService("wordpress", dummy)
+	c.Assert(err, IsNil)
+	c.Assert(wordpress.Name(), Equals, "wordpress")
+	unit, err := wordpress.AddUnit()
+	c.Assert(err, IsNil)
+	resolvedWatcher := unit.WatchResolved()
+
+	go func() {
+		time.Sleep(50 * time.Millisecond)
+		err = unit.SetResolved(state.ResolvedRetryHooks)
+		c.Assert(err, IsNil)
+		time.Sleep(50 * time.Millisecond)
+		err = unit.ClearResolved()
+		c.Assert(err, IsNil)
+		time.Sleep(50 * time.Millisecond)
+		err = unit.SetResolved(state.ResolvedNoHooks)
+		c.Assert(err, IsNil)
+	}()
+
+	var expectedChanges = []state.ResolvedMode{
+		state.ResolvedRetryHooks,
+		state.ResolvedNone,
+		state.ResolvedNoHooks,
+	}
+	for _, want := range expectedChanges {
+		select {
+		case got, ok := <-resolvedWatcher.Changes():
+			c.Assert(ok, Equals, true)
+			c.Assert(got, Equals, want)
+		case <-time.After(200 * time.Millisecond):
+			c.Fatalf("didn't get change: %#v", want)
+		}
+	}
+
+	select {
+	case got, _ := <-resolvedWatcher.Changes():
+		c.Fatalf("got unexpected change: %#v", got)
+	case <-time.After(100 * time.Millisecond):
+	}
+
+	err = resolvedWatcher.Stop()
+	c.Assert(err, IsNil)
+}