Code review comment for lp:~gz/pyjuju/openstack_provider

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#newcode57
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.py#newcode158
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.py#newcode166
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.py#newcode295
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.py#newcode309
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.py#newcode78
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.py#newcode61
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.py#newcode89
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.py#newcode99
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/machine.py
File juju/providers/openstack/machine.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/machine.py#newcode25
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/provider.py
File juju/providers/openstack/provider.py (right):

https://codereview.appspot.com/6312050/diff/1/juju/providers/openstack/provider.py#newcode41
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/provider.py#newcode198
juju/providers/openstack/provider.py:198: return defer.gatherResults(
we have a helper for this in lib/twistutils.py

https://codereview.appspot.com/6312050/