various: Introduce networks constraints

various: Introduce networks constraints

Following the removal of --exclude-networks argument
from juju deploy, now we're introducting a complete
way to specify networks with constraints:

--constraints networks=net1,net2,^net3,^net4

Means "find a machine with net1 and net2 networks,
but not with net3 or net4 networks".

Changes are in 3 places: constraints package, state
(for saving/loading) and cmd/juju/deploy for the CLI.

Now, to specify networks to include/exclude on deploy:
juju deploy svc --networks=logging,db --constraints networks=^dmz,storage

Means: "deploy svc on machines with logging, db, and storage
networks, but not with dmz network".

Only the those, specified with --networks will be enabled
on the machine, once the networker worker is done. The ones
specified with constraints are used as a filter for instance
selection.

https://code.launchpad.net/~dimitern/juju-core/460-network-constraints/+merge/221390

(do not edit description out of merge proposal)

[dimitern, 2014-05-29 14:19:03]

Please take a look.

[fwereade, 2014-05-29 18:22:36]

If you dropped the changes to cmd/juju I'd approve this (with just a little bit
of extra checking of the values when you parse network constraints).

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode95
cmd/juju/deploy.go:95: juju deploy mysql --networks=storage,mynet --constraints
^logging,db
--constraints networks=^logging,db

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode191
cmd/juju/deploy.go:191: includeNetworks :=
mergeNetworks(parseNetworks(c.Networks), c.Constraints.IncludeNetworks())
This is definitely wrong. This'd put the ones in constraints into the set that
got configured on the machine, instead of the ones used *only* for machine
*selection*, as they should be.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode192
cmd/juju/deploy.go:192: excludeNetworks := mergeNetworks(nil,
c.Constraints.ExcludeNetworks())
And I'm pretty sure this is wrong too. The information's already in constraints
and doesn't need to be duplicated.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode195
cmd/juju/deploy.go:195: env, err := environs.New(conf)
I'm a bit concerned about this. I know we currently need the environ to talk to
the charm store, but I think this will change as we move the charm store into
the environment; I'd prefer not to add more dependencies on it at this layer
(apart from everything else, we can't assume that the average user has
permission to look at the env config). Just do the checking in the API server
(which you have to do anyway, you might get any request from the GUI/other
client).

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode240
cmd/juju/deploy.go:240: excludeNetworks,
includeNetworks could stay, if it had the right information in it, but I'm
certain we don't want this. If we haven't released a stable version with
--include/exclude-networks, we should just drop this field. What are dev
versions for, if not for making mistakes? ;p

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode297
cmd/juju/deploy.go:297: parts := strings.Split(networksValue, ",")
cmd.StringsValue

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode314
cmd/juju/deploy.go:314: // It's not a tag, convert it.
We should *always* turn the user's strings into tags. If they try to specify
network-foo, send network-network-foo over the API.

https://codereview.appspot.com/102830044/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/102830044/diff/1/constraints/constraints.go#ne...
constraints/constraints.go:514: return &items, nil
I'm wondering whether we should be restricting the values here, at least in the
networks case. Remember these are meant to refer to juju network names, not
provider ones (where we can't control validity).

Tags may be a bit different; not sure if we can sanely restrict them, even if
we'll get surprising results if they ever contain ",", but I think we can and
should keep network names locked down.

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

https://codereview.appspot.com/102830044/diff/1/state/constraints.go#newcode55
state/constraints.go:55: InstanceType: cons.InstanceType,
Please make the order of fields here (and above) consistent with the order in
the struct definition.

https://codereview.appspot.com/102830044/diff/1/state/constraintsvalidation_t...
File state/constraintsvalidation_test.go (right):

https://codereview.appspot.com/102830044/diff/1/state/constraintsvalidation_t...
state/constraintsvalidation_test.go:43: }{{
Thanks :).

[dimitern, 2014-06-02 09:27:24]

Sorry, what I meant by "follow-up" is actually a prerequisite:
https://codereview.appspot.com/93670046

What I've said below is done, it's there.

I'll repropose this later with the changes to cmd/juju mostly.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode95
cmd/juju/deploy.go:95: juju deploy mysql --networks=storage,mynet --constraints
^logging,db
On 2014/05/29 18:22:36, fwereade wrote:
> --constraints networks=^logging,db

Done in a follow-up.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode191
cmd/juju/deploy.go:191: includeNetworks :=
mergeNetworks(parseNetworks(c.Networks), c.Constraints.IncludeNetworks())
On 2014/05/29 18:22:36, fwereade wrote:
> This is definitely wrong. This'd put the ones in constraints into the set that
> got configured on the machine, instead of the ones used *only* for machine
> *selection*, as they should be.

Taking your suggestion, I'm dropping cmd/juju changes from this branch and will
propose it as a follow-up, fixing these issues.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode192
cmd/juju/deploy.go:192: excludeNetworks := mergeNetworks(nil,
c.Constraints.ExcludeNetworks())
On 2014/05/29 18:22:36, fwereade wrote:
> And I'm pretty sure this is wrong too. The information's already in
constraints
> and doesn't need to be duplicated.

Done in a follow-up.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode195
cmd/juju/deploy.go:195: env, err := environs.New(conf)
On 2014/05/29 18:22:36, fwereade wrote:
> I'm a bit concerned about this. I know we currently need the environ to talk
to
> the charm store, but I think this will change as we move the charm store into
> the environment; I'd prefer not to add more dependencies on it at this layer
> (apart from everything else, we can't assume that the average user has
> permission to look at the env config). Just do the checking in the API server
> (which you have to do anyway, you might get any request from the GUI/other
> client).

Check the follow-up. Doing this check here, in addition to a check in the API
server (sorry, I thought I had that there already - will add it there as well)
will allow us to bail out early, before the API call with a nicer CLI-specific
message, like "cannot use --networks: not supported by the environmen" instead
of "cannot deploy "wordpress" with networks: not supported by the environment".
I'm with having only the latter, less specific error message.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode240
cmd/juju/deploy.go:240: excludeNetworks,
On 2014/05/29 18:22:36, fwereade wrote:
> includeNetworks could stay, if it had the right information in it, but I'm
> certain we don't want this. If we haven't released a stable version with
> --include/exclude-networks, we should just drop this field. What are dev
> versions for, if not for making mistakes? ;p

Good point. In the follow-up, I'll drop that and modify service.Networks() to
return include/excludeNetworks as before, but taking both requested networks and
constraints into account. That's the least painful way to do it I found.

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode297
cmd/juju/deploy.go:297: parts := strings.Split(networksValue, ",")
On 2014/05/29 18:22:36, fwereade wrote:
> cmd.StringsValue

Didn't know about this one! Will use it in the follow-up, thanks!

https://codereview.appspot.com/102830044/diff/1/cmd/juju/deploy.go#newcode314
cmd/juju/deploy.go:314: // It's not a tag, convert it.
On 2014/05/29 18:22:36, fwereade wrote:
> We should *always* turn the user's strings into tags. If they try to specify
> network-foo, send network-network-foo over the API.

The complication here is because I'm using it to process both networks coming
from constraints and the argument. The whole point is to always send tags yes,
but I'll simplify it in the follow-up.

https://codereview.appspot.com/102830044/diff/1/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/102830044/diff/1/constraints/constraints.go#ne...
constraints/constraints.go:514: return &items, nil
On 2014/05/29 18:22:36, fwereade wrote:
> I'm wondering whether we should be restricting the values here, at least in
the
> networks case. Remember these are meant to refer to juju network names, not
> provider ones (where we can't control validity).
> 
> Tags may be a bit different; not sure if we can sanely restrict them, even if
> we'll get surprising results if they ever contain ",", but I think we can and
> should keep network names locked down.

Added validation for network names and tests.

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

https://codereview.appspot.com/102830044/diff/1/state/constraints.go#newcode55
state/constraints.go:55: InstanceType: cons.InstanceType,
On 2014/05/29 18:22:36, fwereade wrote:
> Please make the order of fields here (and above) consistent with the order in
> the struct definition.

Done.

[dimitern, 2014-06-04 11:17:44]

This got migrated to github:

https://github.com/juju/juju/pull/13

Lots of changes done, suggestions applied.