worker/addresspublisher: new package

worker/addresspublisher: new package

This package implements a worker that watches
machines in state and populates their addresses;
it will also publish the addresses of relevant state
server machines in the state and in the provider's
state-info.

This is the start of the implementation of this package,
with internal tests for runMachine completed, but
only a single test for watchMachinesLoop done.

To come: complete tests for watchMachinesLoop;
wiring up the logic to real state, and the state
server publication.

https://code.launchpad.net/~rogpeppe/juju-core/424-worker-publisher/+merge/188114

(do not edit description out of merge proposal)

[rog, 2013-09-29 17:02:13]

Please take a look.

[fwereade, 2013-09-30 08:12:54]

A few preliminary thoughts -- and a vague sensation that there's something a bit
tangly and hard to follow here. The code as it stands looks like an address
*updater*, which is fine and well and good and necessary; but it's overextended
into some slightly weird publishing stuff, which feels like it'd be better off
entirely separate.

Is there any way we can extract what we've got into its own worker and get it
landed ASAP? That'd give us enough data in state to write sensible
StateInfo/APIInfo methods...

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
File worker/addresspublisher/publisher.go (right):

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:74: dying() <-chan struct{}
I'm always a bit suspicious of shutdown mechanisms that don't use tombs -- they
STM to be common and well-understood, and to thus admit to simpler analysis by
those not intimately familiar with the code. Unless there's a reason a tomb is
actively inappropriate here, I think it's best to use one.

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:78: ctxt        publisherContext
If ctx annoys people, ctxt isn't going to make them much happier -- let's just
call such thinks "context" and save everyone else the confusion.

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:150: // We assume that the machine is alive
when this is first called.
I'm not sure that's knowable, is it?

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:210: publisherc <- machineAddress{machine:
m}
Please don't clear out addresses just because the machine's Dying (maybe that's
not what you're doing? but it kinda looks like it is). The machine death
sequence is simple today, because 13.04 schedule, but cannot remain so forever
(and seems like it might be implicated in some CTS bugs regardless, so it might
not even be too long). Dead should be fine, but while a machine is just Dying
there's a reasonable expectation that its address info will remain valuable.

[rog, 2013-09-30 12:09:59]

I'll publish these comments now, although I'm holding off moving forward this
package for the moment, as discussed.

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
File worker/addresspublisher/publisher.go (right):

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:74: dying() <-chan struct{}
On 2013/09/30 08:12:54, fwereade wrote:
> I'm always a bit suspicious of shutdown mechanisms that don't use tombs --
they
> STM to be common and well-understood, and to thus admit to simpler analysis by
> those not intimately familiar with the code. Unless there's a reason a tomb is
> actively inappropriate here, I think it's best to use one.

We are going to use a tomb - the dying struct will come from Tomb.Dying when
the code is wired up.

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:78: ctxt        publisherContext
On 2013/09/30 08:12:54, fwereade wrote:
> If ctx annoys people, ctxt isn't going to make them much happier -- let's just
> call such thinks "context" and save everyone else the confusion.

ok

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:150: // We assume that the machine is alive
when this is first called.
On 2013/09/30 08:12:54, fwereade wrote:
> I'm not sure that's knowable, is it?

The life cycle watcher only tells us about alive machines;
those machines might of course have died since
the watcher told us about them, but for the purposes
of this loop, we assume that the machine is alive.
That is: we don't immediately call m.Refresh to
ascertain its current life state.

If the above scenario did happen, then we will will shortly
be informed that the machine is dead and will receive
a message on the changed channel, which will cause
us to refresh the machine and find out what it's currently
doing.

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:210: publisherc <- machineAddress{machine:
m}
On 2013/09/30 08:12:54, fwereade wrote:
> Please don't clear out addresses just because the machine's Dying (maybe
that's
> not what you're doing? but it kinda looks like it is). The machine death
> sequence is simple today, because 13.04 schedule, but cannot remain so forever
> (and seems like it might be implicated in some CTS bugs regardless, so it
might
> not even be too long). Dead should be fine, but while a machine is just Dying
> there's a reasonable expectation that its address info will remain valuable.

good point.

[rog, 2013-10-02 15:03:19]

Please take a look.

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
File worker/addresspublisher/publisher.go (right):

https://codereview.appspot.com/14038045/diff/3001/worker/addresspublisher/pub...
worker/addresspublisher/publisher.go:78: ctxt        publisherContext
On 2013/09/30 12:09:59, rog wrote:
> On 2013/09/30 08:12:54, fwereade wrote:
> > If ctx annoys people, ctxt isn't going to make them much happier -- let's
just
> > call such thinks "context" and save everyone else the confusion.
> 
> ok

Done.

[fwereade, 2013-10-02 15:44:56]

OK, I think this LGTM, despite the all-internal testing which makes it a bit
hard to follow. Thanks very much.

https://codereview.appspot.com/14038045/diff/8001/worker/addressupdater/updat...
File worker/addressupdater/updater.go (right):

https://codereview.appspot.com/14038045/diff/8001/worker/addressupdater/updat...
worker/addressupdater/updater.go:39: //}
drop these bits?

https://codereview.appspot.com/14038045/diff/8001/worker/addressupdater/updat...
worker/addressupdater/updater.go:197: // in which case we return. The logic will
still work
not quite accurate any more

[rog, 2013-10-02 15:58:04]

Please take a look.

https://codereview.appspot.com/14038045/diff/8001/worker/addressupdater/updat...
File worker/addressupdater/updater.go (right):

https://codereview.appspot.com/14038045/diff/8001/worker/addressupdater/updat...
worker/addressupdater/updater.go:39: //}
On 2013/10/02 15:44:57, fwereade wrote:
> drop these bits?

since they're skeletons for upcoming code, I'd like
to leave them here until I flesh them out, if that's ok.

https://codereview.appspot.com/14038045/diff/8001/worker/addressupdater/updat...
worker/addressupdater/updater.go:197: // in which case we return. The logic will
still work
On 2013/10/02 15:44:57, fwereade wrote:
> not quite accurate any more

I've changed the logic slightly so that this comment
is no longer necessary.