Merge lp:~axwalk/charms/trusty/ceph/trunk into lp:~openstack-charmers-archive/charms/trusty/ceph/next

[Andrew Wilkins]

These changes add support for Juju's new (1.25.0+) storage feature, which enables you to allocate disks specifically to Ceph, rather than using a block device white list. This change requires updating charm-helpers.

We add the "osd-devices" block-type storage, with minimum of 0 and no maximum. Volumes assigned to a unit will be added as OSDs when they are attached, and the block devices visible.

To use this, you would, for example do:
   juju deploy cs:trusty/ceph -n 3 --storage osd-devices=5,50G

which will deploy 3 units of ceph, each with 5x50GiB volumes assigned.

After deployment you can add volumes with, e.g.
    juju storage add ceph/0 osd-devices=50G

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #12264 ceph-next for axwalk mp276727
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/12264/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #13157 ceph-next for axwalk mp276727
    LINT FAIL: lint-test failed
    LINT FAIL: charm-proof failed

LINT Results (max last 2 lines):
make: *** [lint] Error 200
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/13110679/
Build: http://10.245.162.77:8080/job/charm_lint_check/13157/

[Ryan Beisner (1chb1n)]

charm-tools isn't storage-aware yet, hence the lint fail.

https://github.com/juju/charm-tools/issues/41

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7723 ceph-next for axwalk mp276727
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13112876/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7723/

[Chris Holcombe (xfactor973)]

Code looks great. I assume some/all of the charmhelpers changes are not from you. I appreciate you doing a check on json.loads. Great work!

review: Approve

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7725 ceph-next for axwalk mp276727
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/7725/

[Andrew Wilkins (axwalk)]

Chris, sorry, launchpad comments are being swallowed by an errant email filter, didn't see your comment till now. The only charmhelpers changes I personally authored were the storage hookenv functions (storage_get, storage_list).

Ryan, thanks. FYI, charm tools has been updated to do storage linting since then.

[James Page (james-page)]

We can't land this until charm-tools understands storage metadata attributes:

E: Unknown root metadata field (storage)

I however did take a run through the code; see inline comments.

The storage_get and storage_list helpers are not inline with other hookenv functions in that they use "" instead of None for optional paramaters - it would be nice to get that fixed as well.

review: Needs Fixing

[James Page (james-page)]

The current configuration approach has two storage options; osd-devices and osd-journal - it would be nice to see osd-journal handled in the same way - this is typically placed on faster SSD to smooth out write spikes etc...

[James Page (james-page)]

I see storage support committed but not yet released in charm-tools:

https://github.com/juju/charm-tools/commit/6764324cef6017d41de7f0f8fd0252054d82a56e

[James Page (james-page)]

Using charm-tools from github:

E: storage.osd-devices.multiple.range: supported formats are: m (a fixed number), m-n (an explicit range), and m- (a minimum number)

review: Needs Fixing

[James Page (james-page)]

Please set back to 'Needs Review' when ready - thanks!

On Fri, Nov 20, 2015 at 9:48 AM, James Page <email address hidden> wrote:

> The proposal to merge lp:~axwalk/charms/trusty/ceph/trunk into
> lp:~openstack-charmers/charms/trusty/ceph/next has been updated.
>
> Status: Needs review => Work in progress
>
> For more details, see:
> https://code.launchpad.net/~axwalk/charms/trusty/ceph/trunk/+merge/276727
> --
> You are reviewing the proposed merge of
> lp:~axwalk/charms/trusty/ceph/trunk into
> lp:~openstack-charmers/charms/trusty/ceph/next.
>

[Andrew Wilkins (axwalk)]

I've fixed the proof error (proof is stricter than necessary, but follows the docs), and added an optional osd-journal store. This is currently handled the same as the existing config: one-shot, at deployment time only. Storage charmers will need to update it to handle dynamic journal changes if that's ever a requirement.

I've put up another MP to charm-helpers to fix the optional parameters inconsistency, and will update this when it's merged.

[James Page (james-page)]

I've merged the update to charm-helpers - please sync away!

On Mon, Nov 23, 2015 at 1:56 AM, Andrew Wilkins <
<email address hidden>> wrote:

> I've fixed the proof error (proof is stricter than necessary, but follows
> the docs), and added an optional osd-journal store. This is currently
> handled the same as the existing config: one-shot, at deployment time only.
> Storage charmers will need to update it to handle dynamic journal changes
> if that's ever a requirement.
>
> I've put up another MP to charm-helpers to fix the optional parameters
> inconsistency, and will update this when it's merged.
> --
> https://code.launchpad.net/~axwalk/charms/trusty/ceph/trunk/+merge/276727
> You are reviewing the proposed merge of
> lp:~axwalk/charms/trusty/ceph/trunk into
> lp:~openstack-charmers/charms/trusty/ceph/next.
>

[Andrew Wilkins (axwalk)]

Thanks, James. Updated. I've tested with the master branch of Juju, with
   --storage osd-devices=3,50G --storage osd-journal=ebs-ssd,10G

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #13283 ceph-next for axwalk mp276727
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/13283/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #14252 ceph-next for axwalk mp276727
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/14252/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8015 ceph-next for axwalk mp276727
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 124
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13477671/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8015/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8019 ceph-next for axwalk mp276727
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/8019/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8020 ceph-next for axwalk mp276727
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13492604/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8020/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8027 ceph-next for axwalk mp276727
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/8027/

[Andrew Wilkins (axwalk)]

Ping.

[James Page (james-page)]

Pong - LGTM - Merged.

=== modified file 'config.yaml'
--- config.yaml	2015-07-10 14:14:18 +0000
+++ config.yaml	2015-11-23 09:31:27 +0000
@@ -40,7 +40,8 @@
       The devices to format and set up as osd volumes.
       .
       These devices are the range of devices that will be checked for and
-      used across all service units.
+      used across all service units, in addition to any volumes attached
+      via the --storage flag during deployment.
       .
       For ceph >= 0.56.6 these can also be directories instead of devices - the
       charm assumes anything not starting with /dev is a directory instead.
 
=== modified file 'hooks/ceph_hooks.py'
--- hooks/ceph_hooks.py	2015-10-30 02:15:38 +0000
+++ hooks/ceph_hooks.py	2015-11-23 09:31:27 +0000
@@ -29,6 +29,8 @@
     relations_of_type,
     status_set,
     local_unit,
+    storage_get,
+    storage_list
 )
 from charmhelpers.core.host import (
     service_restart,
@@ -145,7 +147,7 @@
     if e_mountpoint and ceph.filesystem_mounted(e_mountpoint):
         umount(e_mountpoint)
 
-    osd_journal = config('osd-journal')
+    osd_journal = get_osd_journal()
     if (osd_journal and not os.path.exists(JOURNAL_ZAPPED) and
             os.path.exists(osd_journal)):
         ceph.zap_disk(osd_journal)
@@ -158,16 +160,36 @@
         ceph.bootstrap_monitor_cluster(config('monitor-secret'))
         ceph.wait_for_bootstrap()
 
-    if ceph.is_bootstrapped():
-        for dev in get_devices():
-            ceph.osdize(dev, config('osd-format'), config('osd-journal'),
-                        reformat_osd(), config('ignore-device-errors'))
-        ceph.start_osds(get_devices())
+    storage_changed()
 
     if relations_of_type('nrpe-external-master'):
         update_nrpe_config()
 
 
[e-mail]('osd-devices-storage-attached', 'osd-devices-storage-detaching')
+def storage_changed():
+    if ceph.is_bootstrapped():
+        for dev in get_devices():
+            ceph.osdize(dev, config('osd-format'), get_osd_journal(),
+                        reformat_osd(), config('ignore-device-errors'))
+        ceph.start_osds(get_devices())
+
+
+def get_osd_journal():
+    '''
+    Returns the block device path to use for the OSD journal, if any.
+
+    If there is an osd-journal storage instance attached, it will be
+    used as the journal. Otherwise, the osd-journal configuration will
+    be returned.
+    '''
+    storage_ids = storage_list('osd-journal')
+    if storage_ids:
+        # There can be at most one osd-journal storage instance.
+        return storage_get('location', storage_ids[0])
+    return config('osd-journal')
+
+
 def get_mon_hosts():
     hosts = []
     addr = get_public_addr()
@@ -207,9 +229,15 @@
 
 def get_devices():
     if config('osd-devices'):
-        return config('osd-devices').split(' ')
+        devices = config('osd-devices').split(' ')
     else:
-        return []
+        devices = []
+    # List storage instances for the 'osd-devices'
+    # store declared for this charm too, and add
+    # their block device paths to the list.
+    storage_ids = storage_list('osd-devices')
+    devices.extend((storage_get('location', s) for s in storage_ids))
+    return devices
 
 
 @hooks.hook('mon-relation-joined')
@@ -231,7 +259,7 @@
         ceph.bootstrap_monitor_cluster(config('monitor-secret'))
         ceph.wait_for_bootstrap()
         for dev in get_devices():
-            ceph.osdize(dev, config('osd-format'), config('osd-journal'),
+            ceph.osdize(dev, config('osd-format'), get_osd_journal(),
                         reformat_osd(), config('ignore-device-errors'))
         ceph.start_osds(get_devices())
         notify_osds()
 
=== modified file 'hooks/charmhelpers/core/hookenv.py'
--- hooks/charmhelpers/core/hookenv.py	2015-11-19 17:16:51 +0000
+++ hooks/charmhelpers/core/hookenv.py	2015-11-23 09:31:27 +0000
@@ -637,7 +637,7 @@
 
 
 @cached
-def storage_get(attribute="", storage_id=""):
+def storage_get(attribute=None, storage_id=None):
     """Get storage attributes"""
     _args = ['storage-get', '--format=json']
     if storage_id:
@@ -651,7 +651,7 @@
 
 
 @cached
-def storage_list(storage_name=""):
+def storage_list(storage_name=None):
     """List the storage IDs for the unit"""
     _args = ['storage-list', '--format=json']
     if storage_name:
 
=== modified file 'hooks/charmhelpers/fetch/bzrurl.py'
--- hooks/charmhelpers/fetch/bzrurl.py	2015-01-26 09:46:20 +0000
+++ hooks/charmhelpers/fetch/bzrurl.py	2015-11-23 09:31:27 +0000
@@ -64,11 +64,15 @@
         except Exception as e:
             raise e
 
-    def install(self, source):
+    def install(self, source, dest=None):
         url_parts = self.parse_url(source)
         branch_name = url_parts.path.strip("/").split("/")[-1]
-        dest_dir = os.path.join(os.environ.get('CHARM_DIR'), "fetched",
-                                branch_name)
+        if dest:
+            dest_dir = os.path.join(dest, branch_name)
+        else:
+            dest_dir = os.path.join(os.environ.get('CHARM_DIR'), "fetched",
+                                    branch_name)
+
         if not os.path.exists(dest_dir):
             mkdir(dest_dir, perms=0o755)
         try:
 
=== added symlink 'hooks/osd-devices-storage-attached'
=== target is u'ceph_hooks.py'
=== added symlink 'hooks/osd-devices-storage-detaching'
=== target is u'ceph_hooks.py'
=== modified file 'metadata.yaml'
--- metadata.yaml	2015-11-18 10:29:56 +0000
+++ metadata.yaml	2015-11-23 09:31:27 +0000
@@ -25,3 +25,12 @@
   nrpe-external-master:
     interface: nrpe-external-master
     scope: container
+storage:
+  osd-devices:
+    type: block
+    multiple:
+      range: 0-
+  osd-journal:
+    type: block
+    multiple:
+      range: 0-1