state/api: Add Client.ServiceDeployWithNetworks

state/api: Add Client.ServiceDeployWithNetworks

This adds a new client API call: ServiceDeployWithNetworks,
which works exactly like ServiceDeploy, but takes two extra
arguments: IncludeNetworks and ExcludeNetworks.
These specify a list of VLANs/networks to enable or disable
at boot time for the machine being deployed, and also record
that into the service document.

AddUnits is also changed (and its tests) to respect networks
set on the service and pass them through MachineTemplate when
deploying or a new machine or container.

https://code.launchpad.net/~dimitern/juju-core/350-api-service-deploy-with-networks/+merge/211773

(do not edit description out of merge proposal)

[dimitern, 2014-03-20 10:16:37]

Please take a look.

[fwereade, 2014-03-20 13:42:47]

1) it's hard to approve this without an implementation in the background

2) should we be talking network tags?

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:243: return c.serviceDeploy(&args, nil)
might be nicer to copy args into a ServiceDeployWithNetworks...

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:256: func (c *Client) serviceDeploy(args0
*params.ServiceDeploy, args1 *params.ServiceDeployWithNetworks) error {
...and just handle one arg type here?

[rog, 2014-03-20 14:06:22]

https://codereview.appspot.com/76910044/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/76910044/diff/20001/state/api/params/params.go...
state/api/params/params.go:155: type ServiceDeployWithNetworks struct {
I suggest that instead of making a new set of parameters, we could just add the
networks parameters to ServiceDeploy, and make ServiceDeployWithNetworks an
alias of Deploy.

We can document the new fields as recognised only in 1.18 (1.19? 1.20?) API
servers.

This means that clients that care that the networks fields are recognised can
use ServiceDeployWithNetworks, but existing clients can continue to use Deploy,
and eventually we should be able to deprecate ServiceDeployWithNetworks.

This adds minimal clutter to the code, and provides the same functionality
AFAICS.

[dimitern, 2014-03-20 15:41:47]

Please take a look.

https://codereview.appspot.com/76910044/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/76910044/diff/20001/state/api/params/params.go...
state/api/params/params.go:155: type ServiceDeployWithNetworks struct {
On 2014/03/20 14:06:23, rog wrote:
> I suggest that instead of making a new set of parameters, we could just add
the
> networks parameters to ServiceDeploy, and make ServiceDeployWithNetworks an
> alias of Deploy.
> 
> We can document the new fields as recognised only in 1.18 (1.19? 1.20?) API
> servers.
> 
> This means that clients that care that the networks fields are recognised can
> use ServiceDeployWithNetworks, but existing clients can continue to use
Deploy,
> and eventually we should be able to deprecate ServiceDeployWithNetworks.
> 
> This adds minimal clutter to the code, and provides the same functionality
> AFAICS.

Done.

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:243: return c.serviceDeploy(&args, nil)
On 2014/03/20 13:42:50, fwereade wrote:
> might be nicer to copy args into a ServiceDeployWithNetworks...

Done as rog suggested - ServiceDeployWithNetworks as an alias for ServiceDeploy.

https://codereview.appspot.com/76910044/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:256: func (c *Client) serviceDeploy(args0
*params.ServiceDeploy, args1 *params.ServiceDeployWithNetworks) error {
On 2014/03/20 13:42:50, fwereade wrote:
> ...and just handle one arg type here?

Done differently.

[rog, 2014-03-20 15:45:03]

much nicer, thanks.
LGTM with one suggestion below.

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:298: if len(args.IncludedNetworks) == 0 &&
len(args.ExcludedNetworks) == 0 {
I'd remove this and make the two commands exactly the same. Then clients can use
this call for all Deploy purposes if they wish.

[jameinel, 2014-03-20 20:38:21]

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:298: if len(args.IncludedNetworks) == 0 &&
len(args.ExcludedNetworks) == 0 {
On 2014/03/20 15:45:03, rog wrote:
> I'd remove this and make the two commands exactly the same. Then clients can
use
> this call for all Deploy purposes if they wish.

As William pointed out in IRC, I think the "ideal" would be to have API
versioning and then we'd just have ServiceDeploy that accepts everything.
However, I think lacking that I agree with Roger. I'd rather have just 1 API
that is always called.
(So we effectively have API versioning, just via a clumsy renaming of the API
each time.)

[dimitern, 2014-03-21 15:09:26]

Please take a look.

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:298: if len(args.IncludedNetworks) == 0 &&
len(args.ExcludedNetworks) == 0 {
On 2014/03/20 15:45:03, rog wrote:
> I'd remove this and make the two commands exactly the same. Then clients can
use
> this call for all Deploy purposes if they wish.

Done.

https://codereview.appspot.com/76910044/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:298: if len(args.IncludedNetworks) == 0 &&
len(args.ExcludedNetworks) == 0 {
On 2014/03/20 20:38:22, jameinel wrote:
> On 2014/03/20 15:45:03, rog wrote:
> > I'd remove this and make the two commands exactly the same. Then clients can
> use
> > this call for all Deploy purposes if they wish.
> 
> As William pointed out in IRC, I think the "ideal" would be to have API
> versioning and then we'd just have ServiceDeploy that accepts everything.
> However, I think lacking that I agree with Roger. I'd rather have just 1 API
> that is always called.
> (So we effectively have API versioning, just via a clumsy renaming of the API
> each time.)

Done as rog suggested. Also, I'm not landing this until mgz's branch lands. Then
I'll integrate the two and drop the related TODOs.

[fwereade, 2014-03-24 09:57:32]

On 2014/03/20 20:38:21, jameinel wrote:
> As William pointed out in IRC, I think the "ideal" would be to have API
> versioning and then we'd just have ServiceDeploy that accepts everything.
> However, I think lacking that I agree with Roger. I'd rather have just 1 API
> that is always called.
> (So we effectively have API versioning, just via a clumsy renaming of the API
> each time.)

I can bear it on this occasion, but this is only suitable in the
ultra-short-term: it will now look as though the old version supports networks,
even though it doesn't (especially if we start autogenerating docs etc).

[dimitern, 2014-03-26 11:04:02]

Please take a look.

[gz, 2014-03-26 12:20:18]

LGTM.