Add more OpenStack provider implementation

Add more OpenStack provider implementation

This branch starts with the initial stub OpenStack provider and fleshes out more infrastructure and code. Initial impementations of Environ methods
Instances() and AllInstances() are done, as well as a bunch more boilerplate to glue in the provider config etc.

The relevant goose library APIs are used to access a live Canonistack instance. For the purpose of the live tests, test server instances are create and destroyed. At the
moment, a knowm image is used to create the servers.

There is at least one limitation in the underlying goose nova client which needs to be addressed - the ability to filter the results returned by ListServers(). This
is required to exclude deleted and other irrelevant servers from AllInstances() but there is no support yet for this in goose.

I've also restructured the initial tests to hook into the juju test infrastructure so that the standard juju tests are run with this OpenStack provider. Of course,
they currently fail but will pass as soon as all of the provider methods are done. I've also included individual tests for each implemented API in the provider's
live test module. These compliment the juju tests which in this case appear more integration rather than unit focussed.

https://code.launchpad.net/~wallyworld/juju-core/more-openstatck-provider-work/+merge/137471

(do not edit description out of merge proposal)

[wallyworld, 2012-12-03 05:33:09]

Please take a look.

[niemeyer, 2012-12-03 22:35:02]

Thanks, that's pretty much ready to go. Just a couple of suggestions, plus one
relevant point on types:

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config.go...
environs/openstack/config.go:82: cred, err :=
identity.CompleteCredentialsFromEnv()
identity.EnvCredentials()? :-)

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config_te...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config_te...
environs/openstack/config_test.go:133: for v, _ := range envVars {
for k, v := range s.savedVars {
    os.Setenv(k, v)
}

I think?

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/live_test...
environs/openstack/live_test.go:34: // environment variables to make the Amazon
testing work:
s/Amazon/OpenStack/

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/live_test...
environs/openstack/live_test.go:67: t.testServers, err = t.createInstances(2)
There should be a comment here (and some proper spacing) pointing out why these
out-of-band instances are being created, and when the logic should be
removed/refactored (if that's the intention).

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:198: servers, err := e.nova().ListServers()
Nice logic in these methods, thanks.

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:203: for j, _ := range *servers {
Hmm.. what's servers' type and why? It's a common mistake to have a pointer to a
slice instead of just a slice.

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:220: insts = append(insts, &instance{e, &s})
Same as above, plus why &s and not &server?

[wallyworld, 2012-12-04 01:12:07]

Please take a look.

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config.go...
environs/openstack/config.go:82: cred, err :=
identity.CompleteCredentialsFromEnv()
On 2012/12/03 22:35:02, niemeyer wrote:
> identity.EnvCredentials()? :-)

I was using the previously implemented goose API call here. There is also a
CredentialsFromEnv() API and this new one was introduced to raise an error if
any of the required env variables is not set. So there's a deliberate semantic
difference between the two goose APIs which is reflected in their names. Perhaps
any API naming concerns can be addressed separately?

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config_te...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config_te...
environs/openstack/config_test.go:133: for v, _ := range envVars {
On 2012/12/03 22:35:02, niemeyer wrote:
> for k, v := range s.savedVars {
>     os.Setenv(k, v)
> }
> 
> I think?

Mine works too but yours is nicer, thanks.

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/live_test...
environs/openstack/live_test.go:34: // environment variables to make the Amazon
testing work:
On 2012/12/03 22:35:02, niemeyer wrote:
> s/Amazon/OpenStack/

Done.

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/live_test...
environs/openstack/live_test.go:67: t.testServers, err = t.createInstances(2)
On 2012/12/03 22:35:02, niemeyer wrote:
> There should be a comment here (and some proper spacing) pointing out why
these
> out-of-band instances are being created, and when the logic should be
> removed/refactored (if that's the intention).

There's a comment above stating that some test service instances are being
created. I guess you're looking for more explanation? Isn't it just the "right
thing to do", and any further explanation would be telling people how to suck
eggs? ie the general principal of writing tests is that one should always set up
any required resources used in the tests and tear them down afterwards. So if
one is looking to test the AllInstances() API (and others requiring instances to
be running) in the test suite, then there had better be some instances setup to
use. I'm happy to add more comments, but also don't want to waste time stating
the obvious.

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:203: for j, _ := range *servers {
On 2012/12/03 22:35:02, niemeyer wrote:
> Hmm.. what's servers' type and why? It's a common mistake to have a pointer to
a
> slice instead of just a slice.

The pointer was used due to our IRC discussion about the underlying API needing
to return a pointer when the return type is a struct. Even though not strictly
required for slices, I made all the APIs return pointers for consistency. I'll
do a goose branch to change it for slices.

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:220: insts = append(insts, &instance{e, &s})
On 2012/12/03 22:35:02, niemeyer wrote:
> Same as above, plus why &s and not &server?

I had &server to start with and the array didn't populate correctly and after
much hair pulling I finally asked for help and Dave told me that I had fallen
into a common Go trap and had to use a local variable to receive a copy of the
loop variable.

[niemeyer, 2012-12-04 01:33:54]

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/config.go...
environs/openstack/config.go:82: cred, err :=
identity.CompleteCredentialsFromEnv()
On 2012/12/04 01:12:07, wallyworld wrote:
> On 2012/12/03 22:35:02, niemeyer wrote:
> > identity.EnvCredentials()? :-)
> 
> I was using the previously implemented goose API call here. There is also a
> CredentialsFromEnv() API and this new one was introduced to raise an error if
> any of the required env variables is not set. So there's a deliberate semantic
> difference between the two goose APIs which is reflected in their names.
Perhaps
> any API naming concerns can be addressed separately?

It was just a suggestion as long names like that are generally unnecessary.
Certainly won't argue over that one.

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/live_test...
environs/openstack/live_test.go:67: t.testServers, err = t.createInstances(2)
On 2012/12/04 01:12:07, wallyworld wrote:
> On 2012/12/03 22:35:02, niemeyer wrote:
> > There should be a comment here (and some proper spacing) pointing out why
> these
> > out-of-band instances are being created, and when the logic should be
> > removed/refactored (if that's the intention).
> 
> There's a comment above stating that some test service instances are being
> created. I guess you're looking for more explanation? Isn't it just the "right
> thing to do", and any further explanation would be telling people how to suck
> eggs? ie the general principal of writing tests is that one should always set
up
> any required resources used in the tests and tear them down afterwards. So if
> one is looking to test the AllInstances() API (and others requiring instances
to
> be running) in the test suite, then there had better be some instances setup
to
> use. I'm happy to add more comments, but also don't want to waste time stating
> the obvious.

It's not obvious why we're creating instances out-of-band from the environment
API, and yet they're being part of that environment. The environment API should
not return instances on AllInstances or Instances if these instances were not
specifically created for this environment, and it should be possible to have
multiple environment under one account, so what exists in SetUpSuite today
doesn't follow logically from anything else, and doesn't feel like it should be
here in the short-term.

By the way, I find the "sucking eggs" reference funny, as I've never done that
nor ever heard about anyone doing so. Maybe people would appreciate a howto
after all.

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:203: for j, _ := range *servers {
On 2012/12/04 01:12:07, wallyworld wrote:
> On 2012/12/03 22:35:02, niemeyer wrote:
> > Hmm.. what's servers' type and why? It's a common mistake to have a pointer
to
> a
> > slice instead of just a slice.
> 
> The pointer was used due to our IRC discussion about the underlying API
needing
> to return a pointer when the return type is a struct. Even though not strictly
> required for slices, I made all the APIs return pointers for consistency. I'll
> do a goose branch to change it for slices.

Slices may be nil. They are actually reference types by themselves already.
Arrays are the underlying data.

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:220: insts = append(insts, &instance{e, &s})
On 2012/12/04 01:12:07, wallyworld wrote:
> On 2012/12/03 22:35:02, niemeyer wrote:
> > Same as above, plus why &s and not &server?
> 
> I had &server to start with and the array didn't populate correctly and after
> much hair pulling I finally asked for help and Dave told me that I had fallen
> into a common Go trap and had to use a local variable to receive a copy of the
> loop variable. 

Yeah, Dave is right. My suggestion would only make sense if it was a pointer, or
you did what you did above and referenced items of the slice individually. It's
fine as it is, either way, sorry for the noise.

[wallyworld, 2012-12-04 05:09:39]

Please take a look.

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/live_test...
environs/openstack/live_test.go:67: t.testServers, err = t.createInstances(2)
On 2012/12/04 01:33:54, niemeyer wrote:
> On 2012/12/04 01:12:07, wallyworld wrote:
> > On 2012/12/03 22:35:02, niemeyer wrote:
> > > There should be a comment here (and some proper spacing) pointing out why
> > these
> > > out-of-band instances are being created, and when the logic should be
> > > removed/refactored (if that's the intention).
> > 
> > There's a comment above stating that some test service instances are being
> > created. I guess you're looking for more explanation? Isn't it just the
"right
> > thing to do", and any further explanation would be telling people how to
suck
> > eggs? ie the general principal of writing tests is that one should always
set
> up
> > any required resources used in the tests and tear them down afterwards. So
if
> > one is looking to test the AllInstances() API (and others requiring
instances
> to
> > be running) in the test suite, then there had better be some instances setup
> to
> > use. I'm happy to add more comments, but also don't want to waste time
stating
> > the obvious.
> 
> It's not obvious why we're creating instances out-of-band from the environment
> API, and yet they're being part of that environment. The environment API
should
> not return instances on AllInstances or Instances if these instances were not
> specifically created for this environment, and it should be possible to have
> multiple environment under one account, so what exists in SetUpSuite today
> doesn't follow logically from anything else, and doesn't feel like it should
be
> here in the short-term.
> 
> By the way, I find the "sucking eggs" reference funny, as I've never done that
> nor ever heard about anyone doing so. Maybe people would appreciate a howto
> after all.

I've added some extra comments to hopefully clarify the situation. The API
implementation is still very much a work in progress so for now, the helper
method is required until the API catches up.

The "sucking eggs" reference is a colloquialism, sorry for the confusion :-)

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

https://codereview.appspot.com/6874049/diff/2001/environs/openstack/provider....
environs/openstack/provider.go:203: for j, _ := range *servers {
On 2012/12/04 01:33:54, niemeyer wrote:
> On 2012/12/04 01:12:07, wallyworld wrote:
> > On 2012/12/03 22:35:02, niemeyer wrote:
> > > Hmm.. what's servers' type and why? It's a common mistake to have a
pointer
> to
> > a
> > > slice instead of just a slice.
> > 
> > The pointer was used due to our IRC discussion about the underlying API
> needing
> > to return a pointer when the return type is a struct. Even though not
strictly
> > required for slices, I made all the APIs return pointers for consistency.
I'll
> > do a goose branch to change it for slices.
> 
> Slices may be nil. They are actually reference types by themselves already.
> Arrays are the underlying data.

I've got a goose branch in review to fix this so will fix it here also.

[niemeyer, 2012-12-04 16:37:06]

LGTM, assuming the following changes are introduced as indicated:

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/config_t...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/config_t...
environs/openstack/config_test.go:134: os.Setenv(k, s.savedVars[k])
This isn't important, but since you're still getting used to Go, the form I
mentioned earlier is preferable:

    for k, v := range ... {

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/live_tes...
File environs/openstack/live_test.go (right):

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/live_tes...
environs/openstack/live_test.go:127: func (t *LiveTests) TestAllInstances(c *C)
{
// FIXME These instances were not started by the environment, and have no
// indication that they should be part of it.

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/live_tes...
environs/openstack/live_test.go:143: func (t *LiveTests) TestInstances(c *C) {
// FIXME These instances were not started by the environment, and have no
// indication that they should be part of it.

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

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/provider...
environs/openstack/provider.go:193: func (e *environ) Instances(ids []string)
([]environs.Instance, error) {
// FIXME Instances must somehow be tagged to be part of the environment.
// This is returning *all* instances, which means it's impossible
// to have two different environments on the same account.

https://codereview.appspot.com/6874049/diff/10008/environs/openstack/provider...
environs/openstack/provider.go:213: // TODO: add filtering to exclude deleted
images etc
// FIXME Instances must somehow be tagged to be part of the environment.
// This is returning *all* instances, which means it's impossible
// to have two different environments on the same account.

[wallyworld, 2012-12-05 02:01:31]

*** Submitted:

Add more OpenStack provider implementation

This branch starts with the initial stub OpenStack provider and fleshes out more
infrastructure and code. Initial impementations of Environ methods
Instances() and AllInstances() are done, as well as a bunch more boilerplate to
glue in the provider config etc.

The relevant goose library APIs are used to access a live Canonistack instance.
For the purpose of the live tests, test server instances are create and
destroyed. At the
moment, a knowm image is used to create the servers.

There is at least one limitation in the underlying goose nova client which needs
to be addressed - the ability to filter the results returned by ListServers().
This
is required to exclude deleted and other irrelevant servers from AllInstances()
but there is no support yet for this in goose.

I've also restructured the initial tests to hook into the juju test
infrastructure so that the standard juju tests are run with this OpenStack
provider. Of course,
they currently fail but will pass as soon as all of the provider methods are
done. I've also included individual tests for each implemented API in the
provider's
live test module. These compliment the juju tests which in this case appear more
integration rather than unit focussed.

R=niemeyer
CC=
https://codereview.appspot.com/6874049