Add a worker to keep disk files up to date

Add a worker to keep disk files up to date

Added the machine environment worker which watches
the environment config and will write changes to the
disk as needed.

A new end point was added on the client and server side
that is just called "Environment", which provies the common
environment watcher.

The only gotcha in this is that if the worker is running on
machine 0 for a local provider, it shouldn't attempt to modify
system files.  There are tests for this.

The two files on disk that are currently being kept up
to date with this are:
 /home/ubuntu/.juju-proxy
 /etc/apt/apt.conf.d/42-juju-proxy-settings

https://code.launchpad.net/~thumper/juju-core/machine-worker/+merge/203460

Requires: https://code.launchpad.net/~thumper/juju-core/fix-uniter-proxy-env-vars/+merge/203459

(do not edit description out of merge proposal)

[thumper, 2014-01-28 04:00:26]

Please take a look.

[wallyworld, 2014-01-28 04:44:50]

Looks pretty good but we need a juju/machine_test to test that the jujud has
started the worker properly. There's a few examples in machine_test to look at.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
File worker/machineenvironmentworker/machineenvironmentworker.go (right):

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:30: ProxyDirectory =
"/home/ubuntu"
Not sure if other workers use ~ubuntu and/or /home/ubuntu, but if they do ,
would be good to make this a const or method call.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:50: first           
bool
Do other workers use first? Don't they just suck up the initial event and deal
with it? It seems a little complicated to use it here. If we do need to use it,
perhaps a comment saying why we need it here.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:77: // an ubuntu
user, it will. This shouldn't be a problem.
I read the above a few times and am still not sure I've parsed it correctly. I
think "should" might need to be "shouldn't"??

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:85: // same way that
the file is writting in the cloud-init process, so
s/writting/written  ?

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:122: aptSettings :=
env.AptProxySettings()
not sure if we want the 2 lots of logic in this method in separate sub methods
so onChange is dead easy to ready

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:137: func (w
*MachineEnvironmentWorker) SetUp() (watcher.NotifyWatcher, error) {
Typically we add a doc string saying what interface SetUp() comes from

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:149: func (w
*MachineEnvironmentWorker) Handle() error {
ditto

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:153: func (w
*MachineEnvironmentWorker) TearDown() error {
ditto

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
File worker/machineenvironmentworker/machineenvironmentworker_test.go (right):

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker_test.go:41: var _ =
gc.Suite(&MachineEnvironmentWatcherSuite{})
I can't see a test that explicitly check that receiving the initial event does
what is needed ie the business logic associated with the "first" attribute. If
it is there, perhaps a comment to show where it is tested. Is that what
TestInitialState does? I'd actually prefer we didn't need the "first" attribute
:-)

[thumper, 2014-01-29 03:27:41]

Please take a look.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
File worker/machineenvironmentworker/machineenvironmentworker.go (right):

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:30: ProxyDirectory =
"/home/ubuntu"
On 2014/01/28 04:44:50, wallyworld wrote:
> Not sure if other workers use ~ubuntu and/or /home/ubuntu, but if they do ,
> would be good to make this a const or method call.

The whole point it is a variable is so we can mock it out during testing.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:50: first           
bool
On 2014/01/28 04:44:50, wallyworld wrote:
> Do other workers use first? Don't they just suck up the initial event and deal
> with it? It seems a little complicated to use it here. If we do need to use
it,
> perhaps a comment saying why we need it here. 

I'll add a comment.  It is only used for testing in order to call the Started
function (which is by default a no-op).  The Started function is mocked out in
the tests so I can demonstrate that not only do the changes happen when it
starts up, but that they happen once it is in the main loop.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:77: // an ubuntu
user, it will. This shouldn't be a problem.
On 2014/01/28 04:44:50, wallyworld wrote:
> I read the above a few times and am still not sure I've parsed it correctly. I
> think "should" might need to be "shouldn't"??

Yes, you are right. Changed.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:85: // same way that
the file is writting in the cloud-init process, so
On 2014/01/28 04:44:50, wallyworld wrote:
> s/writting/written  ?  

Done.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:122: aptSettings :=
env.AptProxySettings()
On 2014/01/28 04:44:50, wallyworld wrote:
> not sure if we want the 2 lots of logic in this method in separate sub methods
> so onChange is dead easy to ready

Yeah I was tossing up on this one as I was writing it.

I'll split it up.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:137: func (w
*MachineEnvironmentWorker) SetUp() (watcher.NotifyWatcher, error) {
On 2014/01/28 04:44:50, wallyworld wrote:
> Typically we add a doc string saying what interface SetUp() comes from

cleaner, logger and machiner all didn't. I looked at your one for the
boilerplate.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:149: func (w
*MachineEnvironmentWorker) Handle() error {
On 2014/01/28 04:44:50, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker.go:153: func (w
*MachineEnvironmentWorker) TearDown() error {
On 2014/01/28 04:44:50, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
File worker/machineenvironmentworker/machineenvironmentworker_test.go (right):

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworke...
worker/machineenvironmentworker/machineenvironmentworker_test.go:41: var _ =
gc.Suite(&MachineEnvironmentWatcherSuite{})
On 2014/01/28 04:44:50, wallyworld wrote:
> I can't see a test that explicitly check that receiving the initial event does
> what is needed ie the business logic associated with the "first" attribute. If
> it is there, perhaps a comment to show where it is tested. Is that what
> TestInitialState does? I'd actually prefer we didn't need the "first"
attribute
> :-)

Almost all the other workers don't test this.  I was just being more thorough.

The purpose of the first attribute is to allow isolated testing of the SetUp,
and the Loop.  The first attribute is needed to potentially write out empty
values, as now documented in the main file.

[wallyworld, 2014-01-29 03:35:56]

LGTM, thanks for the extra code comments, especially for first. Makes it much
clearer for the casual reader.

https://codereview.appspot.com/57600043/diff/20001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/57600043/diff/20001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:80: s.PatchValue(&utils.AptConfFile,
filepath.Join(s.proxyDir, "juju-apt-proxy"))
Small nitpick, feel free to ignore - these set up steps are only for one
specific test and not for the tests in general. My preference would be just to
have them in the one test that needs them.