Implement container lifecycle watcher

Implement container lifecycle watcher

Provide the ability to watch for container lifecycle events for a machine.

https://code.launchpad.net/~wallyworld/juju-core/container-watcher/+merge/169100

Requires: https://code.launchpad.net/~wallyworld/juju-core/container-status/+merge/169096

(do not edit description out of merge proposal)

[wallyworld, 2013-06-13 03:58:48]

Please take a look.

[fwereade, 2013-06-13 15:04:01]

LGTM -- this will clearly work ok -- but I'd be pleased if you were to take the
path I suggest below.

https://codereview.appspot.com/10250044/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10250044/diff/1/state/watcher.go#newcode1041
state/watcher.go:1041: func (m *Machine) WatchContainers(ctype ContainerType)
*LifecycleWatcher {
Hmm. I think the small number of containers on a given machine, relative to the
total likely number of machines, will render a whole-collection watch unsuitable
-- we'll be filtering a *lot* of uninteresting events.

Take a look at UnitsWatcher, near the bottom of the file; it's used for
WatchPrincipalUnits and WatchSubordinateUnits, but isn't actually very
unit-specific at all. I think it could reasonably be parameterized by
collection, and become a useful small-interesting-set variant of
LifecycleWatcher.

Sound sane? If not I'm still happy to land it as is, but we should chat
afterwards about a followup.

[thumper, 2013-06-14 02:07:47]

LGTM - but you should merge trunk first, and I'd love to see your new
implementation of the watcher.

https://codereview.appspot.com/10250044/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10250044/diff/1/state/state.go#newcode74
state/state.go:74: validMachine =
regexp.MustCompile("^(0|[1-9][0-9]*)(/[a-z]+/(0|[1-9][0-9]*))*$")
Why is this showing up here?  I landed this in trunk.

https://codereview.appspot.com/10250044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10250044/diff/1/state/state_test.go#newcode1045
state/state_test.go:1045: assertMachine("0/lxc/1/embedded/2", true)
Ahh... you merged my branch, but not trunk.

https://codereview.appspot.com/10250044/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10250044/diff/1/state/watcher.go#newcode1041
state/watcher.go:1041: func (m *Machine) WatchContainers(ctype ContainerType)
*LifecycleWatcher {
On 2013/06/13 15:04:01, fwereade wrote:
> Hmm. I think the small number of containers on a given machine, relative to
the
> total likely number of machines, will render a whole-collection watch
unsuitable
> -- we'll be filtering a *lot* of uninteresting events.
> 
> Take a look at UnitsWatcher, near the bottom of the file; it's used for
> WatchPrincipalUnits and WatchSubordinateUnits, but isn't actually very
> unit-specific at all. I think it could reasonably be parameterized by
> collection, and become a useful small-interesting-set variant of
> LifecycleWatcher.
> 
> Sound sane? If not I'm still happy to land it as is, but we should chat
> afterwards about a followup.

+1

[wallyworld, 2013-06-17 00:03:14]

Please take a look.

https://codereview.appspot.com/10250044/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10250044/diff/1/state/state.go#newcode74
state/state.go:74: validMachine =
regexp.MustCompile("^(0|[1-9][0-9]*)(/[a-z]+/(0|[1-9][0-9]*))*$")
On 2013/06/14 02:07:48, thumper wrote:
> Why is this showing up here?  I landed this in trunk.

Not sure. My branch was originally against the old trunk. Perhaps it got
confused. I need to retarget the mp to the new trunk.

https://codereview.appspot.com/10250044/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10250044/diff/1/state/watcher.go#newcode1041
state/watcher.go:1041: func (m *Machine) WatchContainers(ctype ContainerType)
*LifecycleWatcher {
On 2013/06/13 15:04:01, fwereade wrote:
> Hmm. I think the small number of containers on a given machine, relative to
the
> total likely number of machines, will render a whole-collection watch
unsuitable
> -- we'll be filtering a *lot* of uninteresting events.
> 
> Take a look at UnitsWatcher, near the bottom of the file; it's used for
> WatchPrincipalUnits and WatchSubordinateUnits, but isn't actually very
> unit-specific at all. I think it could reasonably be parameterized by
> collection, and become a useful small-interesting-set variant of
> LifecycleWatcher.
> 
> Sound sane? If not I'm still happy to land it as is, but we should chat
> afterwards about a followup.

Long story, I'll try and keep it short. The UnitsWatcher works on a collection
belonging to an entity. I refactored it to be a generic collection watcher (in
the process allowing potential reuse for things besides units). However, I then
found it didn't work because the container ref collection is not contained by
the machine doc; it is a separately managed collection which references a
machine doc. So those changes were reverted.

My second attempt involved writing a MachineContainerWatcher implementation;
basically a Watcher on the container ref collection item with specified machine
id. I keep a copy of the known containers on a machine and when the entity
changes, process accordingly. This worked for adding containers to machines but
failed when a container was marked as dying or dead, because the operation acts
only on the container machine doc and not the container reference doc.

So I was back to square one. What I did was move the filtering from the derived
lifecycle watcher class into the txn log event processing loop. This culls any
unnecessary events as close to the source as possible and the lifecycle watcher
only processes relevant events in its loop.

[fwereade, 2013-06-17 13:36:13]

The diff is all messed up, but LGTM on the understanding that the real changes
are just in state and state/watcher.