Merge lp://staging/~jseutter/charms/precise/storage/storage-with-broker-provider into lp://staging/~dpb/charms/precise/storage/trunk

I think you forgot the 'storage-provider.d/blockstorageprovider' dir?

[1] chmod +x storage-provider.d/blockstoragebroker/!(*.py)

[2] remove nova_utils.py from blockstoragebroker dir

[3] you are missing symlinks in hooks/. block-storage-relation-changed, block-storage-relation-broken

[4] please rename the storage-provider.d/blockstoragebroker dir with hyphens in it. (block-storage-broker). Also, the "provider" setting should be expected to be that of course.

[5] Upon relation, the device is not set by block-storage-broker. I needed something like this to proceed:

https://pastebin.canonical.com/104472/

--- a/hooks/storage-provider.d/blockstoragebroker/block-storage-relation-changed
+++ b/hooks/storage-provider.d/blockstoragebroker/block-storage-relation-changed
@@ -18,4 +18,7 @@ if os.path.exists(config_changed):
     subprocess.check_call(config_changed)

 device_path = hookenv.relation_get("block-device-path")
-common_util.mount_volume(device_path) # Will wait until mountpoint is set by principal
+if device_path:
+ common_util.mount_volume(device_path) # Will wait until mountpoint is set by principal
+else:
+ hookenv.log("waiting for relation to define 'block-device-path")

[2-amendment] I think the file can go away, afaict, you are only using one method from it to get an instance id (which should at least be expanded to aws, I think)

Paste of all changes I had to make so far: https://pastebin.canonical.com/104476/ (with this, I get a working deployment, still testing and reviewing though)

[6] There is a fairly serious race condition. If data-relation-changed runs *after* block-storage-relation-changed, block-storage-relation-changed will never be able to probe the relation settings for 'storage_mount_point'. I think this should be fixed by having 'data-relation-*' hooks in the blockstoragebroker subdir and more careful sets and gets. I'll get a paste with something working.

Updated diff including thoughts from [6]: https://pastebin.canonical.com/104482/

I'd like to re-review, but there seems to be some rework necessary here. I'd suggest for testing:

1) deploy
2) add-unit postgres
3) add-unit postgres
4) break relation between postgres and storage, add back
5) destroy-services
6) make sure everything comes down without error

In between each step, make sure the system looks right. That the volume is mounted, that data is being stored there for postgres. This can be seen with the following checks as user postgres:

* ls -ld /var/lib/postgresql/9.1/main
* ls -l /mnt/takeitlikeanova
* psql # CREATE DATABASE foo -- do this once on the main node
* psql -d foo # make sure with each new node, you can see the data copied in.
* pg_lsclusters

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.

> [1] chmod +x storage-provider.d/blockstoragebroker/!(*.py)

Fixed.

dpb, your 1, 2, 3, 4 have been addressed. 5 and your extra comment are still outstanding.

Chad, your 1, 2, 3, 4 and 5 have all been addressed.

> I'd like to re-review, but there seems to be some rework necessary here. I'd
> suggest for testing:
>
> 1) deploy
> 2) add-unit postgres
> 3) add-unit postgres
> 4) break relation between postgres and storage, add back

Without the subsequent MP, I don't think breaking storage/postgresql relations and re-adding will be possible because volume labels of block-storage-broker to storage will have been labeled "storage/0 unit volume" and when you readd the relation it would try to find a storage/1 or storage/2 labelled volume to remount. This is fixed in the storage-volume-label branch below

https://code.launchpad.net/~chad.smith/charms/precise/storage/storage-volume-label-and-volume-map/+merge/205669

Jerry, thanks for the updates and pulling in that volume-map and volume-label branch. I'll remove that MP now.

I've been deploying this now with the following bundle and I'm able to juju add-unit postgresql, juju remove-relation postgresql storage, juju add-relation postgresql storage and see the volumes remounted without hook issues. I'll debug a bit more and look into the race condition between data-relation and block-storage-relation hooks ordering.

cat postgres-storage.cfg
common:
    services:
        postgresql:
            branch: lp:~chad.smith/landscape/postgresql-storage-wip
            constraints: mem=2048
            options:
                extra-packages: python-apt postgresql-contrib postgresql-9.1-debversion
                max_connections: 500
        storage:
            branch: lp:~jseutter/charms/precise/storage/storage-with-broker-provider
            options:
                provider: block-storage-broker
                volume_size: 9
                volume_map: "{postgresql/0: 327fed5b-3ac7-45f7-988e-6a536cb32257}"
        block-storage-broker:
            branch: lp:~chad.smith/charms/precise/block-storage-broker/trunk
            options:
                key: <OS_USERNAME>
                endpoint: https://keystone.canonistack.canonical.com:443/v2.0/
                region: <OS_REGION_NAME>
                secret: <OS_PASSWORD>
                tenant: <OS_TENANT_NAME>

doit:
    inherits: common
    series: precise
    relations:
        - [postgresql, storage]
        - [storage, block-storage-broker]

$ juju-deployer -c postgres-storage.cfg doit

Hey Jerry,
  I had a couple of things I needed to fix in this branch to make it work for remove-relation postgresql storage and add-relation postgresl postgresql

[6] pulled in persist* functions from block-storage-broker charm
[7] persistent mountpoint saved during mount_volume

[8] needed symlink in block-storage-broker between block-storage-relation-broken and data-relation-departed down on block-storage-broker provider. I added block-storage-relation-departed symlink in there too as it's probably better to do this unmount action during departed if we can instead of broken. We should leave broken there too though as that hook will only fire when the instance dies on us without a remove-relation call .

[9] use persistent mountpoint info instead of relation-get mointpoint (as this relation-data might disappear on some departed/broken hooks)

here's the pastebin of what I patched in your branch: including the unit tests just pulled over from block-storage-broker charm
https://pastebin.canonical.com/104745/

Okay enough of the comment steamrolling. I was trying to get the storage-related concerns out of the way so I could prove our postgresql charm was the one having problems.

Your branch with this patch works very well with the postgres-storage.cfg listed above.
add-relation remove-relation between storage works great and postgres can pickup and remount the nova volume fine upon relation-rejoined with a simple juju resolved --retry postgresql/0. As expected postgresql/0 goes into error hook state when the nova volume disappears.

Approving your branch with the following patch above as we keep lumping more features into it. Sorry man