Merge lp:~tribaal/charms/precise/storage/refactor-mount-volume into lp:charms/storage

[Chris Glass]

This branch refactors the switching logic in (what used to be) common_utils.py's mount_volume() function.

I found it strange to have to switch on the provider name again in that common code, since the switch was already effectively done by the storage-provider logic.
Instead of creating a new "storage-only" hook, I decided to keep the current setup as is and simply split the mount_volume() function in two separate helpers: prepare_volume_mount() and call_mount_volume().

What used to be the "switched" code is now inline in the hook's body, so we don't need to switch again - it's clear where and what each piece of code does.

Changing this required me to unwire some of the provider-specific tests. I moved them to the provider subfolders instead, since it makes no sense in my opinion to keep them at the common level. Right now they are not wired in, but there needs to be more refactoring first, and the branch is becoming a monster so I'd like to have eyes on the code first, to at least work the rough edges out and validate the concept.

[Adam Collard (adam-collard)]

Some English fixes plus a stray change at the end

[Chris Glass (tribaal)]

Fixed all inline comments.

[David Britton (dpb)]

Hi Chris -- This change looks wonderful, could you please fix the merge conflict. Also, can you wire the tests back up before pushing up, or are you wanting to do another branch for that? Thanks again for doing all this, much needed.

review: Approve

[David Britton (dpb)]

review: Needs Information

[Chris Glass (tribaal)]

Merge conflict should be gone.. Let's hope someone doesn't commit something on the pre-refactor codebase again :)

[David Britton (dpb)]

Hi Chris -- some more feedback:

[1] please link nfs-relation-changed to 'hooks.py' in the nfs/ dir.

[2] You seem to be missing all the glue code in nfs/hooks.py to actual
run hooks, and you are missing an annotation for the hooks-env. Adding
these things, along with the module imports necessary made a mount
actually work. cool!

[3] could you pastebin the sample deployer.yaml file, or the juju
command steps you take, so I can make sure I'm testing apples to apples.
As you know, this charm is new, so there could be subtle bugs relating
to postgresql vs whatever you are using. I'd like to make sure I'm
doing a good job testing.

[4] please file a bug to re-wire the tests in the nfs/ dir. I think it
should be as simple as adding anothing command in the make 'test'
target. I don't see a reason we need to get overly fancy with things at
this point. If you think of a better idea, add to the bug.

I'm going to mark this needs fixing, and set the MP back to WIP. Please
simply set back to 'needs review' when ready for another round.

Thanks for the change, again it looks like exactly the right direction,
just needs a bit of finishing (which I think you alluded to already, so
this probably isn't a surprise to you).

--
David Britton <email address hidden>

[David Britton (dpb)]

review: Needs Fixing

[Chris Glass (tribaal)]

Hi David,

Thanks a lot for the feedback - that's exactly what I was after (more eyes on the code after such a refactoring is awesome).

So, [1] and [2]: Duh. Fixed. Guess who forgot to commit changes? This guy.

For [3], I'm using my own in-development charm, and it's not 100% ready for prime time yet, but should work as another, simple storage client. Steps to use it:

juju bootstrap
juju deploy cs:~tribaal/trusty/ubuntu-mirror
juju deploy (this branch)
juju set storage "provider=nfs" # Or any other provider, for that matter.
# Point to your NFS storage provider here.
juju add-relation storage ubuntu-mirror

Once the mountpoint is available and mounted, the mirror will create a bunch of folders first, and will start syncing after a while (this takes a *long* time as it will rsync around 2Tb so I advise you not to wait for it to finish :) )

As for [4]: https://bugs.launchpad.net/charms/+source/storage/+bug/1368038

[Chris Glass (tribaal)]

Switching this to needs review again.

[Cory Johns (johnsca)]

These refactors look good and everything seems to work, but when running the moved tests (PYTHONPATH=hooks:$PYTHONPATH CHARM_DIR=`pwd` trial hooks/storage-provider.d/nfs/test_nfs_provider.py) had failures due to missing the setUp and imports that were in place in test_common_util.py, as well as the util.mount_volume -> util.{prepare,call}_volume_mount change.

I suggest fixing the tests and changing the `make test` target to use the PYTHONPATH modification to run these tests (I'd also put the tests in a top-level unit_tests folder so they're more easily discoverable, but that's up to you, as long as the `make test` target can run them).

review: Needs Fixing

[Chris Glass (tribaal)]

Thanks for the review, and indeed, I think tweaking PYTHONPATH is exactly one of the ways to go (although it's not really nice to have to tweak things this way, it's quite un-pythoninc

However I would still like to address this in a later branch, since the diff is already quite large. The tests need rewriting, I think it's better to submit the changes in a separate branch to make sure the changes don't get drowned in the refactoring changes.

If you both insist, I can rewire things in this branch, but I suspect the diff will grow quite drastically.

[David Britton (dpb)]

Hi Chris -- I think the unit tests are fine as is, thanks for the explination here and on IRC. We intend to fix those in a follow-on branch. I just some tests again, and it's failing on postgresql:

unit-postgresql-0: 2014-09-26 14:30:10 INFO juju-log data:1: Making dir /srv/data/postgresql postgres:root 700
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed Traceback (most recent call last):
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed File "/var/lib/juju/agents/unit-postgresql-0/charm/hooks/data-relation-changed", line 2370, in <module>
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed hooks.execute(sys.argv)
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed File "/var/lib/juju/agents/unit-postgresql-0/charm/hooks/charmhelpers/core/hookenv.py", line 478, in execute
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed self._hooks[hook_name]()
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed File "/var/lib/juju/agents/unit-postgresql-0/charm/hooks/data-relation-changed", line 2317, in data_relation_changed
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed config_changed(mount_point=external_volume_mount)
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed File "/var/lib/juju/agents/unit-postgresql-0/charm/hooks/data-relation-changed", line 918, in config_changed
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed if config_changed_volume_apply(mount_point=mount_point):
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed File "/var/lib/juju/agents/unit-postgresql-0/charm/hooks/data-relation-changed", line 865, in config_changed_volume_apply
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed host.mkdir(new_dir, owner="postgres", perms=0o700)
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed File "/var/lib/juju/agents/unit-postgresql-0/charm/hooks/charmhelpers/core/host.py", line 135, in mkdir
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed os.chown(realpath, uid, gid)
unit-postgresql-0: 2014-09-26 14:30:10 INFO data-relation-changed OSError: [Errno 22] Invalid argument: '/srv/data/postgresql'
unit-postgresql-0: 2014-09-26 14:30:10 ERROR juju.worker.uniter uniter.go:486 hook failed: exit status 1

I thought we added a creation of the requested mountpoint? Did that get lost in the refactor? For reference, here is the bundle I'm using:

http://paste.ubuntu.com/8433298/

I'm going to mark this needs fixing, I'd like to make sure at least our base tests pass with nfs before we put this in.

review: Needs Fixing

[David Britton (dpb)]

Pastebin for that error message, sorry about the formatting in here:

http://paste.ubuntu.com/8433316/

[Cory Johns (johnsca)]

David,

I'm not sure if that's an error with the charm code, which does ensure that the mount point exists (the line that's failing is inside charmhelpers.core.host.mkdir which is ensuring that the mount point exists or creating it if it doesn't).

It appears that error is an NFS permissions issue. This blog post http://blog.illogicalextend.com/troubles-writing-to-nfs-share/ suggests using the no_root_squash option for the NFS mount, but the current NFS charm does include that option (if that's how you were doing this test). But I think the problem lies on the source side of the NFS provider, or at least in the matching up of permissions over NFS.

[Cory Johns (johnsca)]

Chris,

I understand wanting to keep the review to a manageable size, but I'm personally leery of giving my +1 review when you're removing coverage of previously tested code. The tests are there to give confidence that the code is continuing to work as expected after exactly this sort of refactor. So, to me, even if the tests aren't added back to the Makefile in this branch, they should at least be fixed so that they're passing if run manually. Then, the later branch can deal with how best to wire them back in so that they're picked up by `make test`.

But it does technically pass on `make test`, and David is a ~charmer, so if he's ok with delaying fixing the tests (once the postgres issue is resolved), I certainly won't fight about it. :)

[David Britton (dpb)]

On Fri, Sep 26, 2014 at 03:30:11PM -0000, Cory Johns wrote:
> Chris,
>
> I understand wanting to keep the review to a manageable size,

Cory, after speaking with Chris, I think he has an idea about how to
better wire in the tests. so I'm going to set this back to WIP for now,
just so it's clear.

Thanks for the review, and I'll look at the NFS issue more closely, it
could ideed just be permissions.

--
David Britton <email address hidden>

[Chris Glass (tribaal)]

Yes indeed, I think it can work better to change how the charm is structured instead - falling straight in the refactoring category :)

So to summarize the IRC discussion (for future reference):
I think I'll rename the storage-providers.d directory to just "storage-providers" or "providers" to make it a proper python module, so everything is one nice tidy tree and we don't need to muck around with PYTHONPATH.

I'll rewire as much as the tests as possible in this branch using this strategy - but the bulk of them will have to be rewritten. I'll try to do so to "best effort" in this branch, and will put another branch up for review doing most of the tests rewriting - nobody wants to decrease coverage, as others have rightfully pointed out.

Thanks a lot for the feedback - it's hard to get these big refactorings right without other eyes on the code. So thanks a lot for your time and pointers David and Cory :) I'll keep hammering at it some more :)

Unmerged revisions

44. By Chris Glass on 2014-09-11

Addressed first review comments.

43. By Chris Glass on 2014-09-09

Merging trunk. Rewrote the submitted fstab fixing code since the code changed a lot (https://code.launchpad.net/~moon127/charms/precise/storage/fstab-fix/+merge/232382).

42. By Chris Glass on 2014-09-09

Rewrote the equivalent of https://code.launchpad.net/~moon127/charms/precise/storage/fstab-fix/+merge/232382
since most of that code moved around.

41. By Chris Glass on 2014-09-08

Fixed inline comments - stray changes and spelling mistakes.

40. By Chris Glass on 2014-08-26

Removed the now useless mount_volume() function.

39. By Chris Glass on 2014-08-26

This refactors the provider-specific tests into the relevant provider subfolder
so that the logic testing common_utils can focus on just the common parts.

Right now the provider-specific tests are not rewired, since this will need a
little more work and this branch is becoming quite big.

38. By Chris Glass on 2014-08-23

Added .coverage to .bzrignore

37. By Chris Glass on 2014-08-23

Most of the refactoring is working - need to port and write tests.

=== modified file '.bzrignore'
--- .bzrignore	2014-01-31 23:49:37 +0000
+++ .bzrignore	2014-09-11 05:39:25 +0000
@@ -1,2 +1,3 @@
 _trial_temp
 charm-helpers
+.coverage

=== modified file 'README.md'
--- README.md	2014-05-16 14:40:47 +0000
+++ README.md	2014-09-11 05:39:25 +0000
@@ -6,6 +6,53 @@
 interface, and the charmed application will start writing data into the provided
 storage.
 
+General idea
+-------------
+
+The storage charm is a subordinate charm that you attach to a main charm in need
+of block storage space on one side (using a scope:container relation), and to
+a storage provider on the other side (using a normal relation).
+
+You need to explicitly configure what provider to use in the subordinate
+configuration, using the aptly named "provider" config variable.
+
+Example usage
+--------------
+
+The following will try to give you an idea of typical usage, using NFS as an
+example:
+
+juju bootstrap
+juju deploy my-main-service
+juju deploy storage  # this charm
+juju deploy some-charm-exposing-nfs
+juju set storage "provider=nfs"
+juju add-relation nfs storage
+juju add-relation storage my-main-service
+
+The charm in need of storage should send a "mountpoint=/where/I/need/space"
+message to the storage subordinate using a "set-relation" call:
+
+// data-relation-changed on my-main-service
+set-relation "mountpoint=/please/mount/here"
+
+Once the storage is ready, the subordinate will also set-relation, so you can
+test whether the mountpoint is ready by checking "get-relation" on the same hook
+Since the subordinate will set-relation, the "relation-changed" hook will fire
+again:
+
+// data-relation-changed on my-main-service
+ready = $(get-relation "mountpoint")
+if $ready == "/please/mount/here":
+    do something with the mount point
+else:
+    exit(0)  # we'll get called later, when it's ready.
+
+On the storage-provider side of the relation, the relevant "relation changed"
+hook will be called once the mountpoint has been requested, and, once it is
+available, the provider will send the 
+relation-set "mountpoint=/please/mount/here" message back to the main charm.
+
 
 Provider-Specific Features
 --------------------------

=== modified file 'hooks/common_util.py'
--- hooks/common_util.py	2014-08-27 11:01:14 +0000
+++ hooks/common_util.py	2014-09-11 05:39:25 +0000
@@ -123,7 +123,7 @@
         sys.exit(1)
 
 
-def _assert_block_device(device_path):
+def assert_block_device(device_path):
     """C{device_path} is a valid block device or exit in error"""
     isBlockDevice = False
     for x in range(0, 10):
@@ -146,7 +146,7 @@
         sys.exit(1)
 
 
-def _set_label(device_path, label):
+def set_label(device_path, label):
     """Set label if necessary on C{device_path}"""
     if not os.path.exists(device_path):
         log("ERROR: Cannot set label of %s. Path does not exist" % device_path)
@@ -177,7 +177,7 @@
     return subprocess.call(command, shell=True) == 0
 
 
-def _format_device(device_path):
+def format_device(device_path):
     """
     Create an ext4 partition, if needed, for C{device_path} and fsck that
     partition.
@@ -211,21 +211,18 @@
     return None
 
 
-def mount_volume(device_path=None):
+def prepare_volume_mount(device_path=None):
     """
-    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.
+    Code that is common to all storage providers and does basic sanity check
+    about the mountpoint, for example, noop if the "mountpoint" was never
+    requested from the "data" relation.
     """
     # mountpoint defined by our related principal in the "data" relation
     mountpoint = _get_from_relation("data", "mountpoint")
 
     if not mountpoint:
         log("No mountpoint defined by relation. Cannot mount volume yet.")
-        return
+        sys.exit(0)
 
     _persist_data("mountpoint", mountpoint)  # Saved for our departed hook
     if is_mounted(mountpoint):
@@ -234,51 +231,18 @@
         for relid in hookenv.relation_ids("data"):
             hookenv.relation_set(
                 relid, relation_settings={"mountpoint": mountpoint})
-        return
-
-    options = "defaults"
-    provider = get_provider()
-    if provider == "nfs":
-        relids = hookenv.relation_ids("nfs")
-        for relid in relids:
-            for unit in hookenv.related_units(relid):
-                relation = hookenv.relation_get(unit=unit, rid=relid)
-                # XXX Only handle 1 related nfs unit. No nfs cluster support
-                nfs_server = relation.get("private-address", None)
-                nfs_path = relation.get("mountpoint", None)
-                fstype = relation.get("fstype", None)
-                options = relation.get("options", "defaults")
-                break
-
-        if len(relids) == 0 or None in [nfs_server, nfs_path, fstype]:
-            log("Missing required relation values. "
-                "nfs-relation-changed hook needs to run first.")
-            return
-
-        device_path = "%s:%s" % (nfs_server, nfs_path)
-    elif provider == "block-storage-broker":
-        if device_path is None:
-            log("Waiting for persistent nova device provided by "
-                "block-storage-broker.")
-            sys.exit(0)
-
-        _assert_block_device(device_path)
-        if os.path.exists("%s1" % device_path):
-            # Then we are supporting upgrade-charm path for deprecated
-            # postgresql volume-map config parameter. Postgresql used to
-            # partition the volume into a single partition at 100% of the
-            # volume size, but the storage charm doesn't do that partitioning
-            device_path = "%s1" % device_path
-            log("Mounting %s as the device path as the root device is already "
-                "partitioned." % device_path)
-        _format_device(device_path)
-        _set_label(device_path, mountpoint)
-        fstype = subprocess.check_output(
-            "blkid -o value -s TYPE %s" % device_path, shell=True).strip()
-    else:
-        log("ERROR: Unknown provider %s. Cannot mount volume." % hookenv.ERROR)
-        sys.exit(1)
-
+
+    return mountpoint
+
+
+def call_volume_mount(device_path, options="defaults", fstype="ext3"):
+    """
+    Actually perform the mounting of the requested volume, forwarding the call
+    to the system's "mount" command (passing along the options and filesystem
+    type specified).
+    """
+    # mountpoint defined by our related principal in the "data" relation
+    mountpoint = _get_from_relation("data", "mountpoint")
     if not os.path.exists(mountpoint):
         os.makedirs(mountpoint)
 

=== modified file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed'
--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	2014-03-07 08:01:15 +0000
+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed	2014-09-11 05:39:25 +0000
@@ -1,22 +1,32 @@
 #!/usr/bin/python
 
+import subprocess
 import common_util
 import sys
+import os
 import yaml
 from yaml.constructor import ConstructorError
-from charmhelpers.core import hookenv
+from charmhelpers.core.hookenv import (
+    log,
+    config,
+    relation_ids,
+    relation_type,
+    relation_set,
+    related_units,
+    WARNING,
+)
 
 
 instance_id = common_util.get_instance_id()
 availability_zone = common_util.get_availability_zone()
-config_data = hookenv.config()
+config_data = 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")
+relids = relation_ids("data")
 for relid in relids:
-    for unit in hookenv.related_units(relid):
+    for unit in related_units(relid):
         label = "%s %s volume" % (unit, availability_zone)
         if volume_map is not None:
             # Grab the volume_label for our instance if it exists so we can
@@ -26,26 +36,46 @@
                 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)
+                log("invalid YAML in 'volume-map': {}".format(e), WARNING)
 
 if label is None:
-    hookenv.log(
-        "No data-relation-changed hook fired. Awaiting data-relation before "
+    log("No data-relation-changed hook fired. Awaiting data-relation before "
         "updating block-storage-broker relation")
     sys.exit(0)
 
-relation_type = hookenv.relation_type()
+relation_type = 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)
+    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
+
+# Perform common pre-mount tasks and checks.
+mountpoint = common_util.prepare_volume_mount(device_path)
+
+# Prepare mount options
+if device_path is None:
+    log("Waiting for persistent nova device provided by block-storage-broker.")
+    sys.exit(0)
+
+common_util.assert_block_device(device_path)
+if os.path.exists("%s1" % device_path):
+    # Then we are supporting upgrade-charm path for deprecated
+    # postgresql volume-map config parameter. Postgresql used to
+    # partition the volume into a single partition at 100% of the
+    # volume size, but the storage charm doesn't do that partitioning
+    device_path = "%s1" % device_path
+    log("Mounting %s as the device path as the root device is already "
+        "partitioned." % device_path)
+common_util.format_device(device_path)
+common_util.set_label(device_path, mountpoint)
+fstype = subprocess.check_output(
+    "blkid -o value -s TYPE %s" % device_path, shell=True).strip()
+
+common_util.call_volume_mount(device_path, fstype=fstype)

=== added file 'hooks/storage-provider.d/block-storage-broker/test_block_storage_provider.py'
--- hooks/storage-provider.d/block-storage-broker/test_block_storage_provider.py	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/block-storage-broker/test_block_storage_provider.py	2014-09-11 05:39:25 +0000
@@ -0,0 +1,147 @@
+import mocker
+import os
+import subprocess
+
+import common_util as util
+
+class TestBlockStorageBroker(mocker.MockerTestCase):
+
+    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 = (
+            ("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 = (
+            "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_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.
+        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.
+        """
+        util.hookenv._incoming_relation_data = (
+            ("data:1", "mountpoint", "/mnt/this"),)
+
+        util.hookenv._config = (("provider", "block-storage-broker"),)
+
+        ismount = self.mocker.replace(os.path.ismount)
+        ismount("/mnt/this")
+        self.mocker.result(False)  # Mock not mounted
+
+        assert_device = self.mocker.replace(util.assert_block_device)
+        assert_device("/dev/vdx")
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdx1")
+        self.mocker.result(False)
+        create_partition = self.mocker.replace(
+            util.format_device)
+        create_partition("/dev/vdx")
+
+        set_label = self.mocker.replace(util.set_label)
+        set_label("/dev/vdx", "/mnt/this")
+
+        # Ensure blkid is called to discover fstype of nova device
+        get_fstype = self.mocker.replace(subprocess.check_output)
+        get_fstype("blkid -o value -s TYPE /dev/vdx", shell=True)
+        self.mocker.result("ext4\n")
+
+        exists("/mnt/this")
+        self.mocker.result(True)
+
+        mount = self.mocker.replace(subprocess.check_call)
+        mount("mount -t ext4 /dev/vdx /mnt/this", shell=True)
+        self.mocker.replay()
+
+        self.assertEqual(util.get_provider(), "block-storage-broker")
+        util.mount_volume("/dev/vdx")
+        message = "Mount (/mnt/this) successful"
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+        # Wrote proper fstab mount info to mount on reboot
+        fstab_content = "/dev/vdx /mnt/this ext4 default 0 0"
+        with open(util.FSTAB_FILE) as input_file:
+            self.assertEqual(input_file.read(), fstab_content)
+
+        # Reported success to principal
+        self.assertEqual(
+            util.hookenv._outgoing_relation_data,
+            (("mountpoint", "/mnt/this"),))
+
+    def test_mount_volume_bsb_provider_success_postgresql_charm_upgrade(self):
+        """
+        Old versions of the postgresql charm would have created a single
+        partition with max size from an external attached volume if the user
+        had used postgresql charm's deprecated volume-map parameter. When the
+        storage charm sees a single partition on the attached device, it will
+        attempt to mount that partition instead of the root device.
+        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.
+        """
+        util.hookenv._incoming_relation_data = (
+            ("data:1", "mountpoint", "/mnt/this"),)
+
+        util.hookenv._config = (("provider", "block-storage-broker"),)
+
+        ismount = self.mocker.replace(os.path.ismount)
+        ismount("/mnt/this")
+        self.mocker.result(False)  # Mock not mounted
+
+        assert_device = self.mocker.replace(util.assert_block_device)
+        assert_device("/dev/vdx")
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdx1")
+        self.mocker.result(True)
+        create_partition = self.mocker.replace(
+            util.format_device)
+        create_partition("/dev/vdx1")
+
+        set_label = self.mocker.replace(util.set_label)
+        set_label("/dev/vdx1", "/mnt/this")
+
+        # Ensure blkid is called to discover fstype of nova device
+        get_fstype = self.mocker.replace(subprocess.check_output)
+        get_fstype("blkid -o value -s TYPE /dev/vdx1", shell=True)
+        self.mocker.result("ext4\n")
+
+        exists("/mnt/this")
+        self.mocker.result(True)
+
+        mount = self.mocker.replace(subprocess.check_call)
+        mount("mount -t ext4 /dev/vdx1 /mnt/this", shell=True)
+        self.mocker.replay()
+
+        self.assertEqual(util.get_provider(), "block-storage-broker")
+        util.mount_volume("/dev/vdx")
+        messages = ["Mount (/mnt/this) successful",
+                    "Mounting /dev/vdx1 as the device path as the root device "
+                    "is already partitioned."]
+        for message in messages:
+            self.assertTrue(
+                util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+        # Wrote proper fstab mount info to mount on reboot
+        fstab_content = "/dev/vdx1 /mnt/this ext4 default 0 0"
+        with open(util.FSTAB_FILE) as input_file:
+            self.assertEqual(input_file.read(), fstab_content)
+
+        # Reported success to principal
+        self.assertEqual(
+            util.hookenv._outgoing_relation_data,
+            (("mountpoint", "/mnt/this"),))

=== modified file 'hooks/storage-provider.d/nfs/data-relation-changed'
--- hooks/storage-provider.d/nfs/data-relation-changed	2014-03-06 16:02:32 +0000
+++ hooks/storage-provider.d/nfs/data-relation-changed	1970-01-01 00:00:00 +0000
@@ -1,6 +0,0 @@
-#!/usr/bin/python
-
-import common_util
-
-# Will NOOP until mountpoint is set by principal and relation setup with nfs
-common_util.mount_volume()

=== target is u'hooks.py'
=== added file 'hooks/storage-provider.d/nfs/hooks.py'
--- hooks/storage-provider.d/nfs/hooks.py	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/hooks.py	2014-09-11 05:39:25 +0000
@@ -0,0 +1,50 @@
+#!/usr/bin/python
+import sys
+
+from charmhelpers.core.hookenv import (
+    log,
+    relation_ids,
+    related_units,
+    relation_get,
+    Hooks,
+    UnregisteredHookError,
+)
+
+import common_util
+
+hooks = Hooks()
+
+
[e-mail]("data-relation-changed")
+def data_relation_changed():
+    # Run the common pre-mount check and code.
+    common_util.prepare_volume_mount()
+
+    relids = relation_ids("nfs")
+    for relid in relids:
+        for unit in related_units(relid):
+            relation = relation_get(unit=unit, rid=relid)
+
+            # XXX Only handle 1 related nfs unit. No nfs cluster support (yet?)
+            nfs_server = relation.get("private-address", None)
+            nfs_path = relation.get("mountpoint", None)
+            fstype = relation.get("fstype", None)
+            options = relation.get("options", "defaults")
+            break
+
+    if len(relids) == 0 or None in [nfs_server, nfs_path, fstype]:
+        log("Missing required relation values. "
+            "nfs-relation-changed hook needs to run first.")
+        return
+
+    device_path = "%s:%s" % (nfs_server, nfs_path)
+
+    # Actually perform the volume mount, calling the "mount" system command.
+    common_util.call_volume_mount(device_path, options=options, fstype=fstype)
+
+
+if __name__ == '__main__':
+    try:
+        hooks.execute(sys.argv)
+    except UnregisteredHookError as e:
+        log('Unknown hook {} - skipping.'.format(e))

=== modified symlink 'hooks/storage-provider.d/nfs/nfs-relation-changed'
=== target changed u'data-relation-changed' => u'hooks.py'
=== added file 'hooks/storage-provider.d/nfs/test_nfs_provider.py'
--- hooks/storage-provider.d/nfs/test_nfs_provider.py	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/test_nfs_provider.py	2014-09-11 05:39:25 +0000
@@ -0,0 +1,98 @@
+import mocker
+import common_util as util
+import os
+import subprocess
+import json
+
+
+class TestNFSProvider(mocker.MockerTestCase):
+    # TODO: This class is not yet run by "make test". Make it run!
+
+    def test_mount_volume_nfs_provider_no_nfs_hook_relation_data(self):
+        """
+        L{mount_volume} will exit in error when set as C{nfs} provider and the
+        nfs relation data does not contain any of the required
+        values: C{mount_path}, C{mount_server}, C{mount_options} or
+        C{mount_type}.
+        """
+        data = (
+            ("data:1", "mountpoint", "/mnt/this"),
+            ("nfs:1", "private-address", "me.com"),
+            ("nfs:1", "mountpoint", "/nfs/server/path"),
+            ("nfs:1", "fstype", "nfs"),)
+        util.hookenv._incoming_relation_data = data
+
+        # Setup persisted data missing values from nfs-relation-changed
+        for skip in range(1, 3):
+            partial_data = ()
+            for index, item in enumerate(data):
+                if index != skip:
+                    partial_data = partial_data + (item,)
+            util.hookenv._incoming_relation_data = partial_data
+            self.assertEqual(
+                util.hookenv.relation_get("mountpoint", rid="data:1"),
+                "/mnt/this")
+            util.mount_volume()
+            message = (
+                "Missing required relation values. "
+                "nfs-relation-changed hook needs to run first.")
+            self.assertTrue(
+                util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_mount_volume_nfs_provider_success(self):
+        """
+        L{mount_volume} will read incoming principal relation data for the
+        C{mountpoint}.  When storage config is set to the C{nfs} provider
+        L{mount_volume} as a result of the hook C{nfs-relation-changed}. The
+        values required nfs relation data are C{mount_path}, C{mount_server},
+        C{mount_type} and C{mount_options}. The success of the mount will log a
+        message and the storage charm will call juju relation-set C{mountpoint}
+        to report the successful initialization to the principal.
+        """
+        util.hookenv._incoming_relation_data = (
+            ("data:1", "mountpoint", "/mnt/this"),
+            ("nfs:1", "private-address", "me.com"),
+            ("nfs:1", "options", "someopts"),
+            ("nfs:1", "mountpoint", "/nfs/server/path"),
+            ("nfs:1", "fstype", "nfs"))
+
+        # Mock not mounted
+        ismount = self.mocker.replace(os.path.ismount)
+        ismount("/mnt/this")
+        self.mocker.result(False)
+
+        # mount_volume calls makedirs when path !exists
+        exists = self.mocker.replace(os.path.exists)
+        exists("/mnt/this")
+        self.mocker.result(False)
+        makedirs = self.mocker.replace(os.makedirs)
+        makedirs("/mnt/this")
+
+        # Called proper mount command based on nfs data
+        command = "mount -t nfs -o someopts me.com:/nfs/server/path /mnt/this"
+        mount_cmd = self.mocker.replace(subprocess.check_call)
+        mount_cmd(command, shell=True)
+        self.mocker.replay()
+
+        self.assertEqual(util.get_provider(), "nfs")
+        util.mount_volume()
+        message = "Mount (/mnt/this) successful"
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+        # Wrote proper fstab mount info to mount on reboot
+        fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"
+        with open(util.FSTAB_FILE) as input_file:
+            self.assertEqual(input_file.read(), fstab_content)
+
+        # Reported success to principal
+        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")
+
+

=== modified file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	2014-03-21 22:53:22 +0000
+++ hooks/test_common_util.py	2014-09-11 05:39:25 +0000
@@ -390,9 +390,9 @@
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_assert_block_device_failure_path_does_not_exist(self):
+    def test_assert_block_device_failure_path_does_not_exist(self):
         """
-        L{_assert_block_device} will exit in error if the C{device_path}
+        L{assert_block_device} will exit in error if the C{device_path}
         provided does not exist.
         """
         exists = self.mocker.replace(os.path.exists)
@@ -404,16 +404,16 @@
         self.mocker.count(10)
         self.mocker.replay()
 
-        self.assertRaises(SystemExit, util._assert_block_device, "/dev/vdx")
+        self.assertRaises(SystemExit, util.assert_block_device, "/dev/vdx")
         messages = ["WARNING: /dev/vdx does not exist",
                     "ERROR: /dev/vdx is not a block device."]
         for message in messages:
             self.assertTrue(
                 util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_assert_block_device_failure_not_block_device(self):
+    def test_assert_block_device_failure_not_block_device(self):
         """
-        L{_assert_block_device} will try 10 times between sleeps to validate
+        L{assert_block_device} will try 10 times between sleeps to validate
         that C{device_path} is a valid block device before logging an error
         and exiting.
         """
@@ -430,30 +430,30 @@
         self.mocker.count(10)
         self.mocker.replay()
 
-        self.assertRaises(SystemExit, util._assert_block_device, "/dev/vdx")
+        self.assertRaises(SystemExit, util.assert_block_device, "/dev/vdx")
         message = "ERROR: /dev/vdx is not a block device."
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_set_label_exits_when_path_does_not_exist(self):
+    def test_set_label_exits_when_path_does_not_exist(self):
         """
-        L{_set_label} will log and exit when C{device_path} does not exist
+        L{set_label} will log and exit when C{device_path} does not exist
         """
         exists = self.mocker.replace(os.path.exists)
         exists("/dev/not-there")
         self.mocker.result(False)
         self.mocker.replay()
         self.assertRaises(
-            SystemExit, util._set_label, "/dev/not-there", "/mnt/this")
+            SystemExit, util.set_label, "/dev/not-there", "/mnt/this")
         message = (
             "ERROR: Cannot set label of /dev/not-there. Path does not exist")
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_set_label_does_nothing_if_label_already_set_correctly(self):
+    def test_set_label_does_nothing_if_label_already_set_correctly(self):
         """
-        L{_set_label} will use e2label command to check if the label is
-        set. If current label is equal to requested label. L{_set_label} will
+        L{set_label} will use e2label command to check if the label is
+        set. If current label is equal to requested label. L{set_label} will
         do nothing.
         """
         exists = self.mocker.replace(os.path.exists)
@@ -463,12 +463,12 @@
         e2label_check("e2label /dev/vdz", shell=True)
         self.mocker.result("/mnt/this\n")
         self.mocker.replay()
-        util._set_label("/dev/vdz", "/mnt/this")
+        util.set_label("/dev/vdz", "/mnt/this")
 
-    def test_wb_set_label_alters_existing_label_when_set_different(self):
+    def test_set_label_alters_existing_label_when_set_different(self):
         """
-        L{_set_label} will use e2label command to check if the label is
-        set. If current label is different from requested label. L{_set_label}
+        L{set_label} will use e2label command to check if the label is
+        set. If current label is different from requested label. L{set_label}
         will reset the filesystem label and log a message.
         """
         exists = self.mocker.replace(os.path.exists)
@@ -480,17 +480,17 @@
         e2label_check = self.mocker.replace(subprocess.check_call)
         e2label_check("e2label /dev/vdz /mnt/this", shell=True)
         self.mocker.replay()
-        util._set_label("/dev/vdz", "/mnt/this")
+        util.set_label("/dev/vdz", "/mnt/this")
         message = (
             "WARNING: /dev/vdz had label=other-label, overwriting with "
             "label=/mnt/this")
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_set_label_alters_existing_label_when_unset(self):
+    def test_set_label_alters_existing_label_when_unset(self):
         """
-        L{_set_label} will use e2label command to check if the label is
-        set. If label is unset, L{_set_label} to reset the filesystem label and
+        L{set_label} will use e2label command to check if the label is
+        set. If label is unset, L{set_label} to reset the filesystem label and
         log a message.
         """
         exists = self.mocker.replace(os.path.exists)
@@ -502,25 +502,25 @@
         e2label_check = self.mocker.replace(subprocess.check_call)
         e2label_check("e2label /dev/vdz /mnt/this", shell=True)
         self.mocker.replay()
-        util._set_label("/dev/vdz", "/mnt/this")
+        util.set_label("/dev/vdz", "/mnt/this")
 
-    def test_wb_format_device_ignores_fsck_when_busy(self):
+    def test_format_device_ignores_fsck_when_busy(self):
         """
         When blockdev reports the C{device_path} is busy,
-        L{_format_device} skips fsck and returns the partition path.
+        L{format_device} skips fsck and returns the partition path.
         """
         partition_table_mock = self.mocker.replace(util._read_partition_table)
         partition_table_mock("/dev/vdz")
         self.mocker.result(1)
         self.mocker.replay()
 
-        util._format_device("/dev/vdz")
+        util.format_device("/dev/vdz")
         message = (
             "INFO: /dev/vdz is busy, no fsck performed. Assuming formatted.")
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_format_device_runs_fsck_if_device_not_busy(self):
+    def test_format_device_runs_fsck_if_device_not_busy(self):
         """
         When L{_read_partition_table} reports the device partition table is
         readable, and L{_assert_fstype} returns C{True}, mkfs.ext4 is not run
@@ -541,12 +541,12 @@
         self.mocker.result(0)   # successful fsck command exit
         self.mocker.replay()
 
-        util._format_device("/dev/vdz")
+        util.format_device("/dev/vdz")
         message = "/dev/vdz already formatted - skipping mkfs.ext4."
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_format_device_non_ext4_is_formatted(self):
+    def test_format_device_non_ext4_is_formatted(self):
         """
         When L{_read_partition_table} reports the device partition table is
         readable, and L{_assert_fstype} returns C{Flase}, mkfs.ext4 and fsck
@@ -567,14 +567,14 @@
         self.mocker.result(0)   # Sucessful command exit 0
         self.mocker.replay()
 
-        util._format_device("/dev/vdz")
+        util.format_device("/dev/vdz")
         message = "NOTICE: Running: mkfs.ext4 /dev/vdz"
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_wb_format_device_responds_to_fsck_error(self):
+    def test_format_device_responds_to_fsck_error(self):
         """
-        When fsck returns an error, L{_format_device} exits in
+        When fsck returns an error, L{format_device} exits in
         error and logs the error.
         """
         partition_table_mock = self.mocker.replace(util._read_partition_table)
@@ -593,7 +593,7 @@
         self.mocker.replay()
 
         result = self.assertRaises(
-            SystemExit, util._format_device, "/dev/vdz")
+            SystemExit, util.format_device, "/dev/vdz")
         self.assertEqual(result.code, 1)
 
         messages = ["NOTICE: Running: mkfs.ext4 /dev/vdz",
@@ -608,17 +608,16 @@
         has not yet defined by the relation principal yet.
         """
         self.assertIsNone(util.hookenv.relation_get("mountpoint"))
-        util.mount_volume()
+        self.assertRaises(SystemExit, util.prepare_volume_mount)
         message = "No mountpoint defined by relation. Cannot mount volume yet."
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
-    def test_mount_volume_mountpoint_persists_mountpoint_from_relation(self):
+    def test_prepare_volume_mount_mountpoint_persists_mountpoint(self):
         """
-        L{mount_volume} will read incoming principal relation data for the
-        C{mountpoint}. When the requested C{mountpoint} is determined as
-        mounted,  it will then persist the mountpoint data, log a message and
-        set juju relation C{mountpoint} in response.
+        L{prepare_volume_mount} will read incoming principal relation data for
+        the C{mountpoint}. When the requested C{mountpoint} is determined as
+        mounted,  it will then persist the mountpoint data, and log a message.
         """
         util.hookenv._incoming_relation_data = (
             ("data:1", "mountpoint", "/mnt/this"),)
@@ -628,7 +627,7 @@
         self.mocker.result(True)
         self.mocker.replay()
 
-        util.mount_volume()
+        util.prepare_volume_mount()
         self.assertIn(("mountpoint", "/mnt/this"),
                       util.hookenv._outgoing_relation_data)
         message = "Volume (/mnt/this) already mounted"
@@ -639,11 +638,11 @@
             data = json.load(infile)
             self.assertEqual(data["mountpoint"], "/mnt/this")
 
-    def test_mount_volume_mountpoint_from_relation_data_already_mounted(self):
+    def test_prepare_mount_volume_mountpoint_already_mounted(self):
         """
-        L{mount_volume} will log a message and return when the C{mountpoint} is
-        already defined in relation data and the requested C{mountpoint} is
-        already mounted.
+        L{prepare_mount_volume} will log a message and return when the
+        C{mountpoint} is already defined in relation data and the requested
+        C{mountpoint} is already mounted.
         """
         util.hookenv._incoming_relation_data = (
             ("data:1", "mountpoint", "/mnt/this"),)
@@ -655,234 +654,7 @@
         self.mocker.result(True)
         self.mocker.replay()
 
-        util.mount_volume()
+        util.prepare_volume_mount()
         message = "Volume (/mnt/this) already mounted"
         self.assertTrue(
             util.hookenv.is_logged(message), "Not logged- %s" % message)
-
-    def test_mount_volume_nfs_provider_no_nfs_hook_relation_data(self):
-        """
-        L{mount_volume} will exit in error when set as C{nfs} provider and the
-        nfs relation data does not contain any of the required
-        values: C{mount_path}, C{mount_server}, C{mount_options} or
-        C{mount_type}.
-        """
-        data = (
-            ("data:1", "mountpoint", "/mnt/this"),
-            ("nfs:1", "private-address", "me.com"),
-            ("nfs:1", "mountpoint", "/nfs/server/path"),
-            ("nfs:1", "fstype", "nfs"),)
-        util.hookenv._incoming_relation_data = data
-
-        # Setup persisted data missing values from nfs-relation-changed
-        for skip in range(1, 3):
-            partial_data = ()
-            for index, item in enumerate(data):
-                if index != skip:
-                    partial_data = partial_data + (item,)
-            util.hookenv._incoming_relation_data = partial_data
-            self.assertEqual(
-                util.hookenv.relation_get("mountpoint", rid="data:1"),
-                "/mnt/this")
-            util.mount_volume()
-            message = (
-                "Missing required relation values. "
-                "nfs-relation-changed hook needs to run first.")
-            self.assertTrue(
-                util.hookenv.is_logged(message), "Not logged- %s" % message)
-
-    def test_mount_volume_nfs_provider_success(self):
-        """
-        L{mount_volume} will read incoming principal relation data for the
-        C{mountpoint}.  When storage config is set to the C{nfs} provider
-        L{mount_volume} as a result of the hook C{nfs-relation-changed}. The
-        values required nfs relation data are C{mount_path}, C{mount_server},
-        C{mount_type} and C{mount_options}. The success of the mount will log a
-        message and the storage charm will call juju relation-set C{mountpoint}
-        to report the successful initialization to the principal.
-        """
-        util.hookenv._incoming_relation_data = (
-            ("data:1", "mountpoint", "/mnt/this"),
-            ("nfs:1", "private-address", "me.com"),
-            ("nfs:1", "options", "someopts"),
-            ("nfs:1", "mountpoint", "/nfs/server/path"),
-            ("nfs:1", "fstype", "nfs"))
-
-        # Mock not mounted
-        ismount = self.mocker.replace(os.path.ismount)
-        ismount("/mnt/this")
-        self.mocker.result(False)
-
-        # mount_volume calls makedirs when path !exists
-        exists = self.mocker.replace(os.path.exists)
-        exists("/mnt/this")
-        self.mocker.result(False)
-        makedirs = self.mocker.replace(os.makedirs)
-        makedirs("/mnt/this")
-
-        # Called proper mount command based on nfs data
-        command = "mount -t nfs -o someopts me.com:/nfs/server/path /mnt/this"
-        mount_cmd = self.mocker.replace(subprocess.check_call)
-        mount_cmd(command, shell=True)
-        self.mocker.replay()
-
-        self.assertEqual(util.get_provider(), "nfs")
-        util.mount_volume()
-        message = "Mount (/mnt/this) successful"
-        self.assertTrue(
-            util.hookenv.is_logged(message), "Not logged- %s" % message)
-
-        # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"
-        with open(util.FSTAB_FILE) as input_file:
-            self.assertEqual(input_file.read(), fstab_content)
-
-        # Reported success to principal
-        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_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 = (
-            ("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 = (
-            "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_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.
-        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.
-        """
-        util.hookenv._incoming_relation_data = (
-            ("data:1", "mountpoint", "/mnt/this"),)
-
-        util.hookenv._config = (("provider", "block-storage-broker"),)
-
-        ismount = self.mocker.replace(os.path.ismount)
-        ismount("/mnt/this")
-        self.mocker.result(False)  # Mock not mounted
-
-        assert_device = self.mocker.replace(util._assert_block_device)
-        assert_device("/dev/vdx")
-        exists = self.mocker.replace(os.path.exists)
-        exists("/dev/vdx1")
-        self.mocker.result(False)
-        create_partition = self.mocker.replace(
-            util._format_device)
-        create_partition("/dev/vdx")
-
-        _set_label = self.mocker.replace(util._set_label)
-        _set_label("/dev/vdx", "/mnt/this")
-
-        # Ensure blkid is called to discover fstype of nova device
-        get_fstype = self.mocker.replace(subprocess.check_output)
-        get_fstype("blkid -o value -s TYPE /dev/vdx", shell=True)
-        self.mocker.result("ext4\n")
-
-        exists("/mnt/this")
-        self.mocker.result(True)
-
-        mount = self.mocker.replace(subprocess.check_call)
-        mount("mount -t ext4 /dev/vdx /mnt/this", shell=True)
-        self.mocker.replay()
-
-        self.assertEqual(util.get_provider(), "block-storage-broker")
-        util.mount_volume("/dev/vdx")
-        message = "Mount (/mnt/this) successful"
-        self.assertTrue(
-            util.hookenv.is_logged(message), "Not logged- %s" % message)
-
-        # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "/dev/vdx /mnt/this ext4 default 0 0"
-        with open(util.FSTAB_FILE) as input_file:
-            self.assertEqual(input_file.read(), fstab_content)
-
-        # Reported success to principal
-        self.assertEqual(
-            util.hookenv._outgoing_relation_data,
-            (("mountpoint", "/mnt/this"),))
-
-    def test_mount_volume_bsb_provider_success_postgresql_charm_upgrade(self):
-        """
-        Old versions of the postgresql charm would have created a single
-        partition with max size from an external attached volume if the user
-        had used postgresql charm's deprecated volume-map parameter. When the
-        storage charm sees a single partition on the attached device, it will
-        attempt to mount that partition instead of the root device.
-        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.
-        """
-        util.hookenv._incoming_relation_data = (
-            ("data:1", "mountpoint", "/mnt/this"),)
-
-        util.hookenv._config = (("provider", "block-storage-broker"),)
-
-        ismount = self.mocker.replace(os.path.ismount)
-        ismount("/mnt/this")
-        self.mocker.result(False)  # Mock not mounted
-
-        assert_device = self.mocker.replace(util._assert_block_device)
-        assert_device("/dev/vdx")
-        exists = self.mocker.replace(os.path.exists)
-        exists("/dev/vdx1")
-        self.mocker.result(True)
-        create_partition = self.mocker.replace(
-            util._format_device)
-        create_partition("/dev/vdx1")
-
-        _set_label = self.mocker.replace(util._set_label)
-        _set_label("/dev/vdx1", "/mnt/this")
-
-        # Ensure blkid is called to discover fstype of nova device
-        get_fstype = self.mocker.replace(subprocess.check_output)
-        get_fstype("blkid -o value -s TYPE /dev/vdx1", shell=True)
-        self.mocker.result("ext4\n")
-
-        exists("/mnt/this")
-        self.mocker.result(True)
-
-        mount = self.mocker.replace(subprocess.check_call)
-        mount("mount -t ext4 /dev/vdx1 /mnt/this", shell=True)
-        self.mocker.replay()
-
-        self.assertEqual(util.get_provider(), "block-storage-broker")
-        util.mount_volume("/dev/vdx")
-        messages = ["Mount (/mnt/this) successful",
-                    "Mounting /dev/vdx1 as the device path as the root device "
-                    "is already partitioned."]
-        for message in messages:
-            self.assertTrue(
-                util.hookenv.is_logged(message), "Not logged- %s" % message)
-
-        # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "/dev/vdx1 /mnt/this ext4 default 0 0"
-        with open(util.FSTAB_FILE) as input_file:
-            self.assertEqual(input_file.read(), fstab_content)
-
-        # Reported success to principal
-        self.assertEqual(
-            util.hookenv._outgoing_relation_data,
-            (("mountpoint", "/mnt/this"),))