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 7c3472e31..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: @@ -19,36 +24,52 @@ 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 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 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/ops/_private/harness.py b/ops/_private/harness.py index bc89374cb..5c95becbc 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, @@ -2439,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: @@ -2463,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 == '': @@ -2472,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) @@ -2490,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/ops/pebble.py b/ops/pebble.py index ddaf5bc8b..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: @@ -1963,8 +2050,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): @@ -1986,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: @@ -3129,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/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_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, 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_pebble.py b/test/test_pebble.py index 4b9c581e7..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: @@ -3951,6 +3980,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): @@ -4032,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) diff --git a/test/test_testing.py b/test/test_testing.py index 6468684f8..22c25b12f 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 @@ -2276,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', + }, ), ] @@ -2301,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 @@ -3816,6 +3832,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, 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/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/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 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_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( 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: 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]