Merge lp:~bjornt/charms/trusty/block-storage-broker/multiple-units into lp:~charmers/charms/trusty/block-storage-broker/trunk

[Björn Tillenius]

Allow the block-storage-broker work with multiple storage units without race conditions.

Currently, a single block-device-path relation setting is used for all storage units. That means that if you add one unit at a time. But in practice, I always get an error when I try to add a second storage unit, since it tries to mount the device before it has been attached.

This branch adds another relation setting, block-device-path-map, which is a mapping of instance id to device path. That way the storage charm can look at that and only try to mount the device when its instance id is in that map.

This change is backwards-compatible so the old storage charm will work in the same way as it works now. I modified the storage charm to look for the new setting here: lp:~bjornt/charms/precise/storage/multiple-units/

[Chris Glass (tribaal)]

Looks good! +1

review: Approve

[Review Queue (review-queue)]

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

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

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

review: Needs Fixing (automated testing)

=== added symlink 'hooks/block-storage-relation-joined'
=== target is u'hooks.py'
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-08-22 07:52:20 +0000
+++ hooks/hooks.py	2016-01-20 15:57:09 +0000
@@ -113,6 +113,16 @@
     storage_util.detach_volume(volume_label)
 
 
[e-mail]("block-storage-relation-joined")
+def block_storage_relation_joined():
+    # Initialize the block-device-path-map, so that units can expect it
+    # to be a dict.
+    if hookenv.relation_get(
+            "block-device-path-map", unit=hookenv.local_unit()) is None:
+        hookenv.relation_set(
+            relation_settings={"block-device-path-map": json.dumps({})})
+
+
 @hooks.hook("block-storage-relation-changed")
 def block_storage_relation_changed():
     """Attach a volume to the C{instance-id} requested by the relation
@@ -140,6 +150,12 @@
 
     # Volume is attached, send the path back to the remote storage unit
     hookenv.relation_set(relation_settings={"block-device-path": device_path})
+    device_map_json = hookenv.relation_get(
+        "block-device-path-map", unit=hookenv.local_unit())
+    device_map = json.loads(device_map_json) if device_map_json else {}
+    device_map[instance_id] = device_path
+    hookenv.relation_set(
+        relation_settings={"block-device-path-map": json.dumps(device_map)})
 
 
 if __name__ == '__main__':

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2014-09-09 16:24:46 +0000
+++ hooks/test_hooks.py	2016-01-20 15:57:09 +0000
@@ -247,6 +247,31 @@
 
         hooks.install(apt_install=apt_install)
 
+    def test_block_storage_relation_joined_init_block_device_map(self):
+        """
+        When a unit joins the block-storage relation,
+        block-device-path-map is initialized if it's not already there.
+        """
+        hooks.hookenv._outgoing_relation_data = ()
+
+        hooks.block_storage_relation_joined()
+        self.assertEqual(
+            hooks.hookenv._outgoing_relation_data,
+            (("block-device-path-map", "{}"),))
+
+    def test_block_storage_relation_joined_with_block_device_map(self):
+        """
+        When a unit joins the block-storage relation,
+        block-device-path-map isn't touched if it's already defined.
+        """
+        hooks.hookenv._outgoing_relation_data = (
+            ("block-device-path-map", "something"),)
+
+        hooks.block_storage_relation_joined()
+        self.assertEqual(
+            hooks.hookenv._outgoing_relation_data,
+            (("block-device-path-map", "something"),))
+
     def test_block_storage_relation_changed_waits_without_instance_id(self):
         """
         L{block_storage_relation_changed} will log a message and wait when
@@ -268,7 +293,8 @@
         L{block_storage_relation_changed} calls L{attach_volume} when
         C{instance-id} is available in the relation data. To report
         a successful device attach, it sets the relation data
-        C{block-device-path} to the attached volume's device path from nova.
+        C{block-device-path} and C{block-device-path-map} to the
+        attached volume's device path from nova.
         """
         device_path = "/dev/vdc"
         self.addCleanup(setattr, os, "environ", os.environ)
@@ -293,14 +319,16 @@
         hooks.block_storage_relation_changed()
         self.assertEqual(
             hooks.hookenv._outgoing_relation_data,
-            (("block-device-path", device_path),))
+            (("block-device-path", device_path),
+             ("block-device-path-map", '{"i-123": "%s"}' % device_path)))
 
     def test_block_storage_relation_changed_with_instance_id_volume_id(self):
         """
         When C{volume-id} and C{instance-id} are both present in the relation
         data, they will both be passed to L{attach_volume}. To report a
         successful device attach, it sets the relation data
-        C{block-device-path} to the attached volume's device path from nova.
+        C{block-device-path} and C{block-device-path-map} to the
+        attached volume's device path from nova.
         """
         device_path = "/dev/vdc"
         volume_id = "123-123-123"
@@ -328,14 +356,16 @@
         hooks.block_storage_relation_changed()
         self.assertEqual(
             hooks.hookenv._outgoing_relation_data,
-            (("block-device-path", device_path),))
+            (("block-device-path", device_path),
+             ("block-device-path-map", '{"i-123": "%s"}' % device_path)))
 
     def test_block_storage_relation_changed_with_instance_id_size(self):
         """
         When C{size} and C{instance-id} are both present in the relation data,
         they will be passed to L{attach_volume}. To report a successful
-        device attach, it sets the relation data C{block-device-path} to the
-        attached volume's device path from nova.
+        device attach, it sets the relation data C{block-device-path}
+        and C{block-device-path-map} to the attached volume's device
+        path from nova.
         """
         device_path = "/dev/vdc"
         size = 12
@@ -361,7 +391,8 @@
         hooks.block_storage_relation_changed()
         self.assertEqual(
             hooks.hookenv._outgoing_relation_data,
-            (("block-device-path", device_path),))
+            (("block-device-path", device_path),
+             ("block-device-path-map", '{"i-123": "%s"}' % device_path)))
 
     def test_block_storage_relation_departed_fails_without_instance_id(self):
         """

=== modified file 'hooks/testing.py'
--- hooks/testing.py	2014-02-11 22:07:32 +0000
+++ hooks/testing.py	2016-01-20 15:57:09 +0000
@@ -17,6 +17,7 @@
     _outgoing_relation_data = ()
     _relation_list = ()
     _config = ()
+    _local_unit = "local-unit/0"
 
     DEBUG = "DEBUG"
     INFO = "INFO"
@@ -68,7 +69,7 @@
         return "localhost"
 
     def local_unit(self):
-        return os.environ["JUJU_UNIT_NAME"]
+        return self._local_unit
 
     def remote_unit(self):
         return os.environ["JUJU_REMOTE_UNIT"]
@@ -96,9 +97,14 @@
             return None
         return dict((key, value) for key, value in self._config)
 
-    def relation_get(self, scope=None, unit_name=None, relation_id=None):
+    def relation_get(self, scope=None, unit=None, relation_id=None):
+        if unit == self._local_unit:
+            relation_data = self._outgoing_relation_data
+        else:
+            relation_data = self._incoming_relation_data
+
         if scope:
-            for (key, value) in self._incoming_relation_data:
+            for (key, value) in relation_data:
                 if key == scope:
                     return value
             return None