Merge branch 'main' into grab-workload-version-in-k8s-tutorial

Author: tromai

docs/.custom_wordlist.txt

@@ -4,6 +4,7 @@ balancer's
 breakpoint
 callables
 Canonical's
+configurator
 cosl
 databag
 databags
@@ -37,6 +38,7 @@ stdout
 storages
 systemd
 subprocess
+teardown
 tinyproxy
 traceback
 tracebacks

docs/howto/initialise-your-project.md

@@ -24,20 +24,41 @@ To check that your preferred name isn't already in use, visit the (future) Charm
 ```text
 https://charmhub.io/mega-calendar-k8s
 ```
-(create-a-repository-and-initialise-it)=
-## Create a repository and initialise it
+
+Some charms do not operate a workload, such as integrator and configurator charms. These categories serve different purposes:
+
+* An integrator charm allows integration with a service that is not managed through Juju. This can both apply to server side integrations (such as `s3-integrator`, which integrates an externally managed S3 object storage) or to client side integrations (such as `data-integrator`, representing the integration of external client application that needs a database).
+
+* A configurator charm provides logic to configure a particular charm or relation that is already in Juju. Examples include `cos-configuration` when it applies to a single charm (such as providing more fine-grained configuration of the Prometheus scraping) or for a relation (such as `ingress-configurator` to provide additional configuration of ingress requests).
+
+Since workload-less charms can work both on machines and on Kubernetes, avoid using the `k8s` suffix when naming integrator charms and configurator charms, unless the charm is only relevant for Kubernetes (for example, managing K8s resources within the charm logic using `lightkube`).
+
+When naming the charm, use `-integrator` and `-configurator` to signal the category of the charm. For example, `foo-integrator` or `bar-configurator`.
+
+(create-a-repository)=
+## Create a repository
 
 Create a repository with your source control of choice.
 
 ```{admonition} Best practice
 :class: hint
 
-Name the repository using the pattern ``<charm name>-operator`` for a single
-charm, or ``<base charm name>-operators`` when the repository will hold
-multiple related charms. For the charm name, see [](#decide-your-charms-name).
+If your charm operates a workload, name the repository `<charm name>-operator`. For advice about the charm name, see [](#decide-your-charms-name). If your charm doesn't operate a workload (as in the case of integrator charms and configurator charms), the `-operator` suffix isn't needed. For example, `foo-integrator` and `bar-configurator`.
+
+Repositories that contain multiple charms or one or more charms and other artefacts (like rocks) will need to use other naming patterns.
 ```
 
-For example, name the repository `mega-calendar-k8s-operator` if your charm will be called `mega-calendar-k8s`.
+Examples:
+
+- [kafka-operator](https://github.com/canonical/kafka-operator) - Contains a single charm that operates a machine workload.
+- [kafka-k8s-operator](https://github.com/canonical/kafka-k8s-operator) - Contains a single charm that operates a K8s workload.
+- [katib-operators](https://github.com/canonical/katib-operators) - Contains multiple charms.
+- [data-integrator](https://github.com/canonical/data-integrator) - Contains a charm that integrates an externally managed service (such as a client application).
+- [s3-integrator](https://github.com/canonical/object-storage-integrators) - Contains multiple charms that integrate an externally managed service (such as different kinds of object storage backends).
+- [request-authentication-configurator](https://github.com/canonical/request-authentication-configurator) - Contains a charm that configures Gateway to perform request authentication.
+
+(initialise-the-repository)=
+## Initialise the repository
 
 Next, use {external+charmcraft:doc}`Charmcraft <index>` to generate the recommended project structure in the repository:
 

docs/howto/manage-storage.md

@@ -86,6 +86,30 @@ self.model.storages.request('cache', 2)  # Request two more instances.
 
 The additional instances won't be available immediately after the call. As with `juju add-storage`, your charm will receive a storage-attached event as each additional instance becomes available.
 
+### Clean up storage
+
+Juju emits a [storage-detaching](ops.StorageDetachingEvent) event whenever attached storage is being released: when a user runs `juju detach-storage` (or `juju remove-storage --force`), and during unit teardown. Observe this event to cleanly release resources before the storage goes away.
+
+In the `src/charm.py` file, in the `__init__` function of your charm, set up an observer for the detaching event associated with your storage and pair that with an event handler. For example:
+
+```python
+framework.observe(
+    self.on['cache'].storage_detaching, self._on_storage_detaching
+)
+```
+
+Now, in the body of the charm class, define the event handler, or adjust an existing holistic one. For example, to warn users that data won't be cached:
+
+```python
+def _on_storage_detaching(self, event: ops.StorageDetachingEvent):
+    """Handle the storage being detached."""
+    self.unit.status = ops.ActiveStatus(
+        'Caching disabled; provide storage to boost performance'
+    )
+```
+
+> Examples: [MongoDB updating the set before storage is removed](https://github.com/canonical/mongodb-operator/blob/b33d036173f47c68823e08a9f03189dc534d38dc/src/charm.py#L596)
+
 ## Manage storage for a Kubernetes charm
 
 ### Define the storage
@@ -164,29 +188,11 @@ charm_cache_root = pathops.LocalPath(charm_cache_path)
 
 Alternatively, use {external+charmlibs:class}`pathops.ContainerPath` to access `web_cache_path` in the workload container. This approach is more appropriate if you need to reference additional data in the workload container.
 
-## Handle storage detaching
-
-In the `src/charm.py` file, in the `__init__` function of your charm, set up an observer for the detaching event associated with your storage and pair that with an event handler. For example:
-
-```python
-framework.observe(
-    self.on['cache'].storage_detaching, self._on_storage_detaching
-)
-```
-
-> See more: [](ops.StorageDetachingEvent)
-
-Now, in the body of the charm definition, define the event handler, or adjust an existing holistic one. For example, to warn users that data won't be cached:
+### Clean up storage
 
-```python
-def _on_storage_detaching(self, event: ops.StorageDetachingEvent):
-    """Handle the storage being detached."""
-    self.unit.status = ops.ActiveStatus(
-        'Caching disabled; provide storage to boost performance'
-    )
-```
+On Kubernetes, `juju detach-storage` isn't supported, so storage is only detached during unit teardown. Juju emits a [storage-detaching](ops.StorageDetachingEvent) event during teardown, along with `stop`, `remove`, and other teardown events.
 
-> Examples: [MySQL handling cluster management](https://github.com/canonical/mysql-k8s-operator/blob/4c575b478b7ae2a28b09dde9cade2d3370dd4db6/src/charm.py#L823), [MongoDB updating the set before storage is removed](https://github.com/canonical/mongodb-operator/blob/b33d036173f47c68823e08a9f03189dc534d38dc/src/charm.py#L596)
+> Examples: [MySQL handling cluster management](https://github.com/canonical/mysql-k8s-operator/blob/4c575b478b7ae2a28b09dde9cade2d3370dd4db6/src/charm.py#L823)
 
 ## Write unit tests
 

docs/reuse/best-practices.txt

@@ -1,5 +1,5 @@
 **[How to initialise your project](https://documentation.ubuntu.com/ops/latest/howto/initialise-your-project/)**
-- Name the repository using the pattern ``<charm name>-operator`` for a single charm, or ``<base charm name>-operators`` when the repository will hold multiple related charms. For the charm name, see [](#decide-your-charms-name). See [Create a repository and initialise it](#create-a-repository-and-initialise-it).
+- If your charm operates a workload, name the repository `<charm name>-operator`. For advice about the charm name, see [](#decide-your-charms-name). If your charm doesn't operate a workload (as in the case of integrator charms and configurator charms), the `-operator` suffix isn't needed. For example, `foo-integrator` and `bar-configurator`. Repositories that contain multiple charms or one or more charms and other artefacts (like Rocks) will need to use other naming patterns. See [Create a repository](#create-a-repository).
 
 **[How to log from your charm](https://documentation.ubuntu.com/ops/latest/howto/log-from-your-charm/)**
 - Capture output to `stdout` and `stderr` in your charm and use the logging and warning functionality to send messages to the charm user, rather than rely on Juju capturing output. In particular, you should avoid `print()` calls, and ensure that any subprocess calls capture output.

docs/tutorial/write-your-first-machine-charm.md

@@ -707,7 +707,7 @@ def test_version(monkeypatch: pytest.MonkeyPatch):
 We'll run all the tests later in the tutorial. But if you'd like to see whether this test passes, run:
 
 ```text
-tox -e unit -- tests/unit/test_tinyproxy.py
+tox -e unit -- -k test_version
 ```
 
 ### Write state-transition tests

ops/framework.py

@@ -647,6 +647,12 @@ def __init__(
         self._breakpoint_welcomed: bool = False
         self._juju_debug_at = juju_debug_at or set()
 
+        # Set to True once close() has run. Callers that close defensively
+        # check this rather than calling close() again: charm libraries (for
+        # example tempo's charm_tracing) replace the instance's close with a
+        # wrapper that is not safe to invoke twice.
+        self._closed: bool = False
+
     def set_breakpointhook(self) -> Any | None:
         """Hook into ``sys.breakpointhook`` so the builtin ``breakpoint()`` works as expected.
 
@@ -669,6 +675,7 @@ def set_breakpointhook(self) -> Any | None:
     def close(self) -> None:
         """Close the underlying backends."""
         self._storage.close()
+        self._closed = True
 
     def _track(self, obj: Serializable):
         """Track object and ensure it is the only object created using its handle path."""

ops/pebble.py

@@ -340,6 +340,8 @@ def __enter__(self) -> typing.IO[typing.AnyStr]: ...
 
 
 class _WebSocket(Protocol):
+    sock: socket.socket | None
+
     def connect(self, url: str, socket: socket.socket): ...
 
     def shutdown(self): ...
@@ -410,12 +412,39 @@ def _format_timeout(timeout: float) -> str:
 
 
 def _start_thread(target: Callable[..., Any], *args: Any, **kwargs: Any) -> threading.Thread:
-    """Helper to simplify starting a thread."""
-    thread = threading.Thread(target=target, args=args, kwargs=kwargs)
+    """Helper to simplify starting a thread.
+
+    The thread is marked as daemon so that exec I/O threads can never keep
+    the interpreter alive at exit, even when teardown is missed (for
+    example, if wait() is never called and the server holds the websockets
+    open).
+    """
+    thread = threading.Thread(target=target, args=args, kwargs=kwargs, daemon=True)
     thread.start()
     return thread
 
 
+def _force_close_websocket(ws: _WebSocket):
+    """Close a websocket so that reads and writes blocked on it unblock.
+
+    ``WebSocket.shutdown()`` only closes the socket's file descriptor, and
+    closing a socket doesn't wake up other threads that are blocked in
+    ``recv()`` on the same socket; an OS-level shutdown of the connection
+    does.
+
+    This reaches into websocket-client's ``WebSocket.sock`` (the underlying
+    ``socket.socket``) because the library exposes no public way to shut the
+    connection down without first closing the fd.
+    """
+    sock = ws.sock
+    if sock is not None:
+        try:
+            sock.shutdown(socket.SHUT_RDWR)
+        except OSError:
+            pass  # Already disconnected or closed.
+    ws.shutdown()
+
+
 class Error(Exception):
     """Base class of most errors raised by the Pebble client."""
 
@@ -1787,20 +1816,35 @@ def _wait(self) -> int:
         if timeout is not None:
             # A bit more than the command timeout to ensure that happens first
             timeout += 1
-        change = self._client.wait_change(self._change_id, timeout=timeout)
+        try:
+            change = self._client.wait_change(self._change_id, timeout=timeout)
+        except BaseException:
+            # The wait failed or was interrupted, for example because the
+            # change didn't finish before the client-side timeout above. The
+            # I/O threads may still be blocked on their websockets, so tear
+            # the connections down to unblock them, and reap the threads
+            # before propagating the error, otherwise they're left running
+            # and at interpreter shutdown block the join of non-daemon
+            # threads (#2556).
+            self._teardown_after_error()
+            raise
 
         # If stdin reader thread is running, stop it
         if self._cancel_stdin is not None:
             self._cancel_stdin()
+            self._cancel_stdin = None
 
         # Wait for all threads to finish (e.g., message barrier sent)
         for thread in self._threads:
             thread.join()
 
         # If we opened a cancel_reader pipe, close the read side now (write
-        # side was already closed by _cancel_stdin().
+        # side was already closed by _cancel_stdin()). Clear it afterwards so
+        # that a second call to _wait() (e.g. wait() then wait_output()) can't
+        # close the same fd again, matching _teardown_after_error().
         if self._cancel_reader is not None:
             os.close(self._cancel_reader)
+            self._cancel_reader = None
 
         # Close websockets (shutdown doesn't send CLOSE message or wait for response).
         self._control_ws.shutdown()
@@ -1816,6 +1860,35 @@ def _wait(self) -> int:
             exit_code = change.tasks[0].data.get('exit-code', -1)
         return exit_code
 
+    def _teardown_after_error(self):
+        # If stdin reader thread is running, stop it.
+        if self._cancel_stdin is not None:
+            self._cancel_stdin()
+            self._cancel_stdin = None
+
+        # Unlike the success path, close the websockets before joining the
+        # I/O threads: the server hasn't finished the exec, so the threads
+        # are still blocked reading from the websockets and would never
+        # finish on their own.
+        _force_close_websocket(self._control_ws)
+        _force_close_websocket(self._stdio_ws)
+        if self._stderr_ws is not None:
+            _force_close_websocket(self._stderr_ws)
+
+        for thread in self._threads:
+            # With the websockets closed the threads exit almost immediately;
+            # the timeout is belt-and-braces (they're daemon threads, so even
+            # a leak here can't block interpreter shutdown).
+            thread.join(timeout=1.0)
+            if thread.is_alive():
+                logger.warning('exec I/O thread did not finish after error in wait')
+
+        # If we opened a cancel_reader pipe, close the read side now (write
+        # side was already closed by _cancel_stdin()).
+        if self._cancel_reader is not None:
+            os.close(self._cancel_reader)
+            self._cancel_reader = None
+
     def wait_output(self) -> tuple[AnyStr, AnyStr | None]:
         """Wait for the process to finish and return tuple of (stdout, stderr).
 
@@ -1896,29 +1969,43 @@ def _reader_to_websocket(
     bufsize: int = 16 * 1024,
 ):
     """Read reader through to EOF and send each chunk read to the websocket."""
-    while True:
-        if cancel_reader is not None:
-            # Wait for either a read to be ready or the caller to cancel stdin.
-            # If the reader is also ready, drain it first so a cancel that
-            # arrives before this thread runs doesn't drop pending input.
-            result = select.select([cancel_reader, reader], [], [])
-            if reader not in result[0]:
-                break
+    try:
+        while True:
+            if cancel_reader is not None:
+                # Wait for either a read to be ready or the caller to cancel stdin.
+                # If the reader is also ready, drain it first so a cancel that
+                # arrives before this thread runs doesn't drop pending input.
+                result = select.select([cancel_reader, reader], [], [])
+                if reader not in result[0]:
+                    break
 
-        chunk = reader.read(bufsize)
-        if not chunk:
-            break
-        if isinstance(chunk, str):
-            chunk = chunk.encode(encoding)
-        ws.send_binary(chunk)
+            chunk = reader.read(bufsize)
+            if not chunk:
+                break
+            if isinstance(chunk, str):
+                chunk = chunk.encode(encoding)
+            ws.send_binary(chunk)
 
-    ws.send('{"command":"end"}')  # Send "end" command as TEXT frame to signal EOF
+        ws.send('{"command":"end"}')  # Send "end" command as TEXT frame to signal EOF
+    except (websocket.WebSocketException, OSError) as e:
+        # The websocket or cancel pipe was closed underneath us (for example,
+        # because the exec failed and ExecProcess._wait tore the connection
+        # down), so there's nowhere to send the rest of the input to.
+        logger.debug('exec stdin forwarder stopped: %s', e)
 
 
 def _websocket_to_writer(ws: _WebSocket, writer: _WebsocketWriter, encoding: str | None):
     """Receive messages from websocket (until end signal) and write to writer."""
     while True:
-        chunk = ws.recv()
+        try:
+            chunk = ws.recv()
+        except (websocket.WebSocketException, OSError) as e:
+            # The websocket was closed underneath us (for example, because
+            # the exec failed and ExecProcess._wait tore the connection
+            # down): treat it as end-of-stream rather than crashing the
+            # thread.
+            logger.debug('exec output forwarder stopped: %s', e)
+            break
 
         if isinstance(chunk, str):
             try:
@@ -1996,7 +2083,16 @@ def read(self, n: int = -1) -> str | bytes:
             return b''
 
         while not self.remaining:
-            chunk = self.ws.recv()
+            try:
+                chunk = self.ws.recv()
+            except (websocket.WebSocketException, OSError) as e:
+                # The websocket was closed underneath us (for example,
+                # because the exec failed and ExecProcess._wait tore the
+                # connection down): treat it as end-of-file. The error that
+                # caused the teardown is reported by wait()/wait_output().
+                logger.debug('exec output stream closed: %s', e)
+                self.eof = True
+                return b''
 
             if isinstance(chunk, str):
                 try:
@@ -3155,14 +3251,21 @@ def exec(
             task_id = resp['result']['task-id']
 
             stderr_ws: _WebSocket | None = None
+            connected: list[_WebSocket] = []
             try:
                 control_ws = self._connect_websocket(task_id, 'control')
+                connected.append(control_ws)
                 stdio_ws = self._connect_websocket(task_id, 'stdio')
+                connected.append(stdio_ws)
                 if not combine_stderr:
                     stderr_ws = self._connect_websocket(task_id, 'stderr')
+                    connected.append(stderr_ws)
             except websocket.WebSocketException as e:
                 # Error connecting to websockets, probably due to the exec/change
-                # finishing early with an error. Call wait_change to pick that up.
+                # finishing early with an error. Close any websockets that did
+                # connect, then call wait_change to pick up the change error.
+                for ws in connected:
+                    ws.shutdown()
                 change = self.wait_change(ChangeID(change_id))
                 if change.err:
                     raise ChangeError(change.err, change) from e

test/charms/test_hookcmds/.gitignore

@@ -0,0 +1,3 @@
+uv.lock
+*.tar.gz
+*.charm