nova: Addresses in ServerDetail

nova: Addresses in ServerDetail

Implemented handling of public/private addresses map inside ServerDetail.
Added tests in the double to check RunServer sets the appropriate addresses.
We need this in juju-core to get the IP address of an instance.

https://code.launchpad.net/~dimitern/goose/nova-addresses2/+merge/145321

(do not edit description out of merge proposal)

[dimitern, 2013-01-29 08:22:22]

Please take a look.

[wallyworld, 2013-01-29 10:22:34]

LGTM. Perhaps we should add a few extra checks to TestListServersDetail. There's
pattern matches for Created, Updated etc. Ideally we would have checks for the
IP addresses also.

Is it possible to use some more realistic fake IPV6 addresses?

https://codereview.appspot.com/7228058/diff/1/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7228058/diff/1/nova/nova.go#newcode159
nova/nova.go:159: Addresses   map[string][]IPAddress
Perhaps a comment explaining why we have AddressIPv4, AddressIPv6 and Addresses,
each of which record IP addresses.

[rog, 2013-01-29 10:38:32]

LGTM modulo the below remarks.

https://codereview.appspot.com/7228058/diff/1/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7228058/diff/1/nova/nova.go#newcode159
nova/nova.go:159: Addresses   map[string][]IPAddress
On 2013/01/29 10:22:34, wallyworld wrote:
> Perhaps a comment explaining why we have AddressIPv4, AddressIPv6 and
Addresses,
> each of which record IP addresses.

+1
i'd also like to see something saying what the key of the map represents.

comments on some of the other fields would be nice too. what should we expect to
see in the "Created" field, for example?

it would be great if we could make the goose APIs usable without needing
detailed knowledge of goose. FWIW goamz succeeds in that respect by having good
doc comments and links to relevant API documentation.

there's a doc link at the top of the page, but it points to a very large body of
information. how about adding links in doc comments that point to more specific
pages? (presumably you've got the information from there, so it shouldn't be too
much problem).

[rog, 2013-01-29 10:38:32]

LGTM modulo the below remarks.

[dimitern, 2013-01-29 14:35:29]

Please take a look.

https://codereview.appspot.com/7228058/diff/1/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7228058/diff/1/nova/nova.go#newcode159
nova/nova.go:159: Addresses   map[string][]IPAddress
On 2013/01/29 10:22:34, wallyworld wrote:
> Perhaps a comment explaining why we have AddressIPv4, AddressIPv6 and
Addresses,
> each of which record IP addresses.

Done.

https://codereview.appspot.com/7228058/diff/1/nova/nova.go#newcode159
nova/nova.go:159: Addresses   map[string][]IPAddress
On 2013/01/29 10:38:32, rog wrote:
> On 2013/01/29 10:22:34, wallyworld wrote:
> > Perhaps a comment explaining why we have AddressIPv4, AddressIPv6 and
> Addresses,
> > each of which record IP addresses.
> 
> +1
> i'd also like to see something saying what the key of the map represents.
> 
> comments on some of the other fields would be nice too. what should we expect
to
> see in the "Created" field, for example?
> 
> it would be great if we could make the goose APIs usable without needing
> detailed knowledge of goose. FWIW goamz succeeds in that respect by having
good
> doc comments and links to relevant API documentation.
> 
> there's a doc link at the top of the page, but it points to a very large body
of
> information. how about adding links in doc comments that point to more
specific
> pages? (presumably you've got the information from there, so it shouldn't be
too
> much problem).
Ok, I added more links and comments.

[dimitern, 2013-01-29 14:38:08]

Done, most of the things. However:
- Try as I might I couldn't find any IPv6 addresses that are guaranteed safe for
testing purposes. Anyway I don't think this is a big deal anyway - it never
connects anywhere.
- TestListServersDetail cannot check reliably Addresses, because it can be
missing.

[gz, 2013-01-29 15:36:12]

LGTM

https://codereview.appspot.com/7228058/diff/7/testservices/novaservice/servic...
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7228058/diff/7/testservices/novaservice/servic...
testservices/novaservice/service_http.go:582: server.Addresses["private"] =
[]nova.IPAddress{{4, addr}, {6, "::face::000f"}}
As mentioned, make these ipv6 addresses valid and semi-sane. You could just use
the loopback ::1 for both, or use similar-style local addresses to the ipv4
ones, see:

<http://en.wikipedia.org/wiki/IPv6_address#Special_addresses>

[dimitern, 2013-01-29 16:06:54]

*** Submitted:

nova: Addresses in ServerDetail

Implemented handling of public/private addresses map inside ServerDetail.
Added tests in the double to check RunServer sets the appropriate addresses.
We need this in juju-core to get the IP address of an instance.

R=wallyworld, rog, gz
CC=
https://codereview.appspot.com/7228058

[rog, 2013-01-29 16:07:59]

that's really useful, thanks. i think having this
kind of documentation in the API really adds
to its value.

a few minor suggestions below, but still LGTM regardless.

https://codereview.appspot.com/7228058/diff/7/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode158
nova/nova.go:158: // First public IP address (if floating IP is assigned,
otherwise "")
i like to see whole-sentence comments for fields, similar to functions although
YMMV.

// AddressIPv4 and AddressIP6 hold the first public
// IPv4 and IPv6 addresses of the server, or ""
// if no floating IP is assigned.
?

https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode162
nova/nova.go:162: // Optional list of all IP addresses assigned to this
// Addresses holds the list of all IP addresses assigned
// to this server, grouped by "network" name ("public",
// "private" or a custom name).

?

https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode167
nova/nova.go:167: Created  string // creation timestamp
for the record, this should probably be a time.Time, but for the time being:

// Created holds the creation timestamp of the
// server in RFC3339 format.

?

https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode174
nova/nova.go:174: Progress int    // completion % of current operation
// Progress holds the completion percentage of the
// current operation.
?

https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode175
nova/nova.go:175: Status   string // One of the Status* constants
// Status holds the current status of the server,
// one of the Status* constants.

?

https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode177
nova/nova.go:177: Updated  string // timestamp of last update
// Updated holds the timestamp of the last update to
// the server in RFC3339 format.
?

[rog, 2013-01-29 16:08:23]

On 2013/01/29 16:07:59, rog wrote:
> that's really useful, thanks. i think having this
> kind of documentation in the API really adds
> to its value.
> 
> a few minor suggestions below, but still LGTM regardless.
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go
> File nova/nova.go (right):
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode158
> nova/nova.go:158: // First public IP address (if floating IP is assigned,
> otherwise "")
> i like to see whole-sentence comments for fields, similar to functions
although
> YMMV.
> 
> // AddressIPv4 and AddressIP6 hold the first public
> // IPv4 and IPv6 addresses of the server, or ""
> // if no floating IP is assigned.
> ?
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode162
> nova/nova.go:162: // Optional list of all IP addresses assigned to this
> // Addresses holds the list of all IP addresses assigned
> // to this server, grouped by "network" name ("public",
> // "private" or a custom name).
> 
> ?
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode167
> nova/nova.go:167: Created  string // creation timestamp
> for the record, this should probably be a time.Time, but for the time being:
> 
> // Created holds the creation timestamp of the
> // server in RFC3339 format.
> 
> ?
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode174
> nova/nova.go:174: Progress int    // completion % of current operation
> // Progress holds the completion percentage of the
> // current operation.
> ?
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode175
> nova/nova.go:175: Status   string // One of the Status* constants
> // Status holds the current status of the server,
> // one of the Status* constants.
> 
> ?
> 
> https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode177
> nova/nova.go:177: Updated  string // timestamp of last update
> // Updated holds the timestamp of the last update to
> // the server in RFC3339 format.
> ?

darn, too late.

[dimitern, 2013-01-29 16:14:18]

On 2013/01/29 16:08:23, rog wrote:
> On 2013/01/29 16:07:59, rog wrote:
> > that's really useful, thanks. i think having this
> > kind of documentation in the API really adds
> > to its value.
> > 
> > a few minor suggestions below, but still LGTM regardless.
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go
> > File nova/nova.go (right):
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode158
> > nova/nova.go:158: // First public IP address (if floating IP is assigned,
> > otherwise "")
> > i like to see whole-sentence comments for fields, similar to functions
> although
> > YMMV.
> > 
> > // AddressIPv4 and AddressIP6 hold the first public
> > // IPv4 and IPv6 addresses of the server, or ""
> > // if no floating IP is assigned.
> > ?
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode162
> > nova/nova.go:162: // Optional list of all IP addresses assigned to this
> > // Addresses holds the list of all IP addresses assigned
> > // to this server, grouped by "network" name ("public",
> > // "private" or a custom name).
> > 
> > ?
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode167
> > nova/nova.go:167: Created  string // creation timestamp
> > for the record, this should probably be a time.Time, but for the time being:
> > 
> > // Created holds the creation timestamp of the
> > // server in RFC3339 format.
> > 
> > ?
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode174
> > nova/nova.go:174: Progress int    // completion % of current operation
> > // Progress holds the completion percentage of the
> > // current operation.
> > ?
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode175
> > nova/nova.go:175: Status   string // One of the Status* constants
> > // Status holds the current status of the server,
> > // one of the Status* constants.
> > 
> > ?
> > 
> > https://codereview.appspot.com/7228058/diff/7/nova/nova.go#newcode177
> > nova/nova.go:177: Updated  string // timestamp of last update
> > // Updated holds the timestamp of the last update to
> > // the server in RFC3339 format.
> > ?
> 
> darn, too late.

Never fear, will do a follow-up CL with your suggestions :)