Code review comment for lp:~gz/pyjuju/openstack_provider
Revision history for this message
| Kapil Thangavelu (hazmat) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
review round 2
https:/
File juju/environmen
https:/
juju/environmen
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:/
File juju/providers/
https:/
juju/providers/
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:/
File juju/providers/
https:/
juju/providers/
reader.
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:/
juju/providers/
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:/
File juju/providers/
https:/
juju/providers/
'auth-url'
re TODO b/c?
https:/
File juju/providers/
https:/
juju/providers/
expectation here?
already documented in the module doc string.
https:/
File juju/providers/
https:/
juju/providers/
complete before cloud-init does the lookup.
While true this is silly. The delta here is an order of magnitude.
https:/
juju/providers/
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:/
juju/providers/
retrieving the id during boot.
This isn't even remotely possible.
https:/
juju/providers/
StringIO(
Has this issue been raised with upstream openstack? ie lack of access to
the ostack api machine id from the server.
https:/
juju/providers/
server.
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_
service.
This block is also untested.
https:/
juju/providers/
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:/
juju/providers/
_assign_
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:/