fix: cherry-pick fixes from main into the 2.23-maintenance branch#2588
Open
tonyandrewmeyer wants to merge 10 commits into
Open
fix: cherry-pick fixes from main into the 2.23-maintenance branch#2588tonyandrewmeyer wants to merge 10 commits into
tonyandrewmeyer wants to merge 10 commits into
Conversation
…nonical#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. canonical#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 canonical#2547 (cherry picked from commit 9d03472)
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 <sqlite3.Connection ...>`. 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 <noreply@anthropic.com> (cherry picked from commit 54376c1)
`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 <noreply@anthropic.com> (cherry picked from commit 9b797aa)
…g Checks (canonical#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 canonical#2566 --------- Co-authored-by: Dave Wilding <tech@dpw.me> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit a1f3afa)
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 <noreply@anthropic.com> (cherry picked from commit 43b322f)
…nonical#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 canonical#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 canonical#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 canonical#2556 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 00b1e6f)
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 canonical#2327 --------- Co-authored-by: James Garner <james.garner@canonical.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 208af9b)
The cherry-picked canonical#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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
canonical#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) <noreply@anthropic.com>
tromai
approved these changes
Jul 1, 2026
| - 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)" |
Contributor
There was a problem hiding this comment.
I wonder why the pattern for ops_tracing wheel is more relaxed compared to ops_wheel here?
Collaborator
Author
There was a problem hiding this comment.
The [0-9] in ops-[0-9]*.whl is defensive: without it, the glob would also match anything starting with ops- (for example a future ops-tools-*.whl). Wheel filenames normalise - to _ in the package portion, so ops_tracing-*.whl already can't be matched by ops-* — there's no similarly-named package to disambiguate against, so the looser pattern is fine.
| # 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 |
Contributor
There was a problem hiding this comment.
Just to double check my understanding, the first sub pattern matches something like
ops = {extras = ["tracing"], version = ">=2.0"}
while the second pattern matches
ops[testing,tracing]
Collaborator
Author
There was a problem hiding this comment.
Yes, that's right. The first alternative (ops[^a-z]+=.*tracing) catches the Poetry table form like ops = {extras = ["tracing"], ...}, and the second (ops\[[^]]*tracing) catches the PEP 508 extras form like ops[testing,tracing] or ops[tracing].
(I wish all the poetry usage would go away, but some people really like it.)
| "opentelemetry-sdk~=1.30", | ||
| "ops==2.23.3.dev0", | ||
| "pydantic>=2,<3", | ||
| "pydantic>=1.10,<3", |
Contributor
There was a problem hiding this comment.
Why did we leave this unconstrained on main? I don't mind a bandaid fix in this PR, but this seems like something we should iron out more consistently. Will we be able to declare >=2,<3 in future and rely on hyrum to force the library to work with the charm? Or do we need a more specific solution to relax our constraints in testing?
Collaborator
Author
There was a problem hiding this comment.
I changed this to be constrained on the 2.23 branch because it was needed for some branch resolution that might be important later on (DM if that is too vague).
I would like Pydantic gone in 3.x, so I didn't bother making the change there, but it probably should be too. I think it may also need to be 1.x and above to handle downstream.
Contributor
There was a problem hiding this comment.
OK, leaving this as-is here for now seems reasonable in that context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A follow-up to the collection that was done a couple of months ago: