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

[Jerry Seutter]

The storage charm now relates to the block-storage-broker charm to request a volume. The storage charm used to communicate with nova directly, but the fact that each unit would have nova credentials was deemed unacceptable. Now only the block-storage-charm unit has the nova credentials.

The postgresql charm is the only one that currently works with the storage subordinate charm. To test, you could use the following juju-deployer config:

####################
common:
    services:
        block-storage-broker:
            branch: lp:~chad.smith/charms/precise/block-storage-broker/trunk
            options:
                key: YOUR_OS_USERNAME
                endpoint: https://keystone.canonistack.canonical.com:443/v2.0/
                region: YOUR_OS_REGION
                secret: YOUR_OS_SECRET
                tenant: YOUR_OS_TENANT_NAME
        storage:
            branch: lp:~jseutter/charms/precise/storage/storage-with-broker-provider/
            options:
                provider: block-storage-broker
                volume_size: 7
        postgresql:
            branch: lp:~jseutter/charms/precise/postgresql/delegate-blockstorage-to-storage-subordinate-patched
            options:
                extra-packages: python-apt postgresql-contrib postgresql-9.1-debversion
                storage_mount_point: /mnt/takeitlikeanova

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

then run something like:
juju-deployer -c storage-deployments.cfg -e jscstack2 doit -B

To verify, ssh to the postgresql/0 unit. You should see a symlink in postgresql that points to the volume that the storage charm mounted:

root@juju-jscstack2-machine-2:/home/ubuntu# ll /var/lib/postgresql/9.1/main
lrwxrwxrwx 1 root root 40 Feb 7 04:01 /var/lib/postgresql/9.1/main -> /mnt/takeitlikeanova/postgresql/9.1/main/

[David Britton (dpb)]

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

[lp:~jseutter/charms/precise/storage/storage-with-broker-provider]

32. By Jerry Seutter on 2014-02-07

Added missing blockstoragebroker directory.

[David Britton (dpb)]

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

[David Britton (dpb)]

[2] remove nova_utils.py from blockstoragebroker dir

[David Britton (dpb)]

[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.

[David Britton (dpb)]

[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")

[David Britton (dpb)]

[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)

[David Britton (dpb)]

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)

[David Britton (dpb)]

[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.

[David Britton (dpb)]

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

[David Britton (dpb)]

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

review: Needs Fixing

[lp:~jseutter/charms/precise/storage/storage-with-broker-provider]

33. By Jerry Seutter on 2014-02-10

Restore execute permission on hooks

[Chad Smith (chad.smith)]

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.

[lp:~jseutter/charms/precise/storage/storage-with-broker-provider]

34. By Jerry Seutter on 2014-02-10

Restoring relation hook symlinks

[Jerry Seutter (jseutter)]

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

Fixed.

[lp:~jseutter/charms/precise/storage/storage-with-broker-provider]

35. By Jerry Seutter on 2014-02-10

Removing nova_util

36. By Jerry Seutter on 2014-02-10

Restoring METADATA_URL.

37. By Jerry Seutter on 2014-02-10

Rename blockstoragebroker to block-storage-broker.

38. By Jerry Seutter on 2014-02-10

get_instance_id no longer uses jsonpipe

39. By Jerry Seutter on 2014-02-10

import json

40. By Jerry Seutter on 2014-02-10

Remove python-jsonpipe

41. By Jerry Seutter on 2014-02-10

Removing openstack environment variable lookups

42. By Jerry Seutter on 2014-02-10

Removing unused hooks.

43. By Jerry Seutter on 2014-02-11

Removing unused code.

[Jerry Seutter (jseutter)]

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

[Jerry Seutter (jseutter)]

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

[Chad Smith (chad.smith)]

> 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

[lp:~jseutter/charms/precise/storage/storage-with-broker-provider]

44. By Jerry Seutter on 2014-02-11

Merged in lp:~chad.smith/charms/precise/storage/storage-volume-label-and-volume-map

[Chad Smith (chad.smith)]

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

[Chad Smith (chad.smith)]

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

review: Approve

[lp:~jseutter/charms/precise/storage/storage-with-broker-provider]

45. By Jerry Seutter on 2014-02-13

Adding setting persistence to storage subordinate.

46. By Jerry Seutter on 2014-02-14

Unmounting the volume upon data-relation-departed.

[David Britton (dpb)]

review: Approve

=== modified file 'Makefile'
--- Makefile	2014-02-04 01:13:16 +0000
+++ Makefile	2014-02-14 00:23:01 +0000
@@ -8,6 +8,9 @@
 
 lint:
 	@flake8 --exclude hooks/charmhelpers hooks
+	# flake any python scripts not named *.py
+	fgrep -r bin/python hooks/ | awk -F :  '{print $$1}' | xargs -r -n1 flake8
+
 
 update-charm-helpers:
 	# Pull latest charm-helpers branch and sync the components based on our

=== modified file 'config.yaml'
--- config.yaml	2014-02-04 00:58:01 +0000
+++ config.yaml	2014-02-14 00:23:01 +0000
@@ -8,9 +8,19 @@
         default: local
         description: |
             The backend storage provider.  Will be mounted at root, and
-            can be one of, local, blockstoragebroker or nfs.  If you set to
-            blockstoragebroker, you should provide a suitable value for
+            can be one of, local, block-storage-broker or nfs.  If you set to
+            block-storage-broker, you should provide a suitable value for
             volume_size.
+    volume_map:
+        type: string
+        default: ""
+        description: |
+          YAML map as e.g. "{postgres/0: vol-0000010, postgres/1: vol-0000016}".
+          A service unit to specific block storage volume-id that should be
+          attached to each unit. This requires that provider is set to
+          block-storage-broker and that volume-ids specified match a listing
+          from nova volume-list. If a related unit does not have a volume-id
+          specified, a new volume of volume_size will be created for that unit.
     volume_size:
         type: int
         description: |

=== added symlink 'hooks/block-storage-relation-broken'
=== target is u'hooks'
=== added symlink 'hooks/block-storage-relation-changed'
=== target is u'hooks'
=== added symlink 'hooks/block-storage-relation-departed'
=== target is u'hooks'
=== modified file 'hooks/common_util.py'
--- hooks/common_util.py	2014-02-05 16:40:11 +0000
+++ hooks/common_util.py	2014-02-14 00:23:01 +0000
@@ -5,15 +5,63 @@
 import sys
 from time import sleep
 from stat import S_ISBLK, ST_MODE
+import json
 
 CHARM_DIR = os.environ.get("CHARM_DIR")
 FSTAB_FILE = "/etc/fstab"
+METADATA_URL = "http://169.254.169.254/openstack/2012-08-10/meta_data.json"
+PERSIST_DATA_FILE = "charm-data"
 
 
 def log(message, level=None):
     """Quaint little log wrapper for juju logging"""
     hookenv.log(message, level)
 
+# XXX We can remove _persist_data when juju bug:1279018 is fixed to provide
+# relation-data from *-relation-departed hook runs
+def _persist_data(key, value):
+    """Save C{key} and C{value} pairs in a persistent file for reference"""
+    data = {}
+    filepath = "%s/%s" % (hookenv.charm_dir(), PERSIST_DATA_FILE)
+    if os.path.exists(filepath):
+        with open(filepath) as inputfile:
+            data = json.load(inputfile)
+    data[key] = value
+    with open(filepath, "w") as outfile:
+        outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+
+
+def _get_persistent_data(key=None, remove_values=False):
+    """Get a persistent data as specified by C{key} from the PERSIST_DATA_FILE
+
+    If specified C{key} does not exist, or if no PERSIST_DATA_FILE exists,
+    return C{None}. If C{remove_values} is C{True} remove the specified C{key}
+    from the data file.
+    """
+    data = {}
+    filepath = "%s/%s" % (hookenv.charm_dir(), PERSIST_DATA_FILE)
+    if os.path.exists(filepath):
+        with open(filepath) as inputfile:
+            data = json.load(inputfile)
+    if key:
+        value = data.get(key, None)
+        if remove_values:
+            data.pop(key, None)
+            if data:
+                with open(filepath, "w") as outfile:
+                    outfile.write(
+                        unicode(json.dumps(data, ensure_ascii=False)))
+            elif os.path.exists(filepath):
+                os.remove(filepath)
+    else:
+        value = data
+        if not data:
+            value = None
+        if remove_values:
+            if os.path.exists(filepath):
+                os.remove(filepath)
+    return value
+
 
 def get_provider():
     """Get the storage charm's provider type as set within it's config.yaml"""
@@ -27,11 +75,10 @@
         sys.exit(1)
 
 
-def is_mounted():
-    """Return True if the defined mountpoint is mounted"""
-    mountpoint = hookenv.relation_get("mountpoint")
+def is_mounted(mountpoint):
+    """Return True if the mountpoint is mounted"""
     if not mountpoint:
-        log("INFO: No mountpoint defined by relation assuming not mounted.")
+        log("INFO: No mountpoint given, assuming not mounted.")
         return False
     return os.path.ismount(mountpoint)
 
@@ -39,12 +86,15 @@
 def unmount_volume():
     """Unmount the relation-requested C{mountpoint}"""
     success = False
-    mountpoint = hookenv.relation_get("mountpoint")
+    # Because relation-data is not guaranteed to exist on relation-departed
+    # We need to obtain mountpoint from the persistent relation data we saved
+    # during data-relation-changed
+    mountpoint = _get_persistent_data("mountpoint")
     if not mountpoint:
         log("INFO: No mountpoint defined by relation no unmount performed.")
         return
 
-    if not is_mounted():
+    if not is_mounted(mountpoint):
         log("%s is not mounted, Done." % mountpoint)
         success = True
     else:
@@ -144,21 +194,40 @@
             sys.exit(1)
 
 
+def _get_from_relation(relation, key, default=None):
+    relids = hookenv.relation_ids(relation)
+    for relid in relids:
+        for unit in hookenv.related_units(relid):
+            relation = hookenv.relation_get(unit=unit, rid=relid)
+            value = relation.get(key, default)
+            if value is not None:
+                return value
+    return None
+
+
 def mount_volume(device_path=None):
     """
-    Mount the attached volume, nfs or blockstoragebroker type, to the principal
-    requested C{mountpoint}.
+    Mount the attached volume, nfs or block-storage-broker type, to the
+    principal requested C{mountpoint}.
 
     If the C{mountpoint} required relation data does not exist, log the
     issue and exit. Otherwise attempt to initialize and mount the blockstorage
     or nfs device.
     """
-    mountpoint = hookenv.relation_get("mountpoint")
+    provider = get_provider()
+    if provider == "block-storage-broker":
+        # Called as part of a block storage hook, but mountpoint comes
+        # from the data relation.
+        mountpoint = _get_from_relation("data", "mountpoint")
+    else:
+        mountpoint = hookenv.relation_get("mountpoint")
+
     if not mountpoint:
         log("No mountpoint defined by relation. Cannot mount volume yet.")
         sys.exit(0)
 
-    if is_mounted():
+    _persist_data("mountpoint", mountpoint)  # Saved for our departed hook
+    if is_mounted(mountpoint):
         log("Volume (%s) already mounted" % mountpoint)
         # publish changes to data relation and exit
         for relid in hookenv.relation_ids("data"):
@@ -166,7 +235,6 @@
                 relid, relation_settings={"mountpoint": mountpoint})
         sys.exit(0)
 
-    provider = get_provider()
     options = "default"
     if provider == "nfs":
         relids = hookenv.relation_ids("nfs")
@@ -187,11 +255,11 @@
             sys.exit(1)
 
         device_path = "%s:%s" % (nfs_server, nfs_path)
-    elif provider == "blockstoragebroker":
+    elif provider == "block-storage-broker":
         if device_path is None:
-            log("ERROR: No persistent nova device provided by "
-                "blockstoragebroker.", hookenv.ERROR)
-            sys.exit(1)
+            log("Waiting for persistent nova device provided by "
+                "block-storage-broker.")
+            sys.exit(0)
 
         _assert_block_device(device_path)
         _format_device(device_path)
@@ -217,3 +285,9 @@
     for relid in hookenv.relation_ids("data"):
         hookenv.relation_set(
             relid, relation_settings={"mountpoint": mountpoint})
+
+
+def get_instance_id():
+    metadata = json.loads(
+        subprocess.check_output("curl %s" % METADATA_URL, shell=True))
+    return metadata["uuid"]

=== added directory 'hooks/storage-provider.d/block-storage-broker'
=== added file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-broken'
--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-broken	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-broken	2014-02-14 00:23:01 +0000
@@ -0,0 +1,6 @@
+#!/usr/bin/python
+
+import common_util
+
+
+common_util.unmount_volume()

=== added file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed'
--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	2014-02-14 00:23:01 +0000
@@ -0,0 +1,50 @@
+#!/usr/bin/python
+
+import common_util
+import sys
+import yaml
+from yaml.constructor import ConstructorError
+from charmhelpers.core import hookenv
+
+
+instance_id = common_util.get_instance_id()
+config_data = hookenv.config()
+volume_size = config_data.get("volume_size", None)
+volume_map = config_data.get("volume_map", None)
+volume_id = None
+label = None
+relids = hookenv.relation_ids("data")
+for relid in relids:
+    for unit in hookenv.related_units(relid):
+        label = "%s nova volume" % unit
+        if volume_map is not None:
+            # Grab the volume_label for our instance if it exists so we can
+            # request that volume-id from the block-storage-broker
+            try:
+                volume_map = yaml.load(volume_map)
+                if volume_map:
+                    volume_id = volume_map.get(unit, None)
+            except ConstructorError as e:
+                hookenv.log(
+                    "invalid YAML in 'volume-map': {}".format(e),
+                    hookenv.WARNING)
+
+if label is None:
+    hookenv.log(
+        "No data-relation-changed hook fired. Awaiting data-relation before "
+        "updating block-storage-broker relation")
+    sys.exit(0)
+
+relation_type = hookenv.relation_type()
+if relation_type == "block-storage":
+    relation_settings = {"instance-id": instance_id, "size": volume_size,
+                         "volume-label": label}
+    if volume_id is not None:
+        relation_settings["volume-id"] = volume_id
+    hookenv.relation_set(relation_settings=relation_settings)
+
+# Grab info from the block-storage relation regardless of whether we are in the
+# data relation
+device_path = common_util._get_from_relation(
+    "block-storage", "block-device-path")
+common_util.mount_volume(device_path)  # Will wait for mountpoint

=== added symlink 'hooks/storage-provider.d/block-storage-broker/data-relation-changed'
=== target is u'block-storage-relation-changed'
=== added symlink 'hooks/storage-provider.d/block-storage-broker/data-relation-departed'
=== target is u'block-storage-relation-broken'
=== modified file 'hooks/storage-provider.d/nfs/data-relation-changed'
--- hooks/storage-provider.d/nfs/data-relation-changed	2014-01-22 20:12:24 +0000
+++ hooks/storage-provider.d/nfs/data-relation-changed	2014-02-14 00:23:01 +0000
@@ -1,12 +1,5 @@
 #!/usr/bin/python
 
 import common_util
-import os
-import subprocess
-
-current_dir = os.path.dirname(os.path.realpath(__file__))
-config_changed = "%s/config-changed" % current_dir
-if os.path.exists(config_changed):
-    subprocess.check_call(config_changed)
 
 common_util.mount_volume()  # Will wait until mountpoint is set by principal

=== modified file 'hooks/storage-provider.d/nfs/data-relation-departed'
--- hooks/storage-provider.d/nfs/data-relation-departed	2014-01-22 02:26:11 +0000
+++ hooks/storage-provider.d/nfs/data-relation-departed	2014-02-14 00:23:01 +0000
@@ -2,4 +2,4 @@
 
 import common_util
 
-common_util.unmount_volume(remove_persistent_data=True)
+common_util.unmount_volume()

=== modified file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	2014-02-04 02:23:53 +0000
+++ hooks/test_common_util.py	2014-02-14 00:23:01 +0000
@@ -1,4 +1,5 @@
 import common_util as util
+import json
 import mocker
 import os
 import subprocess
@@ -12,6 +13,118 @@
         self.maxDiff = None
         util.FSTAB_FILE = self.makeFile()
         util.hookenv = TestHookenv({"provider": "nfs"})
+        self.addCleanup(setattr, os, "environ", os.environ)
+        self.charm_dir = self.makeDir()
+        os.environ = {"CHARM_DIR": self.charm_dir}
+
+    def test_wb_persist_data_creates_persist_file_if_it_doesnt_exist(self):
+        """
+        L{_persist_data} will create a new PERSIST_DATA_FILE in C{CHARM_DIR}
+        if the file does not already exist and store the provided C{key},
+        C{value} pairs.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        self.assertFalse(os.path.exists(persist_path))
+        util._persist_data("key", "value")
+        with open(persist_path) as infile:
+            data = json.load(infile)
+            self.assertEqual(data["key"], "value")
+
+    def test_wb_persist_data_appends_new_values_to_existing_persist_file(self):
+        """
+        L{_persist_data} will add new values to an existing PERSIST_DATA_FILE
+        in C{CHARM_DIR} without removing existing C{key}, C{value} pairs.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        data = {"key1": "value1"}
+        with open(persist_path, "w") as outfile:
+            outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+        util._persist_data("key2", "value2")
+        with open(persist_path) as infile:
+            data = json.load(infile)
+            self.assertEqual(data["key1"], "value1")
+            self.assertEqual(data["key2"], "value2")
+
+    def test_wb_get_persistent_data_returns_none_when_no_persist_file(self):
+        """
+        L{_get_persistent_data} will return C{None} for any C{key} search if
+        the PERSIST_DATA_FILE does not yet exist.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        self.assertFalse(os.path.exists(persist_path))
+        self.assertIsNone(util._get_persistent_data("key2"))
+        self.assertIsNone(util._get_persistent_data())
+
+    def test_wb_get_persistent_data_returns_none_when_no_key_exists(self):
+        """
+        L{_get_persistent_data} will return C{None} for any C{key} search when
+        the C{key} is not defined in the PERSIST_DATA_FILE.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        data = {"key1": "i-123"}
+        with open(persist_path, "w") as outfile:
+            outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+        self.assertIsNone(util._get_persistent_data("cant-find-this-key"))
+
+    def test_wb_get_persistent_data_returns_value_when_key_exists(self):
+        """
+        L{_get_persistent_data} will return the corresponding value for the
+        search C{key} provided when that C{key} is defined in the
+        PERSIST_DATA_FILE.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        data = {"key1": "i-123"}
+        with open(persist_path, "w") as outfile:
+            outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+        self.assertEqual(util._get_persistent_data("key1"), "i-123")
+
+    def test_wb_get_persistent_data_removes_key_when_remove_values_true(self):
+        """
+        L{_get_persistent_data} will return the corresponding value for the
+        search C{key} provided when that C{key} is defined in the
+        PERSIST_DATA_FILE. It will also remove that key from the
+        PERSIST_DATA_FILE when C{remove_values} is C{True}.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        data = {"key1": "i-123", "key2": "i-456"}
+        with open(persist_path, "w") as outfile:
+            outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+        self.assertEqual(
+            util._get_persistent_data("key1", remove_values=True), "i-123")
+        with open(persist_path) as infile:
+            new_data = json.load(infile)
+            self.assertEqual(new_data, {"key2": "i-456"})
+
+    def test_wb_get_persistent_data_removes_file_when_remove_values_true(self):
+        """
+        L{_get_persistent_data} will return the corresponding value for the
+        search C{key} provided when that C{key} is defined in the
+        PERSIST_DATA_FILE. It will also remove the PERSIST_DATA_FILE when
+        C{remove_values} is C{True} and the last remaining C{key} has been
+        removed.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        data = {"key1": "i-123"}
+        with open(persist_path, "w") as outfile:
+            outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+        self.assertEqual(
+            util._get_persistent_data("key1", remove_values=True), "i-123")
+        self.assertFalse(os.path.exists(persist_path))
+
+    def test_wb_get_persistent_data_removes_file_when_no_key_specified(self):
+        """
+        L{_get_persistent_data} will return all existing values and remove the
+        PERSIST_DATA_FILE when C{remove_values} is C{True} and no C{key} is
+        specified.
+        """
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        data = {"key1": "i-123", "key2": "i-456"}
+        with open(persist_path, "w") as outfile:
+            outfile.write(unicode(json.dumps(data, ensure_ascii=False)))
+        self.assertEqual(
+            util._get_persistent_data(key=None, remove_values=True),
+            data)
+        self.assertFalse(os.path.exists(persist_path))
 
     def test_get_provider(self):
         """L{get_provider} returns the C{provider} config setting if present"""
@@ -21,8 +134,6 @@
         """
         L{get_provider} exits with an error if C{provider} configuration unset.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_config", {"provider": "nfs"})
         util.hookenv._config = {}  # No provider defined
 
         self.assertRaises(SystemExit, util.get_provider)
@@ -31,45 +142,32 @@
 
     def test_is_mounted_when_mountpoint_is_mounted(self):
         """
-        L{is_mounted} returns C{True} when persisted mountpath is mounted
+        L{is_mounted} returns C{True} when the mountpath is mounted.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
-        util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
-        self.assertEqual(util.hookenv.relation_get("mountpoint"), "/mnt/this")
-
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
         self.mocker.result(True)
         self.mocker.replay()
-
-        self.assertTrue(util.is_mounted())
+        self.assertTrue(util.is_mounted("/mnt/this"))
 
     def test_is_mounted_when_mountpoint_is_not_mounted(self):
         """
-        L{is_mounted} returns C{False} when persisted mountpath is not mounted
+        L{is_mounted} returns C{False} when the mountpath is not mounted.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
-        util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
-
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
         self.mocker.result(False)
         self.mocker.replay()
 
-        self.assertFalse(util.is_mounted())
+        self.assertFalse(util.is_mounted("/mnt/this"))
 
-    def test_is_mounted_when_mountpoint_not_defined_by_relation(self):
+    def test_is_mounted_when_mountpoint_not_defined(self):
         """
-        L{is_mounted} returns C{False} while the relation doesn't define
-        C{mountpoint}
+        L{is_mounted} returns C{False} when the given mountpoint is undefined.
         """
         self.assertIsNone(util.hookenv.relation_get("mountpoint"))
-        self.assertFalse(util.is_mounted())
-        message = "No mountpoint defined by relation assuming not mounted."
+        self.assertFalse(util.is_mounted(None))
+        message = "No mountpoint given, assuming not mounted."
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
@@ -78,23 +176,18 @@
         L{unmount_volume} will log a message and do nothing if mountpoint is
         not defined.
         """
-        self.assertIsNone(util.hookenv.relation_get("mountpoint"))
+        self.assertIsNone(util._get_persistent_data("mountpoint"))
         util.unmount_volume()
 
         message = "No mountpoint defined by relation no unmount performed."
         self.assertTrue(util.hookenv.is_logged(message), "Message not logged")
-        self.assertFalse(util.is_mounted())
 
     def test_unmount_volume_when_defined_mountpoint_not_mounted(self):
         """
         L{unmount_volume} will log a message and do nothing if the defined
-        mountpoint is not mounted and C{remove_persistent_data} parameter is
-        unspecified.
+        mountpoint is not mounted.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
-        util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
+        util._persist_data("mountpoint", "/mnt/this")
 
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
@@ -113,10 +206,7 @@
         to get a success with a sleep(5) in between attempts. If all attempts
         fail, L{umount_volume} will log the failure and exit 1.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
-        util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
+        util._persist_data("mountpoint", "/mnt/this")
 
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
@@ -144,10 +234,7 @@
         L{unmount_volume} will unmount a mounted filesystem calling the shell
         command umount and will log the success.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
-        util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
+        util._persist_data("mountpoint", "/mnt/this")
 
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
@@ -390,15 +477,11 @@
         """
         L{mount_volume} will read incoming principal relation data for the
         C{mountpoint}. When the requested C{mountpoint} is determined as
-        mounted,  it will log a message and will set juju relation
-        C{mountpoint} in response.
+        mounted,  it will then persist the mountpoint data, log a message and
+        set juju relation C{mountpoint} in response.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
         util.hookenv._incoming_relation_data = (
             (None, "mountpoint", "/mnt/this"),)
-        self.addCleanup(
-            setattr, util.hookenv, "_outgoing_relation_data", ())
 
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
@@ -407,9 +490,16 @@
 
         result = self.assertRaises(SystemExit, util.mount_volume)
         self.assertEqual(result.code, 0)
+        self.assertIn(("mountpoint", "/mnt/this"),
+                      util.hookenv._outgoing_relation_data)
         message = "Volume (/mnt/this) already mounted"
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        with open(persist_path) as infile:
+            data = json.load(infile)
+            self.assertEqual(data["mountpoint"], "/mnt/this")
+            
 
     def test_mount_volume_mountpoint_from_relation_data(self):
         """
@@ -417,8 +507,6 @@
         already defined in relation data and the requested C{mountpoint} is
         already mounted.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
         util.hookenv._incoming_relation_data = (
             (None, "mountpoint", "/mnt/this"),)
         self.assertEqual(util.hookenv.relation_get("mountpoint"), "/mnt/this")
@@ -441,8 +529,6 @@
         values: C{mount_path}, C{mount_server}, C{mount_options} or
         C{mount_type}.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
         data = (
             (None, "mountpoint", "/mnt/this"),
             ("nfs:1", "private-address", "me.com"),
@@ -476,16 +562,12 @@
         message and the storage charm will call juju relation-set C{mountpoint}
         to report the successful initialization to the principal.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
         util.hookenv._incoming_relation_data = (
             (None, "mountpoint", "/mnt/this"),
             ("nfs:1", "private-address", "me.com"),
             ("nfs:1", "options", "someopts"),
             ("nfs:1", "mountpoint", "/nfs/server/path"),
             ("nfs:1", "fstype", "nfs"))
-        self.addCleanup(
-            setattr, util.hookenv, "_outgoing_relation_data", ())
 
         # Mock not mounted
         ismount = self.mocker.replace(os.path.ismount)
@@ -520,49 +602,44 @@
         self.assertEqual(
             util.hookenv._outgoing_relation_data,
             (("mountpoint", "/mnt/this"),))
+        # Perserved mountpoint for future departed hook runs
+        persist_path = "%s/%s" % (self.charm_dir, util.PERSIST_DATA_FILE)
+        with open(persist_path) as infile:
+            data = json.load(infile)
+            self.assertEqual(data["mountpoint"], "/mnt/this")
 
-    def test_mount_volume_bsb_provider_error_no_device_path_provided(self):
-        """
-        L{mount_volume} will exit with errors if called before C{device_path}
-        is specified by the blockstoragebroker charm
-        saved from either the C{start} or C{data-relation-changed} hook runs.
-        """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
+    def test_mount_volume_bsb_provider_no_device_path_provided(self):
+        """
+        L{mount_volume} will exit early if called before C{device_path}
+        is specified by the block-storage-broker charm.
+        """
+        util.hookenv._config = (("provider", "block-storage-broker"),)
         util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
-
-        self.addCleanup(
-            setattr, util.hookenv, "_config", (("provider", "nfs"),))
-        util.hookenv._config = (("provider", "blockstoragebroker"),)
-
-        self.assertEqual(util.get_provider(), "blockstoragebroker")
-        self.assertRaises(SystemExit, util.mount_volume)
+            ("data:1", "mountpoint", "/mnt/this"),)
+
+        self.assertEqual(util.get_provider(), "block-storage-broker")
+        result = self.assertRaises(SystemExit, util.mount_volume)
+        self.assertEqual(result.code, 0)
         message = (
-            "ERROR: No persistent nova device provided by blockstoragebroker.")
+            "Waiting for persistent nova device provided by "
+            "block-storage-broker.")
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_mount_volume_nova_provider_success(self):
+    def test_mount_volume_bsb_provider_success(self):
         """
         L{mount_volume} reads principal relation data for the C{mountpoint}.
         When storage config is set to the C{nova} provider, L{mount_volume}
         will read the optional C{device_path} parameter provided by the nova
-        provider's  C{data-relation-changed} hook.
+        provider's C{data-relation-changed} hook.
         The success of the mount will log a message and the storage charm will
         call relation-set C{mountpoint} to report the successful mount to the
         principal.
         """
-        self.addCleanup(
-            setattr, util.hookenv, "_incoming_relation_data", ())
         util.hookenv._incoming_relation_data = (
-            (None, "mountpoint", "/mnt/this"),)
+            ("data:1", "mountpoint", "/mnt/this"),)
 
-        self.addCleanup(
-            setattr, util.hookenv, "_config", (("provider", "nfs"),))
-        util.hookenv._config = (("provider", "blockstoragebroker"),)
-        self.addCleanup(
-            setattr, util.hookenv, "_outgoing_relation_data", ())
+        util.hookenv._config = (("provider", "block-storage-broker"),)
 
         ismount = self.mocker.replace(os.path.ismount)
         ismount("/mnt/this")
@@ -590,7 +667,7 @@
         mount("mount -t ext4 /dev/vdx /mnt/this", shell=True)
         self.mocker.replay()
 
-        self.assertEqual(util.get_provider(), "blockstoragebroker")
+        self.assertEqual(util.get_provider(), "block-storage-broker")
         util.mount_volume("/dev/vdx")
         message = "Mount (/mnt/this) successful"
         self.assertTrue(

=== modified file 'hooks/testing.py'
--- hooks/testing.py	2014-01-31 23:49:37 +0000
+++ hooks/testing.py	2014-02-14 00:23:01 +0000
@@ -73,6 +73,9 @@
     def local_unit(self):
         return os.environ["JUJU_UNIT_NAME"]
 
+    def charm_dir(self):
+        return os.environ["CHARM_DIR"]
+
     def log(self, message, level):
         self._log = self._log + (message,)
 

=== modified file 'metadata.yaml'
--- metadata.yaml	2014-01-09 04:19:14 +0000
+++ metadata.yaml	2014-02-14 00:23:01 +0000
@@ -16,3 +16,5 @@
         scope: container
     nfs:
         interface: mount
+    block-storage:
+        interface: volume-request