Repository: canonical/operator
Pull request #60 (2019-11-28 15:28:57)

Rework error handling in the model module
* CalledProcessError is no longer propagated up and ModelError is raised
  instead;
* A ModelError exception can be more specific based on parameters of
  CalledProccessError. For now this is used for RelationNotFoundError
  only.
* relation hook tool execution errors are now tested via the fake_script
  mechanism;
* use_json is now set to False for relation_set;
* command output is now always captured but returned only if asked for
  (which is True by default).


Patch:
diff --git a/op/model.py b/op/model.py
index 354dd3e..36dcc9a 100644
--- a/op/model.py
+++ b/op/model.py
@@ -174,7 +174,7 @@ class Relation:
                 self.units.add(unit)
                 if self.app is None:
                     self.app = unit.app
-        except RelationNotFound:
+        except RelationNotFoundError:
             # If the relation is dead, just treat it as if it has no remote units.
             pass
         self.data = RelationData(self, our_unit, backend)
@@ -214,7 +214,7 @@ class RelationUnitData(LazyMapping, MutableMapping):
     def _load(self):
         try:
             return self._backend.relation_get(self.relation.id, self.unit.name)
-        except RelationNotFound:
+        except RelationNotFoundError:
             # Dead relations tell no tales (and have no data).
             return {}
 
@@ -259,7 +259,7 @@ class Resources:
         """Fetch the resource from the controller or store.
 
         If successfully fetched, this returns a Path object to where the resource is stored
-        on disk, otherwise it raises a CalledProcessError.
+        on disk, otherwise it raises a ModelError.
         """
         if name not in self._paths:
             raise RuntimeError(f'invalid resource name: {name}')
@@ -279,7 +279,6 @@ class Pod:
 class ModelError(Exception):
     pass
 
-
 class TooManyRelatedApps(ModelError):
     def __init__(self, relation_name, num_related, max_supported):
         super().__init__(f'Too many remote applications on {relation_name} ({num_related} > {max_supported})')
@@ -292,7 +291,7 @@ class RelationDataError(ModelError):
     pass
 
 
-class RelationNotFound(ModelError):
+class RelationNotFoundError(ModelError):
     pass
 
 
@@ -307,60 +306,54 @@ class ModelBackend:
         self._is_leader = None
         self._leader_check_time = 0
 
-    def _run(self, *args, capture_output=True, use_json=True):
-        if capture_output:
-            kwargs = dict(stdout=PIPE, stderr=PIPE)
-            if use_json:
-                args += ('--format=json',)
-        else:
-            kwargs = dict()
-        result = run(args, check=True, **kwargs)
-        if capture_output:
-            text = result.stdout.decode('utf8')
-            if use_json:
-                return json.loads(text)
+    def _run(self, *args, return_output=False, use_json=False):
+        kwargs = dict(stdout=PIPE, stderr=PIPE)
+        if use_json:
+            args += ('--format=json',)
+        try:
+            result = run(args, check=True, **kwargs)
+        except CalledProcessError as e:
+            raise ModelError(e.stderr)
+        if return_output:
+            if result.stdout is None:
+                return ''
             else:
-                return text
+                text = result.stdout.decode('utf8')
+                if use_json:
+                    return json.loads(text)
+                else:
+                    return text
 
     def relation_ids(self, relation_name):
-        relation_ids = self._run('relation-ids', relation_name)
+        relation_ids = self._run('relation-ids', relation_name, return_output=True, use_json=True)
         return [int(relation_id.split(':')[-1]) for relation_id in relation_ids]
 
     def relation_list(self, relation_id):
         try:
-            return self._run('relation-list', '-r', str(relation_id))
-        except CalledProcessError as e:
-            # TODO: This should use the return code if it is specific enough rather than the message.
-            # It seems to be 2 for this error, but I haven't been able to confirm yet if that might
-            # also apply to other error cases.
-            if b'relation not found' in e.stderr:
-                raise RelationNotFound() from e
-            else:
-                raise
+            return self._run('relation-list', '-r', str(relation_id), return_output=True, use_json=True)
+        except ModelError as e:
+            if 'relation not found' in str(e):
+                raise RelationNotFoundError() from e
+            raise
 
     def relation_get(self, relation_id, member_name):
         try:
-            return self._run('relation-get', '-r', str(relation_id), '-', member_name)
-        except CalledProcessError as e:
-            # TODO: This should use the return code if it is specific enough rather than the message.
-            # It seems to be 2 for this error, but I haven't been able to confirm yet if that might
-            # also apply to other error cases.
-            if b'relation not found' in e.stderr:
-                raise RelationNotFound() from e
-            else:
-                raise
+            return self._run('relation-get', '-r', str(relation_id), '-', member_name, return_output=True, use_json=True)
+        except ModelError as e:
+            if 'relation not found' in str(e):
+                raise RelationNotFoundError() from e
+            raise
 
     def relation_set(self, relation_id, key, value):
         try:
-            return self._run('relation-set', '-r', str(relation_id), f'{key}={value}', capture_output=False)
-        except CalledProcessError as e:
-            if b'relation not found' in e.stderr:
-                raise RelationNotFound() from e
-            else:
-                raise
+            return self._run('relation-set', '-r', str(relation_id), f'{key}={value}')
+        except ModelError as e:
+            if 'relation not found' in str(e):
+                raise RelationNotFoundError() from e
+            raise
 
     def config_get(self):
-        return self._run('config-get')
+        return self._run('config-get', return_output=True, use_json=True)
 
     def is_leader(self):
         """Obtain the current leadership status for the unit the charm code is executing on.
@@ -373,12 +366,12 @@ class ModelBackend:
             # Current time MUST be saved before running is-leader to ensure the cache
             # is only used inside the window that is-leader itself asserts.
             self._leader_check_time = now
-            self._is_leader = self._run('is-leader')
+            self._is_leader = self._run('is-leader', return_output=True, use_json=True)
 
         return self._is_leader
 
     def resource_get(self, resource_name):
-        return self._run('resource-get', resource_name, use_json=False).strip()
+        return self._run('resource-get', resource_name, return_output=True).strip()
 
     def pod_spec_set(self, spec, k8s_resources):
         tmpdir = Path(tempfile.mkdtemp('-pod-spec-set'))
@@ -390,6 +383,6 @@ class ModelBackend:
                 k8s_res_path = tmpdir / 'k8s-resources.json'
                 k8s_res_path.write_text(json.dumps(k8s_resources))
                 args.extend(['--k8s-resources', str(k8s_res_path)])
-            self._run('pod-spec-set', *args, capture_output=False)
+            self._run('pod-spec-set', *args)
         finally:
             shutil.rmtree(tmpdir)


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 1f017f3..60f02bf 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -38,7 +38,7 @@ class FakeModelBackend:
                 6: ['remoteapp2/0'],
             }[relation_id]
         except KeyError:
-            raise op.model.RelationNotFound()
+            raise op.model.RelationNotFoundError()
 
     def relation_get(self, relation_id, member_name):
         try:
@@ -57,7 +57,7 @@ class FakeModelBackend:
                 },
             }[relation_id][member_name]
         except KeyError:
-            raise op.model.RelationNotFound()
+            raise op.model.RelationNotFoundError()
 
     def relation_set(self, relation_id, key, value):
         if relation_id == 5:
@@ -250,7 +250,7 @@ class TestModel(unittest.TestCase):
             model.resources.fetch('qux')
 
         fake_script(self, 'resource-get', 'exit 1')
-        with self.assertRaises(subprocess.CalledProcessError):
+        with self.assertRaises(op.model.ModelError):
             model.resources.fetch('foo')
 
         fake_script(self, 'resource-get', 'echo /var/lib/juju/agents/unit-test-0/resources/$1/$1.tgz')
@@ -278,6 +278,56 @@ class TestModel(unittest.TestCase):
         self.assertEqual(k8s_res_path.read_text(), '{"qux": "baz"}')
 
 
+class TestModelBackend(unittest.TestCase):
+
+    def setUp(self):
+        os.environ['JUJU_UNIT_NAME'] = 'myapp/0'
+        self.addCleanup(os.environ.pop, 'JUJU_UNIT_NAME')
+
+        self.backend = op.model.ModelBackend()
+
+    def test_relation_tool_errors(self):
+        RELATION_ERROR_MESSAGE = "ERROR invalid value \"$1\" for option -r: relation not found"
+
+        test_cases = [(
+            lambda: fake_script(self, 'relation-list', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.relation_list(3),
+            op.model.ModelError,
+            [['relation-list', '-r', '3', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'relation-list', f'echo {RELATION_ERROR_MESSAGE} >&2 ; exit 2'),
+            lambda: self.backend.relation_list(3),
+            op.model.RelationNotFoundError,
+            [['relation-list', '-r', '3', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'relation-set', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.relation_set(3, 'foo', 'bar'),
+            op.model.ModelError,
+            [['relation-set', '-r', '3', 'foo=bar']],
+        ), (
+            lambda: fake_script(self, 'relation-set', f'echo {RELATION_ERROR_MESSAGE} >&2 ; exit 2'),
+            lambda: self.backend.relation_set(3, 'foo', 'bar'),
+            op.model.RelationNotFoundError,
+            [['relation-set', '-r', '3', 'foo=bar']],
+        ), (
+            lambda: fake_script(self, 'relation-get', f'echo fooerror >&2 ; exit 1'),
+            lambda: self.backend.relation_get(3, 'remote/0'),
+            op.model.ModelError,
+            [['relation-get', '-r', '3', '-', 'remote/0', '--format=json']],
+        ), (
+            lambda: fake_script(self, 'relation-get', f'echo {RELATION_ERROR_MESSAGE} >&2 ; exit 2'),
+            lambda: self.backend.relation_get(3, 'remote/0'),
+            op.model.RelationNotFoundError,
+            [['relation-get', '-r', '3', '-', 'remote/0', '--format=json']],
+        )]
+
+        for do_fake, run, exception, calls in test_cases:
+            do_fake()
+            with self.assertRaises(exception):
+                run()
+            self.assertEqual(fake_script_calls(self, clear=True), calls)
+
+
 def fake_script(test_case, name, content):
     if not hasattr(test_case, 'fake_script_path'):
         fake_script_path = tempfile.mkdtemp('-fake_script')
@@ -295,9 +345,12 @@ def fake_script(test_case, name, content):
         f.write('#!/bin/bash\n{ echo -n $(basename $0); for s in "$@"; do echo -n \\;$s; done; echo; } >> $(dirname $0)/calls.txt\n' + content)
     os.chmod(test_case.fake_script_path / name, 0o755)
 
-def fake_script_calls(test_case):
-    with open(test_case.fake_script_path / 'calls.txt') as f:
-        return [line.split(';') for line in f.read().splitlines()]
+def fake_script_calls(test_case, clear=False):
+    with open(test_case.fake_script_path / 'calls.txt', 'r+') as f:
+        calls = [line.split(';') for line in f.read().splitlines()]
+        if clear:
+            f.truncate(0)
+        return calls
 
 
 class FakeScriptTest(unittest.TestCase):
@@ -311,3 +364,20 @@ class FakeScriptTest(unittest.TestCase):
             ['foo', 'a', 'b c'],
             ['bar', 'd e', 'f'],
         ])
+
+    def test_fake_script_clear(self):
+        fake_script(self, 'foo', 'echo foo runs')
+
+        output = subprocess.getoutput('foo a "b c"')
+        self.assertEqual(output, 'foo runs')
+
+        self.assertEqual(fake_script_calls(self, clear=True), [['foo', 'a', 'b c']])
+
+        fake_script(self, 'bar', 'echo bar runs')
+
+        output = subprocess.getoutput('bar "d e" f')
+        self.assertEqual(output, 'bar runs')
+
+        self.assertEqual(fake_script_calls(self, clear=True), [['bar', 'd e', 'f']])
+
+        self.assertEqual(fake_script_calls(self, clear=True), [])