Repository: canonical/operator

Issue:
Confusing / inconsistent errors from interaction between Pebble and Juju events and handlers
I've frequently seen charm authors who are new to the sidecar pattern write something along the lines of this in their charm:

```python
class MyCharm(CharmBase):
    def __init__(self, *args):
        super().__init__(*args)
        self.framework.observe(self.on.mycharm_pebble_ready, self._create_layer)
        self.framework.observe(self.on.config_changed, self._restart_service)

    def _create_layer(self, event):
        container = self.unit.get_container("mycharm")
        container.add_layer("mycharm", {"services": {"mycharm": {...}}})
        container.autostart()

    def _restart_service(self, event):
        container = self.unit.get_container("mycharm")
        # presumably introspect and / or update the service, then...
        container.stop("mycharm")
        container.start("mycharm")
```

This seems like a straightforward and reasonable way for someone to start out approaching this, but can result in any one of 5 different outcomes:

  * It might work fine.  Depending on timing and how the `_restart_service` handler does or doesn't implement updating the service based on config changes, this might deploy and run fine, at least most of the time.

  * It might raise `ops.pebble.ConnectionError`.  Depending on how quickly Pebble becomes ready to accept connections, it might fail trying to talk to Pebble at all. Worse, this could be an intermittent failure. Additionally, this won't ever happen in unit tests because the `_TestingPebbleClient` is always ready immediately.

  * It might raise `RuntimeError: 400 Bad Request: service "mycharm" does not exist`. This is somewhat related to #514 but is slightly different and might be specific to the `_TestingPebbleClient`.

  * It might raise `ops.pebble.APIError: 400 Bad Request: service "mycharm" does not exist`. A charm that raised the previous `RuntimeError` during unit tests would most likely raise this during an actual deployment.

  * It might raise `ops.model.ModelError: service 'mycharm' not found`. If the `_restart_service` handler does an explicit `container.get_service("mycharm")`, then it will get this rather than either of the previous two errors, unless it calls `container.add_layer(layer_name, layer_definition, combine=True)` first.

It would at least be good to ensure that all of the latter 3 cases result in a single `ops.pebble.UnknownServiceError` or something, but I've found that new charmers will still be confused as to why the service isn't recognized despite them having defined it during the `pebble-ready` event. Maybe the additional message on the `UnknownServiceError` can include a hint such as `(typically due to referencing a service before add_layer is called)`.

It would also be good to make the testing harness always raise `ConnectionError` until the `pebble-ready` event is triggered, to force charm authors to consider that possibility.

A `container.restart(service)` helper would be nice, as well.

Discussion:
jnsgruk: @jameinel interested in your take on this. Per my review above, I think we should provide the property to developers, but probably not use it as this PR initially suggests within internal methods.
benhoyt: > Since there are almost no operations we can actually perform if Pebble returns a ConnectionError, check for it first and log it if that's the case to give a hint to Charm authors in case they forget to check.

Further to my comment above, we can probably give them that hint by tweaking `ConnectionError` `__str__` or `__repr__`.
rbarry82: I'm happy to pass through the actual message from the log. I spoke with @jnsgruk a little while ago and his mental model was to memoize it. Essentially, that:

`unit.get_container("foo").get_service(...)` or similar would throw an exception up the stack, but:

```
if unit.get_container("foo").is_ready():
   # handle logic in here
```

Will log only, so we don't surprise charm authors by logging only, or by simply making them handle a different kind of exception. 

Reducing the API calls is easy enough with a memoizing cache which has a timeout, or simply by using `__enter__` and asking users to:

```
with unit.container("foo") as c:
    c.start()
```

I'll grant that it _is_ losing information (which we can happily bubble up with `logger.debug`), but the goal here is to have a better workflow. Otherwise, like #553 points out, there are a plethora of possible exceptions which can happen in otherwise innocuous places. Including, sometimes, during cases where the author isn't expecting them. Unless a charm is carefully written to guard against it, normal handling of the events fired when an application is removed can yield the following in the Juju logs:

```
  self.sock.connect(self.socket.path)
FileNotFoundError

During handling of the above, another exception....

  raise URLError(err)
urllib.error.URLError

During handling...

  raise ConnectionError(e.reason)
ops.pebble.ConnectionError  
```

And, frankly, this is nonsense. How many different exception handlers do we think charm authors should have? Sure, we lose information by guarding, but it's also a better developer experience. From my POV, someone hitting an API endpoint of a microservices application does not want or need to care about the raw error if API call could not resolve because MongoDB was down. They want meaningful information from the API about what happened.

In this case, OF is the API users see, and Pebble is one of the APIs OF authors see. 
benhoyt: Well, I'm okay with the solution, as long as we're not throwing away the original error message. But why can't we just clean up the error message and add some explanatory help text to `ConnectionError.__str__` (and maybe log it)? That way the user will see something like "error connecting to socket /foo/bar (likely cause: Pebble not ready)". Similar for API errors.
rbarry82: Some updates:

There are use case examples now. To satisfy @mthaddon's very good comment that it's hard for authors to determine what happened, this has been moved to a separate `_ContainerReady` class.

`is_ready` now returns an instance of that class, which has a `__bool__` property so a bare `container.is_ready()` will return a truthy value based on the result.

In addition, it can still be used as a contextmanager, with a `completed` property on `Container` which is set based on the success or failure status of a contextmanager invocation (if applicable).

`container.is_ready()` can be used as a normal boolean in an `if/elif` statement. 

```
with container.is_ready() as c:
    c.pull('/does/not/exist')
c.completed # False
```

Is also usable. Testcases and docstrings with examples are updated to match

Patch:
diff --git a/ops/model.py b/ops/model.py
index 88c0e29..0ee56ef 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -18,6 +18,7 @@ import datetime
 import decimal
 import ipaddress
 import json
+import logging
 import os
 import re
 import shutil
@@ -37,6 +38,17 @@ import ops
 import ops.pebble as pebble
 
 
+logger = logging.getLogger(__name__)
+
+ErrorsWithMessage = (
+    pebble.APIError,
+    pebble.ConnectionError,
+    pebble.PathError,
+    pebble.ProtocolError,
+    pebble.TimeoutError,
+)
+
+
 class Model:
     """Represents the Juju Model as seen from this unit.
 
@@ -1040,12 +1052,56 @@ class Container:
             socket_path = '/charm/containers/{}/pebble.socket'.format(name)
             pebble_client = backend.get_pebble(socket_path)
         self._pebble = pebble_client
+        self._completed = None
 
     @property
     def pebble(self) -> 'pebble.Client':
         """The low-level :class:`ops.pebble.Client` instance for this container."""
         return self._pebble
 
+    @property
+    def completed(self) -> bool:
+        """Whether or not a :meth:`is_ready` context finished successfully."""
+        return self._completed
+
+    def is_ready(self) -> '_ContainerReady':
+        """Check whether or not Pebble is ready as a simple property.
+
+        :meth:`is_ready` returns a :class:_ContainerReady `contextmanager` which
+         can be used in charms to wrap :class:`Container` operations which depend
+         on the Pebble backend being available. When `is_ready` is used, exceptions
+         from the underlying Pebble operations will log error messages rather than
+         raising exceptions.
+
+        Example:
+            ```
+            container = self.unit.get_container("example")
+            with container.is_ready() as c:
+                c.pull('/does/not/exist')
+
+                # This point of execution will not be reached if an exception
+                # was caught earlier
+                c.get_service("foo")
+            c.completed # False
+            ```
+
+            This will result in an `ERROR` log from PathError, but not a
+            traceback. In addition, the block running inside the contextmanager
+            will exit and return to the previous point of execution. Whether
+            or not the block completed successfully is available as a property
+
+        :meth:`is_ready` can also be used as a bare function, which will log an
+        error if the container is not ready.
+
+        Example:
+            ```
+            if container.is_ready():
+                do_something()
+            else:
+                do_something_else()
+        """
+        return _ContainerReady(self)
+
     def autostart(self):
         """Autostart all services marked as startup: enabled."""
         self._pebble.autostart_services()
@@ -1054,12 +1110,22 @@ class Container:
         """Start given service(s) by name."""
         if not service_names:
             raise TypeError('start expected at least 1 argument, got 0')
+
+        self._pebble.start_services(service_names)
+
+    def restart(self, *service_names: str):
+        """Restart the given service(s) by name."""
+        if not service_names:
+            raise TypeError('restart expected at least 1 argument, got 0')
+
+        self._pebble.stop_services(service_names)
         self._pebble.start_services(service_names)
 
     def stop(self, *service_names: str):
         """Stop given service(s) by name."""
         if not service_names:
             raise TypeError('stop expected at least 1 argument, got 0')
+
         self._pebble.stop_services(service_names)
 
     # TODO(benhoyt) - should be: layer: typing.Union[str, typing.Dict, 'pebble.Layer'],
@@ -1193,6 +1259,47 @@ class Container:
         self._pebble.remove_path(path, recursive=recursive)
 
 
+class _ContainerReady:
+    """Represents whether or not a container is ready as a Context Manager.
+
+    This class should not be instantiated directly, instead use :meth:`Container.is_ready`
+
+    Attributes:
+        container: A :class:`Container` object
+    """
+
+    def __init__(self, container: Container):
+        self.container = container
+
+    def __bool__(self) -> bool:
+        try:
+            # We don't care at all whether not the services are up in
+            # this case, just whether Pebble throws an error. If it doesn't,
+            # carry on with the contextmanager.
+            self.container._pebble.get_services()
+        except ErrorsWithMessage as e:
+            logger.error("Pebble is not ready! (%s) was raised due to: %s",
+                         e.name, e.message)
+            return False
+        return True
+
+    def __enter__(self) -> 'Container':
+        self.container._completed = True
+        return self.container
+
+    def __exit__(self, exc_type, e, exc_tb):
+        if exc_type in ErrorsWithMessage:
+            logger.error("(%s) was raised due to: %s", e.name, e.message)
+            self.container._completed = False
+            return True
+
+        if exc_type is pebble.ChangeError:
+            logger.error("Pebble could not apply the requested change (%s) "
+                         "due to %s", e.change, e.err)
+            self.container._completed = False
+            return True
+
+
 class ContainerMapping(Mapping):
     """Map of container names to Container objects.
 
@@ -1244,6 +1351,19 @@ class ModelError(Exception):
     pass
 
 
+class UnknownServiceError(Exception):
+    """Raised by :class:`Container` objects when Pebble cannot find a service.
+
+    This is done so authors can have a single catch-all exception if the service
+    cannot be found, typically due to asking for the service before
+    :meth:`Container.add_layer` has been called.
+    """
+
+
+class PebbleNotReadyError(Exception):
+    """Raised by :class:`Container` methods if the underlying Pebble socket returns an error."""
+
+
 class TooManyRelatedAppsError(ModelError):
     """Raised by :meth:`Model.get_relation` if there is more than one related application."""
 
diff --git a/ops/pebble.py b/ops/pebble.py
index c65e8c9..4155d61 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -126,6 +126,17 @@ def _json_loads(s: typing.Union[str, bytes]) -> typing.Dict:
 class Error(Exception):
     """Base class of most errors raised by the Pebble client."""
 
+    def __repr__(self):
+        return '<{}.{} {}>'.format(type(self).__module__, type(self).__name__, self.args)
+
+    def name(self):
+        """Return a string representation of the model plus class."""
+        return '<{}.{}>'.format(type(self).__module__, type(self).__name__)
+
+    def message(self):
+        """Return the message passed as an argument."""
+        return self.args[0]
+
 
 class TimeoutError(TimeoutError, Error):
     """Raised when a polling timeout occurs."""


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 030393a..3f80664 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -1013,6 +1013,22 @@ containers:
             ('remove_path', '/path/2', True),
         ])
 
+    def test_no_exception_with_contextmanager(self):
+        with self.assertLogs() as logs:
+            self.pebble.responses.append('dummy')
+            with self.container.is_ready() as c:
+                raise ops.pebble.ConnectionError("Some dummy message")
+        self.assertIn("was raised due to", logs.records[0].getMessage())
+        self.assertEqual(c.completed, False)
+
+    def test_exception_without_contextmanager(self):
+        with self.assertRaises(ops.pebble.ConnectionError):
+            raise ops.pebble.ConnectionError("Some dummy message")
+
+    def test_bare_is_ready_call(self):
+        self.pebble.responses.append('dummy')
+        self.assertTrue(self.container.is_ready())
+
 
 class MockPebbleBackend(ops.model._ModelBackend):
     def get_pebble(self, socket_path):