state: update MachinePrincipalUnitsWatcher

state: update MachinePrincipalUnitsWatcher

Add lifecycle support to the MachinePrincipalUnitsWatcher making it a
new style watcher that only returns ids. The firewaller and the machiner
have been modified to use the new watcher model.

Additionally fix a bug in the machine units watcher where it did not
return unassigned machines.

https://code.launchpad.net/~aramh/juju-core/106-state-watchers-machine-principals5/+merge/128740

(do not edit description out of merge proposal)

[aram, 2012-10-09 15:30:56]

Please take a look.

[aram, 2012-10-09 15:32:55]

Please take a look.

[TheMue, 2012-10-09 15:58:06]

LGTM

[niemeyer, 2012-10-09 22:13:56]

LGTM with the same comment. You definitely nailed it down.

https://codereview.appspot.com/6625069/diff/3001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6625069/diff/3001/state/watcher.go#newcode1350
state/watcher.go:1350: func (w *MachinePrincipalUnitsWatcher) merge(pending
[]string, unit string) (new []string, err error) {
As per the other CLs, please check if unit is in pending here, so we don't
repeatedly query the database when we've already seen that unit and were unable
to report it.

https://codereview.appspot.com/6625069/diff/3001/state/watcher.go#newcode1362
state/watcher.go:1362: return append(pending, unit), nil
Which means we don't have to do that here (the check is missing, and can remain
missing).

https://codereview.appspot.com/6625069/diff/3001/state/watcher.go#newcode1373
state/watcher.go:1373: for _, v := range pending {
And don't need the loop here.

[aram, 2012-10-10 15:53:20]

Please take a look.

[aram, 2012-10-10 15:54:07]

Addressed review points. Not submitting because I'm waiting for changes in
firewaller and machiner, so I don't break the build.

[aram, 2012-10-11 15:10:54]

Please take a look.

[niemeyer, 2012-10-12 03:51:36]

https://codereview.appspot.com/6625069/diff/10001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6625069/diff/10001/state/unit.go#newcode407
state/unit.go:407: // IsAssigned return true if the unit, which must be a
principal, is assigned
// IsAssigned returns true if u is assigned to a machine.
// It panics if u is not a principal unit.

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode647
state/watcher.go:647: if doc.MachineId == nil || doc.Life != life {
Looks pretty sane, good catch. How did you arrive at the need? Were any tests
broken with this?

It needs to be properly documented.

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1316
state/watcher.go:1316: var needToAppend bool
reportChange := doc.Life != life

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1317
state/watcher.go:1317: if doc.MachineId == nil && doc.Principal == "" {
reportChange = true

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1322
state/watcher.go:1322: if _, ok := w.known[subunit]; ok {
&& !hasString

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1327
state/watcher.go:1327: if doc.Life != life {
d

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1334
state/watcher.go:1334: w.known[unit] = doc.Life
Adding back?

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
worker/firewaller/firewaller.go:77: for _, uname := range change.unames {
Please have a look at the equivalent refactoring for the machinesWatcher. We
need something like an equivalent unitLifeChanged, etc, so we clean up this
select a bit more rather than growing it up further.

https://codereview.appspot.com/6625069/diff/10001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/machiner/machiner.go...
worker/machiner/machiner.go:65: if err != nil {
If the unit isn't found, it should be destroyed.

https://codereview.appspot.com/6625069/diff/10001/worker/machiner/machiner.go...
worker/machiner/machiner.go:70: switch assigned {
if u.IsAssigned() {

(!!!)

[aram, 2012-10-12 15:42:51]

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode647
state/watcher.go:647: if doc.MachineId == nil || doc.Life != life {
> Looks pretty sane, good catch. How did you arrive at the need? Were
> any tests broken with this?

We need to notify about unassigned machines. Previously this was
not always done. Tests did not check for unassignment and this
change broke no existing tests. I have added tests that explicitely
test unassigment. I have found this because the machiner was not
notified about unassigned units.

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1317
state/watcher.go:1317: if doc.MachineId == nil && doc.Principal == "" {
On 2012/10/12 03:51:36, niemeyer wrote:
> reportChange = true

Done.

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1322
state/watcher.go:1322: if _, ok := w.known[subunit]; ok {
On 2012/10/12 03:51:36, niemeyer wrote:
> && !hasString

Done.

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1327
state/watcher.go:1327: if doc.Life != life {
On 2012/10/12 03:51:36, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6625069/diff/10001/state/watcher.go#newcode1334
state/watcher.go:1334: w.known[unit] = doc.Life
On 2012/10/12 03:51:36, niemeyer wrote:
> Adding back?

Done.

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
worker/firewaller/firewaller.go:77: for _, uname := range change.unames {
I did a much cleaner conversion, PTAL.

https://codereview.appspot.com/6625069/diff/10001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/machiner/machiner.go...
worker/machiner/machiner.go:70: switch assigned {
> if u.IsAssigned() {
> 
> (!!!)

Sorry, initially this had three branches inside the switch. It lingered. Also, I
hadn't written *Unit.IsAssigned yet().

[aram, 2012-10-12 15:45:14]

Please take a look.

[niemeyer, 2012-10-15 14:11:44]

https://codereview.appspot.com/6625069/diff/10001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6625069/diff/10001/state/unit.go#newcode407
state/unit.go:407: // IsAssigned return true if the unit, which must be a
principal, is assigned
On 2012/10/12 03:51:36, niemeyer wrote:
> // IsAssigned returns true if u is assigned to a machine.
> // It panics if u is not a principal unit.

Sorry, I've mispoken our own convention. We've been using 'whether' as per the
standard convention in Go:

// IsAssigned returns whether u is assigned to a machine.
// ...

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
worker/firewaller/firewaller.go:77: for _, uname := range change.unames {
On 2012/10/12 15:42:51, aram wrote:
> I did a much cleaner conversion, PTAL.

The conversion is smaller, but it's not cleaner. I'd still prefer to see the
select statement cleaned up in an equivalent way to what the machinesWatcher
change did.

Also, we cannot do the added/removed trick within the watcher, because as a
convention we don't touch the outer data within he watcher to prevent the race
conditions that we'd incur.

https://codereview.appspot.com/6625069/diff/11010/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6625069/diff/11010/state/watcher.go#newcode1318
state/watcher.go:1318: if doc.MachineId == nil && doc.Principal == "" {
Hmm.. so isn't this a condition that may happen irrespective of whether we know
it or not, and irrespective of whether it is in pending or not? Also, what if
MachineId is not nil, but is not the intended machine either?

It's feeling a bit like this should have been done in a separate CL, because now
we're stuck with something that is unrelated to the original goal of the change,
and isn't even exercised by the current system in practice.

https://codereview.appspot.com/6625069/diff/11010/worker/firewaller/firewalle...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6625069/diff/11010/worker/firewaller/firewalle...
worker/firewaller/firewaller.go:372: removed = append(removed,
md.unitds[name].unit)
Cannot touch md.unitds here, or we have a race condition. The previous change
was in the right direction.

https://codereview.appspot.com/6625069/diff/11010/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6625069/diff/11010/worker/machiner/machiner.go...
worker/machiner/machiner.go:70: }
So if there is any other error...? If it's assigned, but to the wrong machine?

I suggest rolling back all of the changes that handle assignment from this CL.
It's derailing the change set and confusing the problem. This is about fixing
the watcher with lifecycle support.

[aram, 2012-10-15 14:54:29]

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
worker/firewaller/firewaller.go:77: for _, uname := range change.unames {
> The conversion is smaller, but it's not cleaner. I'd still prefer
> to see the select statement cleaned up in an equivalent way to
> what the machinesWatcher change did.

I'm sorry but I am missing the bigger picture here. I can't do it
"right" if I don't understand the larger scope. I can't emulate
a change if I don't understand its purpose, even if I understand
its implications.

https://codereview.appspot.com/6625069/diff/11010/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6625069/diff/11010/state/watcher.go#newcode1318
state/watcher.go:1318: if doc.MachineId == nil && doc.Principal == "" {
> Hmm.. so isn't this a condition that may happen irrespective of
> whether we know it or not, and irrespective of whether it is in
> pending or not? Also, what if MachineId is not nil, but is not
> the intended machine either?

Whether the unit is already pending or not does not matter,
but this can happen only if we know about the unit, otherwise we
would not watch it. We need to check here against the expected
machine id, not nil, though.

> It's feeling a bit like this should have been done in a separate
> CL, because now we're stuck with something that is unrelated to
> the original goal of the change, and isn't even exercised by the
> current system in practice.

I found this bug while I was doing the conversion of the
firewaller, so I fixed it. This is used by the firewaller.

https://codereview.appspot.com/6625069/diff/11010/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6625069/diff/11010/worker/machiner/machiner.go...
worker/machiner/machiner.go:70: }
> So if there is any other error...?

What do you mean? If there's an unexpected error we kill the
machiner, and if it's an expected error we handle it and go on.

> If it's assigned, but to the wrong machine?

This can't happen by design. The *Machine*PrincipalUnitsWatcher
returns only units from that particular machine.

> I suggest rolling back all of the changes that handle assignment
> from this CL.  It's derailing the change set and confusing the
> problem. This is about fixing the watcher with lifecycle support.

I believe the watcher is "fixed". I suggested someone else used
my branch as a prerequisite and do the firewaller and machiner
changes, but you asked me to do it, and here is my attempt.
Now you want me to stop working on this?

[niemeyer, 2012-10-15 15:08:14]

I feel some negativity in your comments. Talking generally helps, but I cannot
talk to you if you're not online.

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6625069/diff/10001/worker/firewaller/firewalle...
worker/firewaller/firewaller.go:77: for _, uname := range change.unames {
On 2012/10/15 14:54:29, aram wrote:
> > The conversion is smaller, but it's not cleaner. I'd still prefer
> > to see the select statement cleaned up in an equivalent way to
> > what the machinesWatcher change did.
> 
> I'm sorry but I am missing the bigger picture here. I can't do it
> "right" if I don't understand the larger scope. I can't emulate
> a change if I don't understand its purpose, even if I understand
> its implications.

Understood, and I've pinged you on IRC to talk about it one hour ago, without
responses. Let me know when you want to chat.

https://codereview.appspot.com/6625069/diff/11010/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6625069/diff/11010/state/watcher.go#newcode1318
state/watcher.go:1318: if doc.MachineId == nil && doc.Principal == "" {
On 2012/10/15 14:54:29, aram wrote:
> > Hmm.. so isn't this a condition that may happen irrespective of
> > whether we know it or not, and irrespective of whether it is in
> > pending or not? Also, what if MachineId is not nil, but is not
> > the intended machine either?
> 
> Whether the unit is already pending or not does not matter,
> but this can happen only if we know about the unit, otherwise we
> would not watch it. We need to check here against the expected
> machine id, not nil, though.

I don't understand why you say it doesn't matter. The unit you've just loaded
may be in any state. Why does it matter when we've known about it 1ms ago, and
it doesn't matter if we're knowing about it now? Why does it matter if we've put
it in the pending list 1ms ago, and it doesn't if we haven't? Looks ad-hoc to
me.

> > It's feeling a bit like this should have been done in a separate
> > CL, because now we're stuck with something that is unrelated to
> > the original goal of the change, and isn't even exercised by the
> > current system in practice.
> 
> I found this bug while I was doing the conversion of the
> firewaller, so I fixed it. This is used by the firewaller.

There are many many things we can work on. It's not a good idea to fix them all
at once in the same branch, because we get into the sort of discussion that
we're getting into right here, and don't get to the end of the goal that was
simply refactoring it for lifecycle support, rather than for handling
assignments.

https://codereview.appspot.com/6625069/diff/11010/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6625069/diff/11010/worker/machiner/machiner.go...
worker/machiner/machiner.go:70: }
On 2012/10/15 14:54:29, aram wrote:
> > So if there is any other error...?
> 
> What do you mean? If there's an unexpected error we kill the
> machiner, and if it's an expected error we handle it and go on.
> 
> > If it's assigned, but to the wrong machine?
> 
> This can't happen by design. The *Machine*PrincipalUnitsWatcher
> returns only units from that particular machine.

Until it's unassigned, and unassigned can mean assigned to a different machine
as well. Feels like we don't even understand the problem properly yet.

> > I suggest rolling back all of the changes that handle assignment
> > from this CL.  It's derailing the change set and confusing the
> > problem. This is about fixing the watcher with lifecycle support.
> 
> I believe the watcher is "fixed". I suggested someone else used
> my branch as a prerequisite and do the firewaller and machiner
> changes, but you asked me to do it, and here is my attempt.
> Now you want me to stop working on this?

No, I'm suggesting not working on *assignments* in this branch. It wasn't
handled before, and it doesn't have to be handled now. You're digging a rabbit
hole for yourself.

Also, the "I suggested someone else fix it for me" attitude doesn't work. The
problem is not fixed while the branch is not merged. Fixing a watcher and having
all the code that depends on it broken is barely interesting.

[aram, 2012-10-17 16:13:16]

I've split it into multiple branches.