Code review comment for lp://staging/~jseutter/charms/precise/storage/storage-with-broker-provider

Revision history for this message
Chad Smith (chad.smith) wrote :

Thanks Jerry for posting this MP.
Okay, just wanted to consolidate some review comments from previous email so we can track them and check them off here:

[1] let's kill nova_util.py and migrate get_instance_id up into common_util. and call it from there. Lots of unused code that we can remove if we drop nova_util

[2] get_instance_id should not longer curl and use jsonpipe. Let's do something like this

def _get_instance_id():
    import json
    metadata = json.loads(
        subprocess.check_output("curl %s" % METADATA_URL, shell=True))
    return metadata["uuid"]

[3] in hooks/storage-provider.d/blockstoragebroker/config-changed
  Drop the install of jsonpipe and drop all the OS_env file saving stuff it's not needed as we handle that in block-storage-broker

[4] drop any config-changed file loads or calls from any hooks in hooks/storage-provider.d/blockstoragebroker since config-changed is called before any of the other hooks anyway

# So this stuff should be gone from any other file
current_dir = sys.path[0]
config_changed = "%s/config-changed" % current_dir
if os.path.exists(config_changed):
    subprocess.check_call(config_changed) # OS_* env vars

[5] We can delete the start and stop hooks from hooks/storage-provider.d/blockstoragebroker.
We don't even have those hook links defined up in the global hooks directory so they won't get called anyway

Will continue to review the charm changes with a couple live deployments to see how our unit add/remove story is going.

« Back to merge proposal