Merge lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider into lp:~dpb/charms/precise/storage/trunk

[Chad Smith]

- Add nfs provider type to the storage charm.
- centralize common functions in common_util.py
- move all nova functions into nova_util.py for ease of testing in the future
- Add a heaping pile of unit tests to the storage charm as well as a common TestHookenv class for intercepting juju commands
- Add a makefile with "test" and "lint" targets
- convert most of the bash to python for easier unit testing
- use charm-helpers.core code to standardize juju interaction with other charms
   Note: this MP is not to review any of the charmhelpers code as that didn't change, it was only included unaltered in this charm

In terms of storage charm interaction:
  1. the storage charm now requires the principal charm to set its requested mountpoint via relation-set mountpoint=/desired/mountpoint
  2. when the device (nfs/nova) is attached, initialized, fsck'd and mounted the storage charm will respond to the principal over the "data" relation that the mount is ready by setting the mountpoint to publish the mounted device path

[Chad Smith (chad.smith)]

Simplest deployment using juju-deployer:

common:
    services:
        postgresql:
            branch: lp:charms/precise/postgresql
            constraints: mem=2048
            options:
                extra-packages: python-apt postgresql-contrib postgresql-9.1-debversion
                max_connections: 500
        storage:
            branch: lp:~fcorrea/charms/precise/storage/consume-principal-charm-mountpoint
            options:
                provider: nova
                key: <your_os_name>
                tenant: <your_os_name>_project
                secret: <your_os_password>
                endpoint: https://keystone.canonistack.canonical.com:443/v2.0/
                region: lcy01

doit:
    inherits: common
    series: precise

juju-deployer -c postgres-storage.cfg doit

[Chad Smith (chad.smith)]

Typo corrections on the charm paths
     services:
         postgresql:
             branch: lp:~fcorrea/charms/precise/postgresql/delegate-blockstorage-to-storage-subordinate

         ...
         storage:
             branchL lp~chad.smith/charms/precise/storage/storage-nfs-python-provider

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

85. By Chad Smith on 2014-01-21

merge dpb's upstream storage charm trunk. resolve conflict

86. By Chad Smith on 2014-01-21

unit test fix for umount_volume. needed to mock lsof calls

87. By Chad Smith on 2014-01-21

make sure tearDown removes any provider persistent data for nova and/or nfs left around by unit tests

88. By Chad Smith on 2014-01-21

sync missed relation_set fix to specify relid in hookenv.relation_set calls

[Chad Smith (chad.smith)]

Here's the corrected postgres-storage.cfg deployer file (make sure to set all OS_* to your env values)

common:
    services:
        postgresql:
            branch: lp:~fcorrea/charms/precise/postgresql/delegate-blockstorage-to-storage-subordinate
            constraints: mem=2048
            options:
                extra-packages: python-apt postgresql-contrib postgresql-9.1-debversion
                max_connections: 500
        storage:
            branch: lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider
            options:
                provider: nova
                key: OS_USERNAME
                tenant: OS_TENANT_NAME
                secret: OS_PASSWORD
                endpoint: https://keystone.canonistack.canonical.com:443/v2.0/
                region: OS_REGION_NAME

jfdi:
    inherits: common
    series: precise
    relations:
        - ["postgresql:data", "storage:data"]

https://pastebin.canonical.com/103331/

[David Britton (dpb)]

[0] I would pull out all partition logic from common_util. Creating a whole-disk partition just to turn around and mount it is kind of busy-work. Create the filesystem on the whole disk and mount that.

[David Britton (dpb)]

[1] Makefile: change test directive to "CHARM_DIR=`pwd` trial hooks"

[David Britton (dpb)]

[2] Add _trial_tmp to .bzrignore

[David Britton (dpb)]

[3] Some lint errors:

hooks/__init__.py:1:1: W391 blank line at end of file
hooks/testing.py:66:1: F821 undefined name 'util'
hooks/testing.py:78:19: E702 multiple statements on one line (semicolon)

[David Britton (dpb)]

[4] Remove the revision file.

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

89. By Chad Smith on 2014-01-21

add .bzrignore and Makefile changes to call trial hooks instead cd hooks etc.

[David Britton (dpb)]

[5] Make sure there is docstring on all methods, please.

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

90. By Chad Smith on 2014-01-21

lint fixes

91. By Chad Smith on 2014-01-21

make a number of functions private, add docstrings to all functions

92. By Chad Smith on 2014-01-21

unit tests to test _private functions

[David Britton (dpb)]

[6] Why are we storing PERSIST_MOUNTPOINT. Can't we fetch that from the relation data? Chad seemed to think in IRC that it had to do with the broken/departed scenerio.

[David Britton (dpb)]

[7] in common_util:mount_volume, you are reading persisted data from the
nfs relation. Instead of doing this, I wonder if you should instead use
the relation-get concept of probing the relation to get that data.

It's something like:

    ids=$(relation-ids --type nfs)
    relation-get --id <id>

I'll leave it to you to decide if this is a direction you want to go.

--
David Britton <email address hidden>

[David Britton (dpb)]

[8] There seem to be some mount and unmout functions in charmsupport,
should we use those? The answer could be no, I haven't looked closely
at them.

== nfs/data-relation-changed ==

[9] extra import sys

[10] you have a formatted variable "config_changed" that is used, but
a replacement is never run on it. Seems wrong

[11] General: what are all the calls to config-changed for?

[12] nfs: you could symlink stop and nfs-relation-departed, also get rid
of -broken?? I'm not sure why we need stop actually. I'd get rid of it
and start if possible.

== nova/config-changed ==

[13] remove jsonpipe install, and use python json module to read data in
nova_util.py. I get that this was a fast conversion, but that one
cleanup saves a package installation. :)

== nova/data-relation-* ==

[14] not sure we need a call to config_changed here, do we? I think I'm
seeing the problem. We need to install software in an install hook, but
if someone changes configuration that software will not be installed.
I'm thinking that we should symlink install and config-changed in all
these, and stop calling config_changed at the top of all relation hooks.

== nova/nova-util.py ==

[15] add docs

[16] get_volume_id(): your else here is dangerous, I think we should
fail if either the volume_id or name is not specified, or we create it.
Those are the only two options. Also, if the name doesn't return just
one volume, that should be an error.

[17] attach_nova_vlume(): combine first two sentinals to be check for
"available" status, and exit with error if not in that state?

[18] I would change most of the call() to check_call(), to get the
return code checked.

== nova/ ==

[19] same comments about start/stop hooks, and if they are needed. I
wonder if we just need to worry about the relation changed and departed?

--
David Britton <email address hidden>

[David Britton (dpb)]

The idea is good, but there are some thing I think need to be done before upstream will accept. But I like that it's working for simple cases. I'll do more testing tomorrow and post back with results.

review: Needs Fixing

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

93. By Chad Smith on 2014-01-22

drop PERSIST_MOUNTPOINT and use relation-get data instead

94. By Chad Smith on 2014-01-22

rename variable from charm_data -> persistent_data for clarity

[Chad Smith (chad.smith)]

Thanks david for all the comments:
[1-6] handled

[7] will look at the PERSIST_DATA again tomorrow on a full landscape deployment to ensure I can access "nfs-relation" mount settings data from within the "data-relation" that is running between landscape-charm and storage.

[8] using charmherlpers.host.mount(): I looked at charmhelpers we will need some significant patch (and should do this in phase 2) to make those methods functional for us in both nfs and nova environments and that may take more time than we have at the moment.

[9-10] fixed

[11 & 14] config-changed calls just ensure the dependencies of the provider are installed, the reason this happens in config-changed instead of install is because the config-change could have been changing provider type from "nova" to "nfs" in the config so we wouldn't be hitting an install hook in that case. There probably needs to be a better mechanism to install dependencies in the event the provider changes in the storage config.
-- Maybe the potential solution is to symlink config-changed and install and use the current provider setting to install the needed dependencies WDYT?

[12 & 19] ditched start/stop and all -broken hooks renamed to -departed (because departed still has the relation data available in juju on which we depend for unmounting the "mountpoint" etc) *broken hooks do not have that persistent data so we wouldn't be able to unmount or detach devices that we don't know about

Will resolve 13 and 15-18 tomorrow. Sorry about the size of this review and thanks for the good notes

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

95. By Chad Smith on 2014-01-22

rename (nfs|data)-relation-broken hooks to (nfs|data)-relation-departed so we can reference existing hook relation-data for mountpoint instead of using PERSIST_MOUNTPOINT file. Drop start/stop hooks as they are duplicate function of (data|nova)-relation-changed hooks

[Fernando Correa Neto (fcorrea)]

All seats already taken but I'll leave some feedback:

[1] in hooks/hooks
+charm_hooks_dir="$CHARM_DIR/hooks"
+export PYTHONPATH=$charm_hooks_dir

That one can be condensed in a single line:

export PYTHONPATH="$CHARM_DIR/hooks"

[2] in hooks/storage-provider.d/nova/config-changed
+cat > `dirname $0`/nova_environment <<EOF
+OS_USERNAME=$(config-get key)
+OS_TENANT_NAME=$(config-get tenant)
+OS_PASSWORD=$(config-get secret)
+OS_AUTH_URL=$(config-get endpoint)
+OS_REGION_NAME=$(config-get region)
+EOF
+

Given all the conversation that we had about separating the storage service from the block storage provider, I think that storing the credentials in a file might not be the best thing to do.
I think that "hiding" it in the config state might make a little harder to find the credentials in this case.
Of course, by saying that I'm neglecting the fact that we were logging all the credentials upon making nova calls.
In that case, I think we should make our best to make it hard to find those creds. WDYT?

[3]
+current_dir = sys.path[0]

I wonder if we could use os.path.dirname(os.path.realpath(__file__)) as it seems to be advertised as the best way of getting the current directory of a file.

[4] in hooks/common_util.py Line 157
def _is_ext4_filesystem(device_path):
    """Return C{True} if a filesystem is of type C{ext4}"""
    command = "file -s %s | egrep -q ext4" % device_path
    return subprocess.call(command, shell=True) == 0

Maybe we could have something more generic as _assert_fstype(device_path, fstype)?

[5] in hooks/common_util.py Line 197
I'm fine leaving the "if provider ==" in there and I don't think that part of the code will grow that fast. Maybe if it's the case people add support for handling SAN volumes or other types of storage as a service technology, but in general, this is something I'd delegate to each provider as they will mount things differently and also to concentrate the logic in smaller functions that are easy to maintain.

The dangerous of having a "common" module is that they can grow quite big as new "common" logic are required while having an API that exposes an expected interface might work better as things are kept separate in each provider.

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

96. By Chad Smith on 2014-01-22

add .PHONY targets to makefile

97. By Chad Smith on 2014-01-22

link install hook to config-changed hooks within nova and nfs providers to ensure dependencies for the provider are installed both at install time and during a provider config change

98. By Chad Smith on 2014-01-22

fix symlinks for install hooks

99. By Chad Smith on 2014-01-22

get_volume_id to exit(1) if multiple volumes have labels related to this instance

100. By Chad Smith on 2014-01-22

fix multiple-volume associated with juju unit. error out in this case

[Chad Smith (chad.smith)]

Resolved dpb's
15: docs added
16: get_volume_id will exit in error if multiple potential volume candidates are discovered for this unit through nova volume-list

17: attach_nova_volume will only proceed if volume status is "available" and exit in error for other status
18:
[18] leaving subprocess.call as we do check the exit code. it just doesn't traceback for us like check_call would have

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

101. By Chad Smith on 2014-01-22

address fcorrea's comments:
[1] hooks/hooks variable consolidation
[3] os.path.dirname(os.path.realpath(__file__)) instead of sus.path[0]
[4] _is_ext4_filesystem -> _assert_fstype(path, fstype)

[Adam Collard (adam-collard)]

review: Abstain

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

102. By Chad Smith on 2014-01-31

- drop charmhelpers.contrib|fetch|payload which we don't use
- add update-charm-helpers make target to refresh charmhelpers
- common_util drop PERSIST_DATA

103. By Chad Smith on 2014-02-01

use sys.exit(0) instead of return 0

104. By Chad Smith on 2014-02-01

check subprocess.call output to ensure exit 0 from called script

105. By Chad Smith on 2014-02-03

mova get_volume_status below the initial creation of the volume

106. By Chad Smith on 2014-02-03

instead of using a persistent data file to store the nova attached device_path, allow nova's data-relation-changed hook to provide a device_path parameter to mount_volume

[Chad Smith (chad.smith)]

Thanks for the review fcorrea, I resolved [1]

[2] Given all the conversation that we had about separating the storage service from the block storage provider, I think that storing the credentials in a file might not be the best thing to do.
....

I've resolved [2] and dropping writing any persistent files during the mirgration of nova code to the block-storage-broker charm at lp:~chad.smith/charms/precise/block-storage-broker/trunk
So, I'll leave it in this branch as our next branch against this charm will be to remove all nova provider code.

[3]
+current_dir = sys.path[0] # I wonder if we could use os.path.dirname(os.path.realpath(__file__))

Fixed

[4] in hooks/common_util.py Line 157
def _is_ext4_filesystem(device_path): # Maybe we could have something more generic as _assert_fstype(device_path, fstype)?

Fixed

[5] in hooks/common_util.py Line 197
I'm fine leaving the "if provider ==" in there and I don't think that part of the code will grow that fast.
...

Yeah I can only see about 3-4 providers in the near future, when we cross that threshold where handling provider type is an issue, we can make a quick branch to resolve that.

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

107. By Chad Smith on 2014-02-03

nova data-relation-changed calls mount_volume passing the device returned from attach_nova_volume

108. By Chad Smith on 2014-02-03

drop partitioning actions of nova attached volues. still ensure volume is formatted ext4

109. By Chad Smith on 2014-02-03

lint

110. By Chad Smith on 2014-02-03

unit test updates to drop partitioning a nova volume to a single partition at max capacity. we can mount nova volume without the extra sfdisk work

111. By Chad Smith on 2014-02-04

drop all of nova provider and stub in the blockstoragebroker provider handling for when block-storage-broker charm is accepted

112. By Chad Smith on 2014-02-04

drop any nova-specific provider references and settings from config.yaml, update unit tests to test blockstoragebroker provider stub in preparation for jseutter's branch

113. By Chad Smith on 2014-02-04

fix fcorrea review comment, Makefile to handle _trial_temp directory

114. By Chad Smith on 2014-02-04

test__ changed to test_wb for internal functions

[Fernando Correa Neto (fcorrea)]

Hey Chad.

Just so we keep record as we talked about this already on IRC.
1) s/test__/test_wb/ for the private funtions
2) fs related utility functions are left here because Jerry's branch depends on it.

I looks good to me. +1

review: Approve

[David Britton (dpb)]

== common_util.py ==

[20] I think you can remove the guts of the log function, and just have it do

return hookenv.log(message, level). It seems to handle the 'None' case already

[21] RE: TODO: you don't need to check files existing "below" a mount. mount doesn't care, it will happily eclipse them (not remove them). Once the device is unmounted, the files will be visible again.

+1, I'll test and review further on Jerry's Branch. :)

review: Approve

[lp:~chad.smith/charms/precise/storage/storage-nfs-python-provider]

115. By Chad Smith on 2014-02-05

resolve dpb's comments. simplify hookenv.log wrapper in common_util, drop comment about mount eclipsing existing files

=== added file '.bzrignore'
--- .bzrignore	1970-01-01 00:00:00 +0000
+++ .bzrignore	2014-02-05 16:40:53 +0000
@@ -0,0 +1,2 @@
+_trial_temp
+charm-helpers

=== added file 'Makefile'
--- Makefile	1970-01-01 00:00:00 +0000
+++ Makefile	2014-02-05 16:40:53 +0000
@@ -0,0 +1,19 @@
+.PHONY: test lint clean
+
+clean:
+	find . -name *.pyc | xargs rm
+	find . -name _trial_temp | xargs -n 1 rm -r
+test:
+	CHARM_DIR=`pwd` trial hooks
+
+lint:
+	@flake8 --exclude hooks/charmhelpers hooks
+
+update-charm-helpers:
+	# Pull latest charm-helpers branch and sync the components based on our
+	$ charm-helpers.yaml
+	rm -rf charm-helpers
+	bzr co lp:charm-helpers
+	./charm-helpers/tools/charm_helpers_sync/charm_helpers_sync.py -c charm-helpers.yaml
+	rm -rf charm-helpers
+

=== added file 'charm-helpers.yaml'
--- charm-helpers.yaml	1970-01-01 00:00:00 +0000
+++ charm-helpers.yaml	2014-02-05 16:40:53 +0000
@@ -0,0 +1,5 @@
+destination: hooks/charmhelpers
+branch: lp:charm-helpers
+include:
+    - core
+    - fetch

=== modified file 'config.yaml'
--- config.yaml	2013-12-10 17:05:06 +0000
+++ config.yaml	2014-02-05 16:40:53 +0000
@@ -8,30 +8,11 @@
         default: local
         description: |
             The backend storage provider.  Will be mounted at root, and
-            can be one of, local or nova.  If you set to nova, you
-            must provide suitable values for other config options.
-    key:
-        type: string
-        description: The provider specific api credential key
-        default: ""
-    tenant:
-        type: string
-        description: The provider specific api tenant name
-        default: ""
-    secret:
-        type: string
-        description: The provider specific api credential secret
-        default: ""
-    endpoint:
-        type: string
-        description: The provider specific api endpoint url
-        default: ""
-    region:
-        type: string
-        description: The provider specific region name
-        default: ""
-    id:
-        type: string
-        description: The provider specific device name or identifier
-        default: ""
-
+            can be one of, local, blockstoragebroker or nfs.  If you set to
+            blockstoragebroker, you should provide a suitable value for
+            volume_size.
+    volume_size:
+        type: int
+        description: |
+            The volume size in GB to request from the block-storage-broker.
+        default: 5

=== added file 'hooks/__init__.py'
=== added directory 'hooks/charmhelpers'
=== added file 'hooks/charmhelpers/__init__.py'
=== added directory 'hooks/charmhelpers/cli'
=== added file 'hooks/charmhelpers/cli/README.rst'
--- hooks/charmhelpers/cli/README.rst	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers/cli/README.rst	2014-02-05 16:40:53 +0000
@@ -0,0 +1,57 @@
+==========
+Commandant
+==========
+
+-----------------------------------------------------
+Automatic command-line interfaces to Python functions
+-----------------------------------------------------
+
+One of the benefits of ``libvirt`` is the uniformity of the interface: the C  API (as well as the bindings in other languages) is a set of functions that accept parameters that are nearly identical to the command-line arguments.  If you run ``virsh``, you get an interactive command prompt that supports all of the same commands that your shell scripts use as ``virsh`` subcommands.
+
+Command execution and stdio manipulation is the greatest common factor across all development systems in the POSIX environment.  By exposing your functions as commands that manipulate streams of text, you can make life easier for all the Ruby and Erlang and Go programmers in your life.
+
+Goals
+=====
+
+* Single decorator to expose a function as a command.
+  * now two decorators - one "automatic" and one that allows authors to manipulate the arguments for fine-grained control.(MW)
+* Automatic analysis of function signature through ``inspect.getargspec()``
+* Command argument parser built automatically with ``argparse``
+* Interactive interpreter loop object made with ``Cmd``
+* Options to output structured return value data via ``pprint``, ``yaml`` or ``json`` dumps.
+
+Other Important Features that need writing
+------------------------------------------
+
+* Help and Usage documentation can be automatically generated, but it will be important to let users override this behaviour
+* The decorator should allow specifying further parameters to the parser's add_argument() calls, to specify types or to make arguments behave as boolean flags, etc.
+    - Filename arguments are important, as good practice is for functions to accept file objects as parameters.
+    - choices arguments help to limit bad input before the function is called
+* Some automatic behaviour could make for better defaults, once the user can override them.
+    - We could automatically detect arguments that default to False or True, and automatically support --no-foo for foo=True.
+    - We could automatically support hyphens as alternates for underscores
+    - Arguments defaulting to sequence types could support the ``append`` action.
+
+
+-----------------------------------------------------
+Implementing subcommands
+-----------------------------------------------------
+
+(WIP)
+
+So as to avoid dependencies on the cli module, subcommands should be defined separately from their implementations. The recommmendation would be to place definitions into separate modules near the implementations which they expose.
+
+Some examples::
+
+    from charmhelpers.cli import CommandLine
+    from charmhelpers.payload import execd
+    from charmhelpers.foo import bar
+
+    cli = CommandLine()
+
+    cli.subcommand(execd.execd_run)
+
+    @cli.subcommand_builder("bar", help="Bar baz qux")
+    def barcmd_builder(subparser):
+        subparser.add_argument('argument1', help="yackety")
+        return bar

=== added file 'hooks/charmhelpers/cli/__init__.py'
--- hooks/charmhelpers/cli/__init__.py	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers/cli/__init__.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,147 @@
+import inspect
+import itertools
+import argparse
+import sys
+
+
+class OutputFormatter(object):
+    def __init__(self, outfile=sys.stdout):
+        self.formats = (
+            "raw",
+            "json",
+            "py",
+            "yaml",
+            "csv",
+            "tab",
+        )
+        self.outfile = outfile
+
+    def add_arguments(self, argument_parser):
+        formatgroup = argument_parser.add_mutually_exclusive_group()
+        choices = self.supported_formats
+        formatgroup.add_argument("--format", metavar='FMT',
+                                 help="Select output format for returned data, "
+                                      "where FMT is one of: {}".format(choices),
+                                 choices=choices, default='raw')
+        for fmt in self.formats:
+            fmtfunc = getattr(self, fmt)
+            formatgroup.add_argument("-{}".format(fmt[0]),
+                                     "--{}".format(fmt), action='store_const',
+                                     const=fmt, dest='format',
+                                     help=fmtfunc.__doc__)
+
+    @property
+    def supported_formats(self):
+        return self.formats
+
+    def raw(self, output):
+        """Output data as raw string (default)"""
+        self.outfile.write(str(output))
+
+    def py(self, output):
+        """Output data as a nicely-formatted python data structure"""
+        import pprint
+        pprint.pprint(output, stream=self.outfile)
+
+    def json(self, output):
+        """Output data in JSON format"""
+        import json
+        json.dump(output, self.outfile)
+
+    def yaml(self, output):
+        """Output data in YAML format"""
+        import yaml
+        yaml.safe_dump(output, self.outfile)
+
+    def csv(self, output):
+        """Output data as excel-compatible CSV"""
+        import csv
+        csvwriter = csv.writer(self.outfile)
+        csvwriter.writerows(output)
+
+    def tab(self, output):
+        """Output data in excel-compatible tab-delimited format"""
+        import csv
+        csvwriter = csv.writer(self.outfile, dialect=csv.excel_tab)
+        csvwriter.writerows(output)
+
+    def format_output(self, output, fmt='raw'):
+        fmtfunc = getattr(self, fmt)
+        fmtfunc(output)
+
+
+class CommandLine(object):
+    argument_parser = None
+    subparsers = None
+    formatter = None
+
+    def __init__(self):
+        if not self.argument_parser:
+            self.argument_parser = argparse.ArgumentParser(description='Perform common charm tasks')
+        if not self.formatter:
+            self.formatter = OutputFormatter()
+            self.formatter.add_arguments(self.argument_parser)
+        if not self.subparsers:
+            self.subparsers = self.argument_parser.add_subparsers(help='Commands')
+
+    def subcommand(self, command_name=None):
+        """
+        Decorate a function as a subcommand. Use its arguments as the
+        command-line arguments"""
+        def wrapper(decorated):
+            cmd_name = command_name or decorated.__name__
+            subparser = self.subparsers.add_parser(cmd_name,
+                                                   description=decorated.__doc__)
+            for args, kwargs in describe_arguments(decorated):
+                subparser.add_argument(*args, **kwargs)
+            subparser.set_defaults(func=decorated)
+            return decorated
+        return wrapper
+
+    def subcommand_builder(self, command_name, description=None):
+        """
+        Decorate a function that builds a subcommand. Builders should accept a
+        single argument (the subparser instance) and return the function to be
+        run as the command."""
+        def wrapper(decorated):
+            subparser = self.subparsers.add_parser(command_name)
+            func = decorated(subparser)
+            subparser.set_defaults(func=func)
+            subparser.description = description or func.__doc__
+        return wrapper
+
+    def run(self):
+        "Run cli, processing arguments and executing subcommands."
+        arguments = self.argument_parser.parse_args()
+        argspec = inspect.getargspec(arguments.func)
+        vargs = []
+        kwargs = {}
+        if argspec.varargs:
+            vargs = getattr(arguments, argspec.varargs)
+        for arg in argspec.args:
+            kwargs[arg] = getattr(arguments, arg)
+        self.formatter.format_output(arguments.func(*vargs, **kwargs), arguments.format)
+
+
+cmdline = CommandLine()
+
+
+def describe_arguments(func):
+    """
+    Analyze a function's signature and return a data structure suitable for
+    passing in as arguments to an argparse parser's add_argument() method."""
+
+    argspec = inspect.getargspec(func)
+    # we should probably raise an exception somewhere if func includes **kwargs
+    if argspec.defaults:
+        positional_args = argspec.args[:-len(argspec.defaults)]
+        keyword_names = argspec.args[-len(argspec.defaults):]
+        for arg, default in itertools.izip(keyword_names, argspec.defaults):
+            yield ('--{}'.format(arg),), {'default': default}
+    else:
+        positional_args = argspec.args
+
+    for arg in positional_args:
+        yield (arg,), {}
+    if argspec.varargs:
+        yield (argspec.varargs,), {'nargs': '*'}

=== added file 'hooks/charmhelpers/cli/commands.py'
--- hooks/charmhelpers/cli/commands.py	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers/cli/commands.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,2 @@
+from . import CommandLine
+import host

=== added file 'hooks/charmhelpers/cli/host.py'
--- hooks/charmhelpers/cli/host.py	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers/cli/host.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,15 @@
+from . import cmdline
+from charmhelpers.core import host
+
+
[e-mail]()
+def mounts():
+    "List mounts"
+    return host.mounts()
+
+
[e-mail]_builder('service', description="Control system services")
+def service(subparser):
+    subparser.add_argument("action", help="The action to perform (start, stop, etc...)")
+    subparser.add_argument("service_name", help="Name of the service to control")
+    return host.service

=== added directory 'hooks/charmhelpers/core'
=== added file 'hooks/charmhelpers/core/__init__.py'
=== added file 'hooks/charmhelpers/core/hookenv.py'
--- hooks/charmhelpers/core/hookenv.py	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers/core/hookenv.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,395 @@
+"Interactions with the Juju environment"
+# Copyright 2013 Canonical Ltd.
+#
+# Authors:
+#  Charm Helpers Developers <[e-mail]>
+
+import os
+import json
+import yaml
+import subprocess
+import UserDict
+from subprocess import CalledProcessError
+
+CRITICAL = "CRITICAL"
+ERROR = "ERROR"
+WARNING = "WARNING"
+INFO = "INFO"
+DEBUG = "DEBUG"
+MARKER = object()
+
+cache = {}
+
+
+def cached(func):
+    """Cache return values for multiple executions of func + args
+
+    For example:
+
+        @cached
+        def unit_get(attribute):
+            pass
+
+        unit_get('test')
+
+    will cache the result of unit_get + 'test' for future calls.
+    """
+    def wrapper(*args, **kwargs):
+        global cache
+        key = str((func, args, kwargs))
+        try:
+            return cache[key]
+        except KeyError:
+            res = func(*args, **kwargs)
+            cache[key] = res
+            return res
+    return wrapper
+
+
+def flush(key):
+    """Flushes any entries from function cache where the
+    key is found in the function+args """
+    flush_list = []
+    for item in cache:
+        if key in item:
+            flush_list.append(item)
+    for item in flush_list:
+        del cache[item]
+
+
+def log(message, level=None):
+    """Write a message to the juju log"""
+    command = ['juju-log']
+    if level:
+        command += ['-l', level]
+    command += [message]
+    subprocess.call(command)
+
+
+class Serializable(UserDict.IterableUserDict):
+    """Wrapper, an object that can be serialized to yaml or json"""
+
+    def __init__(self, obj):
+        # wrap the object
+        UserDict.IterableUserDict.__init__(self)
+        self.data = obj
+
+    def __getattr__(self, attr):
+        # See if this object has attribute.
+        if attr in ("json", "yaml", "data"):
+            return self.__dict__[attr]
+        # Check for attribute in wrapped object.
+        got = getattr(self.data, attr, MARKER)
+        if got is not MARKER:
+            return got
+        # Proxy to the wrapped object via dict interface.
+        try:
+            return self.data[attr]
+        except KeyError:
+            raise AttributeError(attr)
+
+    def __getstate__(self):
+        # Pickle as a standard dictionary.
+        return self.data
+
+    def __setstate__(self, state):
+        # Unpickle into our wrapper.
+        self.data = state
+
+    def json(self):
+        """Serialize the object to json"""
+        return json.dumps(self.data)
+
+    def yaml(self):
+        """Serialize the object to yaml"""
+        return yaml.dump(self.data)
+
+
+def execution_environment():
+    """A convenient bundling of the current execution context"""
+    context = {}
+    context['conf'] = config()
+    if relation_id():
+        context['reltype'] = relation_type()
+        context['relid'] = relation_id()
+        context['rel'] = relation_get()
+    context['unit'] = local_unit()
+    context['rels'] = relations()
+    context['env'] = os.environ
+    return context
+
+
+def in_relation_hook():
+    """Determine whether we're running in a relation hook"""
+    return 'JUJU_RELATION' in os.environ
+
+
+def relation_type():
+    """The scope for the current relation hook"""
+    return os.environ.get('JUJU_RELATION', None)
+
+
+def relation_id():
+    """The relation ID for the current relation hook"""
+    return os.environ.get('JUJU_RELATION_ID', None)
+
+
+def local_unit():
+    """Local unit ID"""
+    return os.environ['JUJU_UNIT_NAME']
+
+
+def remote_unit():
+    """The remote unit for the current relation hook"""
+    return os.environ['JUJU_REMOTE_UNIT']
+
+
+def service_name():
+    """The name service group this unit belongs to"""
+    return local_unit().split('/')[0]
+
+
+@cached
+def config(scope=None):
+    """Juju charm configuration"""
+    config_cmd_line = ['config-get']
+    if scope is not None:
+        config_cmd_line.append(scope)
+    config_cmd_line.append('--format=json')
+    try:
+        return json.loads(subprocess.check_output(config_cmd_line))
+    except ValueError:
+        return None
+
+
+@cached
+def relation_get(attribute=None, unit=None, rid=None):
+    """Get relation information"""
+    _args = ['relation-get', '--format=json']
+    if rid:
+        _args.append('-r')
+        _args.append(rid)
+    _args.append(attribute or '-')
+    if unit:
+        _args.append(unit)
+    try:
+        return json.loads(subprocess.check_output(_args))
+    except ValueError:
+        return None
+    except CalledProcessError, e:
+        if e.returncode == 2:
+            return None
+        raise
+
+
+def relation_set(relation_id=None, relation_settings={}, **kwargs):
+    """Set relation information for the current unit"""
+    relation_cmd_line = ['relation-set']
+    if relation_id is not None:
+        relation_cmd_line.extend(('-r', relation_id))
+    for k, v in (relation_settings.items() + kwargs.items()):
+        if v is None:
+            relation_cmd_line.append('{}='.format(k))
+        else:
+            relation_cmd_line.append('{}={}'.format(k, v))
+    subprocess.check_call(relation_cmd_line)
+    # Flush cache of any relation-gets for local unit
+    flush(local_unit())
+
+
+@cached
+def relation_ids(reltype=None):
+    """A list of relation_ids"""
+    reltype = reltype or relation_type()
+    relid_cmd_line = ['relation-ids', '--format=json']
+    if reltype is not None:
+        relid_cmd_line.append(reltype)
+        return json.loads(subprocess.check_output(relid_cmd_line)) or []
+    return []
+
+
+@cached
+def related_units(relid=None):
+    """A list of related units"""
+    relid = relid or relation_id()
+    units_cmd_line = ['relation-list', '--format=json']
+    if relid is not None:
+        units_cmd_line.extend(('-r', relid))
+    return json.loads(subprocess.check_output(units_cmd_line)) or []
+
+
+@cached
+def relation_for_unit(unit=None, rid=None):
+    """Get the json represenation of a unit's relation"""
+    unit = unit or remote_unit()
+    relation = relation_get(unit=unit, rid=rid)
+    for key in relation:
+        if key.endswith('-list'):
+            relation[key] = relation[key].split()
+    relation['__unit__'] = unit
+    return relation
+
+
+@cached
+def relations_for_id(relid=None):
+    """Get relations of a specific relation ID"""
+    relation_data = []
+    relid = relid or relation_ids()
+    for unit in related_units(relid):
+        unit_data = relation_for_unit(unit, relid)
+        unit_data['__relid__'] = relid
+        relation_data.append(unit_data)
+    return relation_data
+
+
+@cached
+def relations_of_type(reltype=None):
+    """Get relations of a specific type"""
+    relation_data = []
+    reltype = reltype or relation_type()
+    for relid in relation_ids(reltype):
+        for relation in relations_for_id(relid):
+            relation['__relid__'] = relid
+            relation_data.append(relation)
+    return relation_data
+
+
+@cached
+def relation_types():
+    """Get a list of relation types supported by this charm"""
+    charmdir = os.environ.get('CHARM_DIR', '')
+    mdf = open(os.path.join(charmdir, 'metadata.yaml'))
+    md = yaml.safe_load(mdf)
+    rel_types = []
+    for key in ('provides', 'requires', 'peers'):
+        section = md.get(key)
+        if section:
+            rel_types.extend(section.keys())
+    mdf.close()
+    return rel_types
+
+
+@cached
+def relations():
+    """Get a nested dictionary of relation data for all related units"""
+    rels = {}
+    for reltype in relation_types():
+        relids = {}
+        for relid in relation_ids(reltype):
+            units = {local_unit(): relation_get(unit=local_unit(), rid=relid)}
+            for unit in related_units(relid):
+                reldata = relation_get(unit=unit, rid=relid)
+                units[unit] = reldata
+            relids[relid] = units
+        rels[reltype] = relids
+    return rels
+
+
+@cached
+def is_relation_made(relation, keys='private-address'):
+    '''
+    Determine whether a relation is established by checking for
+    presence of key(s).  If a list of keys is provided, they
+    must all be present for the relation to be identified as made
+    '''
+    if isinstance(keys, str):
+        keys = [keys]
+    for r_id in relation_ids(relation):
+        for unit in related_units(r_id):
+            context = {}
+            for k in keys:
+                context[k] = relation_get(k, rid=r_id,
+                                          unit=unit)
+            if None not in context.values():
+                return True
+    return False
+
+
+def open_port(port, protocol="TCP"):
+    """Open a service network port"""
+    _args = ['open-port']
+    _args.append('{}/{}'.format(port, protocol))
+    subprocess.check_call(_args)
+
+
+def close_port(port, protocol="TCP"):
+    """Close a service network port"""
+    _args = ['close-port']
+    _args.append('{}/{}'.format(port, protocol))
+    subprocess.check_call(_args)
+
+
+@cached
+def unit_get(attribute):
+    """Get the unit ID for the remote unit"""
+    _args = ['unit-get', '--format=json', attribute]
+    try:
+        return json.loads(subprocess.check_output(_args))
+    except ValueError:
+        return None
+
+
+def unit_private_ip():
+    """Get this unit's private IP address"""
+    return unit_get('private-address')
+
+
+class UnregisteredHookError(Exception):
+    """Raised when an undefined hook is called"""
+    pass
+
+
+class Hooks(object):
+    """A convenient handler for hook functions.
+
+    Example:
+        hooks = Hooks()
+
+        # register a hook, taking its name from the function name
+        @hooks.hook()
+        def install():
+            ...
+
+        # register a hook, providing a custom hook name
+        @hooks.hook("config-changed")
+        def config_changed():
+            ...
+
+        if __name__ == "__main__":
+            # execute a hook based on the name the program is called by
+            hooks.execute(sys.argv)
+    """
+
+    def __init__(self):
+        super(Hooks, self).__init__()
+        self._hooks = {}
+
+    def register(self, name, function):
+        """Register a hook"""
+        self._hooks[name] = function
+
+    def execute(self, args):
+        """Execute a registered hook based on args[0]"""
+        hook_name = os.path.basename(args[0])
+        if hook_name in self._hooks:
+            self._hooks[hook_name]()
+        else:
+            raise UnregisteredHookError(hook_name)
+
+    def hook(self, *hook_names):
+        """Decorator, registering them as hooks"""
+        def wrapper(decorated):
+            for hook_name in hook_names:
+                self.register(hook_name, decorated)
+            else:
+                self.register(decorated.__name__, decorated)
+                if '_' in decorated.__name__:
+                    self.register(
+                        decorated.__name__.replace('_', '-'), decorated)
+            return decorated
+        return wrapper
+
+
+def charm_dir():
+    """Return the root directory of the current charm"""
+    return os.environ.get('CHARM_DIR')

=== added file 'hooks/charmhelpers/core/host.py'
--- hooks/charmhelpers/core/host.py	1970-01-01 00:00:00 +0000
+++ hooks/charmhelpers/core/host.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,291 @@
+"""Tools for working with the host system"""
+# Copyright 2012 Canonical Ltd.
+#
+# Authors:
+#  Nick Moffitt <[e-mail]>
+#  Matthew Wedgwood <[e-mail]>
+
+import os
+import pwd
+import grp
+import random
+import string
+import subprocess
+import hashlib
+
+from collections import OrderedDict
+
+from hookenv import log
+
+
+def service_start(service_name):
+    """Start a system service"""
+    return service('start', service_name)
+
+
+def service_stop(service_name):
+    """Stop a system service"""
+    return service('stop', service_name)
+
+
+def service_restart(service_name):
+    """Restart a system service"""
+    return service('restart', service_name)
+
+
+def service_reload(service_name, restart_on_failure=False):
+    """Reload a system service, optionally falling back to restart if reload fails"""
+    service_result = service('reload', service_name)
+    if not service_result and restart_on_failure:
+        service_result = service('restart', service_name)
+    return service_result
+
+
+def service(action, service_name):
+    """Control a system service"""
+    cmd = ['service', service_name, action]
+    return subprocess.call(cmd) == 0
+
+
+def service_running(service):
+    """Determine whether a system service is running"""
+    try:
+        output = subprocess.check_output(['service', service, 'status'])
+    except subprocess.CalledProcessError:
+        return False
+    else:
+        if ("start/running" in output or "is running" in output):
+            return True
+        else:
+            return False
+
+
+def adduser(username, password=None, shell='/bin/bash', system_user=False):
+    """Add a user to the system"""
+    try:
+        user_info = pwd.getpwnam(username)
+        log('user {0} already exists!'.format(username))
+    except KeyError:
+        log('creating user {0}'.format(username))
+        cmd = ['useradd']
+        if system_user or password is None:
+            cmd.append('--system')
+        else:
+            cmd.extend([
+                '--create-home',
+                '--shell', shell,
+                '--password', password,
+            ])
+        cmd.append(username)
+        subprocess.check_call(cmd)
+        user_info = pwd.getpwnam(username)
+    return user_info
+
+
+def add_user_to_group(username, group):
+    """Add a user to a group"""
+    cmd = [
+        'gpasswd', '-a',
+        username,
+        group
+    ]
+    log("Adding user {} to group {}".format(username, group))
+    subprocess.check_call(cmd)
+
+
+def rsync(from_path, to_path, flags='-r', options=None):
+    """Replicate the contents of a path"""
+    options = options or ['--delete', '--executability']
+    cmd = ['/usr/bin/rsync', flags]
+    cmd.extend(options)
+    cmd.append(from_path)
+    cmd.append(to_path)
+    log(" ".join(cmd))
+    return subprocess.check_output(cmd).strip()
+
+
+def symlink(source, destination):
+    """Create a symbolic link"""
+    log("Symlinking {} as {}".format(source, destination))
+    cmd = [
+        'ln',
+        '-sf',
+        source,
+        destination,
+    ]
+    subprocess.check_call(cmd)
+
+
+def mkdir(path, owner='root', group='root', perms=0555, force=False):
+    """Create a directory"""
+    log("Making dir {} {}:{} {:o}".format(path, owner, group,
+                                          perms))
+    uid = pwd.getpwnam(owner).pw_uid
+    gid = grp.getgrnam(group).gr_gid
+    realpath = os.path.abspath(path)
+    if os.path.exists(realpath):
+        if force and not os.path.isdir(realpath):
+            log("Removing non-directory file {} prior to mkdir()".format(path))
+            os.unlink(realpath)
+    else:
+        os.makedirs(realpath, perms)
+    os.chown(realpath, uid, gid)
+
+
+def write_file(path, content, owner='root', group='root', perms=0444):
+    """Create or overwrite a file with the contents of a string"""
+    log("Writing file {} {}:{} {:o}".format(path, owner, group, perms))
+    uid = pwd.getpwnam(owner).pw_uid
+    gid = grp.getgrnam(group).gr_gid
+    with open(path, 'w') as target:
+        os.fchown(target.fileno(), uid, gid)
+        os.fchmod(target.fileno(), perms)
+        target.write(content)
+
+
+def mount(device, mountpoint, options=None, persist=False):
+    """Mount a filesystem at a particular mountpoint"""
+    cmd_args = ['mount']
+    if options is not None:
+        cmd_args.extend(['-o', options])
+    cmd_args.extend([device, mountpoint])
+    try:
+        subprocess.check_output(cmd_args)
+    except subprocess.CalledProcessError, e:
+        log('Error mounting {} at {}\n{}'.format(device, mountpoint, e.output))
+        return False
+    if persist:
+        # TODO: update fstab
+        pass
+    return True
+
+
+def umount(mountpoint, persist=False):
+    """Unmount a filesystem"""
+    cmd_args = ['umount', mountpoint]
+    try:
+        subprocess.check_output(cmd_args)
+    except subprocess.CalledProcessError, e:
+        log('Error unmounting {}\n{}'.format(mountpoint, e.output))
+        return False
+    if persist:
+        # TODO: update fstab
+        pass
+    return True
+
+
+def mounts():
+    """Get a list of all mounted volumes as [[mountpoint,device],[...]]"""
+    with open('/proc/mounts') as f:
+        # [['/mount/point','/dev/path'],[...]]
+        system_mounts = [m[1::-1] for m in [l.strip().split()
+                                            for l in f.readlines()]]
+    return system_mounts
+
+
+def file_hash(path):
+    """Generate a md5 hash of the contents of 'path' or None if not found """
+    if os.path.exists(path):
+        h = hashlib.md5()
+        with open(path, 'r') as source:
+            h.update(source.read())  # IGNORE:E1101 - it does have update
+        return h.hexdigest()
+    else:
+        return None
+
+
+def restart_on_change(restart_map):
+    """Restart services based on configuration files changing
+
+    This function is used a decorator, for example
+
+        @restart_on_change({
+            '/etc/ceph/ceph.conf': [ 'cinder-api', 'cinder-volume' ]
+            })
+        def ceph_client_changed():
+            ...
+
+    In this example, the cinder-api and cinder-volume services
+    would be restarted if /etc/ceph/ceph.conf is changed by the
+    ceph_client_changed function.
+    """
+    def wrap(f):
+        def wrapped_f(*args):
+            checksums = {}
+            for path in restart_map:
+                checksums[path] = file_hash(path)
+            f(*args)
+            restarts = []
+            for path in restart_map:
+                if checksums[path] != file_hash(path):
+                    restarts += restart_map[path]
+            for service_name in list(OrderedDict.fromkeys(restarts)):
+                service('restart', service_name)
+        return wrapped_f
+    return wrap
+
+
+def lsb_release():
+    """Return /etc/lsb-release in a dict"""
+    d = {}
+    with open('/etc/lsb-release', 'r') as lsb:
+        for l in lsb:
+            k, v = l.split('=')
+            d[k.strip()] = v.strip()
+    return d
+
+
+def pwgen(length=None):
+    """Generate a random pasword."""
+    if length is None:
+        length = random.choice(range(35, 45))
+    alphanumeric_chars = [
+        l for l in (string.letters + string.digits)
+        if l not in 'l0QD1vAEIOUaeiou']
+    random_chars = [
+        random.choice(alphanumeric_chars) for _ in range(length)]
+    return(''.join(random_chars))
+
+
+def list_nics(nic_type):
+    '''Return a list of nics of given type(s)'''
+    if isinstance(nic_type, basestring):
+        int_types = [nic_type]
+    else:
+        int_types = nic_type
+    interfaces = []
+    for int_type in int_types:
+        cmd = ['ip', 'addr', 'show', 'label', int_type + '*']
+        ip_output = subprocess.check_output(cmd).split('\n')
+        ip_output = (line for line in ip_output if line)
+        for line in ip_output:
+            if line.split()[1].startswith(int_type):
+                interfaces.append(line.split()[1].replace(":", ""))
+    return interfaces
+
+
+def set_nic_mtu(nic, mtu):
+    '''Set MTU on a network interface'''
+    cmd = ['ip', 'link', 'set', nic, 'mtu', mtu]
+    subprocess.check_call(cmd)
+
+
+def get_nic_mtu(nic):
+    cmd = ['ip', 'addr', 'show', nic]
+    ip_output = subprocess.check_output(cmd).split('\n')
+    mtu = ""
+    for line in ip_output:
+        words = line.split()
+        if 'mtu' in words:
+            mtu = words[words.index("mtu") + 1]
+    return mtu
+
+
+def get_nic_hwaddr(nic):
+    cmd = ['ip', '-o', '-0', 'addr', 'show', nic]
+    ip_output = subprocess.check_output(cmd)
+    hwaddr = ""
+    words = ip_output.split()
+    if 'link/ether' in words:
+        hwaddr = words[words.index('link/ether') + 1]
+    return hwaddr

=== added file 'hooks/common_util.py'
--- hooks/common_util.py	1970-01-01 00:00:00 +0000
+++ hooks/common_util.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,219 @@
+"""Common python utilities for storage providers"""
+from charmhelpers.core import hookenv
+import os
+import subprocess
+import sys
+from time import sleep
+from stat import S_ISBLK, ST_MODE
+
+CHARM_DIR = os.environ.get("CHARM_DIR")
+FSTAB_FILE = "/etc/fstab"
+
+
+def log(message, level=None):
+    """Quaint little log wrapper for juju logging"""
+    hookenv.log(message, level)
+
+
+def get_provider():
+    """Get the storage charm's provider type as set within it's config.yaml"""
+    provider = hookenv.config("provider")
+    if provider:
+        return provider
+    else:
+        log(
+            "Error: no provider defined in storage charm config",
+            hookenv.ERROR)
+        sys.exit(1)
+
+
+def is_mounted():
+    """Return True if the defined mountpoint is mounted"""
+    mountpoint = hookenv.relation_get("mountpoint")
+    if not mountpoint:
+        log("INFO: No mountpoint defined by relation assuming not mounted.")
+        return False
+    return os.path.ismount(mountpoint)
+
+
+def unmount_volume():
+    """Unmount the relation-requested C{mountpoint}"""
+    success = False
+    mountpoint = hookenv.relation_get("mountpoint")
+    if not mountpoint:
+        log("INFO: No mountpoint defined by relation no unmount performed.")
+        return
+
+    if not is_mounted():
+        log("%s is not mounted, Done." % mountpoint)
+        success = True
+    else:
+        for x in range(0, 20):
+            if subprocess.call(["umount", mountpoint]) == 0:
+                success = True
+                log("Unmounted %s, Done" % mountpoint)
+                break
+            else:  # Then device is in use let's report the offending process
+                process_name = subprocess.check_output(
+                    "lsof %s | awk '(NR == 2){print $1}'" % mountpoint)
+                log("WARNING: umount %s failed. Device in use by (%s)" %
+                    (mountpoint, process_name.strip()))
+            sleep(5)
+
+    if not success:
+        log(
+            "ERROR: Unmount failed, leaving relation in errored state",
+            hookenv.ERROR)
+        sys.exit(1)
+
+
+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):
+        if not os.path.exists(device_path):
+            log("WARNING: %s does not exist" % device_path)
+        else:
+            try:
+                mode = os.stat(device_path)[ST_MODE]
+                if S_ISBLK(mode):
+                    log(
+                        "DEBUG: block device found. Proceeding.",
+                        hookenv.DEBUG)
+                    isBlockDevice = True
+                    break
+            except OSError:
+                pass
+        sleep(5)
+    if not isBlockDevice:
+        log("ERROR: %s is not a block device." % device_path)
+        sys.exit(1)
+
+
+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)
+        sys.exit(1)
+
+    current_label = subprocess.check_output(
+        "e2label %s" % device_path, shell=True)
+    if current_label:
+        current_label = current_label.strip()
+        if current_label == label:
+            # Label already set, return
+            return
+        log("WARNING: %s had label=%s, overwriting with label=%s" %
+            (device_path, current_label, label))
+    subprocess.check_call(
+        "e2label %s %s" % (device_path, label), shell=True)
+
+
+def _read_partition_table(device_path):
+    """Call blockdev to read a valid partition table and return exit code"""
+    return subprocess.call(
+        "blockdev --rereadpt %s" % device_path, shell=True)
+
+
+def _assert_fstype(device_path, fstype):
+    """Return C{True} if a filesystem is of type C{fstype}"""
+    command = "file -s %s | egrep -q %s" % (device_path, fstype)
+    return subprocess.call(command, shell=True) == 0
+
+
+def _format_device(device_path):
+    """
+    Create an ext4 partition, if needed, for C{device_path} and fsck that
+    partition.
+    """
+    result = _read_partition_table(device_path)
+    if result == 1:
+        log("INFO: %s is busy, no fsck performed. Assuming formatted." %
+            device_path)
+    elif result == 0:
+        # Create an ext4 filesystem if NOT already present
+        # use e.g. LABEl=vol-000012345
+        if _assert_fstype(device_path, "ext4"):
+            log("%s already formatted - skipping mkfs.ext4." % device_path)
+        else:
+            command = "mkfs.ext4 %s" % device_path
+            log("NOTICE: Running: %s" % command)
+            subprocess.check_call(command, shell=True)
+        if subprocess.call("fsck -p %s" % device_path, shell=True) != 0:
+            log("ERROR: fsck -p %s failed" % device_path)
+            sys.exit(1)
+
+
+def mount_volume(device_path=None):
+    """
+    Mount the attached volume, nfs or blockstoragebroker 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")
+    if not mountpoint:
+        log("No mountpoint defined by relation. Cannot mount volume yet.")
+        sys.exit(0)
+
+    if is_mounted():
+        log("Volume (%s) already mounted" % mountpoint)
+        # publish changes to data relation and exit
+        for relid in hookenv.relation_ids("data"):
+            hookenv.relation_set(
+                relid, relation_settings={"mountpoint": mountpoint})
+        sys.exit(0)
+
+    provider = get_provider()
+    options = "default"
+    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", "default")
+                break
+
+        if None in [nfs_server, nfs_path, fstype]:
+            log("ERROR: Missing required relation values. "
+                "nfs-relation-changed hook needs to run first.",
+                hookenv.ERROR)
+            sys.exit(1)
+
+        device_path = "%s:%s" % (nfs_server, nfs_path)
+    elif provider == "blockstoragebroker":
+        if device_path is None:
+            log("ERROR: No persistent nova device provided by "
+                "blockstoragebroker.", hookenv.ERROR)
+            sys.exit(1)
+
+        _assert_block_device(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)
+
+    if not os.path.exists(mountpoint):
+        os.makedirs(mountpoint)
+
+    options_string = "" if options == "default" else " -o %s" % options
+    command = "mount -t %s%s %s %s" % (
+        fstype, options_string, device_path, mountpoint)
+    subprocess.check_call(command, shell=True)
+    log("Mount (%s) successful" % mountpoint)
+    with open(FSTAB_FILE, "a") as output_file:
+        output_file.write(
+            "%s %s %s %s 0 0" % (device_path, mountpoint, fstype, options))
+    # publish changes to data relation and exit
+    for relid in hookenv.relation_ids("data"):
+        hookenv.relation_set(
+            relid, relation_settings={"mountpoint": mountpoint})

=== removed symlink 'hooks/data-relation-broken'
=== target was u'hooks'
=== modified file 'hooks/hooks'
--- hooks/hooks	2013-12-20 20:43:12 +0000
+++ hooks/hooks	2014-02-05 16:40:53 +0000
@@ -2,22 +2,14 @@
 set -ue
 
 provider=$(config-get provider)
-requested_mountpoint=""
 hook=$(basename $0)
+export PYTHONPATH="$CHARM_DIR/hooks"
 
 if [ -z "$provider" ]; then
     echo "ERROR: Missing storage_provider in charm config."
     exit 1
 fi
 
-if [ "$hook" == "data-relation-changed" ]; then
-    requested_mountpoint=$(relation-get mountpoint)
-    if [ -z "$requested_mountpoint" ]; then
-        exit 0
-    fi    
-    mkdir -p $requested_mountpoint
-fi
-
 if [ ! -d "hooks/storage-provider.d/$provider" ]; then
     echo "ERROR: Storage provider: $provider not recognized."
     exit 1
@@ -26,10 +18,6 @@
 if [ ! -f "hooks/storage-provider.d/$provider/$hook" ]; then
     echo "Storage provider: $provider has no $hook hook"
 else
-    echo "Calling storage-provider ($provider) specific $hook hook"
-    "hooks/storage-provider.d/$provider/$hook" $requested_mountpoint
-fi
-
-if [ "$hook" == "data-relation-changed" ]; then
-    relation-set "mountpoint=$requested_mountpoint"
+    echo "Storage provider: $provider calling $hook hook"
+    "hooks/storage-provider.d/$provider/$hook"
 fi

=== added symlink 'hooks/install'
=== target is u'hooks'
=== removed file 'hooks/install'
--- hooks/install	2013-11-28 00:29:13 +0000
+++ hooks/install	1970-01-01 00:00:00 +0000
@@ -1,2 +0,0 @@
-#!/bin/bash
-exit 0

=== added symlink 'hooks/nfs-relation-changed'
=== target is u'hooks'
=== added symlink 'hooks/nfs-relation-departed'
=== target is u'hooks'
=== removed symlink 'hooks/start'
=== target was u'hooks'
=== removed symlink 'hooks/stop'
=== target was u'hooks'
=== added directory 'hooks/storage-provider.d/nfs'
=== added file 'hooks/storage-provider.d/nfs/config-changed'
--- hooks/storage-provider.d/nfs/config-changed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/config-changed	2014-02-05 16:40:53 +0000
@@ -0,0 +1,12 @@
+#!/bin/bash
+
+# Called by both install hook and config-changed hook
+# We need to ensure we install this provider's dependencies in the event
+# that the config provider was just changed to nfs from a different provider
+if dpkg -s nfs-common >/dev/null 2>&1; then
+    echo nfs-common already installed.
+else
+    apt-get -y install -qq nfs-common
+    sed -i -e "s/NEED_IDMAPD.*/NEED_IDMAPD=yes/" /etc/default/nfs-common
+fi
+service idmapd restart || service idmapd start

=== added file 'hooks/storage-provider.d/nfs/data-relation-changed'
--- hooks/storage-provider.d/nfs/data-relation-changed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/data-relation-changed	2014-02-05 16:40:53 +0000
@@ -0,0 +1,12 @@
+#!/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

=== added file 'hooks/storage-provider.d/nfs/data-relation-departed'
--- hooks/storage-provider.d/nfs/data-relation-departed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/data-relation-departed	2014-02-05 16:40:53 +0000
@@ -0,0 +1,5 @@
+#!/usr/bin/python
+
+import common_util
+
+common_util.unmount_volume(remove_persistent_data=True)

=== added symlink 'hooks/storage-provider.d/nfs/install'
=== target is u'config-changed'
=== added file 'hooks/storage-provider.d/nfs/nfs-relation-changed'
--- hooks/storage-provider.d/nfs/nfs-relation-changed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/nfs-relation-changed	2014-02-05 16:40:53 +0000
@@ -0,0 +1,23 @@
+#!/usr/bin/python
+
+from charmhelpers.core import hookenv
+import common_util
+import os
+import subprocess
+import sys
+
+current_dir = os.path.dirname(os.path.realpath(__file__))
+config_changed_path = "%s/config-changed" % current_dir
+if os.path.exists(config_changed_path):
+    subprocess.check_call(config_changed_path)
+
+common_util.log("nfs: We've got an nfs mount")
+
+options = hookenv.relation_get("options")
+mountpoint = hookenv.relation_get("mountpoint")
+fstype = hookenv.relation_get("fstype")
+host = hookenv.relation_get("private-address")
+
+if not fstype:
+    common_util.log("nfs: No fstype defined. Waiting for some real data")
+    sys.exit(0)

=== added file 'hooks/storage-provider.d/nfs/nfs-relation-departed'
--- hooks/storage-provider.d/nfs/nfs-relation-departed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/nfs/nfs-relation-departed	2014-02-05 16:40:53 +0000
@@ -0,0 +1,7 @@
+#!/usr/bin/python
+
+import common_util
+
+common_util.log("nfs: We've lost our shared NFS mount")
+common_util.unmount_volume(remove_persistent_data=True)
+common_util.log("nfs: Fairwell nfs mount, we hardly knew you")

=== removed directory 'hooks/storage-provider.d/nova'
=== removed file 'hooks/storage-provider.d/nova/common'
--- hooks/storage-provider.d/nova/common	2013-12-06 04:26:45 +0000
+++ hooks/storage-provider.d/nova/common	1970-01-01 00:00:00 +0000
@@ -1,120 +0,0 @@
-#!/bin/bash -xe
-
-METADATA_URL=http://169.254.169.254/openstack/2012-08-10/meta_data.json
-LETTERS="b c d e f g h i j k l m n o p r s t u v w x y z"
-VOLUME_NAME=$(config-get id)
-MOUNT=${1:-/mnt/storage}
-OPTIONS=default
-ORIG=/tmp/storage-orig-disks
-NEW=/tmp/storage-new-disks
-PERSIST=~/charm-storage-nova-device
-
-export OS_USERNAME=$(config-get key)
-export OS_TENANT_NAME=$(config-get tenant)
-export OS_PASSWORD=$(config-get secret)
-export OS_AUTH_URL=$(config-get endpoint)
-export OS_REGION_NAME=$(config-get region)
-
-# Strip forward and trailing quote if there.
-INSTANCE_ID=`curl $METADATA_URL | jsonpipe | grep uuid | awk '{ print $2 }'`
-INSTANCE_ID=${INSTANCE_ID%\"}
-INSTANCE_ID=${INSTANCE_ID#\"}
-
-# TODO: detect already attached to this instance
-attach_nova_volume() {
-    [ -f $PERSIST ] && return 0
-    rm -f $ORIG $NEW
-    if ! nova volume-show $VOLUME_NAME; then
-        echo "ERROR: Could not find nova volume: $VOLUME_NAME"
-        exit 1
-    fi
-
-    VOLUME_ID=`nova volume-show $VOLUME_NAME | grep ' id ' | awk '{ print $4 }'`
-    echo "Attaching $VOLUME_NAME ($VOLUME_ID)"
-    calc_device
-    nova volume-attach $INSTANCE_ID $VOLUME_ID /dev/vdz
-
-    # Wait for fdisk to show a new device
-    candidate=""
-    for i in `seq 0 20`; do
-        candidate=$(calc_device)
-        if [ -n "$candidate" ]; then
-            echo "Found new device: $candidate"
-            break
-        fi
-        sleep 5
-    done
-
-    if [ -z "$candidate" ]; then
-        echo "ERROR: No device found after attach"
-        exit 1
-    fi
-
-    # Store so other hooks know what device we are dealing with
-    echo $candidate > ~/charm-storage-nova-device
-}
-
-calc_device() {
-    sudo fdisk -l | grep "^Disk /dev/vd.:" | sed 's/://' | awk '{ print $2 }' > $NEW
-    if [ -f $ORIG ]; then
-        diff --unchanged-line-format= --old-line-format= --new-line-format='%L' $ORIG $NEW || /bin/true
-    else
-        mv -f $NEW $ORIG
-    fi
-}
-
-mount_nova_volume() {
-    device=$(cat ~/charm-storage-nova-device)
-    mkdir -p $MOUNT
-    mount $device $MOUNT
-    fstype=`blkid -o value -s TYPE $device`
-    echo "$device $MOUNT $fstype $OPTIONS 0 0" >> /etc/fstab
-}
-
-detach_nova_volume() {
-    INSTANCE_ID=`curl $METADATA_URL | jsonpipe | grep uuid | awk '{ print $2 }'`
-
-    # Strip forward and trailing quote if there.
-    INSTANCE_ID=${INSTANCE_ID%\"}
-    INSTANCE_ID=${INSTANCE_ID#\"}
-
-    if nova volume-show $VOLUME_NAME; then
-        VOLUME_ID=`nova volume-show $VOLUME_NAME | grep ' id ' | awk '{ print $4 }'`
-        STATUS=`nova volume-show $VOLUME_NAME | grep ' status ' | awk '{ print $4 }'`
-    else
-        echo "Cannot find volume name, done"
-        return 0
-    fi
-
-    if [ "$STATUS" == "available" ]; then
-        echo "Volume ($VOLUME_NAME) already detached, Done"
-        return 0
-    fi
-
-    echo "Detaching $VOLUME_NAME ($VOLUME_ID)"
-    nova volume-detach $INSTANCE_ID $VOLUME_ID
-}
-
-is_mounted() {
-    if mount | grep '$MOUNT'; then
-        return 0
-    fi
-    return 1
-}
-
-umount_nova_volume() {
-    if ! is_mounted; then
-        echo "$MOUNT is not mounted, Done."
-        return 0
-    fi
-    for i in `seq 0 20`; do
-        if umount $MOUNT; then
-            echo "Unmounted $MOUNT, Done"
-            return 0
-        fi
-        echo "Cannot unmount, Device probably busy, sleeping and trying again"
-        sleep 5
-    done
-    echo "ERROR: Unmount failed, leaving relation in errored state"
-    exit 1
-}

=== removed file 'hooks/storage-provider.d/nova/config-changed'
--- hooks/storage-provider.d/nova/config-changed	2013-12-04 04:32:42 +0000
+++ hooks/storage-provider.d/nova/config-changed	1970-01-01 00:00:00 +0000
@@ -1,14 +0,0 @@
-#!/bin/bash
-
-if dpkg -s python-novaclient >/dev/null 2>&1; then
-    echo python-novaclient already installed.
-else
-    apt-add-repository -y cloud-archive:havana
-    apt-get update
-    apt-get -y install python-novaclient
-fi
-if dpkg -s python-jsonpipe >/dev/null 2>&1; then
-    echo python-jsonpipe already installed.
-else
-    apt-get -y install python-jsonpipe
-fi

=== removed file 'hooks/storage-provider.d/nova/data-relation-broken'
--- hooks/storage-provider.d/nova/data-relation-broken	2013-11-29 19:44:50 +0000
+++ hooks/storage-provider.d/nova/data-relation-broken	1970-01-01 00:00:00 +0000
@@ -1,7 +0,0 @@
-#!/bin/bash -xe
-
-DIR="$( cd "$( dirname "$0" )" && pwd )"
-source $DIR/common
-
-umount_nova_volume
-detach_nova_volume

=== removed file 'hooks/storage-provider.d/nova/data-relation-changed'
--- hooks/storage-provider.d/nova/data-relation-changed	2013-11-29 19:44:50 +0000
+++ hooks/storage-provider.d/nova/data-relation-changed	1970-01-01 00:00:00 +0000
@@ -1,7 +0,0 @@
-#!/bin/bash -xe
-
-DIR="$( cd "$( dirname "$0" )" && pwd )"
-source $DIR/common
-
-attach_nova_volume
-mount_nova_volume

=== removed file 'hooks/storage-provider.d/nova/start'
--- hooks/storage-provider.d/nova/start	2013-11-29 18:12:40 +0000
+++ hooks/storage-provider.d/nova/start	1970-01-01 00:00:00 +0000
@@ -1,6 +0,0 @@
-#!/bin/bash -xe
-
-DIR="$( cd "$( dirname "$0" )" && pwd )"
-source $DIR/common
-
-attach_nova_volume

=== removed file 'hooks/storage-provider.d/nova/stop'
--- hooks/storage-provider.d/nova/stop	2013-11-29 19:48:06 +0000
+++ hooks/storage-provider.d/nova/stop	1970-01-01 00:00:00 +0000
@@ -1,7 +0,0 @@
-#!/bin/bash -xe
-
-DIR="$( cd "$( dirname "$0" )" && pwd )"
-source $DIR/common
-
-umount_nova_volume
-detach_nova_volume

=== added file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	1970-01-01 00:00:00 +0000
+++ hooks/test_common_util.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,607 @@
+import common_util as util
+import mocker
+import os
+import subprocess
+from testing import TestHookenv
+from time import sleep
+
+
+class TestCommonUtil(mocker.MockerTestCase):
+
+    def setUp(self):
+        self.maxDiff = None
+        util.FSTAB_FILE = self.makeFile()
+        util.hookenv = TestHookenv({"provider": "nfs"})
+
+    def test_get_provider(self):
+        """L{get_provider} returns the C{provider} config setting if present"""
+        self.assertEqual(util.get_provider(), "nfs")
+
+    def test_get_provider_not_set(self):
+        """
+        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)
+        message = "Error: no provider defined in storage charm config"
+        self.assertTrue(util.hookenv.is_logged(message), "Error not logged")
+
+    def test_is_mounted_when_mountpoint_is_mounted(self):
+        """
+        L{is_mounted} returns C{True} when persisted 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())
+
+    def test_is_mounted_when_mountpoint_is_not_mounted(self):
+        """
+        L{is_mounted} returns C{False} when persisted 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())
+
+    def test_is_mounted_when_mountpoint_not_defined_by_relation(self):
+        """
+        L{is_mounted} returns C{False} while the relation doesn't define
+        C{mountpoint}
+        """
+        self.assertIsNone(util.hookenv.relation_get("mountpoint"))
+        self.assertFalse(util.is_mounted())
+        message = "No mountpoint defined by relation assuming not mounted."
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_unmount_volume_when_mountpoint_none(self):
+        """
+        L{unmount_volume} will log a message and do nothing if mountpoint is
+        not defined.
+        """
+        self.assertIsNone(util.hookenv.relation_get("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.
+        """
+        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()
+
+        util.unmount_volume()
+
+        message = "/mnt/this is not mounted, Done."
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_unmount_volume_multiple_failure(self):
+        """
+        L{unmount_volume} will attempt to unmount a mounted filesystem 20 times
+        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"),)
+
+        ismount = self.mocker.replace(os.path.ismount)
+        ismount("/mnt/this")
+        self.mocker.result(True)
+        umount = self.mocker.replace(subprocess.call)
+        umount(["umount", "/mnt/this"])
+        self.mocker.result(2)  # Failure from the shell cmd
+        self.mocker.count(20)
+        lsof = self.mocker.replace(subprocess.check_output)
+        lsof("lsof /mnt/this | awk '(NR == 2){print $1}'",)
+        self.mocker.result("postgresql")  # Process name using the filesystem
+        self.mocker.count(20)
+        sleep_mock = self.mocker.replace(sleep)
+        sleep_mock(5)
+        self.mocker.count(20)
+        self.mocker.replay()
+
+        self.assertRaises(SystemExit, util.unmount_volume)
+        message = "ERROR: Unmount failed, leaving relation in errored state"
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_unmount_volume_success(self):
+        """
+        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"),)
+
+        ismount = self.mocker.replace(os.path.ismount)
+        ismount("/mnt/this")
+        self.mocker.result(True)
+        unmount_cmd = self.mocker.replace(subprocess.call)
+        unmount_cmd(["umount", "/mnt/this"])
+        self.mocker.result(0)  # Success from the shell cmd
+        self.mocker.replay()
+
+        util.unmount_volume()
+        message = "Unmounted /mnt/this, Done"
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_wb_assert_block_device_failure_path_does_not_exist(self):
+        """
+        L{_assert_block_device} will exit in error if the C{device_path}
+        provided does not exist.
+        """
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdx")
+        self.mocker.result(False)
+        self.mocker.count(10)
+        sleep_mock = self.mocker.replace(sleep)
+        sleep_mock(5)
+        self.mocker.count(10)
+        self.mocker.replay()
+
+        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):
+        """
+        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.
+        """
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdx")
+        self.mocker.result(True)  # Mock that file exists
+        self.mocker.count(10)
+        stat = self.mocker.replace(os.stat)
+        stat("/dev/vdx")
+        self.mocker.count(10)
+        self.mocker.throw(OSError)  # When device file doesn't exist
+        sleep_mock = self.mocker.replace(sleep)
+        sleep_mock(5)
+        self.mocker.count(10)
+        self.mocker.replay()
+
+        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):
+        """
+        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")
+        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):
+        """
+        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)
+        exists("/dev/vdz")
+        self.mocker.result(True)
+        e2label_check = self.mocker.replace(subprocess.check_output)
+        e2label_check("e2label /dev/vdz", shell=True)
+        self.mocker.result("/mnt/this\n")
+        self.mocker.replay()
+        util._set_label("/dev/vdz", "/mnt/this")
+
+    def test_wb_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}
+        will reset the filesystem label and log a message.
+        """
+        exists = self.mocker.replace(os.path.exists)
+        exists("/dev/vdz")
+        self.mocker.result(True)
+        e2label_check = self.mocker.replace(subprocess.check_output)
+        e2label_check("e2label /dev/vdz", shell=True)
+        self.mocker.result("other-label\n")
+        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")
+        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):
+        """
+        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)
+        exists("/dev/vdz")
+        self.mocker.result(True)
+        e2label_check = self.mocker.replace(subprocess.check_output)
+        e2label_check("e2label /dev/vdz", shell=True)
+        self.mocker.result("")
+        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")
+
+    def test_wb_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.
+        """
+        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")
+        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):
+        """
+        When L{_read_partition_table} reports the device partition table is
+        readable, and L{_assert_fstype} returns C{True}, mkfs.ext4 is not run
+        but fsck is run.
+        """
+        partition_table_mock = self.mocker.replace(util._read_partition_table)
+        partition_table_mock("/dev/vdz")
+        self.mocker.result(0)  # Report that partition table is readable
+
+        # Check formatted filesystem type
+        ext4_check = self.mocker.replace(util._assert_fstype)
+        ext4_check("/dev/vdz", "ext4")
+        self.mocker.result(True)
+
+        # Assert fsck is called
+        fsck = self.mocker.replace(subprocess.call)
+        fsck("fsck -p /dev/vdz", shell=True)
+        self.mocker.result(0)   # successful fsck command exit
+        self.mocker.replay()
+
+        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):
+        """
+        When L{_read_partition_table} reports the device partition table is
+        readable, and L{_assert_fstype} returns C{Flase}, mkfs.ext4 and fsck
+        are run.
+        """
+        partition_table_mock = self.mocker.replace(util._read_partition_table)
+        partition_table_mock("/dev/vdz")
+        self.mocker.result(0)  # Report that partition table is readable
+
+        # Check formatted filesystem type
+        ext4_check = self.mocker.replace(util._assert_fstype)
+        ext4_check("/dev/vdz", "ext4")
+        self.mocker.result(False)
+        fsck = self.mocker.replace(subprocess.check_call)
+        fsck("mkfs.ext4 /dev/vdz", shell=True)
+        fsck = self.mocker.replace(subprocess.call)
+        fsck("fsck -p /dev/vdz", shell=True)
+        self.mocker.result(0)   # Sucessful command exit 0
+        self.mocker.replay()
+
+        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):
+        """
+        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)
+        partition_table_mock("/dev/vdz")
+        self.mocker.result(0)  # Report that partition table is readable
+
+        # Check formatted filesystem type
+        ext4_check = self.mocker.replace(util._assert_fstype)
+        ext4_check("/dev/vdz", "ext4")
+        self.mocker.result(False)
+        fsck = self.mocker.replace(subprocess.check_call)
+        fsck("mkfs.ext4 /dev/vdz", shell=True)
+        fsck = self.mocker.replace(subprocess.call)
+        fsck("fsck -p /dev/vdz", shell=True)
+        self.mocker.result(1)
+        self.mocker.replay()
+
+        result = self.assertRaises(
+            SystemExit, util._format_device, "/dev/vdz")
+        self.assertEqual(result.code, 1)
+
+        messages = ["NOTICE: Running: mkfs.ext4 /dev/vdz",
+                    "ERROR: fsck -p /dev/vdz failed"]
+        for message in messages:
+            self.assertTrue(
+                util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_mount_volume_mountpoint_not_in_relation(self):
+        """
+        L{mount_volume} will log a message when called while the C{mountpoint}
+        has not yet defined by the relation principal yet.
+        """
+        self.assertIsNone(util.hookenv.relation_get("mountpoint"))
+        result = self.assertRaises(SystemExit, util.mount_volume)
+        self.assertEqual(result.code, 0)
+        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):
+        """
+        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.
+        """
+        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")
+        self.mocker.result(True)
+        self.mocker.replay()
+
+        result = self.assertRaises(SystemExit, util.mount_volume)
+        self.assertEqual(result.code, 0)
+        message = "Volume (/mnt/this) already mounted"
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_mount_volume_mountpoint_from_relation_data(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.
+        """
+        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()
+
+        result = self.assertRaises(SystemExit, util.mount_volume)
+        self.assertEqual(result.code, 0)
+        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}.
+        """
+        self.addCleanup(
+            setattr, util.hookenv, "_incoming_relation_data", ())
+        data = (
+            (None, "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"), "/mnt/this")
+            self.assertRaises(SystemExit, util.mount_volume)
+            message = (
+                "ERROR: 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.
+        """
+        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)
+        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"),))
+
+    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", ())
+        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)
+        message = (
+            "ERROR: No persistent nova device provided by blockstoragebroker.")
+        self.assertTrue(
+            util.hookenv.is_logged(message), "Not logged- %s" % message)
+
+    def test_mount_volume_nova_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.
+        """
+        self.addCleanup(
+            setattr, util.hookenv, "_incoming_relation_data", ())
+        util.hookenv._incoming_relation_data = (
+            (None, "mountpoint", "/mnt/this"),)
+
+        self.addCleanup(
+            setattr, util.hookenv, "_config", (("provider", "nfs"),))
+        util.hookenv._config = (("provider", "blockstoragebroker"),)
+        self.addCleanup(
+            setattr, util.hookenv, "_outgoing_relation_data", ())
+
+        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")
+        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 = self.mocker.replace(os.path.exists)
+        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(), "blockstoragebroker")
+        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"),))

=== added file 'hooks/testing.py'
--- hooks/testing.py	1970-01-01 00:00:00 +0000
+++ hooks/testing.py	2014-02-05 16:40:53 +0000
@@ -0,0 +1,101 @@
+"""Common test classes and functions used by unit tests"""
+import os
+
+
+class TestHookenv(object):
+    """
+    Testing object to intercept juju calls and inject data, or make sure
+    certain data is set.
+    """
+
+    _log = ()
+    _incoming_relation_data = ()
+    _outgoing_relation_data = ()
+    _relation_list = ()
+    _config = ()
+
+    DEBUG = 0
+    INFO = 1
+    WARNING = 2
+    ERROR = 3
+    CRITICAL = 4
+
+    def __init__(self, config={}):
+        for key, value in config.iteritems():
+            self._config = self._config + ((key, value),)
+
+    def config(self, scope=None):
+        """Return our initialized config information or a specific value"""
+        if scope:
+            for key, value in self._config:
+                if key == scope:
+                    return value
+            return None
+        return dict((key, value) for key, value in self._config)
+
+    def relation_set(self, relid=None, relation_settings={}):
+        """
+        Capture result of relation_set into _outgoing_relation_data, which
+        can then be checked later.
+        """
+        for key, value in relation_settings.iteritems():
+            self._outgoing_relation_data = (
+                self._outgoing_relation_data + ((key, value),))
+
+    def relation_ids(self, relation_name="website"):
+        """
+        Hardcode expected relation_ids for tests.  Feel free to expand
+        as more tests are added.
+        """
+        return ["%s:1" % relation_name]
+
+    def related_units(self, relation_id="website:1"):
+        """
+        Hardcode expected related_units for tests.  Feel free to expand
+        as more tests are added.
+        """
+        return ["%s/0" % relation_id.split(":")[0]]
+
+    def relation_list(self):
+        """
+        Hardcode expected relation_list for tests.  Feel free to expand
+        as more tests are added.
+        """
+        return list(self._relation_list)
+
+    def unit_get(self, *args):
+        """
+        for now the only thing this is called for is "public-address",
+        so it's a simplistic return.
+        """
+        return "localhost"
+
+    def local_unit(self):
+        return os.environ["JUJU_UNIT_NAME"]
+
+    def log(self, message, level):
+        self._log = self._log + (message,)
+
+    def is_logged(self, message):
+        for line in self._log:
+            if message in line:
+                return True
+        return False
+
+    def config_get(self, scope=None):
+        if scope:
+            for key, value in self._config:
+                if key == scope:
+                    return scope
+            return None
+        return dict((key, value) for key, value in self._config)
+
+    def relation_get(self, scope=None, unit=None, rid=None):
+        data = self._incoming_relation_data
+        if scope:
+            for (rel_id, key, value) in data:
+                if rel_id == rid and key == scope:
+                    return value
+            return None
+        return dict(
+            (key, value) for (rel_id, key, value) in data if rel_id == rid)

=== modified file 'metadata.yaml'
--- metadata.yaml	2013-12-10 17:19:41 +0000
+++ metadata.yaml	2014-02-05 16:40:53 +0000
@@ -14,4 +14,5 @@
     data:
         interface: block-storage
         scope: container
-
+    nfs:
+        interface: mount