Refactor create_pool.

Refactor create_pool to allow future methods to easily access
the existing request. This change also includes two changes in
behaviour:

 * If an existing request is invalid due to invalid json or missing
   ops attribute then the exception is now propagated rather than
   logged and suppressed. If an existing request is corrupt then
   fail early otherwise bad things will follow.
 * The existing implementation only allows for a single create_pool
   request. Any additional requests would overwrite previous ones.
   This change allows for multiple pools to be requested in a
   single broker request.

Change-Id: I61998c2ce70325cb904add8c51cbc9c34b3c9564

.gitignore

@@ -2,3 +2,6 @@
 *.swp
 .testrepository
 .tox
+.stestr
+.unit-state.db
+__pycache__

requires.py

@@ -67,6 +67,24 @@ class CephClientRequires(RelationBase):
         self.remove_state('{relation_name}.connected')
         self.remove_state('{relation_name}.pools.available')
 
+    def get_current_request(self):
+        """
+        Retrieve the current Ceph broker request.
+
+        If no request has been created yet then create a new one.
+        """
+        json_rq = self.get_local(key='broker_req')
+        current_request = CephBrokerRq()
+        if json_rq:
+            try:
+                j = json.loads(json_rq)
+                current_request.set_ops(j['ops'])
+            except (KeyError, json.decoder.JSONDecodeError) as err:
+                log("Unable to decode broker_req: {}.  Error: {}".format(
+                    json_rq, err))
+                raise
+        return current_request
+
     def create_pool(self, name, replicas=3, weight=None, pg_num=None,
                     group=None, namespace=None):
         """
@@ -84,29 +102,16 @@ class CephClientRequires(RelationBase):
                           will be used to further restrict pool access.
         """
 
-        # json.dumps of the CephBrokerRq()
-        json_rq = self.get_local(key='broker_req')
-
-        if not json_rq:
-            rq = CephBrokerRq()
-            rq.add_op_create_pool(name="{}".format(name),
-                                  replica_count=replicas,
-                                  pg_num=pg_num,
-                                  weight=weight,
-                                  group=group,
-                                  namespace=namespace)
-            self.set_local(key='broker_req', value=rq.request)
-            send_request_if_needed(rq, relation=self.relation_name)
-        else:
-            rq = CephBrokerRq()
-            try:
-                j = json.loads(json_rq)
-                log("Json request: {}".format(json_rq))
-                rq.ops = j['ops']
-                send_request_if_needed(rq, relation=self.relation_name)
-            except ValueError as err:
-                log("Unable to decode broker_req: {}.  Error: {}".format(
-                    json_rq, err))
+        current_request = self.get_current_request()
+        current_request.add_op_create_pool(
+            name="{}".format(name),
+            replica_count=replicas,
+            pg_num=pg_num,
+            weight=weight,
+            group=group,
+            namespace=namespace)
+        self.set_local(key='broker_req', value=current_request.request)
+        send_request_if_needed(current_request, relation=self.relation_name)
 
     def get_remote_all(self, key, default=None):
         """Return a list of all values presented by remote units for key"""

unit_tests/test_requires.py

@@ -11,8 +11,9 @@
 # limitations under the License.
 
 
-import unittest
+import json
 import mock
+import unittest
 
 
 with mock.patch('charmhelpers.core.hookenv.metadata') as _meta:
@@ -171,6 +172,41 @@ class TestCephClientRequires(unittest.TestCase):
             mock.call('{relation_name}.connected'),
             mock.call('{relation_name}.pools.available')])
 
+    def test_get_current_request_new(self):
+        self.patch_kr('get_local', None)
+        req = self.cr.get_current_request()
+        self.assertEqual(req.ops, [])
+
+    def test_get_current_request_existing(self):
+        req = (
+            '{"api-version": 1, '
+            '"ops": [{"op": "create-pool", "name": "bob", "replicas": 3, '
+            '"pg_num": null, "weight": null, "group": null, '
+            '"group-namespace": null}], '
+            '"request-id": "9e34123e-fa0c-11e8-ad9c-fa163ed1cc55"}')
+        self.patch_kr('get_local', req)
+        req = self.cr.get_current_request()
+        self.assertEqual(
+            req.ops,
+            [{
+                'op': 'create-pool',
+                'name': 'bob',
+                'replicas': 3,
+                'group': None,
+                'group-namespace': None,
+                'pg_num': None,
+                'weight': None}])
+
+    def test_get_current_request_not_json(self):
+        self.patch_kr('get_local', '{I am not json')
+        with self.assertRaises(json.decoder.JSONDecodeError):
+            self.cr.get_current_request()
+
+    def test_get_current_request_missing_ops(self):
+        self.patch_kr('get_local', '{}')
+        with self.assertRaises(KeyError):
+            self.cr.get_current_request()
+
     @mock.patch.object(charmhelpers.contrib.storage.linux.ceph.uuid, 'uuid1')
     def test_create_pool_new_request(self, _uuid1):
         _uuid1.return_value = '9e34123e-fa0c-11e8-ad9c-fa163ed1cc55'
@@ -206,18 +242,27 @@ class TestCephClientRequires(unittest.TestCase):
             '"group-namespace": null}], '
             '"request-id": "9e34123e-fa0c-11e8-ad9c-fa163ed1cc55"}')
         self.patch_kr('get_local', req)
-        self.cr.create_pool('bob')
+        self.cr.create_pool('bill')
         ceph_broker_rq = self.send_request_if_needed.mock_calls[0][1][0]
         self.assertEqual(
             ceph_broker_rq.ops,
-            [{
-                'op': 'create-pool',
-                'name': 'bob',
-                'replicas': 3,
-                'group': None,
-                'group-namespace': None,
-                'pg_num': None,
-                'weight': None}])
+            [
+                {
+                    'op': 'create-pool',
+                    'name': 'bob',
+                    'replicas': 3,
+                    'group': None,
+                    'group-namespace': None,
+                    'pg_num': None,
+                    'weight': None},
+                {
+                    'op': 'create-pool',
+                    'name': 'bill',
+                    'replicas': 3,
+                    'group': None,
+                    'group-namespace': None,
+                    'pg_num': None,
+                    'weight': None}])
 
     @mock.patch.object(requires.hookenv, 'related_units')
     @mock.patch.object(requires.hookenv, 'relation_get')