Merge lp:~natefinch/juju-core/005-azure-address into lp:~go-bot/juju-core/trunk

[Nate Finch]

azure addressability changes

Implement Addresses()

https://codereview.appspot.com/13082044/

azure addressability changes

Implement Addresses()

https://codereview.appspot.com/13082044/

[Nate Finch (natefinch)]

Reviewers: mp+181117_code.launchpad.net,

Message:
Please take a look.

Description:
azure addressability changes

Implement Addresses()

https://code.launchpad.net/~natefinch/juju-core/005-azure-address/+merge/181117

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/azure/instance.go
   M environs/azure/instance_test.go
   M testing/checkers/checker.go
   M testing/checkers/checker_test.go

[John A Meinel (jameinel)]

the code and tests for Addresses looks sane to me, though it might be
good to have someone on Red squad confirm the actual APIs you are
calling.

I have some questions about the Checker you implemented. The only bit
I'm genuinely worried about is mutating slices passed in by the caller.
If you add a test to reassure me we aren't changing them, the rest is
all just suggestions.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go
File testing/checkers/checker.go (right):

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go#newcode157
testing/checkers/checker.go:157: // use it on very large slices
Is it hard to create a smart implementation that notices if the types
are hashable and can then just insert them into a map with counts and
then compare it?

counts := make(map[TYPE]int)
for i, v := range expected:
   counts[v] += 1
countsObtained := make(map[TYPE]int)
for i, v := range obtained:
   countsObtained[v] += 1

Obviously you need a bit more reflect magic than I've typed here, but
you also end up with nice summaries of the contents and what is
different. (You could even do it as a delta map and count up with
expected, and down with obtained.)

If it is "cheap" to determine if something is hashable, I think the
algorithm to count them is nice and short and reasonably obvious. And
probably most things we're doing are strings, structs, and ints anyway.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go#newcode198
testing/checkers/checker.go:198: // that way we make sure the count of
duplicate items is the same
I'm a little concerned if removing them would mutate the caller's
slices. (You might compare to make sure they match, and then do other
things with the slice.)

Can you confirm that it doesn't?

Even better, add it as a test case so we ensure that we don't grow that
behavior accidentally in the future.

Another option that avoids rewriting the array N times would be to just
allocate a 'bool' array of the same length, and flag each entry with
True once you've seen it. When you iterate later, you can just check and
'continue' if that entry has already been seen.

You still end up with N^2 comparisons, but you have a lot fewer
reallocations.

My theoretical mind wonders if optimizing for the case of "lists are
identical" is worthwhile and starting the nested search at the offset of
the outer search. Or just doing one pass against all items until we have
something that doesn't match, and only then switch to the N^2 form.
Certainly not terribly important here.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker_test.go
File testing/checkers/checker_test.go (right):

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker_test.go#newcode37
testing/checkers/checker_test.go:37: func (s *CheckerSuite)
TestSameContents(c *gc.C) {
I personally would rather see these split out as separate "Test*" cases,
rather than just commented sections. So a new suite for SameContents and
then test cases in that.

func (s *SameContentsSuite) TestSame(c *gc.C) {
   c.Check([]int(1, 2, 3), jc.SameContents, []int(1,2,3))
}

func (s *SameContentsSuite) TestE...

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/13082044/

[Nate Finch (natefinch)]

https://codereview.appspot.com/13082044/

[Nate Finch (natefinch)]

Somehow my last message contents got dropped on the floor... not sure
how that happened.

The checker stuff was moved into a different branch, and the diffs now
reflect that (and I had implemented the changes john mentioned there).

This code was looked at by allenap, since I was having difficulty
figuring out how to mock out the server responses for the tests, so I'm
pretty sure they're good to go.

https://codereview.appspot.com/13082044/

[John A Meinel (jameinel)]

LGTM as long as it has been tested live.

https://codereview.appspot.com/13082044/

[John A Meinel (jameinel)]

On 2013/08/23 13:33:18, jameinel wrote:
> LGTM as long as it has been tested live.

You commented that this has been reviewed by others, but then didn't
land this. Is it blocked on something else?

https://codereview.appspot.com/13082044/

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/13082044/

[Martin Packman (gz)]

LGTM.

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance.go#newcode64
provider/azure/instance.go:64: err = azInstance.apiCall(false, func(c
*azureManagementContext) error {
I find this closure pretty hard to read, what with it being most of the
outer function and has differing return spelling.

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance_test.go
File provider/azure/instance_test.go (right):

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance_test.go#newcode174
provider/azure/instance_test.go:174: instance.Address{
You don't actually need to repeat the type here.

https://codereview.appspot.com/13082044/

[Go Bot (go-bot)]

The attempt to merge lp:~natefinch/juju-core/005-azure-address into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.764s
ok launchpad.net/juju-core/agent/tools 0.240s
ok launchpad.net/juju-core/bzr 7.547s
ok launchpad.net/juju-core/cert 2.411s
ok launchpad.net/juju-core/charm 0.551s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.021s
ok launchpad.net/juju-core/cmd 0.201s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 120.106s
ok launchpad.net/juju-core/cmd/jujud 37.565s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 1.570s
ok launchpad.net/juju-core/constraints 0.029s
ok launchpad.net/juju-core/container/lxc 0.365s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.298s
ok launchpad.net/juju-core/environs 1.228s
ok launchpad.net/juju-core/environs/bootstrap 6.613s
ok launchpad.net/juju-core/environs/cloudinit 0.438s
ok launchpad.net/juju-core/environs/config 0.708s
? launchpad.net/juju-core/environs/filestorage [no test files]
ok launchpad.net/juju-core/environs/imagemetadata 0.377s
ok launchpad.net/juju-core/environs/instances 0.221s
ok launchpad.net/juju-core/environs/jujutest 0.254s
ok launchpad.net/juju-core/environs/localstorage 0.246s
ok launchpad.net/juju-core/environs/manual 1.039s
ok launchpad.net/juju-core/environs/simplestreams 0.255s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sync 0.247s
ok launchpad.net/juju-core/environs/testing 0.010s
ok launchpad.net/juju-core/environs/tools 27.439s
? launchpad.net/juju-core/errors [no test files]
ok launchpad.net/juju-core/instance 0.019s
ok launchpad.net/juju-core/juju 14.954s
ok launchpad.net/juju-core/juju/osenv 0.247s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.017s
ok launchpad.net/juju-core/names 0.031s
ok launchpad.net/juju-core/provider 0.250s
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 4.455s
ok launchpad.net/juju-core/provider/dummy 17.648s
ok launchpad.net/juju-core/provider/ec2 4.427s
ok launchpad.net/juju-core/provider/local 1.301s
? launchpad.net/juju-core/provider/local/storage [no test files]
ok launchpad.net/juju-core/provider/maas 2.115s
ok launchpad.net/juju-core/provider/openstack 3.982s
ok launchpad.net/juju-core/rpc 0.258s
ok launchpad.net/juju-core/rpc/jsoncodec 0.259s
ok launchpad.net/juju-core/schema 0.018s

----------------------------------------------------------------------
FAIL: relationunit_test.go:555: RelationUnitSuite.TestContainerWatchScope

[LOG] 93.14353 INFO juju.state opening state; mongo addresses: ["localhost:51340"]; entity ""
[LOG] 93.14798 INFO juju.state connection established
[LOG] 93.16266 INFO juju.state initializing environme...

=== modified file 'provider/azure/instance.go'
--- provider/azure/instance.go	2013-08-27 18:33:36 +0000
+++ provider/azure/instance.go	2013-09-04 15:14:28 +0000
@@ -38,8 +38,54 @@
 
 // Addresses is specified in the Instance interface.
 func (azInstance *azureInstance) Addresses() ([]instance.Address, error) {
-	logger.Errorf("azureInstance.Addresses not implemented")
-	return nil, nil
+	addrs := []instance.Address{}
+	ip, netname, err := azInstance.netInfo()
+	if err != nil {
+		return nil, err
+	}
+	if ip != "" {
+		addrs = append(addrs, instance.Address{
+			ip,
+			instance.Ipv4Address,
+			netname,
+			instance.NetworkCloudLocal})
+	}
+
+	name, err := azInstance.DNSName()
+	if err != nil {
+		return nil, err
+	}
+	host := instance.Address{name, instance.HostName, "", instance.NetworkPublic}
+	addrs = append(addrs, host)
+	return addrs, nil
+}
+
+func (azInstance *azureInstance) netInfo() (ip, netname string, err error) {
+	err = azInstance.apiCall(false, func(c *azureManagementContext) error {
+		d, err := c.GetDeployment(&gwacl.GetDeploymentRequest{
+			ServiceName:    azInstance.ServiceName,
+			DeploymentName: azInstance.ServiceName,
+		})
+		if err != nil {
+			return err
+		}
+		switch len(d.RoleInstanceList) {
+		case 0:
+			// nothing to do, this can happen if the instances aren't finished deploying
+			return nil
+		case 1:
+			// success
+			ip = d.RoleInstanceList[0].IPAddress
+			netname = d.VirtualNetworkName
+			return nil
+		default:
+			return fmt.Errorf("Too many instances, expected one, got %d", len(d.RoleInstanceList))
+		}
+	})
+	if err != nil {
+		return "", "", err
+	}
+	return ip, netname, nil
 }
 
 // DNSName is specified in the Instance interface.
@@ -60,6 +106,14 @@
 
 // OpenPorts is specified in the Instance interface.
 func (azInstance *azureInstance) OpenPorts(machineId string, ports []instance.Port) error {
+	return azInstance.apiCall(true, func(context *azureManagementContext) error {
+		return azInstance.openEndpoints(context, ports)
+	})
+}
+
+// apiCall wraps a call to the azure API to ensure it is properly disposed, optionally locking
+// the environment
+func (azInstance *azureInstance) apiCall(lock bool, f func(*azureManagementContext) error) error {
 	env := azInstance.environ
 
 	context, err := env.getManagementAPI()
@@ -68,10 +122,11 @@
 	}
 	defer env.releaseManagementAPI(context)
 
-	env.Lock()
-	defer env.Unlock()
-
-	return azInstance.openEndpoints(context, ports)
+	if lock {
+		env.Lock()
+		defer env.Unlock()
+	}
+	return f(context)
 }
 
 // openEndpoints opens the endpoints in the Azure deployment. The caller is
@@ -113,18 +168,9 @@
 
 // ClosePorts is specified in the Instance interface.
 func (azInstance *azureInstance) ClosePorts(machineId string, ports []instance.Port) error {
-	env := azInstance.environ
-
-	context, err := env.getManagementAPI()
-	if err != nil {
-		return err
-	}
-	defer env.releaseManagementAPI(context)
-
-	env.Lock()
-	defer env.Unlock()
-
-	return azInstance.closeEndpoints(context, ports)
+	return azInstance.apiCall(true, func(context *azureManagementContext) error {
+		return azInstance.closeEndpoints(context, ports)
+	})
 }
 
 // closeEndpoints closes the endpoints in the Azure deployment. The caller is
@@ -185,20 +231,15 @@
 }
 
 // Ports is specified in the Instance interface.
-func (azInstance *azureInstance) Ports(machineId string) ([]instance.Port, error) {
-	env := azInstance.environ
-	context, err := env.getManagementAPI()
-	if err != nil {
-		return nil, err
-	}
-	defer env.releaseManagementAPI(context)
-
-	ports, err := azInstance.listPorts(context)
-	if err != nil {
-		return nil, err
-	}
-	state.SortPorts(ports)
-	return ports, nil
+func (azInstance *azureInstance) Ports(machineId string) (ports []instance.Port, err error) {
+	err = azInstance.apiCall(false, func(context *azureManagementContext) error {
+		ports, err = azInstance.listPorts(context)
+		return err
+	})
+	if ports != nil {
+		state.SortPorts(ports)
+	}
+	return ports, err
 }
 
 // listPorts returns the slice of ports (instance.Port) that this machine

=== modified file 'provider/azure/instance_test.go'
--- provider/azure/instance_test.go	2013-08-09 15:13:09 +0000
+++ provider/azure/instance_test.go	2013-09-04 15:14:28 +0000
@@ -5,13 +5,14 @@
 
 import (
 	"encoding/base64"
+	"fmt"
+	"net/http"
 
 	gc "launchpad.net/gocheck"
 	"launchpad.net/gwacl"
 
-	"fmt"
 	"launchpad.net/juju-core/instance"
-	"net/http"
+	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type instanceSuite struct{}
@@ -95,6 +96,14 @@
 	return []byte(xml)
 }
 
+func prepareDeploymentInfoResponse(
+	c *gc.C, dep gwacl.Deployment) []gwacl.DispatcherResponse {
+	return []gwacl.DispatcherResponse{
+		gwacl.NewDispatcherResponse(
+			serialize(c, &dep), http.StatusOK, nil),
+	}
+}
+
 func preparePortChangeConversation(
 	c *gc.C, service *gwacl.HostedServiceDescriptor,
 	deployments ...gwacl.Deployment) []gwacl.DispatcherResponse {
@@ -146,6 +155,42 @@
 	}
 }
 
+func (*instanceSuite) TestAddresses(c *gc.C) {
+	name := "service-name"
+	vnn := "Virt Net Name"
+	service := makeHostedServiceDescriptor(name)
+	responses := prepareDeploymentInfoResponse(c,
+		gwacl.Deployment{
+			RoleInstanceList: []gwacl.RoleInstance{
+				gwacl.RoleInstance{IPAddress: "1.2.3.4"},
+			},
+			VirtualNetworkName: vnn,
+		})
+
+	gwacl.PatchManagementAPIResponses(responses)
+	inst := azureInstance{*service, makeEnviron(c)}
+
+	expected := []instance.Address{
+		instance.Address{
+			"1.2.3.4",
+			instance.Ipv4Address,
+			vnn,
+			instance.NetworkCloudLocal,
+		},
+		instance.Address{
+			name + "." + AZURE_DOMAIN_NAME,
+			instance.HostName,
+			"",
+			instance.NetworkPublic,
+		},
+	}
+
+	addrs, err := inst.Addresses()
+	c.Check(err, gc.IsNil)
+
+	c.Check(addrs, jc.SameContents, expected)
+}
+
 func (*instanceSuite) TestOpenPorts(c *gc.C) {
 	service := makeHostedServiceDescriptor("service-name")
 	responses := preparePortChangeConversation(c, service,