Merge lp:~bjornt/charms/precise/storage/multiple-units into lp:charms/storage

Proposed by Björn Tillenius on 2016-01-20
Status: Merged
Merged at revision: 43
Proposed branch: lp:~bjornt/charms/precise/storage/multiple-units
Merge into: lp:charms/storage
Diff against target: 35 lines (+21/-0)
1 file modified
hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed (+21/-0)
To merge this branch: bzr merge lp:~bjornt/charms/precise/storage/multiple-units
Reviewer Review Type Date Requested Status
Review Queue (community) automated testing Needs Fixing on 2016-02-19
Chris Glass (community) 2016-01-20 Approve on 2016-01-21
Review via email:

Description of the change

Allow multiple storage units to work reliably.

Currently, the storage charm reads the block-device-path setting to get the path of the attached volume to mount. This is full of race conditions when you have multiple storage units, since that single setting is shared for all units. In practice, when you add a second storage unit, it will see the existing block-device-path setting for the first unit and try to mount it before any volume has been attached.

I've added a new relation setting, block-device-path-map, in lp:~bjornt/charms/trusty/block-storage-broker/multiple-units/. That works for multiple units. This branch will use it if exists, and if it doesn't, block-device-path is used as before. This ensures that using this branch with old versions of block-storage-broker won't break.

To post a comment you must log in.
Chris Glass (tribaal) wrote :

That looks great, thanks a lot.

One little nitpick inline, then I'll go ahead and merge.

review: Approve
45. By Björn Tillenius on 2016-01-21


Review Queue (review-queue) wrote :

This item has failed automated testing! Results available here

review: Needs Fixing (automated testing)
Review Queue (review-queue) wrote :

This item has failed automated testing! Results available here

review: Needs Fixing (automated testing)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed'
--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed 2015-08-24 20:21:07 +0000
+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed 2016-01-21 14:37:26 +0000
@@ -1,4 +1,5 @@
2import json
3import common_util4import common_util
4import sys5import sys
@@ -50,5 +51,25 @@
50# data relation51# data relation
51device_path = common_util._get_from_relation(52device_path = common_util._get_from_relation(
52 "block-storage", "block-device-path")53 "block-storage", "block-device-path")
55# Get the device path from block-device-path-map if it exists, since
56# that works when multiple storage units exist. Older versions of the
57# block-storage-broker charm don't set it, though.
58device_path_map = common_util._get_from_relation(
59 "block-storage", "block-device-path-map")
60if device_path_map is not None:
61 device_path_map = json.loads(device_path_map)
62 hookenv.log(
63 "Getting device_path from block-device-path-map. " +
64 repr(device_path_map))
65 device_path = device_path_map.get(instance_id)
67# Don't try to mount the volume before we set the instance id and the
68# block-storage-broker service gives us back the device path, indicating
69# that the volume has been attached.
70if not device_path:
71 hookenv.log("No block-device specified yet.")
72 sys.exit(0)
53common_util.mount_volume(device_path, # Will wait for mountpoint74common_util.mount_volume(device_path, # Will wait for mountpoint
54 device_timeout=config_data.get('device_timeout'))75 device_timeout=config_data.get('device_timeout'))


People subscribed via source and target branches

to all changes: