[DPE-10841] feat(database): client-relation manager and its mapping caches (1/6) - #209
Closed
marceloneppel wants to merge 65 commits into
Closed
[DPE-10841] feat(database): client-relation manager and its mapping caches (1/6)#209marceloneppel wants to merge 65 commits into
marceloneppel wants to merge 65 commits into
Conversation
…tion Internalizes PostgreSQL parameter calculation (worker-process auto/cap rules, wal_compression, memory-limit conversion) into the library so ConfigManager owns the full config-build flow instead of stopping at a TODO. Resource discovery (cpu_cores, memory_bytes) moves onto the workload rather than a substrate-specific manager: get_available_memory already lives there, ConfigManager already holds self.workload, and K8sManager doesn't exist yet when ConfigManager is constructed in abstract_charm - so the workload is the only place both substrates can share a single no-branch call site. K8sWorkload gains required unit_name/namespace constructor args so its lightkube Pod/Node lookups can resolve "my own pod" without depending on a charm object reference, mirroring what the K8s charm does today via self.unit.name/self.model.name. lightkube becomes a real (not merely transitive) dependency via a new k8s pyproject extra, matching the version pins the K8s test charm already uses. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The parameter-building port placed lightkube node/pod resource lookups on K8sWorkload and injected unit_name/namespace into its constructor. Both duplicate identity the state already holds and put K8s-API code on the workload abstraction, which reviewers flagged. Move the lookups onto K8sManager, which is already K8s-only-loaded and holds the state, sourcing pod/namespace from state.peer.unit_name and state.model_name. ConfigManager reaches the value through a new state.available_resources property backed by a per-substrate provider (the VM workload, or the K8s manager), so the call site stays substrate-agnostic and lightkube never enters the VM import graph. Behavior is unchanged: node allocatable clamped by the container limit, 403 -> DeployedWithoutTrustError. The Node lookups drop the namespace argument, which lightkube ignores for cluster-scoped resources. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…ditional template The two charms each carried their own templates/patroni.yml.j2 and the config manager loaded it with a CWD-relative open(), so the lib could not own the render. Merge both copies into a single template the lib ships and loads as package data, so the charms delete their copies in the adoption PR and the template travels with the code that renders it rather than with each charm's working directory. The VM and K8s templates diverge in section ordering, whole blocks (raft, bypass_api_service, pod_ip), path variables, and pg_hba rules, so the merged template branches at the top level on a substrate context var with each substrate's body kept verbatim. That guarantees byte-for-byte identical output per substrate and avoids interleaving conditionals that could silently shift a byte. render_patroni_yml_file now loads the template via importlib.resources and passes the substrate the template branches on. Golden tests render the merged template against byte copies of each charm's original template across the full matrix of conditional dimensions (tls, connectivity, ldap, restore/pitr, slots, peers, watcher, extra replication endpoints, tags) and assert equality, so any divergent byte fails the suite. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…and bridges The library now owns the full config-update flow (steps 3-10) instead of the charm. update_config renders, applies Patroni-controlled parameters via the API, runs the TLS/pending-restart decision engine, honors the VM snap gate, and persists the config/user hashes -- all in the lib. The substrate-tangled pieces of the restart trigger, endpoint refresh, and monitoring/LDAP service restarts ride charm-side through three injected bridge callables (request_restart, refresh_endpoints, restart_services) until their own migration phases, so the two HIGH-risk substrate diffs in the restart trigger (VM pops postgresql_restarted; K8s updates the metrics scrape job) stay out of the library. is_tls_enabled and generate_config_hash internalize; user_hash stays injected. The config hash is byte-compatible with the charm's, so charm adoption does not force a spurious restart. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…gration Split the migration's unit tests and patroni-template golden fixtures out of the code changes so each code change lands as a small, self-contained PR, with the tests landing together on top of the stack. Covers the peer-state config/user hash accessors, PostgreSQL parameter building and resource introspection, the substrate-conditional patroni template, and the update_config orchestration. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The TLS handler pushes cert files to the workload on certificate_available, and the charm reloads PostgreSQL in a separate observer of the same event. Because defer is per-observer, a push that defers (e.g. a Pebble write fails mid cert renewal) while the charm's reload observer completes leaves the retried push writing the new cert with nothing reloading. Emit a tls_files_pushed event only after push_tls_files() succeeds; a deferred push returns without emitting. The charm's reload bridge observes this instead of certificate_available, so the reload runs only once the files are on disk and a deferred push retries together with its reload. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The update_config + tls_files_pushed stack branched at 16.3.3; 16/edge has since released 16.3.4. Set the version to the next patch so the library is release-ready as soon as this stack lands on 16/edge. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The renderer wrote patroni.yaml on both substrates while the K8s Pebble layer starts Patroni with patroni.yml, so on K8s the config subsystem would drive a file Patroni never reads: a fresh pod misses its config, an upgraded one keeps running against a stale one, and every later TLS, HBA or restore change silently never reaches the cluster. The two names are not interchangeable — each is what its substrate has always used (snap: patroni.yaml, K8s charm: patroni.yml) — so the file name belongs to the paths object, where render and launch both read it from one place and cannot drift apart again. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The template gates the watcher's health-check rule on watcher_addr, but only watcher — the ip:port Raft address — was ever supplied, so Jinja resolved watcher_addr to undefined and the rule was dropped: a related watcher joins Raft yet is refused by PostgreSQL when it probes. The pristine VM charm has the same omission, so this is not a migration regression; the port is where it becomes cheap to fix, since the address is already in hand and only needs its port trimmed. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The `'hostssl' if enable_tls else 'host'` ternary was spelled out at all 41 pg_hba rule sites, 40 characters of ceremony ahead of the part a reader is actually there for, pushing every rule off the left margin. Binding it once says the same thing in the name and lets the rules read as pg_hba again. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Each substrate spelled out replication, rewind and superuser as three copies of the same stanza, so the TLS-guarded certificate lines existed six times across the file and any change to them had to be made six times. Driving the three roles from a list keeps the stanza in one place, and the substrates keep their own copy because their certificate handling genuinely differs: VM emits sslrootcert and always emits the client pair, K8s emits neither unless TLS is on. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The seven charmed_* roles were seven rule lines per substrate that differed only in the role name, so adding a role meant editing two places and the grant itself was buried in fourteen near-identical lines. Listing the roles once puts the set of roles where a reader looks for it and leaves one rule to read. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The template was the two charm templates concatenated under a substrate conditional, so roughly three quarters of it existed twice: the same 43 PostgreSQL parameters, the same restore command, the same pg_hba rules, each needing every edit applied in two places. The two copies had already drifted apart in ways nobody intended, down to a different indent for the same list. Binding the values that genuinely vary per substrate once at the top collapses that to a single document with conditionals only where the substrates really differ - the raft section, the watcher rule, the Kubernetes-only keys and the asymmetric authentication block. Field order now follows one sequence rather than each charm's, which changes the rendered layout; the parsed configuration is unchanged, verified across the full render matrix. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The merged template now emits one field order for both substrates instead of reproducing each charm's, so a byte comparison fails on layout that carries no meaning to Patroni. Comparing the parsed documents keeps the guarantee that matters - same values, same keys, same rule order - and stays strict where order is semantic, since sequences still compare in order and pg_hba is first-match-wins. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The rule this fed is unreachable. The user_databases_map loop emits `postgres watcher 0.0.0.0/0` earlier in the same list whenever a watcher is related - which is exactly when the watcher host is non-null - and pg_hba takes the first matching record, so the narrower /32 rule below it never authorises a connection the broader one has not already allowed. The charm omits the same variable, so restoring the omission keeps this a move rather than a behaviour change, and leaves the dead rule visible for whoever decides what it was meant to do. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
K8s omits the username for the replication and superuser roles, which the merged template expressed as a truthiness test. An empty username would then be dropped on VM too, where the charm template always emitted the key. Testing for none keeps the omission to the roles that mean it. The values are non-empty constants today, so nothing rendered differently - this closes a gap the golden matrix cannot see, since it pins both usernames at one value. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The millicore floor is a behaviour change with no counterpart in the charms this port mirrors, so it does not belong in a parity diff a reviewer checks line by line against them. It now lives in its own fix on 16/edge (#206). Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The behaviour it pins left this stack along with the floor itself, so the assertion now covers code that only exists in its own fix on 16/edge (#206), where the test belongs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
… the base BaseWorkload gained a non-abstract get_snap_revision that only the VM workload can answer, purely so the type checker would accept a call the substrate guard already keeps off K8s. Reach it the way the event handlers reach the other VM-only workload capability, by narrowing with isinstance at the call site, so the shared interface stops advertising a snap the K8s workload has no access to. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…know The ported line carried the VM charm's wording, which dropped the K8s charm's "Updating Patroni config file" from the logs entirely. Operators grepping either substrate's history for that phrase would have found nothing after the move. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Both charms compute this hash once per hook via cached_property; the port turned it into a plain property, so update_config recomputes it on each of its two accesses. The value cannot differ within a hook, so this only restores the charms' cost profile. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Narrowing the snap gate with isinstance needed VMWorkload at runtime, and that module imports the snap charm lib, which ships only in the vm extra. The K8s charm installs postgresql and k8s, imports the config manager, and would have died at startup on `cannot import name 'snap' from 'charmlibs'`. The library's own test env installs every extra, so nothing here would have caught it. Narrow with a type-checking-only cast instead; the substrate guard is what keeps the call off K8s at runtime. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
The K8s charm logged this at info, so operators saw a config render in debug-log at default verbosity; the port left it at the VM charm's debug level and that visibility disappeared on both substrates. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
These base classes are the per-substrate charm definitions, not test scaffolding, and describing them as unit-test only reopens a settled question about whether they belong in the shipped wheel. What the comment needs to say is which side owns the bridges, which the preceding lines already do. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Issued certs land in the relation databag before the push writes them to the workload, and the push can defer — on K8s whenever the container is not ready. Any of the other update_config call sites firing in that window rendered ssl on against files that were not there, which is the non-atomicity the TLS port left open; TLSManager.client_tls_files_on_disk was added for this and had no caller. This diverges from the charms, which check the databag alone. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Pins the window the gate closes: certs present in the databag but not yet written to the workload must not read as TLS enabled. The existing all-files-present test now states the disk input too, since presence in the databag alone no longer decides. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
…es (1/6) The postgresql_provider migration needs a lib-side home for the client relation before either charm can hand it over. This first slice carries the parts the config flow already depends on: the username and prefix-database caches, the user->databases map, the peer user hash, and the installed-plugin list. The generated PostgreSQL role name stays substrate-derived. VM clusters hold roles named relation-<id> and K8s ones relation_id_<id>; converging the two would mean renaming roles on every deployed cluster for no functional gain, and the SQL layer already matches both forms. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
marceloneppel
force-pushed
the
tls-files-pushed
branch
from
August 17, 2026 19:15
c467833 to
01154fb
Compare
Member
Author
|
Closing: this branch was cut from a stale copy of |
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
1 participant
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.
Issue
postgresql_provider.pystill lives in both charms (VM 645 lines, K8s 606, near-identical), and the config flow it feeds already moved into this library. Until the provider follows,update_confighas to be handed arelations_user_databases_mapand auser_hashthe charm computes from provider state, and it calls back into the charm to refresh client endpoints. This is the first of six PRs that move the whole client relation here; it carries the state the config flow already reads.Solution
Adds
managers/database.pywith the parts of the provider that are pure state:custom-usernames,prefix-databases) so deployed clusters keep their existing content;collect_user_relations()anduser_hash, which the config manager consumes in PR 4 in place of the two injected values;are_units_in_sync(), the peer-hash comparison the request handler defers on;get_plugins(), ported from both charms unchanged.The generated PostgreSQL role name is derived from the substrate rather than converged: VM clusters hold roles named
relation-<id>and K8s onesrelation_id_<id>, so a single name would require renaming roles on every deployed K8s cluster.utils/postgresql.pyalready matches both forms in its relation-user SQL.Nothing constructs this manager yet — PR 4 wires it in. Unit tests are in PR 5, following the split used for the
update_configmigration.Stacked on #189. Next: 2/6.
Checklist