api/provisioner: Complete Provisioner API

api/provisioner: Complete Provisioner API

This implements the full client-side provisioner API,
as needed by the provisioner worker.

https://code.launchpad.net/~dimitern/juju-core/142-api-provisioner-1/+merge/186516

Requires: https://code.launchpad.net/~dimitern/juju-core/141-apiserver-provisioner-2/+merge/186355

(do not edit description out of merge proposal)

[dimitern, 2013-09-19 12:13:23]

Please take a look.

[jameinel, 2013-09-19 13:00:14]

Overall it seems a pretty straightforward wrapping of the APIServer side of the
code. So all that is LGTM.

It would be nice if the tests for the Machine object (defined in machine.go)
would be in machine_test.go, but that's the only thing I would really change.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:68: 
If it isn't hard, it would be nice to have these in
state/api/provisioner/machine_test.go since that matches where the object itself
is defined. Certainly it is where I expected to find them.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:254: wc.AssertNoChange()
I'm pretty surprised that WatchContainers takes a container type. Wouldn't the
same provisioning agent do all container types?

However, that is more of an odd-model problem than something wrong with the code
you've written. (so it isn't that you have to change something for this patch,
but I wonder if the overall design is correct.)

[dimitern, 2013-09-19 14:01:42]

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:68: 
On 2013/09/19 13:00:14, jameinel wrote:
> If it isn't hard, it would be nice to have these in
> state/api/provisioner/machine_test.go since that matches where the object
itself
> is defined. Certainly it is where I expected to find them.

This is not the only case where client-side facade tests are in a single file.
For most is not a problem - they're not big, for the uniter it got too big, so I
separated them. If you insist, I can separate them here as well, but in a
follow-up, which will cause some refactoring to reduce duplication of setup
code.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:254: wc.AssertNoChange()
On 2013/09/19 13:00:14, jameinel wrote:
> I'm pretty surprised that WatchContainers takes a container type. Wouldn't the
> same provisioning agent do all container types?
> 
> However, that is more of an odd-model problem than something wrong with the
code
> you've written. (so it isn't that you have to change something for this patch,
> but I wonder if the overall design is correct.)

Getting the supported containers from the authenticated machine could be done,
but it seems unnecessary for now. We can do it later,  if needed.

[dimitern, 2013-09-19 14:28:16]

Please take a look.

[fwereade, 2013-09-19 15:23:04]

LGTM

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:135: }
Hmm. Isn't this what we had for deployer, but with machines instead of units? If
a provisioner sets dead it always wants to remove as well, right?

If this actually mirrors deployer, forget I said anything.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:140: // as well, because it needs to do an API
call.
This may be a case for the "this is immutable and therefore safe to cache" TODO

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner.go:92: func (st *State) WatchEnvironMachines()
(watcher.StringsWatcher, error) {
I do find myself wondering if maybe this should take an environment tag. But I
don't think any of the others do, do they? Probably best to stay consistent for
now regardless.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:68: 
On 2013/09/19 14:01:42, dimitern wrote:
> On 2013/09/19 13:00:14, jameinel wrote:
> > If it isn't hard, it would be nice to have these in
> > state/api/provisioner/machine_test.go since that matches where the object
> itself
> > is defined. Certainly it is where I expected to find them.
> 
> This is not the only case where client-side facade tests are in a single file.
> For most is not a problem - they're not big, for the uniter it got too big, so
I
> separated them. If you insist, I can separate them here as well, but in a
> follow-up, which will cause some refactoring to reduce duplication of setup
> code.

I'm generally +1 on more, smaller, files :).

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:254: wc.AssertNoChange()
On 2013/09/19 14:01:42, dimitern wrote:
> On 2013/09/19 13:00:14, jameinel wrote:
> > I'm pretty surprised that WatchContainers takes a container type. Wouldn't
the
> > same provisioning agent do all container types?
> > 
> > However, that is more of an odd-model problem than something wrong with the
> code
> > you've written. (so it isn't that you have to change something for this
patch,
> > but I wonder if the overall design is correct.)
> 
> Getting the supported containers from the authenticated machine could be done,
> but it seems unnecessary for now. We can do it later,  if needed.

FWIW, it is expected that we have one provisioner per container kind: the idea
is that a provisioner be the connection between a single set of machines and a
single instance broker (which provisions/decommissions machinesof a particular
type).

I *do* think that we shouldn't even run a provisioner on a machine without
containers, and that we should be able to watch the set of active container
kinds to start/stop individual provisioners... but we never got that far.

[dimitern, 2013-09-19 15:28:22]

Sorry, I landed this before I got your comments, but will accommodate the
suggestions in a follow-up, where applicable.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:135: }
On 2013/09/19 15:23:04, fwereade wrote:
> Hmm. Isn't this what we had for deployer, but with machines instead of units?
If
> a provisioner sets dead it always wants to remove as well, right?
> 
> If this actually mirrors deployer, forget I said anything.

I can change that in the course of changing the provisioner to use the API. It's
different from the deployer, but easy to change.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/machine....
state/api/provisioner/machine.go:140: // as well, because it needs to do an API
call.
On 2013/09/19 15:23:04, fwereade wrote:
> This may be a case for the "this is immutable and therefore safe to cache"
TODO

Will add one in a follow-up.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner.go:92: func (st *State) WatchEnvironMachines()
(watcher.StringsWatcher, error) {
On 2013/09/19 15:23:04, fwereade wrote:
> I do find myself wondering if maybe this should take an environment tag. But I
> don't think any of the others do, do they? Probably best to stay consistent
for
> now regardless.

None of the others do, so it's consistent.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:68: 
On 2013/09/19 15:23:04, fwereade wrote:
> On 2013/09/19 14:01:42, dimitern wrote:
> > On 2013/09/19 13:00:14, jameinel wrote:
> > > If it isn't hard, it would be nice to have these in
> > > state/api/provisioner/machine_test.go since that matches where the object
> > itself
> > > is defined. Certainly it is where I expected to find them.
> > 
> > This is not the only case where client-side facade tests are in a single
file.
> > For most is not a problem - they're not big, for the uniter it got too big,
so
> I
> > separated them. If you insist, I can separate them here as well, but in a
> > follow-up, which will cause some refactoring to reduce duplication of setup
> > code.
> 
> I'm generally +1 on more, smaller, files :).

In a follow-up.

https://codereview.appspot.com/13627051/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:254: wc.AssertNoChange()
On 2013/09/19 15:23:04, fwereade wrote:
> On 2013/09/19 14:01:42, dimitern wrote:
> > On 2013/09/19 13:00:14, jameinel wrote:
> > > I'm pretty surprised that WatchContainers takes a container type. Wouldn't
> the
> > > same provisioning agent do all container types?
> > > 
> > > However, that is more of an odd-model problem than something wrong with
the
> > code
> > > you've written. (so it isn't that you have to change something for this
> patch,
> > > but I wonder if the overall design is correct.)
> > 
> > Getting the supported containers from the authenticated machine could be
done,
> > but it seems unnecessary for now. We can do it later,  if needed.
> 
> FWIW, it is expected that we have one provisioner per container kind: the idea
> is that a provisioner be the connection between a single set of machines and a
> single instance broker (which provisions/decommissions machinesof a particular
> type).
> 
> I *do* think that we shouldn't even run a provisioner on a machine without
> containers, and that we should be able to watch the set of active container
> kinds to start/stop individual provisioners... but we never got that far.

Will change as needed.