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

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

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

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/

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

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

[2] Add _trial_tmp to .bzrignore

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

[4] Remove the revision file.

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

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

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

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

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.

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

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.

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

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.

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

== 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. :)