firewaller: added tests for removals

firewaller: added tests for removals

Added two tests for the removing of units and services. It can
happen that the removing has been done before the watchers in
the firewaller are stopped. So here it must be checked if the
watcher fails due to a not-found-error.

https://code.launchpad.net/~themue/juju-core/go-firewaller-remove-tests-unit-service/+merge/126651

(do not edit description out of merge proposal)

[TheMue, 2012-09-27 10:03:06]

Please take a look.

[rog, 2012-09-27 12:17:44]

LGTM with a few thoughts below.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:92: machined := unitd.machined
rather than defining these as shortcuts, i think it might be easier to read if
you renamed unitd to u and used u.serviced and u.machined. as it is, i was
wondering if there was some other significance to saving the values.

if you don't like that idea, at least move the above two lines below the Stop
call.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:96: // Clean up after stopping.
i wonder if this might look better in a separate method. FireWaller.deleteUnit()
?

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:389: log.Debugf("firewaller: unit %q has been
removed", ud.unit)
i think this is routine enough that it doesn't justify a debug message - it'll
happen all the time. i'd suggest something like this:

// Ignore errors caused by the unit's removal - the
// central loop will be notified of this independently.
if !state.IsNotFound(err) {
    ud.fw.tomb.Kill(err)
}

and be done with it.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:472: log.Debugf("firewaller: service %q has been
removed", sd.service)
ditto

[niemeyer, 2012-09-27 16:04:16]

A few comments follow.

It'd be really nice to have a test that broke due to the bug you fixed in
serviced's unitd map keys.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:92: machined := unitd.machined
On 2012/09/27 12:17:44, rog wrote:
> rather than defining these as shortcuts, i think it might be easier to read if
> you renamed unitd to u and used u.serviced and u.machined. as it is, i was
> wondering if there was some other significance to saving the values.
> 
> if you don't like that idea, at least move the above two lines below the Stop
> call.

-1 on renaming. We have both unit and unitd in context. It's nice to be able to
tell what is what, and the current code looks pretty clear.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:96: // Clean up after stopping.
On 2012/09/27 12:17:44, rog wrote:
> i wonder if this might look better in a separate method.
FireWaller.deleteUnit()
> ?

+1 on a helper. This has become a pretty fat function. deleteUnit doesn't sound
right, though. forgetUnit, maybe..

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:130: unitd.serviced.unitds[unit.Name()] = unitd
Wow.. do we have a test covering the functionality here? This isn't just about
removing.. this was plain wrong on normal conditions too.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:389: log.Debugf("firewaller: unit %q has been
removed", ud.unit)
On 2012/09/27 12:17:44, rog wrote:
> i think this is routine enough that it doesn't justify a debug message - it'll
> happen all the time. i'd suggest something like this:
> 
> // Ignore errors caused by the unit's removal - the
> // central loop will be notified of this independently.
> if !state.IsNotFound(err) {
>     ud.fw.tomb.Kill(err)
> }
> 
> and be done with it.

+1, and this is also pretty unreliable. I'll try to fix the UnitWatcher today so
that it returns unit names rather than the unit itself, so that we handle the
situation here rather than trusting on non-defined and non-documented behavior.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:389: log.Debugf("firewaller: unit %q has been
removed", ud.unit)
On 2012/09/27 12:17:44, rog wrote:
> i think this is routine enough that it doesn't justify a debug message - it'll
> happen all the time. i'd suggest something like this:
> 
> // Ignore errors caused by the unit's removal - the
> // central loop will be notified of this independently.
> if !state.IsNotFound(err) {
>     ud.fw.tomb.Kill(err)
> }
> 
> and be done with it.

+1!

This is also pretty unreliable. I'll try to fix the UnitWatcher today so that it
returns unit names rather than the unit itself, so that we handle the situation
here rather than trusting on non-defined and non-documented behavior.

For the moment, though, this would be fine.

[TheMue, 2012-09-27 17:14:18]

Please take a look.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:96: // Clean up after stopping.
On 2012/09/27 16:04:16, niemeyer wrote:
> On 2012/09/27 12:17:44, rog wrote:
> > i wonder if this might look better in a separate method.
> FireWaller.deleteUnit()
> > ?
> 
> +1 on a helper. This has become a pretty fat function. deleteUnit doesn't
sound
> right, though. forgetUnit, maybe..

Done.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:130: unitd.serviced.unitds[unit.Name()] = unitd
On 2012/09/27 16:04:16, niemeyer wrote:
> Wow.. do we have a test covering the functionality here? This isn't just about
> removing.. this was plain wrong on normal conditions too.

Just talked about it.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:389: log.Debugf("firewaller: unit %q has been
removed", ud.unit)
On 2012/09/27 16:04:16, niemeyer wrote:
> On 2012/09/27 12:17:44, rog wrote:
> > i think this is routine enough that it doesn't justify a debug message -
it'll
> > happen all the time. i'd suggest something like this:
> > 
> > // Ignore errors caused by the unit's removal - the
> > // central loop will be notified of this independently.
> > if !state.IsNotFound(err) {
> >     ud.fw.tomb.Kill(err)
> > }
> > 
> > and be done with it.
> 
> +1!
> 
> This is also pretty unreliable. I'll try to fix the UnitWatcher today so that
it
> returns unit names rather than the unit itself, so that we handle the
situation
> here rather than trusting on non-defined and non-documented behavior.
> 
> For the moment, though, this would be fine.

Done.

https://codereview.appspot.com/6567060/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:472: log.Debugf("firewaller: service %q has been
removed", sd.service)
On 2012/09/27 12:17:44, rog wrote:
> ditto

Done.

[niemeyer, 2012-09-27 17:32:32]

LGTM, thanks.

[TheMue, 2012-09-28 08:06:53]

*** Submitted:

firewaller: added tests for removals

Added two tests for the removing of units and services. It can
happen that the removing has been done before the watchers in
the firewaller are stopped. So here it must be checked if the
watcher fails due to a not-found-error.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6567060