refactor: de-vendor the certificate transfer and tracing interface handling#2557
refactor: de-vendor the certificate transfer and tracing interface handling#2557tonyandrewmeyer wants to merge 26 commits into
Conversation
Replace the pydantic models in the two charm libraries vendored under tracing/ops_tracing/vendor/ with stdlib dataclasses plus manual validation, removing pydantic (and its pydantic-core, annotated-types and typing-inspection transitive deps) from ops_tracing's dependency tree. These vendored copies have exactly one consumer (ops_tracing itself) and are already a fork, so this also drops the upstream API surface ops_tracing never exercises: the provider-side classes, the charmhub publish metadata, the redundant relation-direction validation helper, the charm_tracing_config convenience wrapper, and the pydantic-v1 compatibility branches. Each vendored file gains a fork-marker header comment documenting what was dropped vs upstream. - tempo_coordinator_k8s/v0/tracing.py and certificate_transfer_interface/v1/certificate_transfer.py: pydantic BaseModel databag models become @DataClass with file-local load/dump helpers (sets serialise as sorted lists; unknown databag keys are ignored; missing required fields raise the lib-local DataValidationError). - tracing/pyproject.toml: drop the pydantic dependency; re-lock. - tracing/test: drop the now-obsolete pydantic importorskip guards and add round-trip / validation tests for the dataclass models. https://claude.ai/code/session_011fFFqf3gLogCYJk95YCwFW
… modules
The two forked-and-tuned files no longer match the 'vendored copy kept
in sync with upstream' framing — they are ops_tracing's own modules now.
Move them next to the other private modules and rename to the project's
underscore convention:
vendor/charms/.../certificate_transfer.py -> _cert_transfer_models.py
vendor/charms/.../tracing.py -> _tracing_models.py
test/test_vendor_models.py -> test/test_models.py
vendor/otlp_json/ stays where it is (still genuinely vendored).
The fork-marker headers ('Forked from … de-pydantic'd … do not re-sync')
are unchanged — the file is still a fork, the path is just honest now.
Tests: 26 passed, 1 skipped (same as 993c942 baseline).
|
Driveby comment -- there's a larger discussion to be had here as to how we want to handle this long term. With the tracing charm library migrating to charmlibs this cycle, we could in principle depend on it a as a regular Python dependency instead of vendoring. On the other hand, if our needs really do diverge, then maybe we'd prefer to switch to vendoring our own tracing logic in a more first-class way (with this PR being one step on that road). |
Yes, that's one of the reasons this is in draft. I'll address it before moving it out.
We obviously should do that when it happens, if we're still vendoring. But that's not the point of the issue. The point is to get rid of large dependencies that are not really needed, and slim down the code to only what is actually required. |
:)
It's not entirely clear to me that we should, given the desire to keep Ops as dependency free as possible, and given the trickiness introduced by the mutual dependency between Ops and the tracing library. If we can feasibly maintain our own slim subset of the functionality the library provides instead, I think that's worth considering.
The more our vendored copy drifts (e.g. dropping Pydantic), the more involved migrating to the But I fully support the idea of exploring how slim we can make this, that's definitely an important point in the dependency vs own-our-own discussion. |
What's currently here is not a vendored copy that drops Pydantic, it's a library built for just ops-tracing. It's not in a vendor folder, and it doesn't have anything in it that isn't used by ops-tracing. It might be too similar to the old version and need reworking - it's a draft and I haven't looked very closely at the code yet, just given instructions to an agent.
My understanding was that we definitely intend to not use the regular library, wherever it is hosted. I thought when we'd discussed this previously everyone was in favour of doing that, just not of allocating roadmap time to it.
My understanding was we are doing that because it's the library charms should use for workload tracing, and we want to accelerate getting people away from Charmhub hosted libraries, rather than anything to do with our own use. Did I miss something?
If this is more of an open question than I understood, I don't mind that. The direction here will be more-or-less the same either way. I am aware of the love for keeping Pydantic in charms and wouldn't be surprised if, even though my understanding is we already agreed to it, this gets rejected. |
|
I'm definitely in favour of the 'maintain our own slim, Pydantic-free tracing stuff' direction, so that's music to my ears. I must have forgotten the previous discussion where we agreed to that, and gotten confused by the PR description (I haven't looked at the code). |
The promoted _tracing_models.py and _cert_transfer_models.py had been flagged by ruff --preview (15 findings): line-too-long in docstrings and comments carried over from upstream, RUF012 mutable class-attribute defaults, D400, SIM102 nested-if, UP032 .format() calls, and an unused W505 noqa. Apply ruff --fix for the mechanical ones; wrap the docstring/comment lines; add ClassVar typing to the two class attrs; reword the is_ready docstring to end with a period. Tracing tests: 26 passed / 1 skipped, unchanged from baseline.
Quote-style normalisation (f"..." → f'...') on the two de-pydantic'd modules. The 07ac078 commit ran 'ruff --preview' (lint) but not 'ruff format --preview' (formatter); CI's lint job runs both, so this commit closes the format-check arm too. Tests: tracing/test/test_models.py still 11 passed.
ops_tracing only acts as a requirer for tracing and certificate_transfer, but the depydantic'd vendored copies still carried provider-side dump() methods, the _AutoSnapshotEvent + EndpointChangedEvent._receivers payload that _api.Tracing._reconcile never reads, the unused get_all_endpoints public method, dead relation= kwargs on request_protocols/get_endpoint, and the unused module constants DEFAULT_RELATION_NAME and RELATION_INTERFACE_NAME. The _json_safe/_coerce helpers in _tracing_models also had set/frozenset branches that no tracing dataclass field needs. Drop all of the above and update test_models accordingly. Tests and lint pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
_tracing_models and _cert_transfer_models each carried near-identical copies of the json_safe / coerce / build / is_default / load / dump helpers that replace pydantic's DatabagModel round-trip. Extract them into a single private _databag module, parameterised by each module's own DataValidationError so the public exception hierarchy is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ops.Relation.save in ops 3.7+ already handles the dataclass-to-databag encoding that TracingRequirerAppData.dump did by hand. Use it directly in request_protocols and drop the dump() method, along with the now- unused json_safe / _is_default / dump helpers in _databag. The load side stays in _databag because Relation.load's default json.loads decoder leaves nested dataclasses and enums (Receiver / ProtocolType / TransportProtocolType in TracingProviderAppData) as raw dicts and strings, which the runtime accessors don't tolerate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Match the ops convention used by the rest of ops_tracing: replace the per-submodule 'from ops.charm import ...' / 'from ops.framework import ...' / 'from ops.model import ...' imports in _tracing_models and _cert_transfer_models with a single 'import ops', and qualify every usage as ops.CharmBase, ops.Object, ops.Relation, ops.RelationEvent, etc. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ders The fork/dropped/why-no-pydantic prose lives in git history; the only thing future readers need at the top of these files is what they model. Collapse each header to a one-line module docstring with a pointer to the charm-relation-interfaces schema, and drop the pydantic references from the _databag helper docstrings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ema docs The GitHub charm-relation-interfaces URLs were wrong; the canonical charmlibs reference docs are the right pointer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s wrapper ProviderApplicationData modelled a single flat 'certificates: Set[str]' field, and its only consumers immediately unioned the result into another set, so the dataclass + DataValidationError + TLSCertificatesError + _databag.load round-trip was overhead for what is just one JSON list read. Replace it with a module-private _read_certificates helper; the dataclass, the two exception classes, and the _databag import all go. The happy path stays covered by test_https_tracing_destination in test_api.py; drop the now-dead ProviderApplicationData tests from test_models.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…events directly The cert-transfer wrapper class only existed to re-emit relation_changed / relation_broken as bespoke certificate_set_updated / certificates_- removed events that fed straight into Tracing._reconcile, and to wrap a six-line databag read. Hoist the observers and the databag read into Tracing itself and delete the standalone-library scaffolding (the wrapper class, the two custom events, the events container, and the whole _cert_transfer_models module). Tracing now reads the ca-cert databag directly with a local _read_certificates helper and reconciles off the raw juju relation events on the ca relation name. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… relation from Tracing Same shape as the cert-transfer cleanup: TracingEndpointRequirer was an ops.Object that observed relation_changed / relation_broken on the tracing relation and re-emitted them as endpoint_changed / endpoint_removed for Tracing._reconcile, plus wrapped a databag write (request_protocols) and a databag read (get_endpoint). All of that collapses now that Tracing can observe the juju events itself. _tracing_models now keeps only the dataclass shapes used on the wire (ProtocolType / Receiver / TracingProviderAppData / TracingRequirerAppData and the supporting enum / Literal) plus the DataValidationError raised by _databag.load. The TracingEndpointRequirer class, the EndpointChangedEvent / EndpointRemovedEvent / TracingEndpointRequirerEvents trio, and the TracingError / ProtocolNotRequestedError / AmbiguousRelationUsageError hierarchy are gone. _api gains two small helpers: _read_endpoint(relation, protocol) loads TracingProviderAppData and returns the matching url, and _request_protocols(charm, name, protocols) writes TracingRequirerAppData to each tracing relation via ops.Relation.save (leader-only, with the same permission-denied tolerance the wrapper had). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add an opt-in drift check that fetches the canonical/charmlibs tracing v2 and certificate_transfer v1 schemas, normalises them to a comparable signature, and round-trips a wire payload through both the upstream pydantic model and our de-pydantic'd dataclass. If upstream renames a field, changes a type, or adds a required field, this fails and forces a conscious decision about whether to follow. Gated to its own tox env (`tox -e upstream-schemas`) so the default `unit` env stays network- and pydantic-free. A new GitHub Actions workflow runs it only when `tracing/**` (or the workflow itself) is touched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nterface docs Add conformance test suites that quote the canonical interface-spec clauses verbatim and assert that our requirer-side implementation in `ops_tracing/_api.py` matches them. Sources: https://canonical.com/juju/docs/charmlibs/reference/interfaces/tracing/v2/ https://canonical.com/juju/docs/charmlibs/reference/interfaces/certificate_transfer/v1/ Each test names the exact 'Is expected to...' clause it covers, so future upstream drift surfaces as a localised failure rather than as silent behavioural divergence after we depydantic'd the vendored libraries. Drive-by: re-flow a tuple literal in test_upstream_schemas.py that `tox -e format` rewrote. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ation.load ops.Relation.load now recursively constructs nested dataclasses and coerces Enum field values when the target is a non-pydantic dataclass. Lists, tuples, sets and frozensets recurse into their element type; Literal/Union/Dict and other constructed generics are passed through unchanged. Pydantic targets are untouched. This lets the tracing module read its provider databag directly through ops.Relation.load(TracingProviderAppData, relation.app) instead of routing through a private ops_tracing._databag helper that re-implemented the same recursion. Delete the helper module, the .load classmethods on the dataclass models, and the DataValidationError exception; _api._read_endpoint catches the underlying (json.JSONDecodeError, TypeError, ValueError) instead. Tests now drive the load path through ops.testing.Context + Relation.load via a conftest fixture, so the recursive coercion in ops is what gets exercised end-to-end. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add a CertificateTransferProviderAppData dataclass and drive _read_certificates through Relation.load, so both relation interfaces used by Tracing go through the same loader path. The upstream-schemas test now does the same structural + roundtrip comparison as tracing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add four integration tests against the new Tracing wrapper and inlined certificate_transfer path: - buffer replay before the tracing relation is up - relation churn (remove + re-add charm-tracing) - CA rotation via self-signed-certificates' rotate-private-key action - only-leader writes the requirer databag with two units The tests cannot run end-to-end today because tempo-coordinator-k8s rev 143 crashes in upgrade-charm, calling `update-ca-certificates` in the nginx workload container — and no ubuntu/nginx tag we probed ships that binary. Document the gating in the module docstring. Add test_infra_canary.py with one xfail(strict=True) test that pulls the pinned ubuntu/nginx:1.24-24.04_beta image via docker/podman and asserts update-ca-certificates exists. When upstream restores the binary, the canary xpasses, strict trips the suite, and the four gated tests can be re-enabled. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
# Conflicts: # tracing/ops_tracing/vendor/charms/certificate_transfer_interface/v1/certificate_transfer.py # tracing/pyproject.toml
Upstream canonical#2586 rewrote wait_spans() to take a (host, port) tuple returned by kubectl_port_forward, but the four branch-added gated tests still passed status.apps['tempo-worker'].address. Rewrite them to use the port-forward helper, and xfail test_ca_rotation on k8s+Juju 4 like test_with_tls (same JUJU4_K8S_SECRET_RBAC_BUG on self-signed-certificates). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Upstream LIBPATCH 15 of the vendored certificate_transfer library made the interface dual v0/v1: a provider that does not see version=1 in the requirer's app databag falls back to publishing v0 on its unit databag (ca/certificate/chain) rather than v1 on its app databag (certificates). Advertise version=1 on relation-created (leader only, guarded on ModelError like _request_protocols) so a dual provider publishes v1 to us. Additionally, fall back to reading v0 from any provider unit if the app databag has no certificates, so we stay compatible with v0-only providers. Inverts the conformance test that documented "we don't write to the databag" into leader/follower tests, and adds a v0 unit-databag read test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The 36-way parallel integration matrix launches ~36 runners that all try to `snap install concierge` in the same second, and the snap store rate-limits with `error: cannot install "concierge": too many requests`. Every job dies at ~5s in Setup, before any tests run. Retry with jittered backoff (up to 5 attempts, 20-40s + i*20s per attempt) so the retries spread out across the rate-limit window rather than all colliding again on the same second.
Extends the concierge-install retry (654368d) to every other `snap install` in the workflows: charmcraft (charmcraft-pack, example-charm-tests, example-charm-charmcraft-test, published-charms-tests), yq (charmcraft-pack), concierge (example-charm-integration-tests, smoke), and canonical-secscan-client (sbom-secscan). Same pattern everywhere: up to 5 retries with 20-40s + i*20s jittered sleep, then one final un-retried attempt so a persistent outage still fails visibly. The push-triggered CI on this branch had `charm-checks` and `charmcraft-pack` failing on `error: cannot install "charmcraft": too many requests` — the parallel matrix hits the snap store harder than its rate limiter tolerates. Same fix as the integration matrix.
This PR replaces the two charm libraries vendored under
tracing/ops_tracing/vendor/charms/with small requirer-side dataclasses driven throughops.Relation.load/ops.Relation.save, removing pydantic (and itspydantic-core,annotated-types,typing-inspectiontransitive deps) fromops_tracing's dependency tree.The vendored copies had exactly one consumer (
ops_tracingitself) and were already a fork, so this also drops the upstream API surfaceops_tracingnever exercises: the provider-side classes, theTracingEndpointRequirer/CertificateTransferRequireswrappers, the charmhub publish metadata, the redundant relation-direction validation, thecharm_tracing_configconvenience wrapper, and the pydantic-v1 compatibility branches.As well as dropping 4 dependencies, including ones that are not pure Python, we drop hundreds of lines of unused code, significantly simplifying the production side code of ops-tracing, while gaining some additional tests.
tracing/ops_tracing/_tracing_models.py:TracingProviderAppData,TracingRequirerAppData, andCertificateTransferProviderAppDataas plain@dataclasses.Relation.loadrecurses into the nestedReceiver/ProtocolTypedataclasses and theSet[str]of PEMs; the lib-local validation helpers are gone.tracing/ops_tracing/_api.py:Tracingobserves the tracing and certificate-transfer Juju events directly. Reading the provider databag goes throughrelation.load; writing the requested receivers goes throughrelation.save. The two old wrapper classes and their inlined databag helpers are gone.tracing/test/: addtest_tracing_v2_conformance.pyandtest_certificate_transfer_v1_conformance.pypinning requirer-side behaviour against the verbatim clauses in the upstream interface docs, plustest_upstream_schemas.py(opt-in viatox -e upstream-schemas) which fetches the canonical/charmlibs schemas at HEAD and asserts our dataclasses haven't drifted from them.There is a small enhancement to the
Relation.loadfunctionality to support nested dataclasses more cleanly. This is technically breaking, as anyone using nested dataclasses without a custom load would have got plain dicts beyond the top level, but this seems like a bug fix to me, and exercising a more complicated example ourselves makes it so that we notice when the feature is not convenient enough for other users.