provisioner: Start instances with networks

provisioner: Start instances with networks

Provisioner worker is made aware of networks specified
for a machine and passes them to the providers when
starting instances. This involves a few extra things:
 * cloudinit.MachineConfig now has IncludeNetworks and
   ExcludeNetworks params and they can be set using
   NewMachineConfig();
 * StartInstanceParams.Networks dropped and now networks
   are specified via SIP.MachineConfig;
 * provisioner gets specified machine networks and
   passes them through StartInstanceParams.MachineConfig
   to the providers;
 * all providers except maas changed to return an error
   in StartInstance() when networks are specified (using
   MachineConfig.HasNetworks()).

All changes are unit-tested and live-tested on MaaS.

Next steps: get list of networks from the provider,
setup cloudinit scripts to configure the networks
(failing with unknown ones) and update the machine
with the actually installed networks, so they can be
shown in status.

https://code.launchpad.net/~dimitern/juju-core/380-provisioner-honors-networks/+merge/213647

Requires: https://code.launchpad.net/~dimitern/juju-core/379-api-provisioner-networks/+merge/213433

(do not edit description out of merge proposal)

[dimitern, 2014-04-01 13:03:40]

Please take a look.

[fwereade, 2014-04-01 15:39:49]

mostly just recording stuff we discussed live

https://codereview.appspot.com/83200043/diff/1/container/testing/common.go
File container/testing/common.go (right):

https://codereview.appspot.com/83200043/diff/1/container/testing/common.go#ne...
container/testing/common.go:23: machineConfig :=
environs.NewMachineConfig(machineId, "fake-nonce", nil, stateInfo, apiInfo)
apropos of nothing really, is there any reason we still have a state.Info in
here?

https://codereview.appspot.com/83200043/diff/1/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/83200043/diff/1/provider/dummy/environs.go#new...
provider/dummy/environs.go:537: if err := env.checkBroken("SupportNetworks");
err != nil {
This is a kinda nasty way of doing this. Could we use a plain config setting
instead..?

https://codereview.appspot.com/83200043/diff/1/provider/dummy/environs.go#new...
provider/dummy/environs.go:762: if len(args.MachineConfig.IncludeNetworks) >
len(args.Networks.IncludeNetworks) {
This in particular scares the hell out of me. If you need to wait for a branch
of mgz's to fix it, so be it, but this feature is not even MVP-done until
there's a *single* mechanism for passing networks into providers ;).

https://codereview.appspot.com/83200043/diff/1/state/statecmd/machineconfig.go
File state/statecmd/machineconfig.go (right):

https://codereview.appspot.com/83200043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:83: if len(includeNetworks) > 0 ||
len(excludeNetworks) > 0 {
isn't a Networks{nil,nil} valid?

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:234: task, err := p.getStartTask(false, false)
let's drop this supportNetworks stuff, as discussed

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:444: Networks: environs.Networks{
Hey, can't we just drop SIP.Networks full stop, and pass that info via
MachineConfig in this branch? Having two (apparent) ways to do it *really*
upsets me... worse than having it on MachineConfig, even if MachineConfig isn't
really the right place either.

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:521: return nil, errNoNetworksSupport
Already discussed, let's not do this -- we want the usual errors as returned
from StartInstance.

[dimitern, 2014-04-01 16:34:51]

Please take a look.

https://codereview.appspot.com/83200043/diff/1/container/testing/common.go
File container/testing/common.go (right):

https://codereview.appspot.com/83200043/diff/1/container/testing/common.go#ne...
container/testing/common.go:23: machineConfig :=
environs.NewMachineConfig(machineId, "fake-nonce", nil, stateInfo, apiInfo)
On 2014/04/01 15:39:49, fwereade wrote:
> apropos of nothing really, is there any reason we still have a state.Info in
> here?

I suppose because MachineConfig is used as a base for BootstrapMachine config?
Or it's probably a left-over.

https://codereview.appspot.com/83200043/diff/1/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/83200043/diff/1/provider/dummy/environs.go#new...
provider/dummy/environs.go:537: if err := env.checkBroken("SupportNetworks");
err != nil {
On 2014/04/01 15:39:49, fwereade wrote:
> This is a kinda nasty way of doing this. Could we use a plain config setting
> instead..?

No need to do that anymore, reverted.

https://codereview.appspot.com/83200043/diff/1/provider/dummy/environs.go#new...
provider/dummy/environs.go:762: if len(args.MachineConfig.IncludeNetworks) >
len(args.Networks.IncludeNetworks) {
On 2014/04/01 15:39:49, fwereade wrote:
> This in particular scares the hell out of me. If you need to wait for a branch
> of mgz's to fix it, so be it, but this feature is not even MVP-done until
> there's a *single* mechanism for passing networks into providers ;).

Dropped StartInstanceParams.Networks and now using
MachineConfig.Include(Exclude)Networks everywhere instead.

https://codereview.appspot.com/83200043/diff/1/state/statecmd/machineconfig.go
File state/statecmd/machineconfig.go (right):

https://codereview.appspot.com/83200043/diff/1/state/statecmd/machineconfig.g...
state/statecmd/machineconfig.go:83: if len(includeNetworks) > 0 ||
len(excludeNetworks) > 0 {
On 2014/04/01 15:39:49, fwereade wrote:
> isn't a Networks{nil,nil} valid?

If both are slices are nil (or empty) they are treated as not set for the sake
of starting an instance.

This is now simplified.

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner.go:234: task, err := p.getStartTask(false, false)
On 2014/04/01 15:39:49, fwereade wrote:
> let's drop this supportNetworks stuff, as discussed

Done.

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:444: Networks: environs.Networks{
On 2014/04/01 15:39:49, fwereade wrote:
> Hey, can't we just drop SIP.Networks full stop, and pass that info via
> MachineConfig in this branch? Having two (apparent) ways to do it *really*
> upsets me... worse than having it on MachineConfig, even if MachineConfig
isn't
> really the right place either.

Only MachineConfig networks are now used, SIP.Networks is dropped.

https://codereview.appspot.com/83200043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_task.go:521: return nil, errNoNetworksSupport
On 2014/04/01 15:39:49, fwereade wrote:
> Already discussed, let's not do this -- we want the usual errors as returned
> from StartInstance.

Done.

[fwereade, 2014-04-01 17:17:29]

LGTM, just trivials now.

We will want to move those fields of MachineConfig before too long, please make
sure there's a card (or series of them) for making sure it's all done by the
machine agent.

https://codereview.appspot.com/83200043/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/83200043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:389: len(args.MachineConfig.ExcludeNetworks) > 0 {
maybe a NetworksSet method on MachineConfig?

https://codereview.appspot.com/83200043/diff/20001/worker/provisioner/provisi...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/83200043/diff/20001/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:195: }
jc.DeepEquals will work without needing to check len, I think

[dimitern, 2014-04-02 07:48:58]

Please take a look.

https://codereview.appspot.com/83200043/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/83200043/diff/20001/provider/azure/environ.go#...
provider/azure/environ.go:389: len(args.MachineConfig.ExcludeNetworks) > 0 {
On 2014/04/01 17:17:30, fwereade wrote:
> maybe a NetworksSet method on MachineConfig?

Done.

https://codereview.appspot.com/83200043/diff/20001/worker/provisioner/provisi...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/83200043/diff/20001/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:195: }
On 2014/04/01 17:17:30, fwereade wrote:
> jc.DeepEquals will work without needing to check len, I think

My intent was to check for both given networks and for nil networks (when
provisioning without networks). Will try if only DeepEquals will do the same.

[dimitern, 2014-04-02 10:08:22]

Please take a look.