Merge lp:~aramh/juju-core/121-state-machiner-watchers-machine-principals7 into lp:~juju/juju-core/trunk

[Aram Hăvărneanu]

state, machiner: new style principal units watcher

Convert the machine principal units watcher to the new, id only model
and refactor the machiner to make use of it.

https://codereview.appspot.com/6814108/

[Aram Hăvărneanu (aramh)]

Reviewers: mp+133504_code.launchpad.net,

Message:
Please take a look.

Description:
state, machiner: new style principal units watcher

Convert the machine principal units watcher to the new, id only model
and refactor the machiner to make use of it.

https://code.launchpad.net/~aramh/juju-core/121-state-machiner-watchers-machine-principals7/+merge/133504

Requires:
https://code.launchpad.net/~aramh/juju-core/120-firewaller-new-watcher-units7/+merge/133269

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/jujud/machine.go
   M state/machine_test.go
   M state/unit.go
   M state/watcher.go
   M worker/machiner/export_test.go
   M worker/machiner/machiner.go
   M worker/machiner/machiner_test.go

[Roger Peppe (rogpeppe)]

mostly looks good. all superficial comments, other than the logic and
tests in worker/machiner.

https://codereview.appspot.com/6814108/diff/2001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6814108/diff/2001/cmd/jujud/machine.go#newcode107
cmd/jujud/machine.go:107: t = machiner.NewMachiner(m, st,
&a.Conf.StateInfo, a.Conf.DataDir)
i think st should probably be the first argument here, so the workers
are consistent in that respect.

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go#newcode763
state/machine_test.go:763: var machinePrincipalsWatchTests = []struct {
can we please leave this in the same place, please, so i can easily see
what has remained the same?

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go#newcode771
state/machine_test.go:771: []string(nil),
just nil?

https://codereview.appspot.com/6814108/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/unit.go#newcode409
state/unit.go:409: func (u *Unit) IsAssignedTo(m *Machine) bool {
is this necessary? can't we just use AssignedMachineId instead?

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1259
state/watcher.go:1259: type MachinePrincipalUnitsWatcher struct {
please can we put code block moves in their own CL? i don't disagree on
principle (it is indeed better next to the MachineUnitsWatcher) but it
means we can't easily see the diffs.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1278
state/watcher.go:1278: machine: &Machine{m.st, m.doc}, // Copy so
it may be freely refreshed
i wonder if we should have a Clone or Copy method on Machine, Unit, etc.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1293
state/watcher.go:1293: func (w *MachinePrincipalUnitsWatcher)
updateMachine(pending []string) (new []string, err error) {
i found the name "pending" slightly confusing here. how about using
"changed" and "newChanged" throughout (replacing "pending" and "new")?
then the relationship between updateMachine, merge and the main loop
becomes clearer, i think.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1310
state/watcher.go:1310: doc := unitDoc{}
could we just use w.st.Unit(unit) here? then the logic below could just
use the normal Unit methods (e.g. IsPrincipal, AssignedMachineId).

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1344
state/watcher.go:1344: w.st.watcher.Watch(w.st.machines.Name,
w.machine.doc.Id, w.machine.doc.TxnRevno, machineCh)
could we just use MachineWatcher here?

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1346
state/watcher.go:1346: changes, err := w.updateMachine([]string(nil))
s/[]string(nil)/nil/

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/export_test.go
File worker/machiner/export_test.go (right):

https://codereview.appspot.c...

[lp:~aramh/juju-core/121-state-machiner-watchers-machine-principals7]

722. By Aram Hăvărneanu on 2012-11-09

all: merge lp:~aramh/juju-core/120-firewaller-new-watcher-units7

723. By Aram Hăvărneanu on 2012-11-09

firewaller: change NewMachiner signature so that state.State is the first argument

724. By Aram Hăvărneanu on 2012-11-09

machiner: use new style Unit.AssignedMachineId instead of Unit.IsAssignedTo(*state.Machine)

725. By Aram Hăvărneanu on 2012-11-09

state: more principal units watcher tests to original position

726. By Aram Hăvărneanu on 2012-11-09

machiner: make sure we don't redeploy units when they change lifecycle

727. By Aram Hăvărneanu on 2012-11-09

machiner: add lifecycle tests to machiner

[Aram Hăvărneanu (aramh)]

Please take a look.

https://codereview.appspot.com/6814108/diff/2001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6814108/diff/2001/cmd/jujud/machine.go#newcode107
cmd/jujud/machine.go:107: t = machiner.NewMachiner(m, st,
&a.Conf.StateInfo, a.Conf.DataDir)
> i think st should probably be the first argument here, so the workers
are
> consistent in that respect.

I agree. Done.

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go#newcode763
state/machine_test.go:763: var machinePrincipalsWatchTests = []struct {
On 2012/11/09 09:08:08, rog wrote:
> can we please leave this in the same place, please, so i can easily
see what has
> remained the same?

Done.

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go#newcode771
state/machine_test.go:771: []string(nil),
On 2012/11/09 09:08:08, rog wrote:
> just nil?

No, DeepEquals fails with just nil.

https://codereview.appspot.com/6814108/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/unit.go#newcode409
state/unit.go:409: func (u *Unit) IsAssignedTo(m *Machine) bool {
On 2012/11/09 09:08:08, rog wrote:
> is this necessary? can't we just use AssignedMachineId instead?

It was impossible to use the previous version of AssignedMachineId, but
I fixed that function instead to return usable errors and I deleted
this.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1259
state/watcher.go:1259: type MachinePrincipalUnitsWatcher struct {
> please can we put code block moves in their own CL? i don't
> disagree on principle (it is indeed better next to the
> MachineUnitsWatcher) but it means we can't easily see the diffs.

Sorry, no, it was intentional. This is 100% new code, it's not
a variation of the old code. It's a variation of the current
MachineUnitsWatcher code.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1278
state/watcher.go:1278: machine: &Machine{m.st, m.doc}, // Copy so
it may be freely refreshed
> i wonder if we should have a Clone or Copy method on Machine, Unit,
etc.

I wish we had, but niemeyer rules against it in one of the Lisbon
sprints, didn't he?

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1293
state/watcher.go:1293: func (w *MachinePrincipalUnitsWatcher)
updateMachine(pending []string) (new []string, err error) {
> i found the name "pending" slightly confusing here. how about using
"changed"
> and "newChanged" throughout (replacing "pending" and "new")? then the
> relationship between updateMachine, merge and the main loop becomes
clearer, i
> think.

I agree, but what about the MachineUnitsWatcher? This code is a proper
subset of that. If we change anything here, we should change it
everywhere.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1310
state/watcher.go:1310: doc := unitDoc{}
On 2012/11/09 09:08:08, rog wrote:
> could we j...

[Aram Hăvărneanu (aramh)]

https://codereview.appspot.com/6814108/diff/2001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6814108/diff/2001/cmd/jujud/machine.go#newcode107
cmd/jujud/machine.go:107: t = machiner.NewMachiner(m, st,
&a.Conf.StateInfo, a.Conf.DataDir)
> i think st should probably be the first argument here, so the workers
are
> consistent in that respect.

I agree. Done.

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go#newcode763
state/machine_test.go:763: var machinePrincipalsWatchTests = []struct {
On 2012/11/09 09:08:08, rog wrote:
> can we please leave this in the same place, please, so i can easily
see what has
> remained the same?

Done.

https://codereview.appspot.com/6814108/diff/2001/state/machine_test.go#newcode771
state/machine_test.go:771: []string(nil),
On 2012/11/09 09:08:08, rog wrote:
> just nil?

No, DeepEquals fails with just nil.

https://codereview.appspot.com/6814108/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/unit.go#newcode409
state/unit.go:409: func (u *Unit) IsAssignedTo(m *Machine) bool {
On 2012/11/09 09:08:08, rog wrote:
> is this necessary? can't we just use AssignedMachineId instead?

It was impossible to use the previous version of AssignedMachineId, but
I fixed that function instead to return usable errors and I deleted
this.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1259
state/watcher.go:1259: type MachinePrincipalUnitsWatcher struct {
> please can we put code block moves in their own CL? i don't
> disagree on principle (it is indeed better next to the
> MachineUnitsWatcher) but it means we can't easily see the diffs.

Sorry, no, it was intentional. This is 100% new code, it's not
a variation of the old code. It's a variation of the current
MachineUnitsWatcher code.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1278
state/watcher.go:1278: machine: &Machine{m.st, m.doc}, // Copy so
it may be freely refreshed
> i wonder if we should have a Clone or Copy method on Machine, Unit,
etc.

I wish we had, but niemeyer rules against it in one of the Lisbon
sprints, didn't he?

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1293
state/watcher.go:1293: func (w *MachinePrincipalUnitsWatcher)
updateMachine(pending []string) (new []string, err error) {
> i found the name "pending" slightly confusing here. how about using
"changed"
> and "newChanged" throughout (replacing "pending" and "new")? then the
> relationship between updateMachine, merge and the main loop becomes
clearer, i
> think.

I agree, but what about the MachineUnitsWatcher? This code is a proper
subset of that. If we change anything here, we should change it
everywhere.

https://codereview.appspot.com/6814108/diff/2001/state/watcher.go#newcode1310
state/watcher.go:1310: doc := unitDoc{}
On 2012/11/09 09:08:08, rog wrote:
> could we just use w.st.Unit(uni...

[Gustavo Niemeyer (niemeyer)]

That's great. LGTM assuming the following:

https://codereview.appspot.com/6814108/diff/8001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6814108/diff/8001/state/machine_test.go#newcode325
state/machine_test.go:325: "check initial empty event",
Thanks for the summaries.

https://codereview.appspot.com/6814108/diff/8001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6814108/diff/8001/state/unit.go#newcode417
state/unit.go:417: return &notAssignedError{fmt.Sprintf(format+" is not
assigned to a machine", args...)}
Pre-req has a missing test? It should have a test actually expecting
the NotAssignedError suggested there.

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go#newcode82
worker/machiner/machiner.go:82: assigned = machineId == m.machine.Id()
New logic was added here verifying that it is indeed assigned to the
machine we think it is. This is awesome, but deserves proper testing.

https://codereview.appspot.com/6814108/

[Gustavo Niemeyer (niemeyer)]

There are open points and the last review is from mid-last-week. Can you
please see what needs to be done to move this forward?

I'm marking this as WIP.

https://codereview.appspot.com/6814108/

[Aram Hăvărneanu (aramh)]

I'm on vacation for two weeks.

https://codereview.appspot.com/6814108/

[Gustavo Niemeyer (niemeyer)]

On Mon, Nov 19, 2012 at 10:34 AM, <email address hidden> wrote:
> I'm on vacation for two weeks.

Have fun there. That'd be a good note to send to canonical-juju or similar.

As far as this CL is concerned, I suppose it can wait for you to be back.

--
gustavo @ http://niemeyer.net

[William Reade (fwereade)]

I have serious reservations about the Machiner, but I don't think it's
any less good than it was before. FWIW the watcher essentially LGTM;
since Aram's on holiday, I'll be branching from here and trying to make
sure all other comments have been addressed, and will worry about the
machiner in a future CL.

https://codereview.appspot.com/6814108/diff/8001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6814108/diff/8001/state/watcher.go#newcode1316
state/watcher.go:1316: if err == mgo.ErrNotFound || doc.Principal == ""
&& (doc.MachineId == nil || *doc.MachineId != w.machine.doc.Id) {
I think the principal check is redundant; I see no way for a subordinate
to get in here (and if it does, it's a bug, not just a thing-to-ignore).

https://codereview.appspot.com/6814108/diff/8001/state/watcher.go#newcode1339
state/watcher.go:1339: for _, unit := range w.known {
s/_, //

(surely..?)

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go
File worker/machiner/machiner.go (left):

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go#oldcode52
worker/machiner/machiner.go:52: // and restart them if not. Also track
units so
Yeah, this is extremely important. If we're now tracking units to figure
out what to do, we should *surely* be doing so persistently.

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go#oldcode72
worker/machiner/machiner.go:72: if err := m.localContainer.Deploy(u,
m.stateInfo, m.tools); err != nil {
Isn't this actually a serious problem that should cause us to return an
error? If the agent is doing its job, we'll get retried anyway...

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6814108/diff/8001/worker/machiner/machiner.go#newcode92
worker/machiner/machiner.go:92: } else if !assigned && known {
I can accept handling reassignments that never happen in practice, but
we don't seem to be destroying dead units; surely that's necessary?

https://codereview.appspot.com/6814108/

Unmerged revisions

727. By Aram Hăvărneanu on 2012-11-09

machiner: add lifecycle tests to machiner

726. By Aram Hăvărneanu on 2012-11-09

machiner: make sure we don't redeploy units when they change lifecycle

725. By Aram Hăvărneanu on 2012-11-09

state: more principal units watcher tests to original position

724. By Aram Hăvărneanu on 2012-11-09

machiner: use new style Unit.AssignedMachineId instead of Unit.IsAssignedTo(*state.Machine)

723. By Aram Hăvărneanu on 2012-11-09

firewaller: change NewMachiner signature so that state.State is the first argument

722. By Aram Hăvărneanu on 2012-11-09

all: merge lp:~aramh/juju-core/120-firewaller-new-watcher-units7

721. By Aram Hăvărneanu on 2012-11-08

machiner: convert to new style principal units watcher

720. By Aram Hăvărneanu on 2012-11-08

machiner: change NewMachiner signature to take a *state.State

719. By Aram Hăvărneanu on 2012-11-08

state: convert MachinePrincipalUnitsWatcher to a new style watcher

718. By Aram Hăvărneanu on 2012-11-08

state: update machine principal units watcher table driven tests to new watcher model

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2012-10-12 07:40:16 +0000
+++ cmd/jujud/machine.go	2012-11-09 16:51:25 +0000
@@ -104,7 +104,7 @@
 		var t task
 		switch w {
 		case state.MachinerWorker:
-			t = machiner.NewMachiner(m, &a.Conf.StateInfo, a.Conf.DataDir)
+			t = machiner.NewMachiner(st, m, &a.Conf.StateInfo, a.Conf.DataDir)
 		case state.ProvisionerWorker:
 			t = provisioner.NewProvisioner(st)
 		case state.FirewallerWorker:

=== modified file 'state/machine_test.go'
--- state/machine_test.go	2012-11-02 12:25:01 +0000
+++ state/machine_test.go	2012-11-09 16:51:25 +0000
@@ -317,34 +317,39 @@
 }
 
 var machinePrincipalsWatchTests = []struct {
+	summary string
 	test    func(*C, *MachineSuite, *state.Service)
-	added   []string
-	removed []string
+	changes []string
 }{
 	{
-		test:  func(_ *C, _ *MachineSuite, _ *state.Service) {},
-		added: []string{},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
-			unit, err := service.AddUnit()
-			c.Assert(err, IsNil)
-			err = unit.AssignToMachine(s.machine)
-			c.Assert(err, IsNil)
-		},
-		added: []string{"mysql/0"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
-			unit, err := service.AddUnit()
-			c.Assert(err, IsNil)
-			err = unit.AssignToMachine(s.machine)
-			c.Assert(err, IsNil)
-		},
-		added: []string{"mysql/1"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		"check initial empty event",
+		func(_ *C, _ *MachineSuite, _ *state.Service) {},
+		[]string(nil),
+	}, {
+		"add a unit",
+		func(c *C, s *MachineSuite, service *state.Service) {
+			unit, err := service.AddUnit()
+			c.Assert(err, IsNil)
+			err = unit.AssignToMachine(s.machine)
+			c.Assert(err, IsNil)
+		},
+		[]string{"mysql/0"},
+	}, {
+		"ignore unrelated change",
+		func(c *C, s *MachineSuite, service *state.Service) {
+			unit, err := service.Unit("mysql/0")
+			c.Assert(err, IsNil)
+			err = unit.SetPrivateAddress("what.ever")
+			c.Assert(err, IsNil)
+			unit, err = service.AddUnit()
+			c.Assert(err, IsNil)
+			err = unit.AssignToMachine(s.machine)
+			c.Assert(err, IsNil)
+		},
+		[]string{"mysql/1"},
+	}, {
+		"add two units",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit2, err := service.AddUnit()
 			c.Assert(err, IsNil)
 			err = unit2.AssignToMachine(s.machine)
@@ -354,38 +359,45 @@
 			err = unit3.AssignToMachine(s.machine)
 			c.Assert(err, IsNil)
 		},
-		added: []string{"mysql/2", "mysql/3"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"mysql/2", "mysql/3"},
+	}, {
+		"set unit to dying",
+		func(c *C, s *MachineSuite, service *state.Service) {
+			unit3, err := service.Unit("mysql/3")
+			c.Assert(err, IsNil)
+			err = unit3.EnsureDying()
+			c.Assert(err, IsNil)
+		},
+		[]string{"mysql/3"},
+	}, {
+		"set unit to dead",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit3, err := service.Unit("mysql/3")
 			c.Assert(err, IsNil)
 			err = unit3.EnsureDead()
 			c.Assert(err, IsNil)
-			err = service.RemoveUnit(unit3)
-			c.Assert(err, IsNil)
 		},
-		removed: []string{"mysql/3"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"mysql/3"},
+	}, {
+		"set multiple units to dying, dead, and remove already reported dead units",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit0, err := service.Unit("mysql/0")
 			c.Assert(err, IsNil)
-			err = unit0.EnsureDead()
-			c.Assert(err, IsNil)
-			err = service.RemoveUnit(unit0)
+			err = unit0.EnsureDying()
 			c.Assert(err, IsNil)
 			unit2, err := service.Unit("mysql/2")
 			c.Assert(err, IsNil)
 			err = unit2.EnsureDead()
 			c.Assert(err, IsNil)
-			err = service.RemoveUnit(unit2)
+			unit3, err := service.Unit("mysql/3")
+			c.Assert(err, IsNil)
+			err = service.RemoveUnit(unit3)
 			c.Assert(err, IsNil)
 		},
-		removed: []string{"mysql/0", "mysql/2"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"mysql/0", "mysql/2"},
+	}, {
+		"add unit and remove unit at the same time",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit4, err := service.AddUnit()
 			c.Assert(err, IsNil)
 			err = unit4.AssignToMachine(s.machine)
@@ -397,11 +409,10 @@
 			err = service.RemoveUnit(unit1)
 			c.Assert(err, IsNil)
 		},
-		added:   []string{"mysql/4"},
-		removed: []string{"mysql/1"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"mysql/1", "mysql/4"},
+	}, {
+		"add and remove many units at once",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			units := [20]*state.Unit{}
 			var err error
 			for i := 0; i < len(units); i++ {
@@ -417,10 +428,10 @@
 				c.Assert(err, IsNil)
 			}
 		},
-		added: []string{"mysql/10", "mysql/11", "mysql/12", "mysql/13", "mysql/14", "mysql/5", "mysql/6", "mysql/7", "mysql/8", "mysql/9"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"mysql/10", "mysql/11", "mysql/12", "mysql/13", "mysql/14", "mysql/5", "mysql/6", "mysql/7", "mysql/8", "mysql/9"},
+	}, {
+		"report dead when first seen and also add a new unit",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit25, err := service.AddUnit()
 			c.Assert(err, IsNil)
 			err = unit25.AssignToMachine(s.machine)
@@ -432,11 +443,10 @@
 			err = service.RemoveUnit(unit9)
 			c.Assert(err, IsNil)
 		},
-		added:   []string{"mysql/25"},
-		removed: []string{"mysql/9"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"mysql/9", "mysql/25"},
+	}, {
+		"ignore units not assigned to this machine",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit26, err := service.AddUnit()
 			c.Assert(err, IsNil)
 			err = unit26.AssignToMachine(s.machine)
@@ -479,11 +489,10 @@
 			err = service.RemoveUnit(unit14)
 			c.Assert(err, IsNil)
 		},
-		added:   []string{"bacon/0", "bacon/1", "mysql/26", "mysql/27"},
-		removed: []string{"mysql/14"},
-	},
-	{
-		test: func(c *C, s *MachineSuite, service *state.Service) {
+		[]string{"bacon/0", "bacon/1", "mysql/26", "mysql/27", "mysql/14"},
+	}, {
+		"ignore subordinate units",
+		func(c *C, s *MachineSuite, service *state.Service) {
 			unit28, err := service.AddUnit()
 			c.Assert(err, IsNil)
 			err = unit28.AssignToMachine(s.machine)
@@ -502,7 +511,19 @@
 			_, err = logService.AddUnitSubordinateTo(unit29)
 			c.Assert(err, IsNil)
 		},
-		added: []string{"mysql/28", "mysql/29"},
+		[]string{"mysql/28", "mysql/29"},
+	}, {
+		"unassign from machine",
+		func(c *C, s *MachineSuite, service *state.Service) {
+			unit28, err := service.Unit("mysql/28")
+			c.Assert(err, IsNil)
+			err = unit28.UnassignFromMachine()
+			c.Assert(err, IsNil)
+			unit29, err := service.Unit("mysql/29")
+			c.Assert(err, IsNil)
+			err = unit29.UnassignFromMachine()
+		},
+		[]string{"mysql/28", "mysql/29"},
 	},
 }
 
@@ -515,21 +536,23 @@
 		c.Assert(unitWatcher.Stop(), IsNil)
 	}()
 	for i, test := range machinePrincipalsWatchTests {
-		c.Logf("test %d", i)
+		c.Logf("test %d: %s", i, test.summary)
 		test.test(c, s, service)
 		s.State.StartSync()
-		got := &state.MachinePrincipalUnitsChange{}
+		var got []string
 		for {
 			select {
 			case new, ok := <-unitWatcher.Changes():
 				c.Assert(ok, Equals, true)
-				addMachineUnitChanges(got, new)
-				if moreMachinePrincipalUnitsRequired(got, test.added, test.removed) {
+				got = append(got, new...)
+				if len(got) < len(test.changes) {
 					continue
 				}
-				assertSameMachinePrincipalUnits(c, got, test.added, test.removed)
+				sort.Strings(got)
+				sort.Strings(test.changes)
+				c.Assert(got, DeepEquals, test.changes)
 			case <-time.After(500 * time.Millisecond):
-				c.Fatalf("did not get change, want: added: %#v, removed: %#v, got: %#v", test.added, test.removed, got)
+				c.Fatalf("did not get change, want: %#v, got: %#v", test.changes, got)
 			}
 			break
 		}
@@ -541,37 +564,6 @@
 	}
 }
 
-func moreMachinePrincipalUnitsRequired(got *state.MachinePrincipalUnitsChange, added, removed []string) bool {
-	return len(got.Added)+len(got.Removed) < len(added)+len(removed)
-}
-
-func addMachineUnitChanges(changes *state.MachinePrincipalUnitsChange, more *state.MachinePrincipalUnitsChange) {
-	changes.Added = append(changes.Added, more.Added...)
-	changes.Removed = append(changes.Removed, more.Removed...)
-}
-
-func assertSameMachinePrincipalUnits(c *C, change *state.MachinePrincipalUnitsChange, added, removed []string) {
-	c.Assert(change, NotNil)
-	if len(added) == 0 {
-		added = nil
-	}
-	if len(removed) == 0 {
-		removed = nil
-	}
-	sort.Sort(unitSlice(change.Added))
-	sort.Sort(unitSlice(change.Removed))
-	var got []string
-	for _, g := range change.Added {
-		got = append(got, g.Name())
-	}
-	c.Assert(got, DeepEquals, added)
-	got = nil
-	for _, g := range change.Removed {
-		got = append(got, g.Name())
-	}
-	c.Assert(got, DeepEquals, removed)
-}
-
 var machineUnitsWatchTests = []struct {
 	summary string
 	test    func(*C, *MachineSuite, *state.Unit, *state.Charm)

=== modified file 'state/unit.go'
--- state/unit.go	2012-11-09 16:51:25 +0000
+++ state/unit.go	2012-11-09 16:51:25 +0000
@@ -414,7 +414,7 @@
 }
 
 func notAssigned(format string, args ...interface{}) error {
-	return &NotFoundError{fmt.Sprintf(format+" is not assigned to a machine", args...)}
+	return &notAssignedError{fmt.Sprintf(format+" is not assigned to a machine", args...)}
 }
 
 func IsNotAssigned(err error) bool {

=== modified file 'state/watcher.go'
--- state/watcher.go	2012-11-02 12:25:01 +0000
+++ state/watcher.go	2012-11-09 16:51:25 +0000
@@ -44,22 +44,6 @@
 	Left    []string
 }
 
-// MachinePrincipalUnitsWatcher observes the assignment and removal of units
-// to and from a machine.
-type MachinePrincipalUnitsWatcher struct {
-	commonWatcher
-	machine    *Machine
-	changeChan chan *MachinePrincipalUnitsChange
-	knownUnits map[string]*Unit
-}
-
-// MachinePrincipalUnitsChange contains information about units that have been
-// assigned to or removed from the machine.
-type MachinePrincipalUnitsChange struct {
-	Added   []*Unit
-	Removed []*Unit
-}
-
 func hasString(changes []string, name string) bool {
 	for _, v := range changes {
 		if v == name {
@@ -575,130 +559,6 @@
 	return nil
 }
 
-// WatchPrincipalUnits returns a watcher for observing units being
-// added to or removed from the machine.
-func (m *Machine) WatchPrincipalUnits() *MachinePrincipalUnitsWatcher {
-	return newMachinePrincipalUnitsWatcher(m)
-}
-
-// newMachinePrincipalUnitsWatcher creates and starts a watcher to watch information
-// about units being added to or deleted from the machine.
-func newMachinePrincipalUnitsWatcher(m *Machine) *MachinePrincipalUnitsWatcher {
-	w := &MachinePrincipalUnitsWatcher{
-		changeChan:    make(chan *MachinePrincipalUnitsChange),
-		machine:       m,
-		knownUnits:    make(map[string]*Unit),
-		commonWatcher: commonWatcher{st: m.st},
-	}
-	go func() {
-		defer w.tomb.Done()
-		defer close(w.changeChan)
-		w.tomb.Kill(w.loop())
-	}()
-	return w
-}
-
-// Changes returns a channel that will receive changes when units are
-// added or deleted. The Added field in the first event on the channel
-// holds the initial state as returned by Machine.Units.
-func (w *MachinePrincipalUnitsWatcher) Changes() <-chan *MachinePrincipalUnitsChange {
-	return w.changeChan
-}
-
-func (w *MachinePrincipalUnitsWatcher) mergeChange(changes *MachinePrincipalUnitsChange, ch watcher.Change) (err error) {
-	err = w.machine.Refresh()
-	if err != nil {
-		return err
-	}
-	units := make(map[string]*Unit)
-	for _, name := range w.machine.doc.Principals {
-		var unit *Unit
-		doc := &unitDoc{}
-		if _, ok := w.knownUnits[name]; !ok {
-			err = w.st.units.FindId(name).One(doc)
-			if err == mgo.ErrNotFound {
-				continue
-			}
-			if err != nil {
-				return err
-			}
-			unit = newUnit(w.st, doc)
-			changes.Added = append(changes.Added, unit)
-			w.knownUnits[name] = unit
-		}
-		units[name] = unit
-	}
-	for name, unit := range w.knownUnits {
-		if _, ok := units[name]; !ok {
-			changes.Removed = append(changes.Removed, unit)
-			delete(w.knownUnits, name)
-		}
-	}
-	return nil
-}
-
-func (changes *MachinePrincipalUnitsChange) isEmpty() bool {
-	return len(changes.Added)+len(changes.Removed) == 0
-}
-
-func (w *MachinePrincipalUnitsWatcher) getInitialEvent() (initial *MachinePrincipalUnitsChange, err error) {
-	changes := &MachinePrincipalUnitsChange{}
-	docs := []unitDoc{}
-	err = w.st.units.Find(D{{"_id", D{{"$in", w.machine.doc.Principals}}}}).All(&docs)
-	if err != nil {
-		return nil, err
-	}
-	for _, doc := range docs {
-		unit := newUnit(w.st, &doc)
-		w.knownUnits[doc.Name] = unit
-		changes.Added = append(changes.Added, unit)
-	}
-	return changes, nil
-}
-
-func (w *MachinePrincipalUnitsWatcher) loop() (err error) {
-	ch := make(chan watcher.Change)
-	w.st.watcher.Watch(w.st.machines.Name, w.machine.doc.Id, w.machine.doc.TxnRevno, ch)
-	defer w.st.watcher.Unwatch(w.st.machines.Name, w.machine.doc.Id, ch)
-	changes, err := w.getInitialEvent()
-	if err != nil {
-		return err
-	}
-	for {
-		for changes != nil {
-			select {
-			case <-w.st.watcher.Dead():
-				return watcher.MustErr(w.st.watcher)
-			case <-w.tomb.Dying():
-				return tomb.ErrDying
-			case c := <-ch:
-				err := w.mergeChange(changes, c)
-				if err != nil {
-					return err
-				}
-			case w.changeChan <- changes:
-				changes = nil
-			}
-		}
-		select {
-		case <-w.st.watcher.Dead():
-			return watcher.MustErr(w.st.watcher)
-		case <-w.tomb.Dying():
-			return tomb.ErrDying
-		case c := <-ch:
-			changes = &MachinePrincipalUnitsChange{}
-			err := w.mergeChange(changes, c)
-			if err != nil {
-				return err
-			}
-			if changes.isEmpty() {
-				changes = nil
-			}
-		}
-	}
-	return nil
-}
-
 func newRelationScopeWatcher(st *State, scope, ignore string) *RelationScopeWatcher {
 	w := &RelationScopeWatcher{
 		commonWatcher: commonWatcher{st: st},
@@ -1385,3 +1245,135 @@
 	}
 	panic("unreachable")
 }
+
+// MachinePrincipalUnitsWatcher notifies about assignments and lifecycle
+// changes for all principal units assigned to a machine.
+//
+// The first event emitted contains the unit names of all principal units
+// currently assigned to the machine, irrespective of their life state. From
+// then on, a new event is emitted whenever a principal unit is assigned
+// to or unassigned from the machine, or the lifecycle of a unit that is
+// currently assigned to the machine changes.
+//
+// After a unit is found to be Dead, no further event will include it.
+type MachinePrincipalUnitsWatcher struct {
+	commonWatcher
+	machine *Machine
+	out     chan []string
+	in      chan watcher.Change
+	known   map[string]Life
+}
+
+// WatchUnits returns a new MachinePrincipalUnitsWatcher for m.
+func (m *Machine) WatchPrincipalUnits() *MachinePrincipalUnitsWatcher {
+	return newMachinePrincipalUnitsWatcher(m)
+}
+
+func newMachinePrincipalUnitsWatcher(m *Machine) *MachinePrincipalUnitsWatcher {
+	w := &MachinePrincipalUnitsWatcher{
+		commonWatcher: commonWatcher{st: m.st},
+		out:           make(chan []string),
+		in:            make(chan watcher.Change),
+		known:         make(map[string]Life),
+		machine:       &Machine{m.st, m.doc}, // Copy so it may be freely refreshed
+	}
+	go func() {
+		defer w.tomb.Done()
+		defer close(w.out)
+		w.tomb.Kill(w.loop())
+	}()
+	return w
+}
+
+// Changes returns the event channel for w.
+func (w *MachinePrincipalUnitsWatcher) Changes() <-chan []string {
+	return w.out
+}
+
+func (w *MachinePrincipalUnitsWatcher) updateMachine(pending []string) (new []string, err error) {
+	err = w.machine.Refresh()
+	if err != nil {
+		return nil, err
+	}
+	for _, unit := range w.machine.doc.Principals {
+		if _, ok := w.known[unit]; !ok {
+			pending, err = w.merge(pending, unit)
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+	return pending, nil
+}
+
+func (w *MachinePrincipalUnitsWatcher) merge(pending []string, unit string) (new []string, err error) {
+	doc := unitDoc{}
+	err = w.st.units.FindId(unit).One(&doc)
+	if err != nil && err != mgo.ErrNotFound {
+		return nil, err
+	}
+	life, known := w.known[unit]
+	if err == mgo.ErrNotFound || doc.Principal == "" && (doc.MachineId == nil || *doc.MachineId != w.machine.doc.Id) {
+		// Unit was removed or unassigned from w.machine.
+		if known {
+			delete(w.known, unit)
+			w.st.watcher.Unwatch(w.st.units.Name, unit, w.in)
+			if life != Dead && !hasString(pending, unit) {
+				pending = append(pending, unit)
+			}
+		}
+		return pending, nil
+	}
+	if !known {
+		w.st.watcher.Watch(w.st.units.Name, unit, doc.TxnRevno, w.in)
+		pending = append(pending, unit)
+	} else if life != doc.Life && !hasString(pending, unit) {
+		pending = append(pending, unit)
+	}
+	w.known[unit] = doc.Life
+	return pending, nil
+}
+
+func (w *MachinePrincipalUnitsWatcher) loop() (err error) {
+	defer func() {
+		for _, unit := range w.known {
+			w.st.watcher.Unwatch(w.st.units.Name, unit, w.in)
+		}
+	}()
+	machineCh := make(chan watcher.Change)
+	w.st.watcher.Watch(w.st.machines.Name, w.machine.doc.Id, w.machine.doc.TxnRevno, machineCh)
+	defer w.st.watcher.Unwatch(w.st.machines.Name, w.machine.doc.Id, machineCh)
+	changes, err := w.updateMachine([]string(nil))
+	if err != nil {
+		return err
+	}
+	out := w.out
+	for {
+		select {
+		case <-w.st.watcher.Dead():
+			return watcher.MustErr(w.st.watcher)
+		case <-w.tomb.Dying():
+			return tomb.ErrDying
+		case <-machineCh:
+			changes, err = w.updateMachine(changes)
+			if err != nil {
+				return err
+			}
+			if len(changes) > 0 {
+				out = w.out
+			}
+		case c := <-w.in:
+			changes, err = w.merge(changes, c.Id.(string))
+			if err != nil {
+				return err
+			}
+			if len(changes) > 0 {
+				out = w.out
+			}
+		case out <- changes:
+			out = nil
+			changes = nil
+		}
+	}
+	panic("unreachable")
+}

=== modified file 'worker/machiner/export_test.go'
--- worker/machiner/export_test.go	2012-09-12 17:06:25 +0000
+++ worker/machiner/export_test.go	2012-11-09 16:51:25 +0000
@@ -5,6 +5,6 @@
 	"launchpad.net/juju-core/state"
 )
 
-func NewMachinerWithContainer(m *state.Machine, info *state.Info, dataDir string, cont container.Container) *Machiner {
-	return newMachiner(m, info, dataDir, cont)
+func NewMachinerWithContainer(st *state.State, m *state.Machine, info *state.Info, dataDir string, cont container.Container) *Machiner {
+	return newMachiner(st, m, info, dataDir, cont)
 }

=== modified file 'worker/machiner/machiner.go'
--- worker/machiner/machiner.go	2012-10-11 14:52:21 +0000
+++ worker/machiner/machiner.go	2012-11-09 16:51:25 +0000
@@ -13,6 +13,7 @@
 
 // Machiner represents a running machine agent.
 type Machiner struct {
+	st             *state.State
 	tomb           tomb.Tomb
 	machine        *state.Machine
 	localContainer container.Container
@@ -23,17 +24,18 @@
 // NewMachiner starts a machine agent running that
 // deploys agents in the given directory.
 // The Machiner dies when it encounters an error.
-func NewMachiner(machine *state.Machine, info *state.Info, dataDir string) *Machiner {
+func NewMachiner(st *state.State, machine *state.Machine, info *state.Info, dataDir string) *Machiner {
 	cont := &container.Simple{DataDir: dataDir}
-	return newMachiner(machine, info, dataDir, cont)
+	return newMachiner(st, machine, info, dataDir, cont)
 }
 
-func newMachiner(machine *state.Machine, info *state.Info, dataDir string, cont container.Container) *Machiner {
+func newMachiner(st *state.State, machine *state.Machine, info *state.Info, dataDir string, cont container.Container) *Machiner {
 	tools, err := environs.ReadTools(dataDir, version.Current)
 	if err != nil {
 		tools = &state.Tools{Binary: version.Current}
 	}
 	m := &Machiner{
+		st:             st,
 		machine:        machine,
 		stateInfo:      info,
 		tools:          tools,
@@ -47,6 +49,7 @@
 	defer m.tomb.Done()
 	w := m.machine.WatchPrincipalUnits()
 	defer watcher.Stop(w, &m.tomb)
+	units := map[string]*state.Unit{}
 
 	// TODO read initial units, check if they're running
 	// and restart them if not. Also track units so
@@ -60,19 +63,37 @@
 				m.tomb.Kill(watcher.MustErr(w))
 				return
 			}
-			for _, u := range change.Removed {
-				if u.IsPrincipal() {
-					if err := m.localContainer.Destroy(u); err != nil {
-						log.Printf("worker/machiner: cannot destroy unit %s: %v", u.Name(), err)
+			for _, uname := range change {
+				assigned := false
+				u, err := m.st.Unit(uname)
+				if err != nil {
+					if !state.IsNotFound(err) {
+						m.tomb.Kill(err)
+						return
+					}
+				} else {
+					machineId, err := u.AssignedMachineId()
+					if err != nil {
+						if !state.IsNotAssigned(err) {
+							m.tomb.Kill(err)
+							return
+						}
+					} else {
+						assigned = machineId == m.machine.Id()
 					}
 				}
-			}
-			for _, u := range change.Added {
-				if u.IsPrincipal() {
+				_, known := units[uname]
+				if assigned && !known {
 					if err := m.localContainer.Deploy(u, m.stateInfo, m.tools); err != nil {
 						// TODO put unit into a queue to retry the deploy.
 						log.Printf("worker/machiner: cannot deploy unit %s: %v", u.Name(), err)
 					}
+					units[uname] = u
+				} else if !assigned && known {
+					if err := m.localContainer.Destroy(units[uname]); err != nil {
+						log.Printf("worker/machiner: cannot destroy unit %s: %v", u.Name(), err)
+					}
+					delete(units, uname)
 				}
 			}
 		}

=== modified file 'worker/machiner/machiner_test.go'
--- worker/machiner/machiner_test.go	2012-09-28 07:21:43 +0000
+++ worker/machiner/machiner_test.go	2012-11-09 16:51:25 +0000
@@ -26,7 +26,7 @@
 	m, err := s.State.AddMachine(state.MachinerWorker)
 	c.Assert(err, IsNil)
 
-	p := machiner.NewMachiner(m, &state.Info{}, c.MkDir())
+	p := machiner.NewMachiner(s.State, m, &state.Info{}, c.MkDir())
 	c.Assert(p.Stop(), IsNil)
 }
 
@@ -38,6 +38,8 @@
 	c.Assert(err, IsNil)
 	d1, err := s.State.AddService("d1", dummyCharm)
 	c.Assert(err, IsNil)
+	d2, err := s.State.AddService("d2", dummyCharm)
+	c.Assert(err, IsNil)
 	sub0, err := s.State.AddService("sub0", loggingCharm)
 	c.Assert(err, IsNil)
 
@@ -50,6 +52,9 @@
 	ud1, err := d1.AddUnit()
 	c.Assert(err, IsNil)
 
+	ud2, err := d2.AddUnit()
+	c.Assert(err, IsNil)
+
 	m0, err := s.State.AddMachine(state.MachinerWorker)
 	c.Assert(err, IsNil)
 
@@ -67,7 +72,7 @@
 		expectedInfo:  stateInfo,
 		action:        make(chan string, 5),
 	}
-	machiner := machiner.NewMachinerWithContainer(m0, stateInfo, s.DataDir(), dcontainer)
+	machiner := machiner.NewMachinerWithContainer(s.State, m0, stateInfo, s.DataDir(), dcontainer)
 	defer func() {
 		err := machiner.Stop()
 		c.Assert(err, IsNil)
@@ -88,6 +93,18 @@
 			[]string{"+d1/0"},
 		}, {
 			func() {
+				err := ud2.AssignToMachine(m0)
+				c.Assert(err, IsNil)
+			},
+			[]string{"+d2/0"},
+		}, {
+			func() {
+				err := ud2.EnsureDying()
+				c.Assert(err, IsNil)
+			},
+			nil,
+		}, {
+			func() {
 				err := ud0.UnassignFromMachine()
 				c.Assert(err, IsNil)
 			},