From 434bc3b4d8232a195e0fb2b2f17debb454582d8b Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 8 Jun 2026 17:38:05 +1200 Subject: [PATCH 01/10] fix: avoid errors in finalisation due to already-closed websocket (#2548) This PR ensures that when closing the Pebble websocket: * If the socket is already closed, we do nothing. * If we get an error because it's already closed while closing (presumably a race), we ignore that. #2547 has more details on why we have issues in Python 3.13+ when finalising at the moment. An alternative would be to change `_wait` to close, but that is riskier, since it would be a behaviour change and could in theory trigger issues if someone is doing something odd with a wrapped websocket. Quite unlikely in our case, but the change in this PR solves the issue and seems clean enough, and doesn't have the risk. Fixes #2547 (cherry picked from commit 9d03472b82aa63f8ec7797c658854d1a63a9ddd3) --- ops/pebble.py | 19 +++++++++++++++++-- test/test_pebble.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/ops/pebble.py b/ops/pebble.py index ddaf5bc8b..bcf03e3d4 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -1963,8 +1963,23 @@ def write(self, chunk: str | bytes) -> int: return len(chunk) def close(self): - """Send end-of-file message to websocket.""" - self.ws.send('{"command":"end"}') + """Send end-of-file message to websocket. + + Idempotent and tolerant of the underlying websocket already being + closed: ``ExecProcess._wait`` shuts the stdio websocket down before + the writer (or the ``TextIOWrapper`` wrapping it) is finalised, so + ``IOBase.__del__`` would otherwise surface a + ``WebSocketConnectionClosedException`` as an "Exception ignored + while finalizing file" warning on Python 3.13+ (see CPython + gh-62948). + """ + if self.closed: + return + try: + self.ws.send('{"command":"end"}') + except websocket.WebSocketConnectionClosedException: + pass + super().close() class _WebsocketReader(io.BufferedIOBase): diff --git a/test/test_pebble.py b/test/test_pebble.py index 4b9c581e7..16762b3b1 100644 --- a/test/test_pebble.py +++ b/test/test_pebble.py @@ -3951,6 +3951,35 @@ def test_wait_returned_io_bytes(self, client: MockClient): ('TXT', '{"command":"end"}'), ] + def test_writer_close_after_shutdown(self): + """Closing the writer after the websocket is shut down must not raise. + + Regression test for canonical/operator#2547. In the normal exec + lifecycle ``ExecProcess._wait`` shuts the stdio websocket down + before the ``_WebsocketWriter`` (or the ``TextIOWrapper`` wrapping + it) is finalised by the garbage collector. On Python 3.13+ a raised + ``WebSocketConnectionClosedException`` from ``IOBase.__del__`` is + surfaced as an "Exception ignored while finalizing file" warning. + """ + ws = MockWebsocket() + + def send(s: str): + raise websocket.WebSocketConnectionClosedException('socket is already closed.') + + ws.send = send + writer = pebble._WebsocketWriter(typing.cast('pebble._WebSocket', ws)) + writer.close() # must not raise + assert writer.closed + assert ws.sends == [] + + def test_writer_close_is_idempotent(self): + """A second close() must be a no-op (no duplicate "end" sent).""" + ws = MockWebsocket() + writer = pebble._WebsocketWriter(typing.cast('pebble._WebSocket', ws)) + writer.close() + writer.close() + assert ws.sends == [('TXT', '{"command":"end"}')] + def test_connect_websocket_error(self): class Client(MockClient): def _connect_websocket(self, change_id: str, websocket_id: str): From 55b552b97b751880676bde3c2d1116648f7b5d36 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 2 Jun 2026 21:41:50 +1200 Subject: [PATCH 02/10] fix: close SQLite storage in Harness.cleanup() (#2507) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don't normally do fixes in Harness, but this will allow charms that are not yet migrated to Scenario to run with `-Werror` and not get hit by a resource warning we cause. `Harness` builds a `framework.Framework` backed by an in-memory `SQLiteStorage`, but `Harness.cleanup()` never closes it. The sqlite3 connection is only released when the `Harness` is garbage-collected, at which point its destructor emits `unclosed database in `. pytest wraps that as `PytestUnraisableExceptionWarning` and `-W error` turns it into a test failure — even for callers who correctly call `harness.cleanup()`. This is one of the bug classes flagged by a recent `canonical/hyrum` (super-tox) run across the canonical charm collection: under `-Werror`, charms whose tests use `ops.testing.Harness` regress even though their own code is clean. Changes: - `Harness.cleanup()`: call `self._framework.close()` after the backend cleanup so the `SQLiteStorage` connection is closed eagerly. sqlite3's `close()` is idempotent, so repeated `cleanup()` calls stay safe. - Add a regression test that asserts the storage's underlying connection is closed after `harness.cleanup()`. --------- Co-authored-by: Claude Sonnet 4.6 (cherry picked from commit 54376c144cf8d636b4e9ed43e2c1dc7abb31ea0f) --- ops/_private/harness.py | 1 + test/test_testing.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/ops/_private/harness.py b/ops/_private/harness.py index bc89374cb..e80d79b9a 100644 --- a/ops/_private/harness.py +++ b/ops/_private/harness.py @@ -554,6 +554,7 @@ def cleanup(self) -> None: Always call ``self.addCleanup(harness.cleanup)`` after creating a :class:`Harness`. """ self._backend._cleanup() + self._framework.close() def _create_meta( self, diff --git a/test/test_testing.py b/test/test_testing.py index 6468684f8..6bf7d833a 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -26,6 +26,7 @@ import platform import pwd import shutil +import sqlite3 import sys import tempfile import textwrap @@ -3816,6 +3817,19 @@ def test_lazy_resource_directory(self, request: pytest.FixtureRequest): f'expected {path} to be a subdirectory of {backend._resource_dir.name}' ) + def test_cleanup_closes_framework_storage(self): + # Regression test: harness.cleanup() must close the SQLiteStorage + # connection. Otherwise the connection's destructor emits a + # ResourceWarning at GC, which surfaces as a test failure for callers + # running under -W error (such as downstream charm suites on + # Python 3.14). + harness = ops.testing.Harness(ops.CharmBase, meta='name: test-app') + harness.begin() + storage = harness._storage + harness.cleanup() + with pytest.raises(sqlite3.ProgrammingError): + storage._db.execute('SELECT 1') + def test_resource_get_no_resource(self, request: pytest.FixtureRequest): harness = ops.testing.Harness( ops.CharmBase, From 0221cc58b459efb2b92bde77e4b4604dcb631ebc Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 8 Jun 2026 17:12:04 +1200 Subject: [PATCH 03/10] fix: restore sys.breakpointhook on _Manager teardown (#2542) `Framework.set_breakpointhook()` has always returned the old hook so the caller can restore it, but `_Manager._make_framework()` discarded the return value. In a real Juju hook execution this doesn't matter, since the process is short-lived, but tests that drive `ops.main()` directly leave a polluted `sys.breakpointhook` for every subsequent test in the same process. The leaked hook is a bound `framework.breakpoint` method whose `_juju_debug_at` came from whichever test set `JUJU_DEBUG_AT`, so a later test calling `breakpoint()` drops into `pdb` regardless of its own `PYTHONBREAKPOINT`. The PR fixes this by saving the return value in `_make_framework()` and restoring it in `destroy()`. The attribute is a class-level `None` so that `destroy()` is safe even if `_make_framework()` never ran. Also fixes a test that relied on the bug. Co-authored-by: Claude Opus 4.7 (cherry picked from commit 9b797aa95c60b518e33aeea3f3637622fca67bf8) --- ops/_main.py | 11 ++++++++++- test/test_main.py | 33 +++++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/ops/_main.py b/ops/_main.py index 8aaa07cb7..7c17b8953 100644 --- a/ops/_main.py +++ b/ops/_main.py @@ -268,6 +268,8 @@ class _Manager: - graceful teardown of the storage """ + _saved_breakpointhook: Any = None + def __init__( self, charm_class: type[_charm.CharmBase], @@ -402,7 +404,12 @@ def _make_framework(self, dispatcher: _Dispatcher): event_name=dispatcher.event_name, juju_debug_at=self._juju_context.debug_at, ) - framework.set_breakpointhook() + # set_breakpointhook() returns the old sys.breakpointhook; capture it + # so destroy() can put the global state back rather than leaving the + # framework's bound method dangling. In production this is moot (the + # process is exiting anyway), but tests and other long-running + # ops.main() callers inherit the polluted hook otherwise. + self._saved_breakpointhook = framework.set_breakpointhook() return framework def _emit(self): @@ -476,6 +483,8 @@ def destroy(self): """Finalise the manager.""" from . import tracing # break circular import + if self._saved_breakpointhook is not None: + sys.breakpointhook = self._saved_breakpointhook self._tracing_context.__exit__(*sys.exc_info()) if tracing: tracing._shutdown() diff --git a/test/test_main.py b/test/test_main.py index d7b8514bc..8848eecea 100644 --- a/test/test_main.py +++ b/test/test_main.py @@ -86,27 +86,40 @@ class EventSpec: class TestCharmInit: @patch('sys.stderr', new_callable=io.StringIO) def test_breakpoint(self, fake_stderr: io.StringIO): + pdb_set_trace_calls: list[int] = [] + class MyCharm(ops.CharmBase): - pass + def __init__(self, framework: ops.Framework): + super().__init__(framework) + # breakpoint() must be exercised inside the charm: ops.main() + # installs framework.breakpoint as sys.breakpointhook for the + # duration of the event and restores the previous hook on + # teardown, so calling breakpoint() after _check() returns + # would not exercise the framework's hook at all. + with patch('pdb.Pdb.set_trace') as mock: + breakpoint() + pdb_set_trace_calls.append(mock.call_count) self._check(MyCharm, extra_environ={'JUJU_DEBUG_AT': 'all'}) - with patch('pdb.Pdb.set_trace') as mock: - breakpoint() - - assert mock.call_count == 1 + assert pdb_set_trace_calls == [1] assert 'Starting pdb to debug charm operator' in fake_stderr.getvalue() def test_no_debug_breakpoint(self): + pdb_set_trace_calls: list[int] = [] + class MyCharm(ops.CharmBase): - pass + def __init__(self, framework: ops.Framework): + super().__init__(framework) + # See note in test_breakpoint about why this runs inside the + # charm. + with patch('pdb.Pdb.set_trace') as mock: + breakpoint() + pdb_set_trace_calls.append(mock.call_count) self._check(MyCharm, extra_environ={'JUJU_DEBUG_AT': ''}) - with patch('pdb.Pdb.set_trace') as mock: - breakpoint() - - assert mock.call_count == 0 + assert pdb_set_trace_calls == [0] def _check( self, From f83ad82977565eaa74878a484f9afbae85024c35 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sun, 21 Jun 2026 18:05:36 +1200 Subject: [PATCH 04/10] fix: take Pebble defaults into consideration when consistency checking Checks (#2567) When Pebble returns information about a check, it copies the level, startup, and threshold into the response from the computed plan. If none of the layers have provided values for these, the plan has defaults (unset, enabled, and 3, respectively). When we check a `CheckInfo` for consistency in Scenario (meaning that the values should be ones Pebble could sensibly return, given the rest of the state), we need to take that default value into account, as Pebble would. We don't actually model checks going up and down currently, so it's really only the consistency that we are concerned with. This PR fixes the consistency check so that it will fall back to the Pebble defaults when needed. It repeats the defaults, because the existing copy is buried in the mock Pebble client in Harness, and the source of truth is in the Pebble code. Fixes #2566 --------- Co-authored-by: Dave Wilding Co-authored-by: Claude Opus 4.7 (cherry picked from commit a1f3afa13ecd8065d1dd0f1eb726766ed9a9d9b3) --- testing/src/scenario/_consistency_checker.py | 41 ++++++++++++++++---- testing/tests/test_consistency_checker.py | 21 ++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/testing/src/scenario/_consistency_checker.py b/testing/src/scenario/_consistency_checker.py index b26f27b04..1f27ae529 100644 --- a/testing/src/scenario/_consistency_checker.py +++ b/testing/src/scenario/_consistency_checker.py @@ -33,6 +33,8 @@ NamedTuple, ) +from ops import pebble + from .errors import InconsistentScenarioError from ._runtime import logger as scenario_logger from .state import ( @@ -685,13 +687,38 @@ def check_containers_consistency( ) continue plan_check = plan.checks[check.name] - for attr in ('level', 'startup', 'threshold'): - if getattr(check, attr) != getattr(plan_check, attr): - errors.append( - f'container {container.name!r} has a check {check.name!r} with a ' - f'different {attr!r} ({getattr(check, attr)}) ' - f'than the plan ({getattr(plan_check, attr)}).', - ) + # Scenario allows None for unset, Pebble defaults to UNSET. + check_level = check.level if check.level is not None else pebble.CheckLevel.UNSET + if check_level != plan_check.level: + errors.append( + f'container {container.name!r} has a check {check.name!r} with a ' + f"different 'level' ({check.level}) than the plan ({plan_check.level}).", + ) + # Scenario defaults to None, Pebble defaults to UNSET and collapses to ENABLED. + check_startup = ( + pebble.CheckStartup.ENABLED + if check.startup in (None, pebble.CheckStartup.UNSET) + else check.startup + ) + plan_startup = ( + pebble.CheckStartup.ENABLED + if plan_check.startup == pebble.CheckStartup.UNSET + else plan_check.startup + ) + if check_startup != plan_startup: + errors.append( + f'container {container.name!r} has a check {check.name!r} with a ' + f"different 'startup' ({check.startup}) than the plan ({plan_check.startup}).", + ) + # Scenario and Pebble allow None, collapsing to 3. + check_threshold = 3 if check.threshold is None else check.threshold + plan_threshold = 3 if plan_check.threshold is None else plan_check.threshold + if check_threshold != plan_threshold: + errors.append( + f'container {container.name!r} has a check {check.name!r} with a ' + f"different 'threshold' ({check.threshold}) than the plan " + f'({plan_check.threshold}).', + ) return Results(errors, []) diff --git a/testing/tests/test_consistency_checker.py b/testing/tests/test_consistency_checker.py index f7d212d26..fc89ca7b3 100644 --- a/testing/tests/test_consistency_checker.py +++ b/testing/tests/test_consistency_checker.py @@ -235,6 +235,27 @@ def test_checkinfo_matches_layer(check: CheckInfo, consistent: bool): ) +def test_checkinfo_matches_layer_with_defaults(): + # Pebble fills in default values for attributes the plan omits, so a + # CheckInfo reporting the Pebble defaults must be considered consistent + # with a layer that does not specify them. See #2566. + layer = ops.pebble.Layer({ + 'checks': {'chk1': {'override': 'replace', 'exec': {'command': 'echo'}}} + }) + check = CheckInfo( + 'chk1', + level=ops.pebble.CheckLevel.UNSET, + startup=ops.pebble.CheckStartup.ENABLED, + threshold=3, + ) + state = State(containers={Container('foo', check_infos={check}, layers={'base': layer})}) + assert_consistent( + state, + _Event('foo-pebble-ready', container=Container('foo')), + _CharmSpec(MyCharm, {'containers': {'foo': {}}}), + ) + + @pytest.mark.parametrize('suffix', sorted(_RELATION_EVENTS_SUFFIX)) def test_evt_bad_relation_name(suffix: str): assert_inconsistent( From 253d811c3e839492d51326db13dcb279e9bac451 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 2 Jun 2026 21:22:17 +1200 Subject: [PATCH 05/10] fix: ensure resources are cleaned up in testing.Context (#2506) A few fixes to ensure we do not leak resources (until gc time) in Scenario: * In runtime, move the cleanup to a finally block. * In the Context, instead of using TemporaryDirectory, use mktmpdir directly, and register a weakref cleanup -- but also let users explicitly clean up if they prefer (and make the context manager case do that automatically). * In state, replace a few opens with read_texts (we could do the open with a context manager is read but this seemed the more minimal change, and none of the YAML files should be large enough that there is any difference). * In a couple of Scenario's own tests, avoid leaking when using Container.pull. After these changes, the tests can be run with -Werror and not have resource warnings cause failures. --------- Co-authored-by: Claude Opus 4.7 (cherry picked from commit 43b322f8d60d53669cb1d7f55b029db4aad579d1) --- testing/src/scenario/_runtime.py | 26 +++++++++++++--------- testing/src/scenario/context.py | 32 ++++++++++++++++++++++++--- testing/src/scenario/state.py | 8 +++---- testing/tests/test_e2e/test_pebble.py | 13 +++++------ 4 files changed, 54 insertions(+), 25 deletions(-) diff --git a/testing/src/scenario/_runtime.py b/testing/src/scenario/_runtime.py index 5a331cd17..281ef345e 100644 --- a/testing/src/scenario/_runtime.py +++ b/testing/src/scenario/_runtime.py @@ -261,18 +261,22 @@ def _virtual_charm_root(self): config_yaml.write_text(yaml.safe_dump(spec.config or {})) actions_yaml.write_text(yaml.safe_dump(spec.actions or {})) - yield virtual_charm_root - - if charm_virtual_root_is_custom: - for file, previous_content in metadata_files_present.items(): - if previous_content is None: # None == file did not exist before - file.unlink() - else: - file.write_text(previous_content) + try: + yield virtual_charm_root + finally: + if charm_virtual_root_is_custom: + for file, previous_content in metadata_files_present.items(): + if previous_content is None: # None == file did not exist before + file.unlink() + else: + file.write_text(previous_content) - else: - # charm_virtual_root is a tempdir - typing.cast('tempfile.TemporaryDirectory', charm_virtual_root).cleanup() # type: ignore + else: + # charm_virtual_root is a tempdir + typing.cast( + 'tempfile.TemporaryDirectory', + charm_virtual_root, + ).cleanup() # type: ignore @contextmanager def _exec_ctx(self, ctx: Context): diff --git a/testing/src/scenario/context.py b/testing/src/scenario/context.py index 849670cd7..ab133d78f 100644 --- a/testing/src/scenario/context.py +++ b/testing/src/scenario/context.py @@ -12,7 +12,9 @@ import copy import functools import pathlib +import shutil import tempfile +import weakref from contextlib import contextmanager from typing import ( Generic, @@ -668,7 +670,13 @@ def __init__( self._app_name = app_name self._unit_id = unit_id self.app_trusted = app_trusted - self._tmp = tempfile.TemporaryDirectory() + # Allocate the per-context tempdir for simulated container/storage roots. + # We use mkdtemp + weakref.finalize rather than tempfile.TemporaryDirectory + # so that fallback GC-time cleanup doesn't emit a ResourceWarning when a + # Context isn't used as a context manager. Callers should still prefer + # `with Context(...) as ctx:` (or call ctx.close()) for eager cleanup. + self._tmp_path = pathlib.Path(tempfile.mkdtemp(prefix='scenario-')) + self._tmp_finalizer = weakref.finalize(self, shutil.rmtree, str(self._tmp_path), True) # config for what events to be captured in emitted_events. self.capture_deferred_events = capture_deferred_events @@ -701,11 +709,11 @@ def _set_output_state(self, output_state: State): def _get_container_root(self, container_name: str): """Get the path to a tempdir where this container's simulated root will live.""" - return pathlib.Path(self._tmp.name) / 'containers' / container_name + return self._tmp_path / 'containers' / container_name def _get_storage_root(self, name: str, index: int) -> pathlib.Path: """Get the path to a tempdir where this storage's simulated root will live.""" - storage_root = pathlib.Path(self._tmp.name) / 'storages' / f'{name}-{index}' + storage_root = self._tmp_path / 'storages' / f'{name}-{index}' # in the case of _get_container_root, _MockPebbleClient will ensure the dir exists. storage_root.mkdir(parents=True, exist_ok=True) return storage_root @@ -717,6 +725,24 @@ def _record_status(self, state: State, is_app: bool): else: self.unit_status_history.append(state.unit_status) + def close(self) -> None: + """Delete the temporary directory used for simulated container and storage roots. + + Prefer using the Context as a context manager (``with Context(...) as ctx:``), + or call ``close()`` when you're done with it. This is especially important + when running tests with ``-W error``, where leaving cleanup to the garbage + collector can clash with pytest's teardown. If you do neither, the + temporary directory is still removed when the Context is garbage-collected. + """ + if self._tmp_finalizer.alive: + self._tmp_finalizer() + + def __enter__(self) -> Context[CharmType]: + return self + + def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None: + self.close() + def __call__(self, event: _Event, state: State) -> Manager[CharmType]: """Context manager to introspect live charm object before and after the event is emitted. diff --git a/testing/src/scenario/state.py b/testing/src/scenario/state.py index eb01d0122..e503b13ac 100644 --- a/testing/src/scenario/state.py +++ b/testing/src/scenario/state.py @@ -1980,14 +1980,14 @@ def _load_metadata_legacy(charm_root: pathlib.Path): # files for charm metadata. metadata_path = charm_root / 'metadata.yaml' meta: dict[str, Any] = ( - yaml.safe_load(metadata_path.open()) if metadata_path.exists() else {} + yaml.safe_load(metadata_path.read_text()) if metadata_path.exists() else {} ) config_path = charm_root / 'config.yaml' - config = yaml.safe_load(config_path.open()) if config_path.exists() else None + config = yaml.safe_load(config_path.read_text()) if config_path.exists() else None actions_path = charm_root / 'actions.yaml' - actions = yaml.safe_load(actions_path.open()) if actions_path.exists() else None + actions = yaml.safe_load(actions_path.read_text()) if actions_path.exists() else None return meta, config, actions @staticmethod @@ -1995,7 +1995,7 @@ def _load_metadata(charm_root: pathlib.Path): """Load metadata from charm projects created with Charmcraft >= 2.5.""" metadata_path = charm_root / 'charmcraft.yaml' meta: dict[str, Any] = ( - yaml.safe_load(metadata_path.open()) if metadata_path.exists() else {} + yaml.safe_load(metadata_path.read_text()) if metadata_path.exists() else {} ) if not _is_valid_charmcraft_25_metadata(meta): meta = {} diff --git a/testing/tests/test_e2e/test_pebble.py b/testing/tests/test_e2e/test_pebble.py index 99c86c3db..f9e3c3fd9 100644 --- a/testing/tests/test_e2e/test_pebble.py +++ b/testing/tests/test_e2e/test_pebble.py @@ -80,8 +80,8 @@ def test_fs_push(charm_cls): def callback(self: CharmBase): container = self.unit.get_container('foo') - baz = container.pull('/bar/baz.txt') - assert baz.read() == text + with container.pull('/bar/baz.txt') as baz: + assert baz.read() == text trigger( State( @@ -109,8 +109,8 @@ def callback(self: CharmBase): if make_dirs: container.push('/foo/bar/baz.txt', text, make_dirs=make_dirs) # check that pulling immediately 'works' - baz = container.pull('/foo/bar/baz.txt') - assert baz.read() == text + with container.pull('/foo/bar/baz.txt') as baz: + assert baz.read() == text else: with pytest.raises(pebble.PathError): container.push('/foo/bar/baz.txt', text, make_dirs=make_dirs) @@ -145,9 +145,8 @@ def callback(self: CharmBase): assert file == Path(out.get_container('foo').mounts['foo'].source) / 'bar' / 'baz.txt' # but that is actually a symlink to the context's root tmp folder: - assert ( - Path(ctx._tmp.name) / 'containers' / 'foo' / 'foo' / 'bar' / 'baz.txt' - ).read_text() == text + base = ctx._tmp_path + assert (base / 'containers' / 'foo' / 'foo' / 'bar' / 'baz.txt').read_text() == text assert file.read_text() == text # shortcut for API niceness purposes: From 206115b8dec4289d77c7d5fa46dfd91470deb47a Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 17 Jun 2026 09:10:51 +1200 Subject: [PATCH 06/10] fix: don't leak exec I/O threads when waiting on the change fails (#2558) If the change for an exec doesn't finish before the client-side timeout, or waiting on it fails in any other way, `ExecProcess._wait()` propagates the exception without any teardown. The I/O pump threads are left blocked in `recv()` on the still open websockets, and being non-daemon threads they then block interpreter shutdown indefinitely, wedging the Juju hook. See #2556 for the reproduction details (note that the leak needs the change to outlive the client-side wait, for example an orphan holding the exec's stdio pipe open; a plain exec timeout completes the change server-side and already cleans up correctly). This tears the exec down when the wait fails: * Shut the websocket connections down at the OS level (`SHUT_RDWR`) rather than only closing the file descriptors. Closing a socket doesn't wake other threads blocked in `recv()` on it, so the `finally` approach suggested in the issue (or joining the threads first, as the success path does) would hang the hook immediately instead of at exit. * Treat a connection closed underneath an I/O pump thread (`WebSocketException` or `OSError`) as end-of-stream rather than letting the exception escape the thread, which printed noisy `WebSocketConnectionClosedException` tracebacks via `threading.excepthook`. * Create the exec I/O threads as daemon threads, so any teardown path that's still missed (including never calling `wait()` at all) can't block interpreter shutdown. * Close any already-connected websockets when connecting the exec websockets fails partway through, rather than leaking them (found while auditing for related leaks). The behaviour of the success path is unchanged: the threads are still joined before the websockets are shut down, so output is never truncated. Verified against a real Pebble (v1.31.0) on Python 3.10 through 3.14: before the fix the reproduction from #2556 leaks two `copyfileobj` threads and hangs at exit; with the fix it raises `ops.pebble.TimeoutError` as before, leaves no threads behind, and exits cleanly. Fixes #2556 --------- Co-authored-by: Claude Fable 5 (cherry picked from commit 00b1e6fb535821caae5c84ffb6eecd40f7910213) --- ops/pebble.py | 143 +++++++++++++++++++++++---- test/test_pebble.py | 207 ++++++++++++++++++++++++++++++++++++++- test/test_real_pebble.py | 18 ++++ 3 files changed, 347 insertions(+), 21 deletions(-) diff --git a/ops/pebble.py b/ops/pebble.py index bcf03e3d4..a08096bbd 100644 --- a/ops/pebble.py +++ b/ops/pebble.py @@ -347,6 +347,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): ... @@ -417,12 +419,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.""" @@ -1794,20 +1823,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() @@ -1823,6 +1867,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). @@ -1903,27 +1976,41 @@ 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 - result = select.select([cancel_reader, reader], [], []) - if cancel_reader 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 + result = select.select([cancel_reader, reader], [], []) + if cancel_reader 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: @@ -2001,7 +2088,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: @@ -3144,14 +3240,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 diff --git a/test/test_pebble.py b/test/test_pebble.py index 16762b3b1..40b5ada65 100644 --- a/test/test_pebble.py +++ b/test/test_pebble.py @@ -23,6 +23,7 @@ import signal import socket import tempfile +import threading import typing import unittest import unittest.util @@ -3384,8 +3385,10 @@ def test_str_truncated(self): class MockWebsocket: def __init__(self): + self.sock: socket.socket | None = None self.sends: list[tuple[str, str | bytes]] = [] self.receives: list[str | bytes] = [] + self.shutdown_calls = 0 def send_binary(self, b: bytes): self.sends.append(('BIN', b)) @@ -3397,7 +3400,33 @@ def recv(self): return self.receives.pop(0) def shutdown(self): - pass + self.shutdown_calls += 1 + + +class BlockingMockWebsocket(MockWebsocket): + """Websocket mock whose recv() blocks on a real socket until it's closed.""" + + def __init__(self): + super().__init__() + self.sock, self._peer = socket.socketpair() + + def recv(self): + assert self.sock is not None + try: + data = self.sock.recv(16) + except OSError: + data = b'' + if not data: + raise websocket.WebSocketConnectionClosedException( + 'Connection to remote host was lost.' + ) + return data + + def shutdown(self): + super().shutdown() + assert self.sock is not None + self.sock.close() + self._peer.close() class TestExec: @@ -4061,6 +4090,182 @@ def recv(): 'ignore::pytest.PytestUnhandledThreadExceptionWarning' )(test_websocket_recv_raises) + def add_blocking_responses(self, client: MockClient, change_id: str): + """Set up an exec response whose websockets block in recv() until closed.""" + task_id = f'T{change_id}' + client.responses.append({ + 'change': change_id, + 'result': {'task-id': task_id}, + }) + stdio = BlockingMockWebsocket() + stderr = BlockingMockWebsocket() + control = BlockingMockWebsocket() + client.websockets = { + (task_id, 'stdio'): stdio, + (task_id, 'stderr'): stderr, + (task_id, 'control'): control, + } + return (stdio, stderr, control) + + def test_wait_change_error_reaps_io_threads( + self, client: MockClient, time: MockTime, monkeypatch: pytest.MonkeyPatch + ): + """If wait_change raises, the I/O threads must be unblocked and reaped. + + Regression test for canonical/operator#2556. The exec change didn't + finish before the client-side timeout, so wait_change() raises; the + I/O threads were left blocked in recv() on the still-open websockets, + and being non-daemon they then hung interpreter shutdown. + """ + thread_errors: list[threading.ExceptHookArgs] = [] + monkeypatch.setattr(threading, 'excepthook', thread_errors.append) + + stdio, stderr, control = self.add_blocking_responses(client, '123') + + def timeout_response(): + time.sleep(3) # simulate passing of time due to wait_change call + raise pebble.APIError({}, 504, 'Gateway Timeout', 'timed out') + + client.responses.append(timeout_response) + + process = client.exec(['foo'], timeout=1) + with pytest.raises(pebble.TimeoutError): + process.wait_output() + + for thread in process._threads: + thread.join(timeout=5) + assert [t.name for t in process._threads if t.is_alive()] == [] + assert thread_errors == [] + for ws in (stdio, stderr, control): + assert ws.shutdown_calls > 0 + + def test_wait_change_error_with_stdin(self, client: MockClient, time: MockTime): + """A failed wait must also stop the stdin thread and close the cancel pipe.""" + self.add_blocking_responses(client, '123') + + def timeout_response(): + time.sleep(3) # simulate passing of time due to wait_change call + raise pebble.APIError({}, 504, 'Gateway Timeout', 'timed out') + + client.responses.append(timeout_response) + + with tempfile.TemporaryFile() as stdin: + stdin.write(b'foo\n') + stdin.seek(0) + process = client.exec(['foo'], stdin=stdin, encoding=None, timeout=1) + with pytest.raises(pebble.TimeoutError): + process.wait() + + for thread in process._threads: + thread.join(timeout=5) + assert [t.name for t in process._threads if t.is_alive()] == [] + # The cancel pipe must have been closed (and not be double-closed if + # wait() is called again). + assert process._cancel_stdin is None + assert process._cancel_reader is None + + def test_io_threads_are_daemon(self, client: MockClient): + stdio, stderr, _ = self.add_responses(client, '123', 0) + stdio.receives.append('{"command":"end"}') + stderr.receives.append('{"command":"end"}') + + process = client.exec(['true']) + process.wait_output() + + assert process._threads + assert all(thread.daemon for thread in process._threads) + + def test_wait_twice_does_not_double_close_cancel_pipe(self, client: MockClient): + """A successful _wait() must clear the cancel pipe so a second call is safe. + + wait() and wait_output() both call _wait(); calling them in turn (or + wait() twice) used to os.close() the already-closed cancel_reader fd + and re-run _cancel_stdin(), raising OSError on the second call. + """ + self.add_responses(client, '123', 0) + # A second wait_change response for the second _wait() call. + change = build_mock_change_dict('123') + assert 'tasks' in change and change['tasks'] is not None + change['tasks'][0]['data'] = {'exit-code': 0} + client.responses.append({'result': change}) + + with tempfile.TemporaryFile() as stdin: + stdin.write(b'foo\n') + stdin.seek(0) + process = client.exec(['foo'], stdin=stdin, encoding=None) + process.wait() + assert process._cancel_stdin is None + assert process._cancel_reader is None + # Must not raise (e.g. OSError from closing the fd a second time). + process._wait() + + def test_connect_websocket_error_closes_connected_websockets(self): + """If a websocket fails to connect, the connected ones must be closed.""" + + class Client(MockClient): + def _connect_websocket(self, task_id: str, websocket_id: str): + if websocket_id == 'stderr': + raise websocket.WebSocketException('conn!') + return super()._connect_websocket(task_id, websocket_id) + + client = Client() + stdio, stderr, control = self.add_responses(client, '123', 0, change_err='change error!') + with pytest.raises(pebble.ChangeError): + client.exec(['foo']) + assert control.shutdown_calls == 1 + assert stdio.shutdown_calls == 1 + assert stderr.shutdown_calls == 0 + + def test_websocket_reader_eof_on_closed_connection(self): + """A websocket closed underneath a reader is EOF, not an error. + + The reader is read from threads (shutil.copyfileobj in wait_output) + and from user code streaming process.stdout, and the websocket may be + torn down by ExecProcess._wait at any point, most notably when the + wait fails (canonical/operator#2556). + """ + ws = MockWebsocket() + + def recv(): + raise websocket.WebSocketConnectionClosedException('socket is already closed.') + + ws.recv = recv + reader = pebble._WebsocketReader(typing.cast('pebble._WebSocket', ws)) + assert reader.read() == b'' + assert reader.read() == b'' # Still EOF when called again. + + def test_websocket_to_writer_stops_on_closed_connection(self): + ws = MockWebsocket() + chunks: list[bytes] = [b'foo'] + + def recv(): + if chunks: + return chunks.pop(0) + raise websocket.WebSocketConnectionClosedException('socket is already closed.') + + ws.recv = recv + out = io.BytesIO() + pebble._websocket_to_writer( + typing.cast('pebble._WebSocket', ws), + typing.cast('pebble._WebsocketWriter', out), + None, + ) # Must return cleanly rather than raise. + assert out.getvalue() == b'foo' + + def test_reader_to_websocket_stops_on_closed_connection(self): + ws = MockWebsocket() + + def send_binary(b: bytes): + raise websocket.WebSocketConnectionClosedException('socket is already closed.') + + ws.send_binary = send_binary + reader = io.BytesIO(b'foo') + pebble._reader_to_websocket( + typing.cast('pebble._WebsocketReader', reader), + typing.cast('pebble._WebSocket', ws), + 'utf-8', + ) # Must return cleanly rather than raise. + class TestIdentity: def test_local_identity_from_dict(self): diff --git a/test/test_real_pebble.py b/test/test_real_pebble.py index 928ad7c56..8a5a0f361 100644 --- a/test/test_real_pebble.py +++ b/test/test_real_pebble.py @@ -223,6 +223,24 @@ def test_exec_timeout(self, client: pebble.Client): process.wait() assert 'timed out' in excinfo.value.err + def test_exec_wait_timeout_reaps_io_threads(self, client: pebble.Client): + # Regression test for canonical/operator#2556. + # + # Repro: the orphaned grandchild keeps the exec's stdio pipe open, so + # the change can't finish even after Pebble kills the command at the + # 0.1s timeout, and the wait then times out client-side. + # + # Before the fix, the I/O threads were left blocked on the websockets, + # and being non-daemon they hung interpreter shutdown. This test + # asserts that after wait_output() raises TimeoutError, the I/O + # threads exit promptly. + process = client.exec(['/bin/sh', '-c', 'setsid sleep 3 & sleep 3'], timeout=0.1) + with pytest.raises(pebble.TimeoutError): + process.wait_output() + for thread in process._threads: + thread.join(timeout=2) + assert [t.name for t in process._threads if t.is_alive()] == [] + def test_exec_working_dir(self, client: pebble.Client): with tempfile.TemporaryDirectory() as temp_dir: process = client.exec(['pwd'], working_dir=temp_dir) From b98d64d966d853b030be928100081f901c9a07b9 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 27 May 2026 11:27:47 +1200 Subject: [PATCH 07/10] fix: pass the endpoint name through to relation-get (#2499) This PR passes the endpoint name from the Relation down to the actual hook command, where Juju will use it to validate that it matches the provided relation ID. Without this, you can trick Ops into a mismatch between ID and name and get misleading data (although Juju enforces security, so there is no leakage of data that you couldn't get anyway). This is mostly just plumbing the extra information through the backend and into the hookcmds call from the various places we call this command. There are also a bunch of test updates to include the relation name to match. Scenario is updated to reflect the Juju behaviour more closely, and otherwise expect / accept the name in calls. I've updated Harness to match, even though we are not typically doing Harness fixes, because the signature changes and it seems less likely we would break anyone's existing Harness tests this way, and it's a pretty straightforward set of changes. Fixes #2327 --------- Co-authored-by: James Garner Co-authored-by: Claude Opus 4.7 (1M context) (cherry picked from commit 208af9b80b54bd93200c5a9c13a1b7db82672976) --- ops/_private/harness.py | 46 +++++++++-- ops/model.py | 71 ++++++++++++---- test/bin/relation-list | 73 +++++++++++++++-- test/test_model.py | 140 ++++++++++++++++++++------------ test/test_testing.py | 35 +++++--- testing/src/scenario/mocking.py | 60 ++++++++++---- 6 files changed, 316 insertions(+), 109 deletions(-) diff --git a/ops/_private/harness.py b/ops/_private/harness.py index e80d79b9a..5c95becbc 100644 --- a/ops/_private/harness.py +++ b/ops/_private/harness.py @@ -2440,19 +2440,41 @@ def relation_ids(self, relation_name: str) -> list[int]: no_ids: list[int] = [] return no_ids - def relation_list(self, relation_id: int): + def _check_relation_name(self, relation_id: int, relation_name: str | None) -> None: + """Mimic Juju's ``endpoint:id`` validation by erroring on a mismatch.""" + if relation_name is None: + return + actual = self._relation_names.get(relation_id) + if actual is None or actual != relation_name: + raise model.RelationNotFoundError() + + def relation_list(self, relation_id: int, *, relation_name: str | None = None): + self._check_relation_name(relation_id, relation_name) try: return self._relation_list_map[relation_id] except KeyError: raise model.RelationNotFoundError from None - def relation_remote_app_name(self, relation_id: int) -> str | None: + def relation_remote_app_name( + self, relation_id: int, *, relation_name: str | None = None + ) -> str | None: + if relation_name is not None and self._relation_names.get(relation_id) != relation_name: + # Mismatched endpoint:id -- treat as if the relation does not exist. + return None if relation_id not in self._relation_app_and_units: # Non-existent or dead relation return None return self._relation_app_and_units[relation_id]['app'] - def relation_get(self, relation_id: int, member_name: str, is_app: bool): + def relation_get( + self, + relation_id: int, + member_name: str, + is_app: bool, + *, + relation_name: str | None = None, + ): + self._check_relation_name(relation_id, relation_name) if is_app and '/' in member_name: member_name = member_name.split('/')[0] if relation_id not in self._relation_data_raw: @@ -2464,8 +2486,11 @@ def update_relation_data( relation_id: int, entity: model.Unit | model.Application, data: Mapping[str, str], + *, + relation_name: str | None = None, ): # this is where the 'real' backend would call relation-set. + self._check_relation_name(relation_id, relation_name) raw_data = self._relation_data_raw[relation_id][entity.name] for key, value in data.items(): if value == '': @@ -2473,10 +2498,18 @@ def update_relation_data( else: raw_data[key] = value - def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) -> None: + def relation_set( + self, + relation_id: int, + data: Mapping[str, str], + is_app: bool, + *, + relation_name: str | None = None, + ) -> None: if not isinstance(is_app, bool): raise TypeError('is_app parameter to relation_set must be a boolean') + self._check_relation_name(relation_id, relation_name) if relation_id not in self._relation_data_raw: raise RelationNotFoundError(relation_id) @@ -2491,8 +2524,11 @@ def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) else: bucket[key] = value - def relation_model_get(self, relation_id: int) -> dict[str, Any]: + def relation_model_get( + self, relation_id: int, *, relation_name: str | None = None + ) -> dict[str, Any]: # For Harness, ignore relation_id and assume relation is never cross-model. + self._check_relation_name(relation_id, relation_name) return {'uuid': self.model_uuid} def config_get(self) -> _TestingConfig: diff --git a/ops/model.py b/ops/model.py index 0f03dc12f..cbb3847d2 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1755,7 +1755,7 @@ def __init__( app = our_unit.app if is_peer else None try: - for unit_name in backend.relation_list(self.id): + for unit_name in backend.relation_list(self.id, relation_name=relation_name): unit = cache.get(Unit, unit_name) self.units.add(unit) if app is None: @@ -1768,7 +1768,7 @@ def __init__( # If we didn't get the remote app via our_unit.app or the units list, # look it up via JUJU_REMOTE_APP or "relation-list --app". if app is None: - app_name = backend.relation_remote_app_name(relation_id) + app_name = backend.relation_remote_app_name(self.id, relation_name=relation_name) if app_name is not None: app = cache.get(Application, app_name) @@ -1808,7 +1808,7 @@ def remote_model(self) -> RemoteModel: "relation-model-get" hook tool. """ if self._remote_model is None: - d = self._backend.relation_model_get(self.id) + d = self._backend.relation_model_get(self.id, relation_name=self.name) self._remote_model = RemoteModel(uuid=d['uuid']) return self._remote_model @@ -2055,7 +2055,12 @@ def _hook_is_running(self) -> bool: def _load(self) -> _RelationDataContent_Raw: """Load the data from the current entity / relation.""" try: - return self._backend.relation_get(self.relation.id, self._entity.name, self._is_app) + return self._backend.relation_get( + self.relation.id, + self._entity.name, + self._is_app, + relation_name=self.relation.name, + ) except RelationNotFoundError: # Dead relations tell no tales (and have no data). return {} @@ -2157,7 +2162,10 @@ def __setitem__(self, key: str, value: str): def _commit(self, data: Mapping[str, str]) -> None: self._backend.update_relation_data( - relation_id=self.relation.id, entity=self._entity, data=data + relation_id=self.relation.id, + entity=self._entity, + data=data, + relation_name=self.relation.name, ) def _update_cache(self, data: Mapping[str, str]) -> None: @@ -3665,10 +3673,11 @@ def relation_ids(self, relation_name: str) -> list[int]: relation_ids = typing.cast('Iterable[str]', relation_ids) return [int(relation_id.split(':')[-1]) for relation_id in relation_ids] - def relation_list(self, relation_id: int) -> list[str]: + def relation_list(self, relation_id: int, *, relation_name: str | None = None) -> list[str]: + relation = f'{relation_name}:{relation_id}' if relation_name else str(relation_id) try: rel_list = self._run( - 'relation-list', '-r', str(relation_id), return_output=True, use_json=True + 'relation-list', '-r', relation, return_output=True, use_json=True ) return typing.cast('list[str]', rel_list) except ModelError as e: @@ -3676,7 +3685,9 @@ def relation_list(self, relation_id: int) -> list[str]: raise RelationNotFoundError() from e raise - def relation_remote_app_name(self, relation_id: int) -> str | None: + def relation_remote_app_name( + self, relation_id: int, *, relation_name: str | None = None + ) -> str | None: """Return remote app name for given relation ID, or None if not known.""" if ( self._juju_context.relation_id is not None @@ -3690,8 +3701,9 @@ def relation_remote_app_name(self, relation_id: int) -> str | None: # If caller is asking for information about another relation, use # "relation-list --app" to get it. try: + relation = f'{relation_name}:{relation_id}' if relation_name else str(relation_id) rel_id = self._run( - 'relation-list', '-r', str(relation_id), '--app', return_output=True, use_json=True + 'relation-list', '-r', relation, '--app', return_output=True, use_json=True ) # if it returned anything at all, it's a str. return typing.cast('str', rel_id) @@ -3706,7 +3718,12 @@ def relation_remote_app_name(self, relation_id: int) -> str | None: raise def relation_get( - self, relation_id: int, member_name: str, is_app: bool + self, + relation_id: int, + member_name: str, + is_app: bool, + *, + relation_name: str | None = None, ) -> _RelationDataContent_Raw: if not isinstance(is_app, bool): raise TypeError('is_app parameter to relation_get must be a boolean') @@ -3717,7 +3734,8 @@ def relation_get( f'{self._juju_context.version}' ) - args = ['relation-get', '-r', str(relation_id), '-', member_name] + relation = f'{relation_name}:{relation_id}' if relation_name else str(relation_id) + args = ['relation-get', '-r', relation, '-', member_name] if is_app: args.append('--app') @@ -3729,7 +3747,14 @@ def relation_get( raise RelationNotFoundError() from e raise - def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) -> None: + def relation_set( + self, + relation_id: int, + data: Mapping[str, str], + is_app: bool, + *, + relation_name: str | None = None, + ) -> None: if not data: raise ValueError('at least one key:value pair is required for relation-set') if not isinstance(is_app, bool): @@ -3741,7 +3766,8 @@ def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) f'{self._juju_context.version}' ) - args = ['relation-set', '-r', str(relation_id)] + relation = f'{relation_name}:{relation_id}' if relation_name else str(relation_id) + args = ['relation-set', '-r', relation] if is_app: args.append('--app') args.extend(['--file', '-']) @@ -3754,8 +3780,11 @@ def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) raise RelationNotFoundError() from e raise - def relation_model_get(self, relation_id: int) -> dict[str, Any]: - args = ['relation-model-get', '-r', str(relation_id)] + def relation_model_get( + self, relation_id: int, *, relation_name: str | None = None + ) -> dict[str, Any]: + relation = f'{relation_name}:{relation_id}' if relation_name else str(relation_id) + args = ['relation-model-get', '-r', relation] try: result = self._run(*args, return_output=True, use_json=True) return typing.cast('dict[str, Any]', result) @@ -4002,10 +4031,18 @@ def planned_units(self) -> int: return num_alive def update_relation_data( - self, relation_id: int, entity: Unit | Application, data: Mapping[str, str] + self, + relation_id: int, + entity: Unit | Application, + data: Mapping[str, str], + *, + relation_name: str | None = None, ): self.relation_set( - relation_id=relation_id, data=data, is_app=isinstance(entity, Application) + relation_id=relation_id, + data=data, + is_app=isinstance(entity, Application), + relation_name=relation_name, ) def secret_get( diff --git a/test/bin/relation-list b/test/bin/relation-list index 884901597..459600db3 100755 --- a/test/bin/relation-list +++ b/test/bin/relation-list @@ -5,12 +5,67 @@ fail_not_found() { exit 2 } -case $2 in - 1) echo '["remote/0"]' ;; - 2) echo '["remote/0"]' ;; - 3) fail_not_found $2 ;; - 4) echo '["remoteapp1/0"]' ;; - 5) echo '["remoteapp1/0"]' ;; - 6) echo '["remoteapp2/0"]' ;; - *) fail_not_found $2 ;; -esac +format_json=false +app_flag=false +relation_id="" + +while [[ $# -gt 0 ]]; do + case $1 in + --format=json) + format_json=true + shift + ;; + --app) + app_flag=true + shift + ;; + -r) + if [[ -n "$2" ]]; then + relation_id="$2" + shift 2 + else + echo "ERROR: -r requires a value" >&2 + exit 1 + fi + ;; + *) + # For backward compatibility, treat second positional argument as relation ID + if [[ -z "$relation_id" && "$1" =~ ^[0-9]+$ ]]; then + relation_id="$1" + fi + shift + ;; + esac +done + +get_relation_data() { + local rel_id="$1" + # Strip the optional ``endpoint:`` prefix so the case-statement keeps working. + # For example, db:1 -> 1. + rel_id="${rel_id##*:}" + case $rel_id in + 1) echo "remote/0" ;; + 2) echo "remote/0" ;; + 3) fail_not_found $rel_id ;; + 4) echo "remoteapp1/0" ;; + 5) echo "remoteapp1/0" ;; + 6) echo "remoteapp2/0" ;; + *) fail_not_found $rel_id ;; + esac +} + +if [[ -n "$relation_id" ]]; then + data=$(get_relation_data "$relation_id") + if [[ "$format_json" == true ]]; then + if [[ "$app_flag" == true ]]; then + echo "\"$data\"" + else + echo "[\"$data\"]" + fi + else + echo "$data" + fi +else + echo "ERROR: relation ID required" >&2 + exit 1 +fi diff --git a/test/test_model.py b/test/test_model.py index 04f47af73..fc1f9a275 100644 --- a/test/test_model.py +++ b/test/test_model.py @@ -138,8 +138,8 @@ def test_relations_keys(self, harness: ops.testing.Harness[ops.CharmBase]): harness, [ ('relation_ids', 'db1'), - ('relation_list', rel_app1), - ('relation_list', rel_app2), + ('relation_list', rel_app1, {'relation_name': 'db1'}), + ('relation_list', rel_app2, {'relation_name': 'db1'}), ], ) @@ -168,7 +168,7 @@ def test_get_relation(self, harness: ops.testing.Harness[ops.CharmBase]): harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id_db1), + ('relation_list', relation_id_db1, {'relation_name': 'db1'}), ], ) dead_rel = self.ensure_relation(harness, 'db1', 7) @@ -178,9 +178,9 @@ def test_get_relation(self, harness: ops.testing.Harness[ops.CharmBase]): self.assertBackendCalls( harness, [ - ('relation_list', 7), - ('relation_remote_app_name', 7), - ('relation_get', 7, 'myapp/0', False), + ('relation_list', 7, {'relation_name': 'db1'}), + ('relation_remote_app_name', 7, {'relation_name': 'db1'}), + ('relation_get', 7, 'myapp/0', False, {'relation_name': 'db1'}), ], ) @@ -199,13 +199,29 @@ def test_get_relation(self, harness: ops.testing.Harness[ops.CharmBase]): harness, [ ('relation_ids', 'db0'), - ('relation_list', relation_id_db0), - ('relation_remote_app_name', 0), - ('relation_list', relation_id_db0_b), - ('relation_remote_app_name', 2), + ('relation_list', relation_id_db0, {'relation_name': 'db0'}), + ('relation_remote_app_name', 0, {'relation_name': 'db0'}), + ('relation_list', relation_id_db0_b, {'relation_name': 'db0'}), + ('relation_remote_app_name', 2, {'relation_name': 'db0'}), ], ) + def test_get_relation_wrong_endpoint(self, harness: ops.testing.Harness[ops.CharmBase]): + # Regression test for https://github.com/canonical/operator/issues/2327 + # When the endpoint name doesn't match the relation_id, Juju treats the + # relation as if it doesn't exist; ops should model that cleanly. + rel_id = harness.add_relation('db1', 'remoteapp1') + harness.add_relation_unit(rel_id, 'remoteapp1/0') + with harness._event_context('foo_event'): + harness.update_relation_data(rel_id, 'remoteapp1', {'host': 'remoteapp1-0'}) + + wrong = harness.model.get_relation('db0', rel_id) + assert isinstance(wrong, ops.Relation) + # The relation looks dead from the charm's point of view. + assert wrong.active is False + assert wrong.data[harness.model.unit] == {} + assert wrong.data[harness.model.app] == {} + def test_peer_relation_app(self, harness: ops.testing.Harness[ops.CharmBase]): harness.add_relation('db2', 'myapp') rel_dbpeer = self.ensure_relation(harness, 'db2') @@ -225,7 +241,7 @@ def test_remote_units_is_our(self, harness: ops.testing.Harness[ops.CharmBase]): harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), + ('relation_list', relation_id, {'relation_name': 'db1'}), ], ) @@ -298,8 +314,8 @@ def test_unit_relation_data(self, harness: ops.testing.Harness[ops.CharmBase]): harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'remoteapp1/0', False), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'remoteapp1/0', False, {'relation_name': 'db1'}), ], ) @@ -326,8 +342,8 @@ def test_remote_app_relation_data(self, harness: ops.testing.Harness[ops.CharmBa harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'remoteapp1', True), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'remoteapp1', True, {'relation_name': 'db1'}), ], ) @@ -357,8 +373,8 @@ def test_relation_data_modify_remote(self, harness: ops.testing.Harness[ops.Char harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'remoteapp1/0', False), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'remoteapp1/0', False, {'relation_name': 'db1'}), ], ) @@ -390,13 +406,14 @@ def test_relation_data_modify_our(self, harness: ops.testing.Harness[ops.CharmBa self.assertBackendCalls( harness, [ - ('relation_get', relation_id, 'myapp/0', False), + ('relation_get', relation_id, 'myapp/0', False, {'relation_name': 'db1'}), ( 'update_relation_data', { 'relation_id': relation_id, 'entity': harness.model.unit, 'data': {'host': 'bar'}, + 'relation_name': 'db1', }, ), ], @@ -424,11 +441,16 @@ def test_app_relation_data_modify_local_as_leader( harness, [ ('relation_ids', 'db1'), - ('relation_list', 0), - ('relation_get', 0, 'myapp', True), + ('relation_list', 0, {'relation_name': 'db1'}), + ('relation_get', 0, 'myapp', True, {'relation_name': 'db1'}), ( 'update_relation_data', - {'relation_id': 0, 'entity': harness.model.app, 'data': {'password': 'foo'}}, + { + 'relation_id': 0, + 'entity': harness.model.app, + 'data': {'password': 'foo'}, + 'relation_name': 'db1', + }, ), ], ) @@ -457,8 +479,8 @@ def test_app_relation_data_modify_local_as_minion( harness, [ ('relation_ids', 'db1'), - ('relation_list', 0), - ('relation_get', 0, 'myapp', True), + ('relation_list', 0, {'relation_name': 'db1'}), + ('relation_get', 0, 'myapp', True, {'relation_name': 'db1'}), ('is_leader',), ], ) @@ -503,14 +525,15 @@ def test_relation_data_del_key(self, harness: ops.testing.Harness[ops.CharmBase] harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'myapp/0', False), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'myapp/0', False, {'relation_name': 'db1'}), ( 'update_relation_data', { 'relation_id': relation_id, 'entity': harness.model.unit, 'data': {'host': ''}, + 'relation_name': 'db1', }, ), ], @@ -536,8 +559,8 @@ def test_relation_data_del_missing_key(self, harness: ops.testing.Harness[ops.Ch harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'myapp/0', False), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'myapp/0', False, {'relation_name': 'db1'}), ], ) @@ -558,8 +581,15 @@ def broken_update_relation_data( relation_id: int, entity: ops.Unit | ops.Application, data: Mapping[str, str], + relation_name: str | None = None, ): - backend._calls.append(('update_relation_data', relation_id, entity, data)) + backend._calls.append(( + 'update_relation_data', + relation_id, + entity, + data, + relation_name, + )) raise ops.ModelError() backend.update_relation_data = broken_update_relation_data @@ -580,10 +610,10 @@ def broken_update_relation_data( harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'myapp/0', False), - ('update_relation_data', relation_id, harness.model.unit, {'host': 'bar'}), - ('update_relation_data', relation_id, harness.model.unit, {'host': ''}), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'myapp/0', False, {'relation_name': 'db1'}), + ('update_relation_data', relation_id, harness.model.unit, {'host': 'bar'}, 'db1'), + ('update_relation_data', relation_id, harness.model.unit, {'host': ''}, 'db1'), ], ) @@ -615,8 +645,8 @@ def test_relation_data_type_check(self, harness: ops.testing.Harness[ops.CharmBa harness, [ ('relation_ids', 'db1'), - ('relation_list', relation_id), - ('relation_get', relation_id, 'myapp/0', False), + ('relation_list', relation_id, {'relation_name': 'db1'}), + ('relation_get', relation_id, 'myapp/0', False, {'relation_name': 'db1'}), ], ) @@ -655,7 +685,7 @@ def test_relation_local_app_data_readability_leader( harness, [ ('is_leader',), - ('relation_get', 0, 'myapp', True), + ('relation_get', 0, 'myapp', True, {'relation_name': 'db1'}), ], ) @@ -710,11 +740,11 @@ def test_relation_local_app_data_readability_follower( assert isinstance(repr(rel_db1.data), str) expected_backend_calls = [ - ('relation_get', 0, 'myapp/0', False), + ('relation_get', 0, 'myapp/0', False, {'relation_name': 'db1'}), ('is_leader',), - ('relation_get', 0, 'remoteapp1/0', False), + ('relation_get', 0, 'remoteapp1/0', False, {'relation_name': 'db1'}), ('is_leader',), - ('relation_get', 0, 'remoteapp1', True), + ('relation_get', 0, 'remoteapp1', True, {'relation_name': 'db1'}), ] self.assertBackendCalls(harness, expected_backend_calls) @@ -727,8 +757,8 @@ def test_relation_no_units(self, harness: ops.testing.Harness[ops.CharmBase]): harness, [ ('relation_ids', 'db1'), - ('relation_list', 0), - ('relation_remote_app_name', 0), + ('relation_list', 0, {'relation_name': 'db1'}), + ('relation_remote_app_name', 0, {'relation_name': 'db1'}), ], ) @@ -785,7 +815,7 @@ def check_remote_units(): [ ('is_leader',), ('relation_ids', 'db1'), - ('relation_list', relation_id), + ('relation_list', relation_id, {'relation_name': 'db1'}), ('is_leader',), ], ) @@ -1202,8 +1232,8 @@ def test_relation_remote_model(self, fake_script: FakeScript, fake_juju_version: assert fake_script.calls() == [ ['relation-ids', 'db', '--format=json'], - ['relation-list', '-r', '1', '--format=json'], - ['relation-model-get', '-r', '1', '--format=json'], + ['relation-list', '-r', 'db:1', '--format=json'], + ['relation-model-get', '-r', 'db:1', '--format=json'], ] @@ -2437,7 +2467,9 @@ def model(self, fake_script: FakeScript, fake_juju_version: None): model = ops.Model(meta, backend) fake_script.write('relation-ids', """([ "$1" = db0 ] && echo '["db0:4"]') || echo '[]'""") - fake_script.write('relation-list', """[ "$2" = 4 ] && echo '["remoteapp1/0"]' || exit 2""") + fake_script.write( + 'relation-list', """[ "$2" = db0:4 ] && echo '["remoteapp1/0"]' || exit 2""" + ) self.network_get_out = """{ "bind-addresses": [ { @@ -2618,7 +2650,7 @@ def test_binding_by_relation(self, fake_script: FakeScript, model: ops.Model): expected_calls = [ ['relation-ids', 'db0', '--format=json'], # The two invocations below are due to the get_relation call. - ['relation-list', '-r', '4', '--format=json'], + ['relation-list', '-r', 'db0:4', '--format=json'], ['network-get', 'db0', '-r', '4', '--format=json'], ] binding = self.ensure_binding(model, self.ensure_relation(model, binding_name)) @@ -4002,8 +4034,8 @@ def test_grant(self, model: ops.Model, fake_script: FakeScript): assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ - ['relation-list', '-r', '123', '--format=json'], - ['relation-list', '-r', '234', '--format=json'], + ['relation-list', '-r', 'test:123', '--format=json'], + ['relation-list', '-r', 'test:234', '--format=json'], ['secret-grant', f'secret://{model._backend.model_uuid}/x', '--relation', '123'], [ 'secret-grant', @@ -4013,7 +4045,7 @@ def test_grant(self, model: ops.Model, fake_script: FakeScript): '--unit', 'app/0', ], - ['relation-list', '-r', '345', '--format=json'], + ['relation-list', '-r', 'test:345', '--format=json'], ['secret-info-get', '--label', 'y', '--format=json'], ['secret-grant', f'secret://{model._backend.model_uuid}/z', '--relation', '345'], ] @@ -4039,8 +4071,8 @@ def test_revoke(self, model: ops.Model, fake_script: FakeScript): assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ - ['relation-list', '-r', '123', '--format=json'], - ['relation-list', '-r', '234', '--format=json'], + ['relation-list', '-r', 'test:123', '--format=json'], + ['relation-list', '-r', 'test:234', '--format=json'], ['secret-revoke', f'secret://{model._backend.model_uuid}/x', '--relation', '123'], [ 'secret-revoke', @@ -4050,7 +4082,7 @@ def test_revoke(self, model: ops.Model, fake_script: FakeScript): '--unit', 'app/0', ], - ['relation-list', '-r', '345', '--format=json'], + ['relation-list', '-r', 'test:345', '--format=json'], ['secret-info-get', '--label', 'y', '--format=json'], ['secret-revoke', f'secret://{model._backend.model_uuid}/z', '--relation', '345'], ] @@ -4451,10 +4483,10 @@ def test_departing_unit_data_available(fake_script: FakeScript): calls = fake_script.calls(clear=True) assert calls[:2] == [ ['relation-ids', 'db', '--format=json'], - ['relation-list', '-r', '1', '--format=json'], + ['relation-list', '-r', 'db:1', '--format=json'], ] - assert ['relation-get', '-r', '1', '-', 'db/0', '--format=json'] in calls - assert ['relation-get', '-r', '1', '-', 'db/1', '--format=json'] in calls + assert ['relation-get', '-r', 'db:1', '-', 'db/0', '--format=json'] in calls + assert ['relation-get', '-r', 'db:1', '-', 'db/1', '--format=json'] in calls if __name__ == '__main__': diff --git a/test/test_testing.py b/test/test_testing.py index 6bf7d833a..22c25b12f 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -2277,18 +2277,23 @@ def test_get_backend_calls(self, request: pytest.FixtureRequest): assert harness._get_backend_calls() == [ ('relation_ids', 'db'), - ('relation_list', rel_id), - ('relation_remote_app_name', 0), + ('relation_list', rel_id, {'relation_name': 'db'}), + ('relation_remote_app_name', 0, {'relation_name': 'db'}), ] # update_relation_data ensures the cached data for the relation is wiped harness.update_relation_data(rel_id, 'test-charm/0', {'foo': 'bar'}) test_charm_unit = harness.model.get_unit('test-charm/0') assert harness._get_backend_calls(reset=True) == [ - ('relation_get', 0, 'test-charm/0', False), + ('relation_get', 0, 'test-charm/0', False, {'relation_name': 'db'}), ( 'update_relation_data', - {'relation_id': 0, 'entity': test_charm_unit, 'data': {'foo': 'bar'}}, + { + 'relation_id': 0, + 'entity': test_charm_unit, + 'data': {'foo': 'bar'}, + 'relation_name': 'db', + }, ), ] @@ -2302,21 +2307,31 @@ def test_get_backend_calls(self, request: pytest.FixtureRequest): assert harness._get_backend_calls(reset=False) == [ ('relation_ids', 'db'), - ('relation_list', rel_id), - ('relation_get', 0, 'postgresql/0', False), + ('relation_list', rel_id, {'relation_name': 'db'}), + ('relation_get', 0, 'postgresql/0', False, {'relation_name': 'db'}), ( 'update_relation_data', - {'relation_id': 0, 'entity': pgql_unit, 'data': {'foo': 'bar'}}, + { + 'relation_id': 0, + 'entity': pgql_unit, + 'data': {'foo': 'bar'}, + 'relation_name': 'db', + }, ), ] # If we check again, they are still there, but now we reset it assert harness._get_backend_calls(reset=True) == [ ('relation_ids', 'db'), - ('relation_list', rel_id), - ('relation_get', 0, 'postgresql/0', False), + ('relation_list', rel_id, {'relation_name': 'db'}), + ('relation_get', 0, 'postgresql/0', False, {'relation_name': 'db'}), ( 'update_relation_data', - {'relation_id': 0, 'entity': pgql_unit, 'data': {'foo': 'bar'}}, + { + 'relation_id': 0, + 'entity': pgql_unit, + 'data': {'foo': 'bar'}, + 'relation_name': 'db', + }, ), ] # And the calls are gone diff --git a/testing/src/scenario/mocking.py b/testing/src/scenario/mocking.py index 87476051f..77f5fad8b 100644 --- a/testing/src/scenario/mocking.py +++ b/testing/src/scenario/mocking.py @@ -200,11 +200,15 @@ def get_pebble(self, socket_path: str) -> Client: ), ) - def _get_relation_by_id(self, rel_id: int) -> RelationBase: + def _get_relation_by_id(self, rel_id: int, relation_name: str | None = None) -> RelationBase: try: - return self._state.get_relation(rel_id) + relation = self._state.get_relation(rel_id) except KeyError: raise RelationNotFoundError() from None + if relation_name is not None and relation.endpoint != relation_name: + # Mimic Juju's behaviour when given a mismatched ``endpoint:id``. + raise RelationNotFoundError() + return relation def _get_secret(self, id: str | None = None, label: str | None = None): if JujuVersion(self._context.juju_version) < '3.0.2': @@ -252,13 +256,28 @@ def _check_app_data_access(self, is_app: bool): f'setting application data is not supported on Juju version {version}', ) - def relation_get(self, relation_id: int, member_name: str, is_app: bool): + def relation_get( + self, + relation_id: int, + member_name: str, + is_app: bool, + *, + relation_name: str | None = None, + ): self._check_app_data_access(is_app) - data = self._relation_get(relation_id, member_name=member_name, is_app=is_app) + data = self._relation_get( + relation_id, member_name=member_name, is_app=is_app, relation_name=relation_name + ) return data.copy() - def _relation_get(self, relation_id: int, member_name: str, is_app: bool): - relation = self._get_relation_by_id(relation_id) + def _relation_get( + self, + relation_id: int, + member_name: str, + is_app: bool, + relation_name: str | None = None, + ): + relation = self._get_relation_by_id(relation_id, relation_name) if is_app and member_name == self.app_name: return relation.local_app_data if is_app: @@ -273,11 +292,13 @@ def _relation_get(self, relation_id: int, member_name: str, is_app: bool): unit_id = int(member_name.split('/')[-1]) return relation._get_databag_for_remote(unit_id) # noqa - def relation_model_get(self, relation_id: int) -> dict[str, Any]: + def relation_model_get( + self, relation_id: int, *, relation_name: str | None = None + ) -> dict[str, Any]: if JujuVersion(self._context.juju_version) < '3.6.2': raise ModelError('Relation.remote_model is only available on Juju >= 3.6.2') - relation = self._get_relation_by_id(relation_id) + relation = self._get_relation_by_id(relation_id, relation_name) # Only Relation has remote_model_uuid, not the other subclasses of RelationBase. if isinstance(relation, Relation) and relation.remote_model_uuid is not None: uuid = relation.remote_model_uuid @@ -295,8 +316,10 @@ def status_get(self, *, is_app: bool = False): def relation_ids(self, relation_name: str): return [rel.id for rel in self._state.relations if rel.endpoint == relation_name] - def relation_list(self, relation_id: int) -> tuple[str, ...]: - relation = self._get_relation_by_id(relation_id) + def relation_list( + self, relation_id: int, *, relation_name: str | None = None + ) -> tuple[str, ...]: + relation = self._get_relation_by_id(relation_id, relation_name) if isinstance(relation, PeerRelation): # The current unit should never be in `peers_data`, and there is a @@ -308,7 +331,7 @@ def relation_list(self, relation_id: int) -> tuple[str, ...]: for unit_id in relation.peers_data if unit_id != this_unit ) - remote_name = self.relation_remote_app_name(relation_id) + remote_name = self.relation_remote_app_name(relation_id, relation_name=relation_name) return tuple(f'{remote_name}/{unit_id}' for unit_id in relation._remote_unit_ids) def config_get(self): @@ -392,7 +415,14 @@ def status_set( def juju_log(self, level: str, message: str): self._context.juju_log.append(JujuLogLine(level, message)) - def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) -> None: + def relation_set( + self, + relation_id: int, + data: Mapping[str, str], + is_app: bool, + *, + relation_name: str | None = None, + ) -> None: self._check_app_data_access(is_app) # NOTE: The code below currently does not have any effect, because # the dictionary has already had the same set/delete operations @@ -400,7 +430,7 @@ def relation_set(self, relation_id: int, data: Mapping[str, str], is_app: bool) # where this method calls out to Juju's relation-set to operate on # the real databag, this method currently operates on the same # dictionary object that RelationDataContent does. - relation = self._get_relation_by_id(relation_id) + relation = self._get_relation_by_id(relation_id, relation_name) if is_app: if not self._state.leader: # will in practice not be reached because RelationData will check leadership @@ -601,11 +631,13 @@ def secret_remove(self, id: str, *, revision: int | None = None): def relation_remote_app_name( self, relation_id: int, + *, + relation_name: str | None = None, _raise_on_error: bool = False, ) -> str | None: # ops catches RelationNotFoundErrors and returns None: try: - relation = self._get_relation_by_id(relation_id) + relation = self._get_relation_by_id(relation_id, relation_name) except RelationNotFoundError: if _raise_on_error: raise From 40d1936732fdc7fb3b27a9fdd9e9c366301b6962 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 29 Jun 2026 18:57:44 +1200 Subject: [PATCH 08/10] ci: bump postgresql-k8s charm pin to fix Data Charm Tests The cherry-picked #2507 ("close SQLite storage in Harness.cleanup()") makes Harness.cleanup() close the framework's SQLite connection. The stale postgresql-k8s pin (a6753b27) had a test that called harness.cleanup() before harness.begin(), which now fails with "Cannot operate on a closed database". Bump the pin to c434df80 (the commit main already uses), where the charm reordered the test to begin() then cleanup(). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/db-charm-tests.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/db-charm-tests.yaml b/.github/workflows/db-charm-tests.yaml index 7c3472e31..95a2432c0 100644 --- a/.github/workflows/db-charm-tests.yaml +++ b/.github/workflows/db-charm-tests.yaml @@ -19,7 +19,7 @@ jobs: - charm-repo: canonical/postgresql-operator commit: 33ca7f308fa4289cdf45ceefcb335f373713431d # 2025-06-21T22:22:22Z - charm-repo: canonical/postgresql-k8s-operator - commit: a6753b27440a17ed5f37d4c2c5c6f53b1d3a1f7f # 2025-06-22T11:27:17Z + commit: c434df80953409f290e7ea6e76f78a3ec46cecc2 # 2026-05-30T03:35:40Z - charm-repo: canonical/mysql-operator commit: 411fb45ecfe200d22b46af9cbacaf72e8eb09da7 # 2025-06-23T14:51:47Z - charm-repo: canonical/mysql-k8s-operator From 5a7cdeaa115dca847a5e0d0b26a77139b058170a Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 30 Jun 2026 12:34:57 +1200 Subject: [PATCH 09/10] ci: build ops_tracing wheel for Data Charm Tests Bumping the postgresql-k8s pin to c434df80 (previous commit) fixed the SQLite-close failure but exposed a second one: that charm now declares the ops[tracing] extra, so its tests import ops_tracing. The stale db-charm-tests workflow only patched the local 'ops' dependency and never installed ops_tracing, giving "ModuleNotFoundError: No module named 'ops_tracing'". Backport main's machinery: a _build-wheels reusable workflow that runs `uv build --all` (producing ops and ops_tracing wheels) and the tracing-aware install step that adds the ops_tracing wheel when the charm asks for the extra. The other three charm pins are left at their known-good older commits since only postgresql-k8s needs tracing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/_build-wheels.yaml | 38 +++++++++++++++++++++++++++ .github/workflows/db-charm-tests.yaml | 37 ++++++++++++++++++++------ 2 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 .github/workflows/_build-wheels.yaml diff --git a/.github/workflows/_build-wheels.yaml b/.github/workflows/_build-wheels.yaml new file mode 100644 index 000000000..d76becaf4 --- /dev/null +++ b/.github/workflows/_build-wheels.yaml @@ -0,0 +1,38 @@ +name: Build Operator Wheels + +on: + workflow_call: + outputs: + artifact-name: + description: "Name of the artifact containing the wheels" + value: ${{ jobs.build-wheels.outputs.artifact-name }} + +permissions: {} + +jobs: + build-wheels: + runs-on: ubuntu-latest + outputs: + artifact-name: ${{ steps.artifact-name.outputs.name }} + steps: + - name: Checkout the operator repository + uses: actions/checkout@v6.0.2 + with: + persist-credentials: false + + - name: Set up uv + uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0 + + - name: Build wheels + run: uv build --all + + - name: Set artifact name + id: artifact-name + run: echo "name=operator-wheels-${{ github.sha }}" >> "$GITHUB_OUTPUT" + + - name: Upload wheels as artifact + uses: actions/upload-artifact@v7 + with: + name: ${{ steps.artifact-name.outputs.name }} + path: dist/*.whl + retention-days: 1 diff --git a/.github/workflows/db-charm-tests.yaml b/.github/workflows/db-charm-tests.yaml index 95a2432c0..fc8bf840d 100644 --- a/.github/workflows/db-charm-tests.yaml +++ b/.github/workflows/db-charm-tests.yaml @@ -6,12 +6,17 @@ on: - main pull_request: workflow_call: + workflow_dispatch: permissions: {} jobs: + build-wheels: + uses: ./.github/workflows/_build-wheels.yaml + db-charm-tests: runs-on: ubuntu-latest + needs: build-wheels strategy: fail-fast: false matrix: @@ -26,29 +31,45 @@ jobs: commit: 7b5775c9d07fd32c4453d93dfc51cf1b4d6c8024 # rev257 rev256 2025-06-23T15:19:19Z steps: - name: Checkout the ${{ matrix.charm-repo }} repository - uses: actions/checkout@v4 + uses: actions/checkout@v6.0.2 with: repository: ${{ matrix.charm-repo }} persist-credentials: false ref: ${{ matrix.commit }} - - name: Checkout the operator repository - uses: actions/checkout@v4 + - name: Download operator wheels + uses: actions/download-artifact@v8 with: - path: myops - persist-credentials: false + name: ${{ needs.build-wheels.outputs.artifact-name }} + path: operator-wheels - name: Install patch dependencies run: pip install poetry~=2.0 - name: Update 'ops' dependency in test charm to latest run: | + ops_wheel="$(ls operator-wheels/ops-[0-9]*.whl)" + ops_tracing_wheel="$(ls operator-wheels/ops_tracing-*.whl)" if [ -e "requirements.txt" ]; then sed -i -e "/^ops[ ><=]/d" -e "/canonical\/operator/d" -e "/#egg=ops/d" requirements.txt - echo -e "\ngit+$GITHUB_SERVER_URL/$GITHUB_REPOSITORY@$GITHUB_SHA#egg=ops" >> requirements.txt + echo -e "\n${ops_wheel}" >> requirements.txt else - sed -i -e "s/^ops[ ><=].*/ops = {path = \"myops\"}/" pyproject.toml - poetry lock + # The charm may declare ops with the 'tracing' extra, which pulls in + # ops-tracing from PyPI. Replacing ops with a local wheel drops the + # extra, so install the matching ops-tracing wheel explicitly when + # the charm asked for it. Check before `poetry remove`, which + # strips the ops line from pyproject.toml. + if grep -qE 'ops[^a-z]+=.*tracing|ops\[[^]]*tracing' pyproject.toml; then + wants_tracing=1 + else + wants_tracing=0 + fi + poetry remove ops --lock + if [ "${wants_tracing}" = "1" ]; then + poetry add "${ops_wheel}" "${ops_tracing_wheel}" --lock + else + poetry add "${ops_wheel}" --lock + fi fi - name: Install dependencies From 4cc0f79ac61ea02fadde0204457ca7347189226c Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 30 Jun 2026 13:10:43 +1200 Subject: [PATCH 10/10] fix: allow pydantic 1.10+ in ops-tracing dependency #2583 pinned ops-tracing to pydantic>=2,<3 on this branch (main leaves it unconstrained). That makes ops[tracing] uninstallable for charms that require pydantic 1 -- notably postgresql-k8s, which pins pydantic ^1.10 via data_platform_libs -- breaking Data Charm Tests version resolution. ops-tracing's own code is pydantic-version agnostic (the vendored libs branch on the major version), so widen the floor to >=1.10 while keeping the <3 upper bound, matching main's ability to coexist with pydantic-1 charms. Co-Authored-By: Claude Opus 4.8 (1M context) --- tracing/pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tracing/pyproject.toml b/tracing/pyproject.toml index b83fc3f21..982432963 100644 --- a/tracing/pyproject.toml +++ b/tracing/pyproject.toml @@ -33,7 +33,7 @@ classifiers = [ dependencies = [ "opentelemetry-sdk~=1.30", "ops==2.23.3.dev0", - "pydantic>=2,<3", + "pydantic>=1.10,<3", ] [project.urls] diff --git a/uv.lock b/uv.lock index 2ec409dd1..34cd1e644 100644 --- a/uv.lock +++ b/uv.lock @@ -2113,7 +2113,7 @@ testing = [ requires-dist = [ { name = "opentelemetry-sdk", specifier = "~=1.30" }, { name = "ops", editable = "." }, - { name = "pydantic", specifier = ">=2.0,<3" }, + { name = "pydantic", specifier = ">=1.10,<3" }, ] [package.metadata.requires-dev]