Repository: canonical/operator
Pull request #605 (2021-09-09 11:08:47)

Proposed update to container.is_ready implementation
Proposed implementation for `is_ready()` as a method for checking that a given Pebble instance is 'up' and its API is reachable. The primary difference here is removal of the context manager in favour of a simpler approach. This implementation also renames `is_ready` -> `can_connect`.

The primary logic here is that checking whether the Pebble API is available should only be a point-in-time check. This change was initially implemented to catch the case where some hook (usually `config-changed`) fires before the Pebble API was up, and prevent *all the charms* from needing a big long try/except in the same place.

With this implementation, we only provide that point-in-time check, that can be used before some operation in an event handler. If the Pebble API becomes unavailable *later in the same hook*, then something *very bad* has happened, and an error bubbling up is probably the right thing.

jnsgruk: > LGTM. Sneaking `venv` into `.gitignore` is a nice addition.
> 
> I'll walk back some of the changes in Grafana to trace the `FileNotFoundError`. It could either be in `Container.__init__()` instantiating `pebble.Client` (though considering these go into a `ContainerMapping`, it's unlikely), or more likely it's `ops.pebble.Client._request_raw()` when we make a call, which would also make it appropriate to call in `can_connect()`, but I'll find out and comment back in a bit

You're right, the last commit catches the case when the socket has not yet been created by Pebble.

Patch:
diff --git a/.gitignore b/.gitignore
index 0df66f7..74243df 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,4 +7,5 @@ __pycache__
 /docs/_build
 *~
 .venv
+venv
 .vscode
diff --git a/ops/model.py b/ops/model.py
index 03a808b..3735a0e 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -26,28 +26,18 @@ import tempfile
 import time
 import typing
 import weakref
-
 from abc import ABC, abstractmethod
 from collections.abc import Mapping, MutableMapping
 from pathlib import Path
-from subprocess import run, PIPE, CalledProcessError
+from subprocess import PIPE, CalledProcessError, run
 
-from ops._private import yaml
-from ops.jujuversion import JujuVersion
 import ops
 import ops.pebble as pebble
-
+from ops._private import yaml
+from ops.jujuversion import JujuVersion
 
 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.
@@ -262,6 +252,24 @@ class Application:
         self._backend.status_set(value.name, value.message, is_app=True)
         self._status = value
 
+    def planned_units(self) -> int:
+        """Get the number of units that Juju has "planned" for this application.
+
+        E.g., if an operator ran "juju deploy foo", then "juju add-unit -n 2 foo", the
+        planned unit count for foo would be 3.
+
+        We deliberately do not attempt to inspect whether these units are actually running
+        or not at present.
+
+        This only works for this charm's application -- the unit agent isn't able to get
+        planned units for other applications in the model.
+        """
+        if not self._is_our_app:
+            raise RuntimeError(
+                'cannot get planned units for a remote application {}.'.format(self))
+
+        return self._backend.planned_units()
+
     def __repr__(self):
         return '<{}.{} {}>'.format(type(self).__module__, type(self).__name__, self.name)
 
@@ -1044,7 +1052,6 @@ class Container:
     Attributes:
         name: The name of the container from metadata.yaml (eg, 'postgres').
     """
-
     def __init__(self, name, backend, pebble_client=None):
         self.name = name
 
@@ -1052,54 +1059,52 @@ 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 can_connect(self) -> bool:
+        """Report whether the Pebble API is reachable in the container.
 
-    def is_ready(self) -> '_ContainerReady':
-        """Check whether or not Pebble is ready as a simple property.
+        :meth:`can_connect` returns a bool that indicates whether the Pebble API is available at
+        the time the method is called. It does not guard against the Pebble API becoming
+        unavailable, and should be treated as a 'point in time' status only.
 
-        :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.
+        If the Pebble API later fails, serious consideration should be given as to the reason for
+        this.
 
         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()
+            if container.can_connect():
+                try:
+                    c.pull('/does/not/exist')
+                except ProtocolError, PathError:
+                    # handle it
             else:
-                do_something_else()
+                event.defer()
         """
-        return _ContainerReady(self)
+        try:
+            # TODO: This call to `get_system_info` should be replaced with a call to a more
+            # appropriate endpoint that has stronger connotations of what constitutes a Pebble
+            # instance that is in fact 'ready'.
+            self._pebble.get_system_info()
+        except pebble.ConnectionError as e:
+            logger.debug("Pebble API is not ready; ConnectionError: %s", e.message())
+            return False
+        except FileNotFoundError as e:
+            # In some cases, charm authors can attempt to hit the Pebble API before it has had the
+            # chance to create the UNIX socket in the shared volume.
+            logger.debug("Pebble API is not ready; UNIX socket not found:", str(e))
+            return False
+        except pebble.APIError as e:
+            # An API error is only raised when the Pebble API returns invalid JSON, or the response
+            # cannot be read. Both of these are a likely indicator that something is wrong.
+            logger.warning("Pebble API is not ready; APIError: %s", str(e))
+            return False
+        return True
 
     def autostart(self):
         """Autostart all services marked as startup: enabled."""
@@ -1260,47 +1265,6 @@ 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.
 
@@ -1352,19 +1316,6 @@ 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."""
 
@@ -1676,6 +1627,19 @@ class _ModelBackend:
         """Create a pebble.Client instance from given socket path."""
         return pebble.Client(socket_path=socket_path)
 
+    def planned_units(self) -> int:
+        """Count of "planned" units that will run this application.
+
+        Includes the current unit in the count.
+
+        """
+        # The goal-state tool will return the information that we need. Goal state as a general
+        # concept is being deprecated, however, in favor of approaches such as the one that we use
+        # here.
+        app_state = self._run('goal-state', return_output=True, use_json=True)
+        # Planned units can be zero. We don't need to do error checking here.
+        return len(app_state.get('units', []))
+
 
 class _ModelBackendValidator:
     """Provides facilities for validating inputs and formatting them for model backends."""


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 3cfbfc9..733fb7c 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -1060,6 +1060,19 @@ class _TestingModelBackend:
             self._pebble_clients[socket_path] = client
         return client
 
+    def planned_units(self):
+        units = []
+        peer_names = set(self._meta.peers.keys())
+        for peer_id, peer_name in self._relation_names.items():
+            if peer_name not in peer_names:
+                continue
+            peer_units = self._relation_list_map[peer_id]
+            units += peer_units
+
+        count = len(set(units))  # de-dupe and get length.
+
+        return count + 1  # Account for this unit.
+
 
 @_copy_docstrings(pebble.Client)
 class _TestingPebbleClient:
diff --git a/test/test_model.py b/test/test_model.py
index b025c1e..6c673d3 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -13,23 +13,21 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from collections import OrderedDict
-import json
 import ipaddress
+import json
 import os
 import pathlib
-from textwrap import dedent
 import unittest
+from collections import OrderedDict
+from test.test_helpers import fake_script, fake_script_calls
+from textwrap import dedent
 
-import ops.model
 import ops.charm
+import ops.model
 import ops.pebble
 import ops.testing
-from ops.charm import RelationMeta, RelationRole
-
 from ops._private import yaml
-
-from test.test_helpers import fake_script, fake_script_calls
+from ops.charm import RelationMeta, RelationRole
 
 
 class TestModel(unittest.TestCase):
@@ -749,6 +747,48 @@ class TestModel(unittest.TestCase):
         self.assertEqual(expected, self.harness._get_backend_calls(reset=reset))
 
 
+class TestApplication(unittest.TestCase):
+
+    def setUp(self):
+        self.harness = ops.testing.Harness(ops.charm.CharmBase, meta='''
+            name: myapp
+            provides:
+              db0:
+                interface: db0
+            requires:
+              db1:
+                interface: db1
+            peers:
+              db2:
+                interface: db2
+            resources:
+              foo: {type: file, filename: foo.txt}
+              bar: {type: file, filename: bar.txt}
+        ''')
+        self.peer_rel_id = self.harness.add_relation('db2', 'db2')
+        self.app = self.harness.model.app
+
+    def test_planned_units(self):
+        rel_id = self.peer_rel_id
+
+        # Test that we always count ourself.
+        self.assertEqual(self.app.planned_units(), 1)
+
+        # Add some units, and verify count.
+        self.harness.add_relation_unit(rel_id, 'myapp/1')
+        self.harness.add_relation_unit(rel_id, 'myapp/2')
+
+        self.assertEqual(self.app.planned_units(), 3)
+
+        self.harness.add_relation_unit(rel_id, 'myapp/3')
+        self.assertEqual(self.app.planned_units(), 4)
+
+        # And remove a unit
+        self.harness.remove_relation_unit(rel_id, 'myapp/2')
+
+        self.assertEqual(self.app.planned_units(), 3)
+
+
 class TestContainers(unittest.TestCase):
     def setUp(self):
         meta = ops.charm.CharmMeta.from_yaml("""
@@ -816,6 +856,10 @@ containers:
         self.container.autostart()
         self.assertEqual(self.pebble.requests, [('autostart',)])
 
+    def test_get_system_info(self):
+        self.container.can_connect()
+        self.assertEqual(self.pebble.requests, [('get_system_info',)])
+
     def test_start(self):
         self.container.start('foo')
         self.container.start('foo', 'bar')
@@ -1031,21 +1075,9 @@ 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):
+    def test_bare_can_connect_call(self):
         self.pebble.responses.append('dummy')
-        self.assertTrue(self.container.is_ready())
+        self.assertTrue(self.container.can_connect())
 
 
 class MockPebbleBackend(ops.model._ModelBackend):
@@ -1062,6 +1094,9 @@ class MockPebbleClient:
     def autostart_services(self):
         self.requests.append(('autostart',))
 
+    def get_system_info(self):
+        self.requests.append(('get_system_info',))
+
     def start_services(self, service_names):
         self.requests.append(('start', service_names))