state, machiner: new style principal units watcher

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)

[aram, 2012-11-08 15:59:46]

Please take a look.

[rog, 2012-11-09 09:08:08]

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#newcod...
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#newcod...
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.com/6814108/diff/2001/worker/machiner/export_test....
worker/machiner/export_test.go:8: func NewMachinerWithContainer(m
*state.Machine, st *state.State, info *state.Info, dataDir string, cont
container.Container) *Machiner {
make st the first arg.

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#...
worker/machiner/machiner.go:27: func NewMachiner(machine *state.Machine, st
*state.State, info *state.Info, dataDir string) *Machiner {
make st the first arg.

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:32: func newMachiner(machine *state.Machine, st
*state.State, info *state.Info, dataDir string, cont container.Container)
*Machiner {
ditto.

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:67: var assigned bool
assigned := false ?

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:78: if err := m.localContainer.Deploy(u,
m.stateInfo, m.tools); err != nil {
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.

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

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner_tes...
worker/machiner/machiner_test.go:76: tests := []struct {
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.

[aram, 2012-11-09 16:50:06]

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#newcod...
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#newcod...
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#...
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#...
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#...
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#...
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_tes...
File worker/machiner/machiner_test.go (right):

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner_tes...
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.

[aram, 2012-11-09 16:50:07]

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#newcod...
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#newcod...
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#...
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#...
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#...
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#...
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_tes...
File worker/machiner/machiner_test.go (right):

https://codereview.appspot.com/6814108/diff/2001/worker/machiner/machiner_tes...
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.

[niemeyer, 2012-11-14 23:34:08]

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#newcod...
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#...
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.

[niemeyer, 2012-11-19 12:27:36]

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.

[aram, 2012-11-19 12:34:00]

I'm on vacation for two weeks.

[gustavo.niemeyer, 2012-11-19 13:52:49]

On Mon, Nov 19, 2012 at 10:34 AM,  <aram@mgk.ro> 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

[fwereade, 2012-11-20 06:55:10]

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#...
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#...
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#...
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?