uniter, state: handle upgrades better

worker/uniter/filter_test.go

 package uniter
 
 import (
         "fmt"
         . "launchpad.net/gocheck"
         "launchpad.net/juju-core/charm"
         "launchpad.net/juju-core/juju/testing"
         "launchpad.net/juju-core/state"
         "launchpad.net/juju-core/worker"
+        "launchpad.net/tomb"
         "time"
 )
 
 type FilterSuite struct {
         testing.JujuConnSuite
         wordpress  *state.Service
         unit       *state.Unit
         mysqlcharm *state.Charm
+        wpcharm    *state.Charm
 }
 
 var _ = Suite(&FilterSuite{})
 
 func (s *FilterSuite) SetUpTest(c *C) {
         s.JujuConnSuite.SetUpTest(c)
         var err error
-        s.wordpress, err = s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
+        s.wpcharm = s.AddTestingCharm(c, "wordpress")
+        s.wordpress, err = s.State.AddService("wordpress", s.wpcharm)
         c.Assert(err, IsNil)
         s.unit, err = s.wordpress.AddUnit()
         c.Assert(err, IsNil)
 }
 
 func (s *FilterSuite) TestUnitDeath(c *C) {
         f, err := newFilter(s.State, s.unit.Name())
         c.Assert(err, IsNil)
         defer f.Stop()
         assertNotClosed := func() {
@@ skipping 120 matching lines @@
         err = s.unit.ClearResolved()
         c.Assert(err, IsNil)
         f.WantResolvedEvent()
         err = s.unit.SetResolved(state.ResolvedNoHooks)
         c.Assert(err, IsNil)
         f.WantResolvedEvent()
         assertChange(state.ResolvedNoHooks)
         assertNoChange()
 }
 
-func (s *FilterSuite) TestCharmEvents(c *C) {
+func (s *FilterSuite) TestCharmUpgradeEvents(c *C) {
         oldCharm := s.AddTestingCharm(c, "upgrade1")
         svc, err := s.State.AddService("upgradetest", oldCharm)
         c.Assert(err, IsNil)
         unit, err := svc.AddUnit()
         c.Assert(err, IsNil)
 
         f, err := newFilter(s.State, unit.Name())
         c.Assert(err, IsNil)
         defer f.Stop()
 
         // No initial event is sent.
         assertNoChange := func() {
                 s.State.StartSync()
                 select {
                 case sch := <-f.UpgradeEvents():
                         c.Fatalf("unexpected %#v", sch)
                 case <-time.After(50 * time.Millisecond):
                 }
         }
         assertNoChange()
 
-        // Request an event relative to the existing state; nothing.
+        // Setting a charm generates no new events if it already matches.
+        err = f.SetCharm(oldCharm.URL())
+        c.Assert(err, IsNil)
+        assertNoChange()
+
+        // Explicitly request an event relative to the existing state; nothing.
         f.WantUpgradeEvent(false)
-        // TODO: is this needed here?

[TheMue, 2013/03/01 10:31:07]

See above.

[dimitern, 2013/03/01 16:42:37]

Gone now.

-        f.SetCharm(oldCharm.URL())
         assertNoChange()
 
         // Change the service in an irrelevant way; no events.
         err = svc.SetExposed()
         c.Assert(err, IsNil)
         assertNoChange()
 
         // Change the service's charm; new event received.
         newCharm := s.AddTestingCharm(c, "upgrade2")
         err = svc.SetCharm(newCharm, false)
         c.Assert(err, IsNil)
         assertChange := func(url *charm.URL) {
                 s.State.Sync()
                 select {
                 case upgradeCharm := <-f.UpgradeEvents():
                         c.Assert(upgradeCharm, DeepEquals, url)
                 case <-time.After(50 * time.Millisecond):
                         c.Fatalf("timed out")
                 }
         }
         assertChange(newCharm.URL())
         assertNoChange()
 
-        // Request a change relative to the original state, unforced;
-        // same event is sent.
+        // Request a new upgrade *unforced* upgrade event, we should see one.
         f.WantUpgradeEvent(false)
-        // TODO: is this needed here?
-        f.SetCharm(oldCharm.URL())
         assertChange(newCharm.URL())
         assertNoChange()
 
-        // Request a forced change relative to the initial state; no change...
+        // Request only *forced* upgrade events; nothing.
         f.WantUpgradeEvent(true)
-        // TODO: is this needed here?
-        f.SetCharm(oldCharm.URL())
-        assertNoChange()
-
-        // ...and still no change when we have a forced upgrade to that state...
+        assertNoChange()
+
+        // But when we have a forced upgrade to the same URL, no new event.
         err = svc.SetCharm(oldCharm, true)
         c.Assert(err, IsNil)
         assertNoChange()
 
-        // ...but a *forced* change to a different charm does generate an event.
+        // ...but a *forced* change to a different URL should generate an event.
         err = svc.SetCharm(newCharm, true)
         assertChange(newCharm.URL())
         assertNoChange()
 }
 
 func (s *FilterSuite) TestConfigEvents(c *C) {
         f, err := newFilter(s.State, s.unit.Name())
         c.Assert(err, IsNil)
         defer f.Stop()
 
-        // Initial event.
+        // Test no changes before the charm URL is set.
+        assertNoChange := func() {
+                s.State.StartSync()
+                select {
+                case <-f.ConfigEvents():
+                        c.Fatalf("unexpected config event")
+                case <-time.After(50 * time.Millisecond):
+                }
+        }
+        assertNoChange()
+
         assertChange := func() {
                 s.State.Sync()
                 select {
                 case _, ok := <-f.ConfigEvents():
                         c.Assert(ok, Equals, true)
                 case <-time.After(50 * time.Millisecond):
                         c.Fatalf("timed out")
                 }
-        }
+                assertNoChange()
+        }
+
+        // Set the charm URL to trigger config events.
+        err = f.SetCharm(s.wpcharm.URL())
+        c.Assert(err, IsNil)
         assertChange()
-        assertNoChange := func() {
-                s.State.StartSync()
-                select {
-                case <-f.ConfigEvents():
-                        c.Fatalf("unexpected config event")
-                case <-time.After(50 * time.Millisecond):
-                }
-        }
-        assertNoChange()
+
+        // Make sure the charm URL is set now.
+        s.unit.Refresh()
+        curl, ok := s.unit.CharmURL()
+        c.Assert(ok, Equals, true)
+        c.Assert(curl, DeepEquals, s.wpcharm.URL())
 
         // Change the config; new event received.
         node, err := s.wordpress.Config()
         c.Assert(err, IsNil)
         node.Set("skill-level", 9001)
         _, err = node.Write()
         c.Assert(err, IsNil)
         assertChange()
-        assertNoChange()
 
         // Change the config a couple of times, then reset the events.
         node.Set("title", "20,000 leagues in the cloud")
         _, err = node.Write()
         c.Assert(err, IsNil)
         node.Set("outlook", "precipitous")
         _, err = node.Write()
         c.Assert(err, IsNil)
+        // We make sure the event has come into the filter before we tell it to discard any received.
         s.State.Sync()
         f.DiscardConfigEvent()
         assertNoChange()
 
         // Check that a filter's initial event works with DiscardConfigEvent
         // as expected.
         f, err = newFilter(s.State, s.unit.Name())
         c.Assert(err, IsNil)
         defer f.Stop()
+        s.State.Sync()
         f.DiscardConfigEvent()
-        s.State.Sync()
         assertNoChange()
 
         // Further changes are still collapsed as appropriate.
         node.Set("skill-level", 123)
         _, err = node.Write()
         c.Assert(err, IsNil)
         node.Set("outlook", "expressive")
         _, err = node.Write()
         c.Assert(err, IsNil)
         assertChange()
-        assertNoChange()
+}
+
+func (s *FilterSuite) TestCharmErrorEvents(c *C) {
+        f, err := newFilter(s.State, s.unit.Name())
+        c.Assert(err, IsNil)
+        defer f.Stop()
+
+        assertNoChange := func() {
+                s.State.StartSync()
+                select {
+                case <-f.ConfigEvents():
+                        c.Fatalf("unexpected config event")
+                case <-time.After(50 * time.Millisecond):
+                }
+        }
+        assertDead := func(f *filter) {
+                select {
+                case <-f.Dead():
+                case <-time.After(50 * time.Millisecond):
+                        c.Fatalf("filter did not die")
+                }
+        }
+
+        // Check setting an invalid charm URL does not send events.
+        err = f.SetCharm(charm.MustParseURL("cs:missing/one-1"))
+        c.Assert(err, Equals, tomb.ErrDying)
+        assertNoChange()
+        assertDead(f)
+
+        // Filter died after the error, so restart it.
+        f, err = newFilter(s.State, s.unit.Name())
+        c.Assert(err, IsNil)
+        defer f.Stop()
+
+        // Check with a nil charm URL, again no changes.
+        err = f.SetCharm(nil)
+        c.Assert(err, Equals, tomb.ErrDying)
+        assertNoChange()
+        assertDead(f)
 }
 
 func (s *FilterSuite) TestRelationsEvents(c *C) {
         f, err := newFilter(s.State, s.unit.Name())
         c.Assert(err, IsNil)
         defer f.Stop()
 
         assertNoChange := func() {
                 s.State.Sync()
                 select {
                 case ids := <-f.RelationsEvents():
                         c.Fatalf("unexpected relations event %#v", ids)
                 case <-time.After(50 * time.Millisecond):
                 }
         }
         assertNoChange()
 
         // Add a couple of relations; check the event.
         rel0 := s.addRelation(c)
         rel1 := s.addRelation(c)
         assertChange := func(expect []int) {
                 s.State.Sync()
                 select {
                 case got := <-f.RelationsEvents():
                         c.Assert(got, DeepEquals, expect)
                 case <-time.After(50 * time.Millisecond):
                         c.Fatalf("timed out")
                 }
+                assertNoChange()
         }
         assertChange([]int{0, 1})
-        assertNoChange()
-
-        // TODO: remove if safe

[fwereade, 2013/03/01 11:59:56]

We definitely need to test this behaviour, as we do with the config stuff that
is also hit by the SetCharm changes, but I think it's best to do it in an
independent test

[dimitern, 2013/03/01 16:42:37]

I'll do a separate test.

-        // 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)
         ru0, err := rel0.Unit(s.unit)
         c.Assert(err, IsNil)
         err = ru0.EnterScope(nil)
         c.Assert(err, IsNil)
         err = rel0.Destroy()
         c.Assert(err, IsNil)
         assertChange([]int{0, 2})
-        assertNoChange()
 
         // Remove a relation completely; check event.
         err = rel1.Destroy()
         c.Assert(err, IsNil)
         assertChange([]int{1})
-        assertNoChange()
 
         // Start a new filter, check initial event.
         f, err = newFilter(s.State, s.unit.Name())
         c.Assert(err, IsNil)
         defer f.Stop()
         assertChange([]int{0, 2})
-        assertNoChange()
+
+        // Check setting the charm URL generates all new relation events.
+        err = f.SetCharm(s.wpcharm.URL())
+        c.Assert(err, IsNil)
+        assertChange([]int{0, 2})
 }
 
 func (s *FilterSuite) addRelation(c *C) *state.Relation {
         if s.mysqlcharm == nil {
                 s.mysqlcharm = s.AddTestingCharm(c, "mysql")
         }
         rels, err := s.wordpress.Relations()
         c.Assert(err, IsNil)
         svcName := fmt.Sprintf("mysql%d", len(rels))
         _, err = s.State.AddService(svcName, s.mysqlcharm)
         c.Assert(err, IsNil)
         eps, err := s.State.InferEndpoints([]string{svcName, "wordpress"})
         c.Assert(err, IsNil)
         rel, err := s.State.AddRelation(eps...)
         c.Assert(err, IsNil)
         return rel
 }