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

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

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

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

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

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/

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/

I'm on vacation for two weeks.

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

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

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/