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 just use w.st.Unit(unit) here? then the logic below could just use the > normal Unit methods (e.g. IsPrincipal, AssignedMachineId). I don't know, but see above. 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) On 2012/11/09 09:08:08, rog wrote: > could we just use MachineWatcher here? See above. Also, I don't want to change anything in this watcher, and the machine units watcher. The implementation will change completely in a few days once we have containers inside state. 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/ No, clients really expect []string(nil). Perhaps we should fix teh clients but that's outside of the scope of this CL. https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go File worker/machiner/machiner.go (right): https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#newcode27 worker/machiner/machiner.go:27: func NewMachiner(machine *state.Machine, st *state.State, info *state.Info, dataDir string) *Machiner { On 2012/11/09 09:08:08, rog wrote: > make st the first arg. Done. https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#newcode32 worker/machiner/machiner.go:32: func newMachiner(machine *state.Machine, st *state.State, info *state.Info, dataDir string, cont container.Container) *Machiner { On 2012/11/09 09:08:08, rog wrote: > ditto. Done. https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#newcode67 worker/machiner/machiner.go:67: var assigned bool On 2012/11/09 09:08:08, rog wrote: > assigned := false ? Done. https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#newcode78 worker/machiner/machiner.go:78: if err := m.localContainer.Deploy(u, m.stateInfo, m.tools); err != nil { On 2012/11/09 09:08:08, rog wrote: > i'm not sure this is quite right. if the unit's life state changes from Alive to > Dying, the unit will still appear to be assigned to the machine, and we're going > to call Deploy again, even though the unit is already in the units map. > i think we should check that the unit is not in the units map before calling > Deploy. Done. https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner_test.go File worker/machiner/machiner_test.go (right): https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner_test.go#newcode76 worker/machiner/machiner_test.go:76: tests := []struct { On 2012/11/09 09:08:08, rog wrote: > we need some tests that check what happens when lifecycle changes happen here. i > suggest adding the tests before changing the code to verify that current > behaviour is actually broken. Done. https://codereview.appspot.com/6814108/