Repository: canonical/operator
Pull request #477 (2021-02-11 04:24:47)

Add custom pebble.Error subtypes for better error handling
Also do some `isinstance` checks so you know sooner (rather than the Pebble server having to tell you).

This came from testing Harry did, where he was passing a list of service names like `['foo']` to `container.start()`, instead of varargs (partly because the low-level client takes a list not varargs). So this makes two improvements:

1) If an error like this did happen again, the traceback and error string would include the helpful error message from the server. While we're at it, we make all client errors subclasses of a new `pebble.Error`. Strictly speaking this is a breaking change, but we haven't released anything Pebble-related yet, so that's fine.
2) Make the `pebble.Client` `start_services` and `stop_services` methods do isinstance checks to check the types upfront. This just lets the user (e.g., Charm writer) know sooner rather than later about this type error, before the client even tries to send the request. This fits how we're type checking argument types in a bunch of other places in the framework.

jameinel: I agree that message is good. But it is ServiceError vs APIError. How would
we trigger APIError?

John
=:->

On Thu, Feb 11, 2021, 21:38 Ben Hoyt <notifications@github.com> wrote:

> *@benhoyt* commented on this pull request.
> ------------------------------
>
> In ops/pebble.py
> <https://github.com/canonical/operator/pull/477#discussion_r574958685>:
>
> > +
> +
> +class SocketError(Error):
> +    """Raised when the Pebble client can't connect to the socket."""
> +
> +
> +class APIError(Error):
> +    """Raised when an HTTP API error occurs talking to the Pebble server."""
> +
> +    def __init__(self, body, code, status, message):
> +        """This shouldn't be instantiated directly."""
> +        super().__init__(message)
> +        self.body = body
> +        self.code = code
> +        self.status = status
> +        self.message = message
>
> For reference, here is an example traceback:
>
> Traceback (most recent call last):
>   File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
>     return _run_code(code, main_globals, None,
>   ...
>   File "/home/ben/w/operator/ops/pebble.py", line 603, in _services_action
>     self.wait_change(change_id, timeout=timeout, delay=delay)
>   File "/home/ben/w/operator/ops/pebble.py", line 616, in wait_change
>     raise ServiceError(change.err, change)
> ops.pebble.ServiceError: cannot perform the following tasks:
> - Start service "test" (service "test" was previously started)
>
> Which I think is quite nice and understandable by the user.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/477#discussion_r574958685>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7MD64TUHYRJ5NTDVF3S6SIBBANCNFSM4XOFBZOQ>
> .
>

benhoyt: > I agree that message is good. But it is ServiceError vs APIError. How would we trigger APIError?

@jameinel It happens (for example) if you provide an invalid service name to start/stop. For example:

```python
>>> try:
...     client.start_services(['notexist'])
... except pebble.APIError as e:
...     print(e)
service "notexist" does not exist
```

That message comes from the returned JSON, the `body['result']['message']` field, and that's what's used for the `__str__`.

Patch:
diff --git a/ops/model.py b/ops/model.py
index e295e9c..4ad7483 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1029,11 +1029,11 @@ class Container:
         """Autostart all default services."""
         self._pebble.autostart_services()
 
-    def start(self, *service_names: typing.List[str]):
+    def start(self, *service_names: str):
         """Start given service(s) by name."""
         self._pebble.start_services(service_names)
 
-    def stop(self, *service_names: typing.List[str]):
+    def stop(self, *service_names: str):
         """Stop given service(s) by name."""
         self._pebble.stop_services(service_names)
 
diff --git a/ops/pebble.py b/ops/pebble.py
index 14f6f0a..1fddfe8 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -17,7 +17,7 @@
 For a command-line interface for local testing, see test/pebble_cli.py.
 """
 
-from typing import Dict, List, Optional, Union
+from typing import Dict, Iterable, List, Optional, Union
 import datetime
 import enum
 import http.client
@@ -25,6 +25,7 @@ import json
 import re
 import socket
 import time
+import urllib.error
 import urllib.parse
 import urllib.request
 import sys
@@ -106,14 +107,64 @@ def _parse_timestamp(s):
                              microsecond=microsecond, tzinfo=tz)
 
 
-class ServiceError(Exception):
-    """Raised by wait_change() when a service change is ready but has an error."""
+def _json_loads(s: Union[str, bytes]) -> Dict:
+    """Like json.loads(), but handle str or bytes.
 
-    def __init__(self, err, change):
-        super().__init__(err)
+    This is needed because an HTTP response's read() method returns bytes on
+    Python 3.5, and json.load doesn't handle bytes.
+    """
+    if isinstance(s, bytes):
+        s = s.decode('utf-8')
+    return json.loads(s)
+
+
+class Error(Exception):
+    """Base class of most errors raised by the Pebble client."""
+
+
+class TimeoutError(TimeoutError, Error):
+    """Raised when a polling timeout occurs."""
+
+
+class ConnectionError(Error):
+    """Raised when the Pebble client can't connect to the socket."""
+
+
+class APIError(Error):
+    """Raised when an HTTP API error occurs talking to the Pebble server."""
+
+    def __init__(self, body: Dict, code: int, status: str, message: str):
+        """This shouldn't be instantiated directly."""
+        super().__init__(message)  # Makes str(e) return message
+        self.body = body
+        self.code = code
+        self.status = status
+        self.message = message
+
+    def __repr__(self):
+        return 'APIError({!r}, {!r}, {!r}, {!r})'.format(
+            self.body, self.code, self.status, self.message)
+
+
+class ChangeError(Error):
+    """Raised by actions when a change is ready but has an error.
+
+    For example, this happens when you attempt to start an already-started
+    service:
+
+    cannot perform the following tasks:
+    - Start service "test" (service "test" was previously started)
+    """
+
+    def __init__(self, err: str, change: 'Change'):
+        """This shouldn't be instantiated directly."""
+        super().__init__(err)  # Makes str(e) return err
         self.err = err
         self.change = change
 
+    def __repr__(self):
+        return 'ChangeError({!r}, {!r})'.format(self.err, self.change)
+
 
 class WarningState(enum.Enum):
     """Enum of states for get_warnings() select parameter."""
@@ -451,12 +502,25 @@ class Client:
             headers['Content-Type'] = 'application/json'
 
         request = urllib.request.Request(url, method=method, data=data, headers=headers)
-        response = self.opener.open(request, timeout=self.timeout)
+
+        try:
+            response = self.opener.open(request, timeout=self.timeout)
+        except urllib.error.HTTPError as e:
+            code = e.code
+            status = e.reason
+            try:
+                body = _json_loads(e.read())
+                message = body['result']['message']
+            except (IOError, ValueError, KeyError) as e2:
+                # Will only happen on read error or if Pebble sends invalid JSON.
+                body = {}
+                message = '{} - {}'.format(type(e2).__name__, e2)
+            raise APIError(body, code, status, message)
+        except urllib.error.URLError as e:
+            raise ConnectionError(e.reason)
+
         response_data = response.read()
-        if isinstance(response_data, bytes):
-            # read() returns bytes on Python 3.5, and json.load doesn't handle bytes
-            response_data = response_data.decode('utf-8')
-        result = json.loads(response_data)
+        result = _json_loads(response_data)
         return result
 
     def get_system_info(self) -> SystemInfo:
@@ -500,7 +564,9 @@ class Client:
     def autostart_services(self, timeout: float = 30.0, delay: float = 0.1) -> ChangeID:
         """Start the autostart services and wait (poll) for them to be started.
 
-        If timeout is 0, submit the action but don't wait.
+        Raises ChangeError if one or more of the services didn't start. If
+        timeout is 0, submit the action but don't wait; just return the change
+        ID immediately.
         """
         return self._services_action('autostart', [], timeout, delay)
 
@@ -509,7 +575,9 @@ class Client:
     ) -> ChangeID:
         """Start services by name and wait (poll) for them to be started.
 
-        If timeout is 0, submit the action but don't wait.
+        Raises ChangeError if one or more of the services didn't start. If
+        timeout is 0, submit the action but don't wait; just return the change
+        ID immediately.
         """
         return self._services_action('start', services, timeout, delay)
 
@@ -518,18 +586,29 @@ class Client:
     ) -> ChangeID:
         """Stop services by name and wait (poll) for them to be started.
 
-        If timeout is 0, submit the action but don't wait.
+        Raises ChangeError if one or more of the services didn't stop. If
+        timeout is 0, submit the action but don't wait; just return the change
+        ID immediately.
         """
         return self._services_action('stop', services, timeout, delay)
 
     def _services_action(
-        self, action: str, services: List[str], timeout: float, delay: float,
+        self, action: str, services: Iterable[str], timeout: float, delay: float,
     ) -> ChangeID:
+        if not isinstance(services, (list, tuple)):
+            raise TypeError('services must be a list of str, not {}'.format(
+                type(services).__name__))
+        for s in services:
+            if not isinstance(s, str):
+                raise TypeError('service names must be str, not {}'.format(type(s).__name__))
+
         body = {'action': action, 'services': services}
         result = self._request('POST', '/v1/services', body=body)
         change_id = ChangeID(result['change'])
         if timeout:
-            self.wait_change(change_id, timeout=timeout, delay=delay)
+            change = self.wait_change(change_id, timeout=timeout, delay=delay)
+            if change.err:
+                raise ChangeError(change.err, change)
         return change_id
 
     def wait_change(
@@ -541,8 +620,6 @@ class Client:
         while time.time() < deadline:
             change = self.get_change(change_id)
             if change.ready:
-                if change.err:
-                    raise ServiceError(change.err, change)
                 return change
 
             time.sleep(delay)


Test patch:
diff --git a/test/pebble_cli.py b/test/pebble_cli.py
index 77a3e38..cbe8721 100644
--- a/test/pebble_cli.py
+++ b/test/pebble_cli.py
@@ -23,10 +23,8 @@ PEBBLE=pebble_dir python -m test.pebble_cli system-info
 
 import argparse
 import datetime
-import json
 import os
 import sys
-import urllib.error
 
 from ops import pebble
 
@@ -104,17 +102,15 @@ def main():
             result = client.get_warnings(select=pebble.WarningState(args.select))
         else:
             raise AssertionError("shouldn't happen")
-    except urllib.error.HTTPError as e:
-        print(e, file=sys.stderr)
-        obj = json.load(e)
-        print(json.dumps(obj, sort_keys=True, indent=4), file=sys.stderr)
+    except pebble.APIError as e:
+        print('{} {}: {}'.format(e.code, e.status, e.message), file=sys.stderr)
         sys.exit(1)
-    except urllib.error.URLError as e:
-        print('cannot connect to Pebble socket {!r}: {}'.format(socket_path, e.reason),
+    except pebble.ConnectionError as e:
+        print('cannot connect to socket {!r}: {}'.format(socket_path, e),
               file=sys.stderr)
         sys.exit(1)
-    except pebble.ServiceError as e:
-        print('ServiceError:', e.err, file=sys.stderr)
+    except pebble.ChangeError as e:
+        print('ChangeError:', e, file=sys.stderr)
         sys.exit(1)
 
     if isinstance(result, list):
diff --git a/test/test_model.py b/test/test_model.py
index 489554c..96c8843 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -816,6 +816,24 @@ containers:
             ('stop', ('foo', 'bar')),
         ])
 
+    def test_type_errors(self):
+        meta = ops.charm.CharmMeta.from_yaml("""
+name: k8s-charm
+containers:
+  c1:
+    k: v
+""")
+        # Only the real pebble Client checks types, so use actual backend class
+        backend = ops.model._ModelBackend('myapp/0')
+        model = ops.model.Model(meta, backend)
+        container = model.unit.containers['c1']
+
+        with self.assertRaises(TypeError):
+            container.start(['foo'])
+
+        with self.assertRaises(TypeError):
+            container.stop(['foo'])
+
     def test_add_layer(self):
         self.container.add_layer('summary: str\n')
         self.container.add_layer({'summary': 'dict'})
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 3060050..d1255b1 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -17,7 +17,6 @@ import datetime
 import unittest
 import unittest.mock
 import unittest.util
-import urllib.error
 import sys
 
 import ops.pebble as pebble
@@ -85,7 +84,39 @@ class TestHelpers(unittest.TestCase):
 class TestTypes(unittest.TestCase):
     maxDiff = None
 
-    def test_service_error(self):
+    def test_error(self):
+        error = pebble.Error('error')
+        self.assertIsInstance(error, Exception)
+
+    def test_timeout_error(self):
+        error = pebble.TimeoutError('timeout!')
+        self.assertIsInstance(error, pebble.Error)
+        self.assertIsInstance(error, TimeoutError)
+        self.assertEqual(str(error), 'timeout!')
+
+    def test_connection_error(self):
+        error = pebble.ConnectionError('connerr!')
+        self.assertIsInstance(error, pebble.Error)
+        self.assertEqual(str(error), 'connerr!')
+
+    def test_api_error(self):
+        body = {
+            "result": {
+                "message": "no services to start provided"
+            },
+            "status": "Bad Request",
+            "status-code": 400,
+            "type": "error"
+        }
+        error = pebble.APIError(body, 400, "Bad Request", "no services")
+        self.assertIsInstance(error, pebble.Error)
+        self.assertEqual(error.body, body)
+        self.assertEqual(error.code, 400)
+        self.assertEqual(error.status, 'Bad Request')
+        self.assertEqual(error.message, 'no services')
+        self.assertEqual(str(error), 'no services')
+
+    def test_change_error(self):
         change = pebble.Change(
             id=pebble.ChangeID('1234'),
             kind='start',
@@ -97,7 +128,8 @@ class TestTypes(unittest.TestCase):
             spawn_time=datetime.datetime.now(),
             ready_time=datetime.datetime.now(),
         )
-        error = pebble.ServiceError('Some error', change)
+        error = pebble.ChangeError('Some error', change)
+        self.assertIsInstance(error, pebble.Error)
         self.assertEqual(error.err, 'Some error')
         self.assertEqual(error.change, change)
         self.assertEqual(str(error), 'Some error')
@@ -700,6 +732,15 @@ class TestClient(unittest.TestCase):
             return self.client.start_services(['svc'])
         self._services_action_helper('start', api_func, ['svc'])
 
+        with self.assertRaises(TypeError):
+            self.client.start_services(1)
+
+        with self.assertRaises(TypeError):
+            self.client.start_services([1])
+
+        with self.assertRaises(TypeError):
+            self.client.start_services([['foo']])
+
     def test_start_services_async(self):
         def api_func(timeout=30):
             return self.client.start_services(['svc'], timeout=timeout)
@@ -710,12 +751,21 @@ class TestClient(unittest.TestCase):
             return self.client.stop_services(['svc'])
         self._services_action_helper('stop', api_func, ['svc'])
 
+        with self.assertRaises(TypeError):
+            self.client.stop_services(1)
+
+        with self.assertRaises(TypeError):
+            self.client.stop_services([1])
+
+        with self.assertRaises(TypeError):
+            self.client.stop_services([['foo']])
+
     def test_stop_services_async(self):
         def api_func(timeout=30):
             return self.client.stop_services(['svc'], timeout=timeout)
         self._services_action_async_helper('stop', api_func, ['svc'])
 
-    def test_service_error(self):
+    def test_change_error(self):
         self.client.responses.append({
             "change": "70",
             "result": None,
@@ -731,8 +781,9 @@ class TestClient(unittest.TestCase):
             "status-code": 200,
             "type": "sync"
         })
-        with self.assertRaises(pebble.ServiceError) as cm:
+        with self.assertRaises(pebble.ChangeError) as cm:
             self.client.autostart_services()
+        self.assertIsInstance(cm.exception, pebble.Error)
         self.assertEqual(cm.exception.err, 'Some kind of service error')
         self.assertIsInstance(cm.exception.change, pebble.Change)
         self.assertEqual(cm.exception.change.id, '70')
@@ -749,8 +800,10 @@ class TestClient(unittest.TestCase):
                     "type": "sync"
                 })
 
-            with self.assertRaises(TimeoutError):
+            with self.assertRaises(pebble.TimeoutError) as cm:
                 self.client.wait_change('70', timeout=3, delay=1)
+            self.assertIsInstance(cm.exception, pebble.Error)
+            self.assertIsInstance(cm.exception, TimeoutError)
 
             self.assertEqual(self.client.requests, [
                 ('GET', '/v1/changes/70', None, None),
@@ -758,13 +811,28 @@ class TestClient(unittest.TestCase):
                 ('GET', '/v1/changes/70', None, None),
             ])
 
+    def test_wait_change_error(self):
+        change = self.build_mock_change_dict()
+        change['err'] = 'Some kind of service error'
+        self.client.responses.append({
+            "result": change,
+            "status": "OK",
+            "status-code": 200,
+            "type": "sync"
+        })
+        # wait_change() itself shouldn't raise an error
+        response = self.client.wait_change('70')
+        self.assertEqual(response.id, '70')
+        self.assertEqual(response.err, 'Some kind of service error')
+
 
 class TestSocketClient(unittest.TestCase):
     @unittest.skipIf(sys.platform == 'win32', "Unix sockets don't work on Windows")
     def test_socket_not_found(self):
         client = pebble.Client(socket_path='does_not_exist')
-        with self.assertRaises(urllib.error.URLError):
+        with self.assertRaises(pebble.ConnectionError) as cm:
             client.get_system_info()
+        self.assertIsInstance(cm.exception, pebble.Error)
 
     @unittest.skipIf(sys.platform == 'win32', "Unix sockets don't work on Windows")
     def test_real_client(self):
@@ -778,9 +846,12 @@ class TestSocketClient(unittest.TestCase):
             change_id = client.start_services(['foo'], timeout=0)
             self.assertEqual(change_id, '1234')
 
-            with self.assertRaises(urllib.error.HTTPError) as cm:
+            with self.assertRaises(pebble.APIError) as cm:
                 client.start_services(['bar'], timeout=0)
+            self.assertIsInstance(cm.exception, pebble.Error)
             self.assertEqual(cm.exception.code, 400)
+            self.assertEqual(cm.exception.status, 'Bad Request')
+            self.assertEqual(cm.exception.message, 'service "bar" does not exist')
 
         finally:
             shutdown()