Merge lp:~hduran-8/juju-core/add_network_check_on_specified_machine into lp:~go-bot/juju-core/trunk

[Horacio Durán]

Added --not/network compat verification for --to

I added ValidateNetworksForInstance to Environ
This method checks if te requested networks are
available in the instance selected with --to
and if the excluded networks are not in conflict
with already existing ones.
I added this to the Environ interface and mocked
the method in all the available providers.
This method is only required when SupportNetworks
from EnvironCapability is true

https://codereview.appspot.com/84880045/

[Horacio Durán (hduran-8)]

Reviewers: mp+214463_code.launchpad.net,

Message:
Please take a look.

Description:
Added --not/network compat verification for --to

I added ValidateNetworksForInstance to Environ
This method checks if te requested networks are
available in the instance selected with --to
and if the excluded networks are not in conflict
with already existing ones.
I added this to the Environ interface and mocked
the method in all the available providers.
This method is only required when SupportNetworks
from EnvironCapability is true

https://code.launchpad.net/~hduran-8/juju-core/add_network_check_on_specified_machine/+merge/214463

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/84880045/

Affected files (+149, -2 lines):
   A [revision details]
   M cmd/juju/deploy.go
   M environs/interface.go
   M juju/deploy.go
   M provider/azure/environ.go
   M provider/dummy/environs.go
   M provider/ec2/ec2.go
   M provider/joyent/environ.go
   M provider/local/environ.go
   M provider/maas/environ.go
   M provider/maas/environ_whitebox_test.go
   M provider/manual/environ.go
   M provider/openstack/provider.go
   M state/apiserver/client/client.go

[Dimiter Naydenov (dimitern)]

Good direction, but I have some concerns and suggestions to make it
better.

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

https://codereview.appspot.com/84880045/diff/1/cmd/juju/deploy.go#newcode195
cmd/juju/deploy.go:195:
d

https://codereview.appspot.com/84880045/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/84880045/diff/1/environs/interface.go#newcode168
environs/interface.go:168: // ValidateNetworksForInstance returns nil if
the networks are
// ValidateNetworksForInstance returns and error if the given networks
// are not compatible for the given instance id.

s/instance_id string/instanceId instance.Id/

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

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode37
juju/deploy.go:37: func (parms *DeployServiceParams) ClearMachineSpecs()
(string, instance.ContainerType) {
s/parms/args/ ?

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode38
juju/deploy.go:38: // TODO: Add this to AddUnit and replace wherever
required
// TODO(hduran-8) ...

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode568
provider/maas/environ.go:568: maasObj_list, err :=
e.instances([]instance.Id{instance.Id(instance_id)})
s/maasObj_list/maasObjList/ (no underscores please)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode574
provider/maas/environ.go:574: return fmt.Errorf("Found unexpected amunt
of instances with the given id")
return fmt.Errorf("expected one result, got %d", len(maasObjList)) ?

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode580
provider/maas/environ.go:580: fmt.Print(networks)
logger.Debugf("got networks from MAAS: %v", networks) ?

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode586
provider/maas/environ.go:586: for _, i_network := range includedNetworks
{
s/i_network/network/

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode588
provider/maas/environ.go:588: fmt.Print(netmap)
logger.Debugf("got networks map from MAAS: %v", netmap)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode589
provider/maas/environ.go:589: return fmt.Errorf("Ip %q is not present in
the requested unit", i_network)
Why IP ? We're getting network names in the command and we should check
for that in maas result list, not IPs.

return fmt.Errorf("network %q not found for instance %q in MAAS",
network, instanceId)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode595
provider/maas/environ.go:595: return fmt.Errorf("Ip %q is set up in the
requested unit and can not be excluded", i_network)
Similarly, return fmt.Errorf("network %q is required for instance %q and
cannot be excluded", network, instanceId)

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ_whitebox_test.go
File provider/maas/environ_whitebox_test.go (rig...

[Dimiter Naydenov (dimitern)]

Also, the kanban card mentioned deploy and add-unit for --to, and I
can't see the changes to the latter (add --(exclude)-networks args and
check them if --to is given, otherwise return an error if networks are
specified I think). Finally, we need to make sure if the user specifies
networks for both commands, all given networks must be in
include/excludeNetworks for the service itself (i.e. you cannot
add/remove networks from the service when deploying new units, just
restrict what networks will be enabled out of the initially given
networks for the service).

https://codereview.appspot.com/84880045/

[William Reade (fwereade)]

As it stands, this does not lgtm. I think this needs to happen inside
state, based on the contents of state, at unit assignment time, and in a
txn that checks against the current service networks -- doing it that
way should make it possible to correctly handle both deploy and
add-unit.

https://codereview.appspot.com/84880045/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/84880045/diff/1/environs/interface.go#newcode170
environs/interface.go:170: ValidateNetworksForInstance(includedNetworks,
excludedNetworks []string, instance_id string) error
Not 100%sure about this direction -- I'd rather not talk to the provider
if we have the info available in state. Dimitern, what's the status of
the pipeline that sets actual networks on machines?

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

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode37
juju/deploy.go:37: func (parms *DeployServiceParams) ClearMachineSpecs()
(string, instance.ContainerType) {
Needs documentation, intent is not clear from the name IMO.

https://codereview.appspot.com/84880045/diff/1/juju/deploy.go#newcode49
juju/deploy.go:49: if containerType, err =
instance.ParseContainerType(firstPart); err == nil {
You surely didn't have the context to know this, but this is *not* a
container type -- the stuff in --to is a "placement directive", of which
the only one implemented is "lxc".

Haven't looked further; so, if this is consolidating the couple of bits
of code that already do this sort of thing, +1, because it reduces the
number of places we need to fix as we do placement directives properly;
if it's further duplication, -1.

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode85
provider/maas/environ.go:85: return nil
This needs to return an error; but I have a pretty serious problem with
the existence of this exported method in the first place. We really
shouldn't be casting Environs like this...

https://codereview.appspot.com/84880045/diff/1/provider/maas/environ.go#newcode572
provider/maas/environ.go:572: if len(maasObj_list) == 0 {
!= 1?

but... again, I don't think this should be an environ method anyway.

https://codereview.appspot.com/84880045/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/84880045/diff/1/state/apiserver/client/client.go#newcode315
state/apiserver/client/client.go:315: err =
env.ValidateNetworksForInstance(args.IncludeNetworks,
args.ExcludeNetworks, mid)
If I were happy with the environ method, I'd still rather keep this
check inside state, so we can keep ensuring db integrity as networks
become dynamic -- not directly relevant for *deploy*, I guess, but
trying to do the same job in different layers at different times should
generally be avoided ;).

https://codereview.appspot.com/84880045/

[Dimiter Naydenov (dimitern)]

https://codereview.appspot.com/84880045/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/84880045/diff/1/environs/interface.go#newcode170
environs/interface.go:170: ValidateNetworksForInstance(includedNetworks,
excludedNetworks []string, instance_id string) error
On 2014/04/07 09:29:40, fwereade wrote:
> Not 100%sure about this direction -- I'd rather not talk to the
provider if we
> have the info available in state. Dimitern, what's the status of the
pipeline
> that sets actual networks on machines?

We can set networks and NICs on machines in state, but the provisioner
does not do it yet - it will some time later today I hope.

https://codereview.appspot.com/84880045/

Unmerged revisions

2549. By Horacio Durán on 2014-04-07

Ran GoFmt on files

2548. By Horacio Durán on 2014-04-07

Fixed ValidateNetworksForInstance after testing and added tests

2547. By Horacio Durán on 2014-04-04

Finished implementing ValidateNetworksForInstance in environs, still missing tests

2546. By Horacio Durán on 2014-04-03

Added most of the required methods to check if requested methods are possible
on the requierd environ

2545. By Horacio Durán on 2014-04-03

Did a base implementtion for checking netowrk compatibility on --to

=== modified file 'cmd/juju/deploy.go'
--- cmd/juju/deploy.go	2014-04-03 04:46:51 +0000
+++ cmd/juju/deploy.go	2014-04-07 02:15:57 +0000
@@ -76,7 +76,7 @@
    juju deploy mysql --to 23       (Deploy to machine 23)
    juju deploy mysql --to 24/lxc/3 (Deploy to lxc container 3 on host machine 24)
    juju deploy mysql --to lxc:25   (Deploy to a new lxc container on host machine 25)
-   
+
    juju deploy mysql -n 5 --constraints mem=8G (deploy 5 instances of mysql with at least 8 GB of RAM each)
 
    juju deploy mysql --networks=storage,mynet --exclude-networks=logging
@@ -194,6 +194,7 @@
 		if !env.SupportNetworks() {
 			return errors.New("cannot use --networks/--exclude-networks: not supported by the environment")
 		}
+
 	}
 
 	charmInfo, err := client.CharmInfo(curl.String())

=== modified file 'environs/interface.go'
--- environs/interface.go	2014-04-03 03:37:45 +0000
+++ environs/interface.go	2014-04-07 02:15:57 +0000
@@ -153,6 +153,10 @@
 	// FwGlobal firewall mode.
 	Ports() ([]instance.Port, error)
 
+	// ValidateNetworksForInstance returns nil if the networks are
+	// compatible with the given insance or error in case they arent
+	ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error
+
 	// Provider returns the EnvironProvider that created this Environ.
 	Provider() EnvironProvider
 

=== modified file 'juju/deploy.go'
--- juju/deploy.go	2014-04-03 09:10:10 +0000
+++ juju/deploy.go	2014-04-07 02:15:57 +0000
@@ -34,6 +34,27 @@
 	ExcludeNetworks []string
 }
 
+func (parms *DeployServiceParams) ClearMachineSpecs() (string, instance.ContainerType) {
+	// TODO: Add this to AddUnit and replace wherever required
+	// machineIdSpec may be an existing machine or container, eg 3/lxc/2
+	// or a new container on a machine, eg lxc:3
+
+	mid := parms.ToMachineSpec
+	var containerType instance.ContainerType
+	specParts := strings.SplitN(mid, ":", 2)
+	if len(specParts) > 1 {
+		firstPart := specParts[0]
+		var err error
+		// If the type is present in the first part, the seccond is the id
+		if containerType, err = instance.ParseContainerType(firstPart); err == nil {
+			mid = specParts[1]
+		} else {
+			mid = parms.ToMachineSpec
+		}
+	}
+	return mid, containerType
+}
+
 // DeployService takes a charm and various parameters and deploys it.
 func DeployService(st *state.State, args DeployServiceParams) (*state.Service, error) {
 	if args.NumUnits > 1 && args.ToMachineSpec != "" {

=== modified file 'provider/azure/environ.go'
--- provider/azure/environ.go	2014-04-03 09:07:57 +0000
+++ provider/azure/environ.go	2014-04-07 02:15:57 +0000
@@ -387,6 +387,11 @@
 	return env.supportedArchitectures, err
 }
 
+func (e *azureEnviron) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// If SupportNetworks is true, this should be implemented
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (env *azureEnviron) SupportNetworks() bool {
 	return false

=== modified file 'provider/dummy/environs.go'
--- provider/dummy/environs.go	2014-04-04 16:52:41 +0000
+++ provider/dummy/environs.go	2014-04-07 02:15:57 +0000
@@ -536,6 +536,11 @@
 	return []string{arch.AMD64, arch.PPC64}, nil
 }
 
+func (e *environ) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// TODO: Actually check if networks are compatible with the instace
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (*environ) SupportNetworks() bool {
 	return true

=== modified file 'provider/ec2/ec2.go'
--- provider/ec2/ec2.go	2014-04-03 04:46:51 +0000
+++ provider/ec2/ec2.go	2014-04-07 02:15:57 +0000
@@ -350,6 +350,11 @@
 	return e.supportedArchitectures, err
 }
 
+func (e *environ) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// If SupportNetworks is true, this should be implemented
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (e *environ) SupportNetworks() bool {
 	// TODO(dimitern) Once we have support for VPCs and advanced

=== modified file 'provider/joyent/environ.go'
--- provider/joyent/environ.go	2014-04-03 20:05:38 +0000
+++ provider/joyent/environ.go	2014-04-07 02:15:57 +0000
@@ -96,6 +96,11 @@
 	return env.supportedArchitectures, err
 }
 
+func (e *joyentEnviron) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// If SupportNetworks is true, this should be implemented
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (e *joyentEnviron) SupportNetworks() bool {
 	return false

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-04-03 03:37:45 +0000
+++ provider/local/environ.go	2014-04-07 02:15:57 +0000
@@ -81,6 +81,11 @@
 	return []string{localArch}, nil
 }
 
+func (e *localEnviron) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// If SupportNetworks is true, this should be implemented
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (*localEnviron) SupportNetworks() bool {
 	return false

=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2014-04-03 03:37:45 +0000
+++ provider/maas/environ.go	2014-04-07 02:15:57 +0000
@@ -81,6 +81,15 @@
 	return env, nil
 }
 
+func GetFromEnvironInterface(interfaceEnv environs.Environ) *maasEnviron {
+	maasEnv, ok := interfaceEnv.(*maasEnviron)
+
+	if !ok {
+		return nil
+	}
+	return maasEnv
+}
+
 // Name is specified in the Environ interface.
 func (env *maasEnviron) Name() string {
 	return env.name
@@ -558,7 +567,41 @@
 	Description string
 }
 
-// GetNetworksList returns a list of strings which contain networks for a gien maas node instance.
+func (e *maasEnviron) ValidateNetworksForInstance(includedNetworks, ExcludeNetworks []string, instance_id string) error {
+	maasObj_list, err := e.instances([]instance.Id{instance.Id(instance_id)})
+	if err != nil {
+		return err
+	}
+	if len(maasObj_list) == 0 {
+		//XXX: I am not sure this can actually happen
+		return fmt.Errorf("Found unexpected amunt of instances with the given id")
+	}
+	maasObj := maasObj_list[0]
+
+	networks, _ := e.GetNetworksList(maasObj)
+	netmap := make(map[string]string)
+	fmt.Print(networks)
+
+	for _, value := range networks {
+		netmap[value.Ip] = ""
+	}
+
+	for _, i_network := range includedNetworks {
+		if _, ok := netmap[i_network]; !ok {
+			fmt.Print(netmap)
+			return fmt.Errorf("Ip %q is not present in the requested unit", i_network)
+		}
+	}
+
+	for _, i_network := range ExcludeNetworks {
+		if _, ok := netmap[i_network]; ok {
+			return fmt.Errorf("Ip %q is set up in the requested unit and can not be excluded", i_network)
+		}
+	}
+	return nil
+}
+
+// GetNetworksList returns a list of strings which contain networks for a given maas node instance.
 func (e *maasEnviron) GetNetworksList(inst instance.Instance) ([]MAASNetworkDetails, error) {
 	maasInst := inst.(*maasInstance)
 	maasObj := maasInst.maasObject

=== modified file 'provider/maas/environ_whitebox_test.go'
--- provider/maas/environ_whitebox_test.go	2014-04-02 07:50:15 +0000
+++ provider/maas/environ_whitebox_test.go	2014-04-07 02:15:57 +0000
@@ -542,3 +542,24 @@
 	// once gomaasapi testing server supports networks.
 	c.Assert(env.SupportNetworks(), jc.IsFalse)
 }
+
+func (suite *environSuite) TestValidateNetworksForNode(c *gc.C) {
+	test_environ := suite.makeEnviron()
+	suite.getNetwork("test_network")
+	suite.getInstance("instance_for_network")
+	suite.testMAASObject.TestServer.ConnectNodeToNetwork("instance_for_network", "test_network")
+
+	err := test_environ.ValidateNetworksForInstance([]string{"127.0.0.1"}, []string{}, "instance_for_network")
+	c.Assert(err, gc.IsNil)
+
+	err = test_environ.ValidateNetworksForInstance([]string{}, []string{"127.0.0.2"}, "instance_for_network")
+	c.Assert(err, gc.IsNil)
+
+	expected_err := fmt.Errorf("Ip %q is set up in the requested unit and can not be excluded", "127.0.0.1")
+	err = test_environ.ValidateNetworksForInstance([]string{}, []string{"127.0.0.1"}, "instance_for_network")
+	c.Check(err, gc.DeepEquals, expected_err)
+
+	expected_err = fmt.Errorf("Ip %q is not present in the requested unit", "127.0.0.2")
+	err = test_environ.ValidateNetworksForInstance([]string{"127.0.0.2"}, []string{}, "instance_for_network")
+	c.Check(err, gc.DeepEquals, expected_err)
+}

=== modified file 'provider/manual/environ.go'
--- provider/manual/environ.go	2014-04-03 04:03:00 +0000
+++ provider/manual/environ.go	2014-04-07 02:15:57 +0000
@@ -96,6 +96,11 @@
 	return arch.AllSupportedArches, nil
 }
 
+func (e *manualEnviron) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// If SupportNetworks is true, this should be implemented
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (e *manualEnviron) SupportNetworks() bool {
 	return false

=== modified file 'provider/openstack/provider.go'
--- provider/openstack/provider.go	2014-04-03 04:46:51 +0000
+++ provider/openstack/provider.go	2014-04-07 02:15:57 +0000
@@ -515,6 +515,11 @@
 	return e.supportedArchitectures, err
 }
 
+func (e *environ) ValidateNetworksForInstance(includedNetworks, excludedNetworks []string, instance_id string) error {
+	// If SupportNetworks is true, this should be implemented
+	return nil
+}
+
 // SupportNetworks is specified on the EnvironCapability interface.
 func (e *environ) SupportNetworks() bool {
 	// TODO(dimitern) Once we have support for networking, inquire

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-04-04 10:00:45 +0000
+++ state/apiserver/client/client.go	2014-04-07 02:15:57 +0000
@@ -298,6 +298,26 @@
 // allows specifying networks to include or exclude on the machine
 // where the charm gets deployed.
 func (c *Client) ServiceDeployWithNetworks(args params.ServiceDeploy) error {
+	// XXX Get the actual node
+	parms := juju.DeployServiceParams{
+		ToMachineSpec:   args.ToMachineSpec,
+		IncludeNetworks: args.IncludeNetworks,
+		ExcludeNetworks: args.ExcludeNetworks}
+
+	mid, _ := parms.ClearMachineSpecs()
+
+	conn, err := juju.NewConnFromState(c.api.state)
+	env := conn.Environ
+	if env == nil {
+		return err
+	}
+
+	err = env.ValidateNetworksForInstance(args.IncludeNetworks, args.ExcludeNetworks, mid)
+
+	if err != nil {
+		return err
+	}
+
 	return c.ServiceDeploy(args)
 }