Config for assigning public (floating) IP to nodes

Config for assigning public (floating) IP to nodes

A new config key, use-floating-ip (default true), is provided. If true, nodes are given a public IP address.

When adding new tests, I ran into the problem that some existing tests rely on state created by other tests. I fixed this and
also removed unnecessary Env duplication in the local live tests.

https://code.launchpad.net/~wallyworld/juju-core/bootstrap-publicip-config/+merge/149752

(do not edit description out of merge proposal)

[wallyworld, 2013-02-21 06:54:07]

Please take a look.

[dimitern, 2013-02-21 11:06:37]

I like where this is heading, but I think it deserves a bit of a discussion.
Some comments so far:

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

https://codereview.appspot.com/7397047/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:23: "expose-bootstrap-node": schema.Bool(),
Not sure about this. How about having smth like "assign-public-ips" instead? Or
maybe even better, a 3-state config setting "assign-public-address" with "all"
(to all nodes), "none" (default), "bootstrap" (only the bootstrap node).
Thoughts?

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

https://codereview.appspot.com/7397047/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:97: t.LiveTests.SetUpSuite(c)
I was thinking of that, but when I did it I run into random test failures - I
assume you run the tests enough times to see local live ones pass?

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

https://codereview.appspot.com/7397047/diff/1/environs/openstack/local_test.g...
environs/openstack/local_test.go:242: func ResetEnvironment(e environs.Environ)
{
Good, thanks for this.

[jameinel, 2013-02-21 12:22:19]

I believe we decided to go back to the Python implementation, which is to just
have "use-floating-ip" which is an all-or-nothing hack. And then as we are able
to write the code, we can deprecate the flag entirely, and do the automatic
detection of when we need to assign a public IP.

The cases seem to be:

HP
 1) Set an IP that is actually publicly accessible into the Private IP
configuration
 2) Detect that the IP isn't 10.*/192.*/127? and just never assign a Floating IP
(during bootstrap or during expose)

Canonistack
 1) We'll be using SSH tunneling to the bootstrap node, so we can avoid
always-assigning a Floating IP there
 2) All other nodes only get a Floating IP during Expose.

[wallyworld, 2013-02-22 01:31:16]

I've changed the configuration parameter to "use-floating-ip" and it is a global
yes/no configuration for all calls to startInstance().

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

https://codereview.appspot.com/7397047/diff/1/environs/openstack/config.go#ne...
environs/openstack/config.go:23: "expose-bootstrap-node": schema.Bool(),
On 2013/02/21 11:06:38, dimitern wrote:
> Not sure about this. How about having smth like "assign-public-ips" instead?
Or
> maybe even better, a 3-state config setting "assign-public-address" with "all"
> (to all nodes), "none" (default), "bootstrap" (only the bootstrap node).
> Thoughts?

Changed to use-floating-ip and used as a global yes/no configuration for all
calls to startInstance().

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

https://codereview.appspot.com/7397047/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:97: t.LiveTests.SetUpSuite(c)
On 2013/02/21 11:06:38, dimitern wrote:
> I was thinking of that, but when I did it I run into random test failures - I
> assume you run the tests enough times to see local live ones pass?

I have run the tests lots of times. So far, so good :-)

[wallyworld, 2013-02-22 01:34:34]

Please take a look.

[dimitern, 2013-02-22 09:52:14]

LGTM

[jameinel, 2013-02-22 17:35:02]

LGTM though I would avoid chaining function calls and defer.

defer foo(...)

waits to call foo until the current function returns.

However:
defer foo(...)()

actually calls foo and defers the call to the return value, but when ... gets
big it is hard to see those () at the end. (I imagine with practice it would get
easier, but you'll always have a bit of action at a distance when '...' is big.)

https://codereview.appspot.com/7397047/diff/8001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/7397047/diff/8001/environs/openstack/local_tes...
environs/openstack/local_test.go:229: )()
The nesting is getting deep enough that it is a bit hard to see what bits are
what. (Like that () which is 6 lines away from the matching defer.)

It would probably be a little easier to read if it were slightly less
functional:

cleanup := s.Service.Nova.RegisterControlPoint(...)
defer cleanup()

Otherwise my natural tendency in reading it is: Oh, we are deferring the call to
RegisterControlPoint. Oh wait, here at the end we actually call the return value
not the actual call.
I think the unnamed func is probably ok. Though maybe we would like it to call
c.Fail() at that point instead of just fmt.Errorf? I'm not sure which would give
us better context in debugging why addFloatingIP got called.

[wallyworld, 2013-02-24 23:00:22]

*** Submitted:

Config for assigning public (floating) IP to nodes

A new config key, use-floating-ip (default true), is provided. If true, nodes
are given a public IP address.

When adding new tests, I ran into the problem that some existing tests rely on
state created by other tests. I fixed this and
also removed unnecessary Env duplication in the local live tests.

R=dimitern, jameinel
CC=
https://codereview.appspot.com/7397047

[wallyworld, 2013-02-24 23:00:48]

https://codereview.appspot.com/7397047/diff/8001/environs/openstack/local_tes...
File environs/openstack/local_test.go (right):

https://codereview.appspot.com/7397047/diff/8001/environs/openstack/local_tes...
environs/openstack/local_test.go:229: )()
> I think the unnamed func is probably ok. Though maybe we would like it to call
> c.Fail() at that point instead of just fmt.Errorf? I'm not sure which would
give
> us better context in debugging why addFloatingIP got called.

Calling Fail() provides no context, just "test failed". Returning an error
prints the reason for failure.