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: mp+283321@code.launchpad.net

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

Typos.

Review Queue (review-queue) wrote :

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/2277/

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

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/2256/

review: Needs Fixing (automated testing)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed'
2--- hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed 2015-08-24 20:21:07 +0000
3+++ hooks/storage-provider.d/block-storage-broker/block-storage-relation-changed 2016-01-21 14:37:26 +0000
4@@ -1,4 +1,5 @@
5 #!/usr/bin/python
6+import json
7
8 import common_util
9 import sys
10@@ -50,5 +51,25 @@
11 # data relation
12 device_path = common_util._get_from_relation(
13 "block-storage", "block-device-path")
14+
15+# Get the device path from block-device-path-map if it exists, since
16+# that works when multiple storage units exist. Older versions of the
17+# block-storage-broker charm don't set it, though.
18+device_path_map = common_util._get_from_relation(
19+ "block-storage", "block-device-path-map")
20+if device_path_map is not None:
21+ device_path_map = json.loads(device_path_map)
22+ hookenv.log(
23+ "Getting device_path from block-device-path-map. " +
24+ repr(device_path_map))
25+ device_path = device_path_map.get(instance_id)
26+
27+# Don't try to mount the volume before we set the instance id and the
28+# block-storage-broker service gives us back the device path, indicating
29+# that the volume has been attached.
30+if not device_path:
31+ hookenv.log("No block-device specified yet.")
32+ sys.exit(0)
33+
34 common_util.mount_volume(device_path, # Will wait for mountpoint
35 device_timeout=config_data.get('device_timeout'))

Subscribers

People subscribed via source and target branches

to all changes: