state: machine units watcher reports unassignment

state: machine units watcher reports unassignment



https://code.launchpad.net/~aramh/juju-core/107-state-watchers-units-unassignments6/+merge/130118

(do not edit description out of merge proposal)

[aram, 2012-10-17 16:14:19]

Please take a look.

[niemeyer, 2012-10-19 00:10:25]

Seems very complex, poorly tested, and quite problematic.

My recommendation is still the same: we do not depend on this logic. Let's
convert the watcher, and focus on getting the logic we do need right now
correctly. It'll be a while before we depend on unassignment and reassignment.

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

https://codereview.appspot.com/6720049/diff/2001/state/machine_test.go#newcod...
state/machine_test.go:824: err := principal.UnassignFromMachine()
This seems extremely light compared to the amount of non-trivial logic on that
implementation.

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

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1308
state/watcher.go:1308: isprincipal := doc.Principal == ""
If it wasn't found, that's not right.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1309
state/watcher.go:1309: w.known[unit] = doc.Life
If it wasn't found, that's certainly not right either.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1311
state/watcher.go:1311: if _, ok := w.known[doc.Principal]; !ok && !isprincipal {
Why are we looking for doc.Principal when it can be ""?

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1314
state/watcher.go:1314: w.st.watcher.Watch(w.st.units.Name, unit, doc.TxnRevno,
w.in)
If the document is !known && !found, we'll get here.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1315
state/watcher.go:1315: pending = append(pending, unit)
So if the document was previously unknown, we don't care about the fact it was
just unassigned, or assigned to the wrong machine after we decided to merge it?

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1319
state/watcher.go:1319: unassigned = (doc.MachineId != nil) && *doc.MachineId !=
w.machine.doc.Id || doc.MachineId == nil
doc.MachineId == nil || *doc.MachineId != w.machine.doc.Id

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1326
state/watcher.go:1326: if doc.Life != Dead && !hasString(pending, unit) {
By now I'm finding pretty hard to recognize all the ins and outs of the
implementation. I can't tell with certainty if this is free from the kind of
issue I described above.