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#newcode26 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/findzookeepers.py File juju/providers/common/findzookeepers.py (right): https://codereview.appspot.com/6312050/diff/9001/juju/providers/common/findzookeepers.py#newcode40 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/client.py File juju/providers/openstack/client.py (right): https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/client.py#newcode115 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/client.py#newcode131 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/credentials.py File juju/providers/openstack/credentials.py (right): https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/credentials.py#newcode8 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/files.py File juju/providers/openstack/files.py (right): https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/files.py#newcode41 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/launch.py File juju/providers/openstack/launch.py (right): https://codereview.appspot.com/6312050/diff/9001/juju/providers/openstack/launch.py#newcode18 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/launch.py#newcode67 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/launch.py#newcode87 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/launch.py#newcode89 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/launch.py#newcode91 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/launch.py#newcode99 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/launch.py#newcode109 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? https://codereview.appspot.com/6312050/