Add Instance.Addresses() and use for openstack

Add Instance.Addresses() and use for openstack

Changes the Instance interface to require a new Addresses()
method. This will be used to present cloud-specific address
details in the generic fashion, which a future branch will
record in state.

The code includes the required implementation for Openstack,
maintaining support for the current quirks but shifting some
of the general logic into the shared instance/address.go file.

Currently all calls still go back through the cloud api, but
when the results are recorded in state and can be updated by
the state server, it will be possible make users of DNSName and
WaitDNSName consumers of machineDoc only.

https://code.launchpad.net/~gz/juju-core/openstack_addresses/+merge/175714

Requires: https://code.launchpad.net/~gz/juju-core/instance_address/+merge/175713

(do not edit description out of merge proposal)

[gz, 2013-07-19 01:31:49]

Please take a look.

[thumper, 2013-07-19 02:27:22]

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

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:245: addrs = server.Addresses
Should you be saving this as "inst.ServerDetail.Addresses" too?

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:263: //                it may be significant, see
lp:1188126 for instance.
Can we not deal with a list of tuples then? Instead of a map.

https://codereview.appspot.com/11548044/diff/1/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/11548044/diff/1/instance/address.go#newcode73
instance/address.go:73: mostpublic = addr.Value
Is the last address the most public?

[dimitern, 2013-07-19 06:10:50]

Overall LGTM, with a few suggestions.

https://codereview.appspot.com/11548044/diff/1/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/container/lxc/instance.go#newc...
container/lxc/instance.go:25: return nil, nil
return nil, fmt.Errorf("not implemented") or something?

https://codereview.appspot.com/11548044/diff/1/environs/azure/instance.go
File environs/azure/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/azure/instance.go#new...
environs/azure/instance.go:32: return nil, nil
ditto

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

https://codereview.appspot.com/11548044/diff/1/environs/dummy/environs.go#new...
environs/dummy/environs.go:742: return nil, nil
ditto

https://codereview.appspot.com/11548044/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/ec2/ec2.go#newcode86
environs/ec2/ec2.go:86: return nil, nil
ditto

https://codereview.appspot.com/11548044/diff/1/environs/local/instance.go
File environs/local/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/local/instance.go#new...
environs/local/instance.go:27: return nil, nil
ditto

https://codereview.appspot.com/11548044/diff/1/environs/maas/instance.go
File environs/maas/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/maas/instance.go#newc...
environs/maas/instance.go:39: return nil, nil
ditto

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

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:262: // GZ 2013-07-18: Really want to have an
ordering on nova addresses as
Please cut the leading whitespace in the comment. Also perhaps format it like
TODO(gz): ...

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:267: // GZ 2013-07-18: For canonistack and
hpcloud, public floating
Here as well.

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:280: machineAddr :=
instance.Address{address.Address,
IMO it will look better like this maybe:
machineAddr := instance.Address{
    address.Address,
    addrtype,
    network,
    networkscope,
}

[dimitern, 2013-07-19 06:10:50]

Overall LGTM, with a few suggestions.

https://codereview.appspot.com/11548044/diff/1/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/container/lxc/instance.go#newc...
container/lxc/instance.go:25: return nil, nil
return nil, fmt.Errorf("not implemented") or something?

https://codereview.appspot.com/11548044/diff/1/environs/azure/instance.go
File environs/azure/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/azure/instance.go#new...
environs/azure/instance.go:32: return nil, nil
ditto

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

https://codereview.appspot.com/11548044/diff/1/environs/dummy/environs.go#new...
environs/dummy/environs.go:742: return nil, nil
ditto

https://codereview.appspot.com/11548044/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/ec2/ec2.go#newcode86
environs/ec2/ec2.go:86: return nil, nil
ditto

https://codereview.appspot.com/11548044/diff/1/environs/local/instance.go
File environs/local/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/local/instance.go#new...
environs/local/instance.go:27: return nil, nil
ditto

https://codereview.appspot.com/11548044/diff/1/environs/maas/instance.go
File environs/maas/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/environs/maas/instance.go#newc...
environs/maas/instance.go:39: return nil, nil
ditto

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

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:262: // GZ 2013-07-18: Really want to have an
ordering on nova addresses as
Please cut the leading whitespace in the comment. Also perhaps format it like
TODO(gz): ...

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:267: // GZ 2013-07-18: For canonistack and
hpcloud, public floating
Here as well.

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:280: machineAddr :=
instance.Address{address.Address,
IMO it will look better like this maybe:
machineAddr := instance.Address{
    address.Address,
    addrtype,
    network,
    networkscope,
}

[gz, 2013-07-19 17:53:34]

Please take a look.

https://codereview.appspot.com/11548044/diff/1/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/11548044/diff/1/container/lxc/instance.go#newc...
container/lxc/instance.go:25: return nil, nil
On 2013/07/19 06:10:50, dimitern wrote:
> return nil, fmt.Errorf("not implemented") or something?

I wanted the option of leaving some of these to be implemented by other people,
without actually causing errors. In practice it probably won't matter, but no
addresses and no error is a more reasonable default than blowing everything up.
See for example the DNSName impl. below that just says there's no name, rather
than giving an error the caller couldn't handle.

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

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:245: addrs = server.Addresses
On 2013/07/19 02:27:22, thumper wrote:
> Should you be saving this as "inst.ServerDetail.Addresses" too?

I wavered a little on this. In practice, it won't matter as the Instance type is
not really persistent in any context. But arguably the detail should be replaced
wholesale if we just requeried the server.

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:262: // GZ 2013-07-18: Really want to have an
ordering on nova addresses as
On 2013/07/19 06:10:50, dimitern wrote:
> Please cut the leading whitespace in the comment. Also perhaps format it like
> TODO(gz): ...

Will adapt.

https://codereview.appspot.com/11548044/diff/1/environs/openstack/provider.go...
environs/openstack/provider.go:263: //                it may be significant, see
lp:1188126 for instance.
On 2013/07/19 02:27:22, thumper wrote:
> Can we not deal with a list of tuples then? Instead of a map.

Yes, but that also needs a goose change, and I'm not certain if the json parsing
we use actually support that. I think living with networks being unordered and
getting the ordering preference from elsewhere may be as effective.

Will update the comment a little.

https://codereview.appspot.com/11548044/diff/1/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/11548044/diff/1/instance/address.go#newcode73
instance/address.go:73: mostpublic = addr.Value
On 2013/07/19 02:27:22, thumper wrote:
> Is the last address the most public?

It's a bit of a cheat for now, but in the context of a public address being
added after machine creation, it goes to the end of the list, even if it's not
marked public. So, this logic makes the existing tests for
will-this-code-work-on-hp-and-canonistack continue to pass. However, a better
answer is peeking into the addresses themselves, which can happen in a following
branch.

[rog, 2013-07-23 11:25:54]

LGTM with some trivial suggestion.
i now see dimiter also suggested returning an error.
feel free to follow your instinct there.

https://codereview.appspot.com/11548044/diff/14001/container/lxc/instance.go
File container/lxc/instance.go (right):

https://codereview.appspot.com/11548044/diff/14001/container/lxc/instance.go#...
container/lxc/instance.go:24: logger.Errorf("lxcInstance.Addresses not
implemented")
or return an error since we can?

https://codereview.appspot.com/11548044/diff/14001/environs/azure/instance.go
File environs/azure/instance.go (right):

https://codereview.appspot.com/11548044/diff/14001/environs/azure/instance.go...
environs/azure/instance.go:31: logger.Errorf("azureInstance.Addresses not
implemented")
ditto (and the rest)

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/export_...
File environs/openstack/export_test.go (left):

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/export_...
environs/openstack/export_test.go:85: func InstanceAddress(addresses
map[string][]nova.IPAddress) (string, error) {
always good to lose an error return.

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/provide...
File environs/openstack/provider.go (right):

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/provide...
environs/openstack/provider.go:239: addrs := inst.ServerDetail.Addresses
if len(addrs) > 0 {
    return addrs, nil
}
then lose a level of indentation in the rest?

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/provide...
environs/openstack/provider.go:280: machineAddr :=
instance.Address{address.Address,
i'd prefer to see a tagged initialisation literal here

https://codereview.appspot.com/11548044/diff/14001/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/11548044/diff/14001/instance/address.go#newcode68
instance/address.go:68: if addr.Type != Ipv6Address {
if addr.Type == IPv6Address {
    continue
}
?

[gz, 2013-07-24 12:11:11]

Please take a look.

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/provide...
File environs/openstack/provider.go (right):

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/provide...
environs/openstack/provider.go:239: addrs := inst.ServerDetail.Addresses
On 2013/07/23 11:25:54, rog wrote:
> if len(addrs) > 0 {
>     return addrs, nil
> }
> then lose a level of indentation in the rest?

Hm, just tweaked that, but then I deunify the success return statement...

https://codereview.appspot.com/11548044/diff/14001/environs/openstack/provide...
environs/openstack/provider.go:280: machineAddr :=
instance.Address{address.Address,
On 2013/07/23 11:25:54, rog wrote:
> i'd prefer to see a tagged initialisation literal here

Seems fair, should be a NewSomething eventually I think.

https://codereview.appspot.com/11548044/diff/14001/instance/address.go
File instance/address.go (right):

https://codereview.appspot.com/11548044/diff/14001/instance/address.go#newcode68
instance/address.go:68: if addr.Type != Ipv6Address {
On 2013/07/23 11:25:54, rog wrote:
> if addr.Type == IPv6Address {
>     continue
> }
> ?

I generally prefer conditional bodies unless using break anyway, to limit the
gotoness.