Openstack juju provider

Implementation of a provider using native OpenStack apis, and using swift for file storage. There is still some important features to complete here, but the core is done and the remaining gaps documented.

Also included is an openstack_s3 provider fudge that uses the Nova api, with the S3 api for storage. This allows for running against deployments using nova-objectstore rather than swift, which currently includes canonistack.

The basics are modelled on the EC2 provider implementation, with the addition of a client module that handles the details of the OpenStack api in the manner of txaws. Along the way I also refactored the security_groups module into helper class for port management.

I've made an effort to document all the tricky bits as clearly as possible, to guard against important details getting lost in translation. See those docstrings for a deeper description of the implementation.

Apologies for the size of the delta, all feedback welcome.

https://code.launchpad.net/~gz/juju/openstack_provider/+merge/110860

(do not edit description out of merge proposal)

[gz, 2012-06-19 14:55:13]

Please take a look.

[hazmat, 2012-06-22 06:03:25]

Hi Martin,

this is a big branch so i'm going to break up the review into multiple passes.
Overall it looks good, but i have a few questions about some of the behavior ic
wrt to waiting for post-boot network setup and md server availability (bootstrap
node instance id access) that i'd like to discuss with you and document.

https://codereview.appspot.com/6312050/diff/1/juju/environment/config.py
File juju/environment/config.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/environment/config.py#newc...
juju/environment/config.py:57: "default-instance-type": String(),
given constraint support default-instance-type shouldn't be needed as a config
option.

https://codereview.appspot.com/6312050/diff/1/juju/errors.py
File juju/errors.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/errors.py#newcode162
juju/errors.py:162: ", ".join(map(str, self.instance_ids)))
looks fine, but curious why needed, instance ids should always be integers.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client.py
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:158: response, body = yield deferred
style minors, deferred transient isn't needed.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:166: [self.token] =
response.headers.getRawHeaders("X-Auth-Token")
haven't seen this style before, preferable to just index 0 off the result.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:295: flavor_id = yield
self._lookup_flavor(flavor_name)
please document the kw  params supported (sec groups and ipv4).

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:309: server["accessIPv4"] = ip
what's this for?

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/files.py
File juju/providers/openstack/files.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/files....
juju/providers/openstack/files.py:78: if response.code != 201:
its likely the original error is more interesting at this point.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch.py
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:61: if self._master:
is the md service not available?

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:89: yield filestorage.put(id_name,
StringIO(server['id']))
have you run into this in practice?, it seems hard to achieve, but good to note
nonetheless.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:99: reactor.callLater(5, deferred.callback,
None)
ouch!.. you have to wait for the servers to boot to attach a public ip?

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/machin...
File juju/providers/openstack/machine.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/machin...
juju/providers/openstack/machine.py:25: return _SERVER_STATE_MAP.get(status,
'unknown')
nice. thanks for mapping this. its not a general requirement atm, but its very
nice to use a common vocabulary here.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/provid...
File juju/providers/openstack/provider.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/provid...
juju/providers/openstack/provider.py:41: # XXX: Why pass empty strings rather
than raising on missing config?
good question, better to just raise, and have it required in config.py

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/provid...
juju/providers/openstack/provider.py:198: return defer.gatherResults(
we have a helper for this in lib/twistutils.py

[gz, 2012-06-22 09:53:10]

Thanks for the comments! Have replied to some below, will act on the others.

https://codereview.appspot.com/6312050/diff/1/juju/errors.py
File juju/errors.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/errors.py#newcode162
juju/errors.py:162: ", ".join(map(str, self.instance_ids)))
Instance ids are normally UUIDs and stored as strings, but Nova currently also
allows the integer form of the ec2 id. This shouldn't come up in production, but
is exercised in tests as writing out UUIDs is annoying.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client.py
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:309: server["accessIPv4"] = ip
Serves no purpose now, was added when I was experimenting with ways to solve the
problem of juju needing a public ip.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/files.py
File juju/providers/openstack/files.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/files....
juju/providers/openstack/files.py:78: if response.code != 201:
Right, the swift backend should probably raise exceptions then this block could
reraise the original one.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch.py
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:61: if self._master:
The metadata service only exposes the EC2 form of the server id, but the rest of
the code deals with the UUID form. Sticking the UUID in file storage stops juju
getting confused with two different ids for the same server. Trick should no
longer be required with the Folsom release, as the metadata service will be
adding some OpenStack values.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:89: yield filestorage.put(id_name,
StringIO(server['id']))
Not had any problems in testing, the conditions should heavily favour the
correct outcome. Sticking a small string in the object store should beat the
server boot process even giving it a head start.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:99: reactor.callLater(5, deferred.callback,
None)
Doesn't need to finish booting, but doesn't work till the network setup has been
done, which is not synchronous on creating the server.

This is one the reasons I'd really like the public ip assignment to move to the
port handling code, and juju to not expect a public address for all units.

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/provid...
File juju/providers/openstack/provider.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/provid...
juju/providers/openstack/provider.py:198: return defer.gatherResults(
I saw that helper but it wasn't obvious what it was adding. I think the correct
behaviour here requires using DeferredList directly and examining exceptions
anyway.

[gz, 2012-07-03 11:31:53]

Please take a look.

[hazmat, 2012-07-03 17:25:44]

review round 2

https://codereview.appspot.com/6312050/diff/9001/juju/environment/config.py
File juju/environment/config.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/environment/config.py#n...
juju/environment/config.py:26: _OPENSTACK_AUTH_MODE = OneOf(
This could use a doc string, at least describing the correspondence to ostack
rels and what it entails wrt to the ostack setup, ie. is this a user choice or a
provider configuration matter. Eventually we'll start dropping support for old
ostack releases, and it will be good to understand what bits correspond to what
so we can keep this tidy.

EDIT: a ref to credentials.py for details would work

https://codereview.appspot.com/6312050/diff/9001/juju/providers/common/findzo...
File juju/providers/common/findzookeepers.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/common/findzo...
juju/providers/common/findzookeepers.py:40: % missing_instance_ids)
There's a broken test with this change not addressed by the branch. Its not
clear if this is adding value, rather than just leaking impl details. i assume
this is to help distinguish the int/str form of openstack id provider.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cli...
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cli...
juju/providers/openstack/client.py:115: body = yield reader.onConnectionLost
please don't name instance attributes the same as framework methods, thats very
confusing, it looks like your yielding on a method instead of its invocation
here.

its good to keep in mind and perhaps note here, if connectionLost method
callstack should return a deferred twisted will not wait on it before proceeding
and processing other things.

this method is also lacking any tests.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cli...
juju/providers/openstack/client.py:131: def make_url(self, service, parts):
minor. make_url should be prolly be private, since it requires self.services
which is not initialized in the constructor, so requires internal knowledge to
use properly in the correct initialization order.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cre...
File juju/providers/openstack/credentials.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cre...
juju/providers/openstack/credentials.py:8: TODO: rename 'nova-uri' to 'auth-url'
re TODO b/c?

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/fil...
File juju/providers/openstack/files.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/fil...
juju/providers/openstack/files.py:41: # XXX: what exactly is the expectation
here?
already documented in the module doc string.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:18: * Storing of server id needs to complete
before cloud-init does the lookup.
While true this is silly. The delta here is an order of magnitude.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:67: # For openstack deployments, really need
instance type and image id
instance type should be provided via constraints not config. image id really
wants for an external resource mapping (even if local) that takes flavor ids and
matches them to distros, else this ends up being a single release provider.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:87: #                cloud-init tries
retrieving the id during boot.
This isn't even remotely possible.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:89: yield filestorage.put(id_name,
StringIO(server['id']))
Has this issue been raised with upstream openstack? ie lack of access to the
ostack api machine id from the server.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:91: while not server.get('addresses'):
please document this loop and why its nesc. inline. also the loop may not even
be nesc. given provider configuration which can have the ostack provider
automatically hand out floating addresses, which this code doesn't check for. ie
if auto_assign_floating_ip=True in nova-network service.

This block is also untested.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:99: reactor.callLater(5, deferred.callback,
None)
This seems like quite a long poll period. Please add a debug log statement here.
also there is helper for this in lib/twistutils.py

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:109: def _assign_floating_ip(provider,
server_id):
This feels like it could be restructured to better handle concurrency around
floating ip assignment, depending on the api behavior. ie move assignment into
loop conditional with try/except, and fall out to new loop of allocation of new
ip and try/except assignment. What's the exc on assignment of already used ip?
does it just magically move them around?

[gz, 2012-07-04 10:54:44]

Thanks Kapil, is really useful input.

https://codereview.appspot.com/6312050/diff/9001/juju/environment/config.py
File juju/environment/config.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/environment/config.py#n...
juju/environment/config.py:26: _OPENSTACK_AUTH_MODE = OneOf(
On 2012/07/03 17:25:44, hazmat wrote:
> EDIT: a ref to credentials.py for details would work

Will add, most users should be able to ignore this setting and have the right
thing autodetected.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/common/findzo...
File juju/providers/common/findzookeepers.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/common/findzo...
juju/providers/common/findzookeepers.py:40: % missing_instance_ids)
On 2012/07/03 17:25:44, hazmat wrote:
> There's a broken test with this change not addressed by the branch. Its not
> clear if this is adding value, rather than just leaking impl details. i assume
> this is to help distinguish the int/str form of openstack id provider.

Have changed this back and forth as I saw that test fail before then forgot
about it and needed to fix int ids. Will resolve in some sensible manner.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cli...
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cli...
juju/providers/openstack/client.py:115: body = yield reader.onConnectionLost
Right, still some rearrangements needed here for sanity.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cli...
juju/providers/openstack/client.py:131: def make_url(self, service, parts):
On 2012/07/03 17:25:44, hazmat wrote:
> minor. make_url should be prolly be private, since it requires self.services
> which is not initialized in the constructor, so requires internal knowledge to
> use properly in the correct initialization order.

Sounds sensible. _SwiftClient/public_object_url needs it but  that's a somewhat
special case.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cre...
File juju/providers/openstack/credentials.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/cre...
juju/providers/openstack/credentials.py:8: TODO: rename 'nova-uri' to 'auth-url'
On 2012/07/03 17:25:44, hazmat wrote:
> re TODO b/c?

Because 'nova-uri' is wrong, in a modern openstack deployment it's a keystone
identity service url and used to get credentials for swift as well. There aren't
legacy compat issues in juju so we may as well use a less misleading name.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/fil...
File juju/providers/openstack/files.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/fil...
juju/providers/openstack/files.py:41: # XXX: what exactly is the expectation
here?
On 2012/07/03 17:25:44, hazmat wrote:
> already documented in the module doc string.

Right, will remove this.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:67: # For openstack deployments, really need
instance type and image id
On 2012/07/03 17:25:44, hazmat wrote:
> instance type should be provided via constraints not config. image id really
> wants for an external resource mapping (even if local) that takes flavor ids
and
> matches them to distros, else this ends up being a single release provider.

Will flag instance type as pending on the constraints changes. Getting image id
is a bit harder across multiple clouds, so we probably want to keep at least a
fallback means of supplying it via config.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:87: #                cloud-init tries
retrieving the id during boot.
On 2012/07/03 17:25:44, hazmat wrote:
> This isn't even remotely possible.

I can just about envision a case where it might happen, but it's unlikely enough
(and a temporary workaround) so probably doesn't justify this much scary
comment.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:89: yield filestorage.put(id_name,
StringIO(server['id']))
On 2012/07/03 17:25:44, hazmat wrote:
> Has this issue been raised with upstream openstack? ie lack of access to the
> ostack api machine id from the server.

See <https://lists.launchpad.net/openstack/msg11790.html>

The plan for folsom is to have an /openstack namespace on the metadata service.
I need to catch up with Scott Moser about that as it's his area.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:91: while not server.get('addresses'):
On 2012/07/03 17:25:44, hazmat wrote:
> please document this loop and why its nesc. inline. also the loop may not even
> be nesc. given provider configuration which can have the ostack provider
> automatically hand out floating addresses, which this code doesn't check for.
ie
> if auto_assign_floating_ip=True in nova-network service.
> 
> This block is also untested.

I was hoping I could just rip it out, but that will probably have to wait. No
openstack deployments I know of automatically assign floating ips, but juju
could shift this burden to the port opening stage. Will tidy up and add tests as
you suggest.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:99: reactor.callLater(5, deferred.callback,
None)
On 2012/07/03 17:25:44, hazmat wrote:
> This seems like quite a long poll period. Please add a debug log statement
here.
> also there is helper for this in lib/twistutils.py

Right, this wants to use shared infrastructure, will look at the helper.

https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/lau...
juju/providers/openstack/launch.py:109: def _assign_floating_ip(provider,
server_id):
On 2012/07/03 17:25:44, hazmat wrote:
> This feels like it could be restructured to better handle concurrency around
> floating ip assignment, depending on the api behavior. ie move assignment into
> loop conditional with try/except, and fall out to new loop of allocation of
new
> ip and try/except assignment. What's the exc on assignment of already used ip?
> does it just magically move them around?

It magically moves them around, otherwise this would be much easier. Always
creating new floating ips and deleting them on server teardown would also be
less racy but complicates cleanup further.

[gz, 2012-07-11 10:36:21]

Please take a look.