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#newcode26 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/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) 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/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 Right, still some rearrangements needed here for sanity. 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): 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/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' 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/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? 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/launch.py File juju/providers/openstack/launch.py (right): 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 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/launch.py#newcode87 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/launch.py#newcode89 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 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/launch.py#newcode91 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/launch.py#newcode99 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/launch.py#newcode109 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. https://codereview.appspot.com/6312050/