[DPE-11045] test(logical-replication): cover the ported module (5/5) - #270
Draft
marceloneppel wants to merge 35 commits into
Draft
marceloneppel wants to merge 35 commits into
marceloneppel wants to merge 35 commits into
Conversation
marceloneppel
force-pushed
the
lr-4-circular-validation
branch
from
September 4, 2026 21:51
6e55d8e to
9d7aaf0
Compare
marceloneppel
force-pushed
the
lr-5-tests
branch
from
September 4, 2026 21:51
33c8fcb to
f76c14a
Compare
The LDAP relation module ported in the next commit needs the glauth_k8s ldap interface library, but charm libs are not shipped with the charm build, so the library carries its own copy under single_kernel_postgresql/lib the same way it already does for data_platform_libs. This keeps the library self-contained for its unit tests and for the eventual integration tests, while the charms keep their own charmhub-fetched copies. The file is a verbatim copy of the charmhub lib at LIBPATCH 13, so it is excluded from formatting and lint like the other bundled libs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
… (2/2) (#236) * feat(ldap): vendor the glauth_k8s ldap interface library (1/2) The LDAP relation module ported in the next commit needs the glauth_k8s ldap interface library, but charm libs are not shipped with the charm build, so the library carries its own copy under single_kernel_postgresql/lib the same way it already does for data_platform_libs. This keeps the library self-contained for its unit tests and for the eventual integration tests, while the charms keep their own charmhub-fetched copies. The file is a verbatim copy of the charmhub lib at LIBPATCH 13, so it is excluded from formatting and lint like the other bundled libs. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * feat(ldap): port the LDAP relation module from the charms (2/2) Ported from the PostgreSQL VM and K8s charms at 16/edge: it owns the ldap requirer relation, flips the leader-only ldap_enabled peer flag on ready/unavailable, and maps the provider relation data to the Patroni LDAP auth parameters. Sourcing the parameters from the handler inside update_config replaces the ldap_parameters argument every charm had to thread through on each call, mirroring how the client-relation mapping caches were internalized in the config manager. Tests ported from the charms' test_ldap.py with the harness fixture and secret grant adapted to this repo's dual-substrate conftest. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * chore: bump library patch version to 16.3.8 The LDAP module migration adds new import surface to the library, and the release flow refuses to publish a version that is already tagged, so the merge of this stack must carry a fresh version for PyPI publishing. The charms switch to this release as their dependency pin, since charm builds hash-verify the exported requirements and cannot resolve a git dependency. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Add weekly scan * Remove manual run
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Update GitHub actions * Add cache-mode: none to all jobs Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Marcelo Henrique Neppel <marcelo.neppel@gmail.com>
…ackrest log paths (1/19) (#238) * feat(workload): run_cmd, by-name service control, and pgbackrest logs paths Land the workload-layer foundation the backup manager stands on: - Add a shared CommandResult dataclass (return_code, stdout, stderr, ok) and implement run_cmd on VM (subprocess, as root) and K8s (container.exec as the workload user/group, ExecError raised unless use_errors_replace). - Add by-name service control to BaseWorkload (start/stop/restart/reload/ service_is_running) with snap semantics on VM and Pebble semantics on K8s, including SIGHUP-based reload on K8s (restart-only on VM). - Add pgbackrest_logs to Paths (abstract) with per-substrate implementations mirroring patroni_logs. - Add S3_RELATION_NAME and the pgBackRest service/logs-path literals. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(workload): cover run_cmd, service control, and pgbackrest logs paths Dual-substrate tests (substrate fixture) for the workload foundation: - VM run_cmd: returncode/stdout/stderr, stdin and timeout passthrough, TimeoutExpired raised vs replaced (use_errors_replace). - K8s run_cmd: wait_output result, workload user/group, stdin and timeout passthrough, ExecError raised vs replaced. - By-name service control per substrate: snap start/stop/restart on VM (reload = restart, no signal channel); container start/stop/restart and SIGHUP-on-running reload on K8s; missing services read as not running. - pgbackrest_logs paths per substrate. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(workload): reload snap services by SIGHUPing the daemon process Address review comments on the by-name service control: - reload_service can't reach the daemons through systemd: the snap runs them through wrapper scripts (start-patroni.sh, start-pgbackrest.sh) that keep the daemon as a child, so the unit's main process is the wrapper. Find the daemon process by command line (the same patterns the PostgreSQL VM charm uses for Patroni), send SIGHUP to it, restart when the service is not running, and warn+skip when the daemon can't be found. - Add the POSTGRESQL_SNAP_NAME constant and derive BASE_SNAP_DIR, SNAP, and VM_PATRONI_SERVICE_NAME from it. #238 (comment) #238 (comment) Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
Constructs the handler in the abstract charm (the composition root) and hands its replication_slots view to the config manager; the test charms gain the two logical-replication relations in their metadata. Ports the circular-replication coverage from the charms' fix PR onto the library test charms: chain building, subscriber and publisher circular checks, validation blocking, and stale-error handling on both substrates.
- observe config-changed in the VM test charm and hand the event to apply_changed_config, mirroring canonical/postgresql-operator#1941 - uncomment the logical-replication-subscription-request options in the test charm config - construct the PostgreSQL client like the real charm does (Patroni primary endpoint + operator secret) and feed the per-user pg_hba map from the update_config bridge - fix the test-charm bootstrap gaps hit during e2e validation: storage check against the metadata storage location, data dir mkdir before chown, TLS file push after internal cert generation, cluster_initialised in the peer app databag, leader-elected Patroni render
marceloneppel
force-pushed
the
lr-5-tests
branch
from
September 18, 2026 12:32
f76c14a to
c055368
Compare
… library (2/19) (#239) * feat(workload): run_cmd, by-name service control, and pgbackrest logs paths Land the workload-layer foundation the backup manager stands on: - Add a shared CommandResult dataclass (return_code, stdout, stderr, ok) and implement run_cmd on VM (subprocess, as root) and K8s (container.exec as the workload user/group, ExecError raised unless use_errors_replace). - Add by-name service control to BaseWorkload (start/stop/restart/reload/ service_is_running) with snap semantics on VM and Pebble semantics on K8s, including SIGHUP-based reload on K8s (restart-only on VM). - Add pgbackrest_logs to Paths (abstract) with per-substrate implementations mirroring patroni_logs. - Add S3_RELATION_NAME and the pgBackRest service/logs-path literals. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * test(workload): cover run_cmd, service control, and pgbackrest logs paths Dual-substrate tests (substrate fixture) for the workload foundation: - VM run_cmd: returncode/stdout/stderr, stdin and timeout passthrough, TimeoutExpired raised vs replaced (use_errors_replace). - K8s run_cmd: wait_output result, workload user/group, stdin and timeout passthrough, ExecError raised vs replaced. - By-name service control per substrate: snap start/stop/restart on VM (reload = restart, no signal channel); container start/stop/restart and SIGHUP-on-running reload on K8s; missing services read as not running. - pgbackrest_logs paths per substrate. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * fix(workload): reload snap services by SIGHUPing the daemon process Address review comments on the by-name service control: - reload_service can't reach the daemons through systemd: the snap runs them through wrapper scripts (start-patroni.sh, start-pgbackrest.sh) that keep the daemon as a child, so the unit's main process is the wrapper. Find the daemon process by command line (the same patterns the PostgreSQL VM charm uses for Patroni), send SIGHUP to it, restart when the service is not running, and warn+skip when the daemon can't be found. - Add the POSTGRESQL_SNAP_NAME constant and derive BASE_SNAP_DIR, SNAP, and VM_PATRONI_SERVICE_NAME from it. #238 (comment) #238 (comment) Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * feat(backup): vendor the data_platform_libs s3 interface library (2/19) The backup manager ports behind the S3Requirer events the s3-integrator relation drives. The lib vendors the same data_platform_libs s3 charm library both charms already carry, verbatim and unformatted, following the data_interfaces and glauth_k8s ldap vendoring precedent, and gains boto3/botocore as minimum-version dependencies of the postgresql extra: the S3 client the next PR adds talks to the bucket directly. Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> * chore(backup): drop vendored-lib smoke test per review comment Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com> --------- Signed-off-by: Marcelo Henrique Neppel <marcelo.neppel@canonical.com>
# Conflicts: # pyproject.toml # single_kernel_postgresql/charms/abstract_charm.py # uv.lock
charmlibs-snap 2.0.0 removed charmlibs.snap.SnapCache, which the workload's snap service control still uses (the 16/edge lock refresh silently bumped it and broke the unit suite). Pin to the 1.x API until the workload migrates.
_fail_validation now stores the exact blocked message (status_msg or the generic fallback) in the peer application databag, and the success paths clear it. The composition-root status gate reads this key so the unit status keeps surfacing the SPECIFIC issue (e.g. 'Circular replication detected for table public.users') instead of the generic 'Logical replication setup is invalid. Check logs' after update-status (canonical/postgresql-k8s-operator#1052).
With both logical replication relations established first (canonical/postgresql-k8s-operator#1052 exact order) and the subscription requests configured afterwards, the config-time validations ran before either side had published, and _on_relation_changed then created the subscriptions without consulting the guards again: both sides ended up subscribed to each other's publication of the same table - the cyclic setup the guards exist to prevent. Re-run the validation at creation time: it reads the CURRENT relation data, so the publications that now exist make the subscriber-side circular checks fire and block the subscribe.
…ntics The migrated apply_changed_config pushed the NEW subscription request into the relation data BEFORE validating, so the per-table already-subscribed check (derived from the relation request) always saw the new tables as already subscribed and never fired the empty-table check on config-driven subscribes. An intermediate fix derived the flag from the created-subscriptions bookkeeping instead, but that broke the extend/restore flows: re-adding a previously replicated table (whose local data is stale) skipped the empty check. Restore the original #982 semantics: capture the PREVIOUSLY APPLIED request before validating and use it as the comparison baseline. The empty-table check now fires for newly added non-empty tables (test_pg2_dynamic_error) and stays skipped for tables already being replicated (test_pg3_extend_subscription). Also drop the push-before-validate: pushes happen only after a successful validation (in _apply_updated_subscription_request), so the relation request stays a truthful record of what is applied.
…tatus gate The status gate blocks on validation=error OR remote publisher errors. When the local validation passed (clearing the persisted message) but the publisher still reports errors in the relation data, the gate wrote the generic message and hid the publisher's exact complaint. Expose remote_publisher_error_message() so the gate surfaces the publisher's error verbatim (e.g. 'circular replication detected for tables public.users in database testdb'). canonical/postgresql-k8s-operator#1052 follow-up.
…re the multihop round-trip The migration moved the subscription-request push after local validation. That broke the multi-hop circular detection (A->B->C->A): the chain data lives in the publisher's publications, which don't exist until it sees a request, so the subscriber-side chain check was vacuous and the local empty-table check blocked first -- with no status_msg, surfacing the generic message instead of the circular one (canonical/postgresql-operator#1085). - apply_changed_config pushes the request to the relation before validating and derives the empty-table baseline from a new persisted 'logical-replication-applied-request' peer-data key (the relation data now holds the just-pushed request). - retry_validations persists the same baseline after a successful retry. - The empty-table guard gets an enforcement mode: config-changed validation enforces it only for extensions of an already-subscribed database ('auto'); creation cases defer the guard to the subscription-creation gate, which still enforces it -- and whose publisher-error check (running first) surfaces the publisher's 'circular replication detected ...' verdict with the exact blocked message. Creation gate, retries, and publisher-error re-validation always enforce ('enforce'). Unit tests: 4451 passed.
…us-gate helpers has_remote_publisher_errors() and remote_publisher_error_message() read the publisher's errors verbatim; after a rejected request is replaced by a new one, the publisher's old errors are still in the relation data and the gate blocked with a stale message until the publisher reprocessed (test_multihop_different_table_allowed timeout on canonical/postgresql-k8s-operator#1052). Mirror the _check_publisher_errors() staleness rules: the relation request must match the configured request and the error must be relevant to it.
…r directly The subscriber creates its subscription with create_slot=false, expecting the slot to exist on the publisher. The slot was only declared in the rendered Patroni config's slots: block, but Patroni (3.3) does not apply file-declared permanent slots on reload, and the K8s-only ensure_slots_controller_by_patroni sync never runs on VM -- so new subscriptions referenced a nonexistent slot and replicated nothing (test_basic_one_way_replication: subscriber rows stayed at 0). Create the slot with pg_create_logical_replication_slot right after the publication is created, and drop it when the published resources are cleaned up. Unit tests: 4451 passed.
…ntifier pg_create_logical_replication_slot takes text values; rendering the name as an SQL identifier made PostgreSQL resolve it as a column reference.
…ion break The baseline survived the subscriber relation break while the live subscriptions were cleared, so a remove/re-integrate cycle saw the dropped table as already subscribed, skipped the empty-table guard and re-subscribed with copy_data onto a non-empty table (the duplication the guard exists to prevent). Clear the baseline together with the subscriptions on break so the next validation treats every table as newly added.
…request The blocked request is already pushed (push-before-validate), so on the retry every configured table counts as in-flight and the empty-table guard must not re-fire -- otherwise a mid-flight blocked extend can never unblock once the local blocker is fixed (the refresh copies nothing for already-replicated tables, so no duplication). The creation gate keeps the applied-request baseline: it still blocks new subscriptions onto non-empty tables.
_on_relation_broken wrote an empty string where the readers expect JSON;
every later _subscriptions_info() call raised JSONDecodeError, failing
the subscriber hooks after a relation break and stalling the
remove/re-integrate flow. Clear to '{}' and tolerate empty values.
…e retry The retry re-validates against the configured (in-flight) request; persisting it as the applied baseline marked never-replicated tables as already subscribed, silenced the creation gate's empty-table guard and re-subscribed with copy_data onto a non-empty table on remove/re-integrate (6 rows became 12). The baseline only advances in apply_changed_config, where the previously applied request was captured before the push.
The earlier edit matched an identical block in apply_changed_config and removed that function's legitimate baseline update while leaving the retry's persist in place -- the exact inversion the note describes. Put each in its right place.
Fixes the CI lint failures: collapsible-if and unused-noqa in events/logical_replication.py, hardcoded-temp-file and unused-noqa in charms/vm_charm.py, plus formatting drift in the reformat candidates. 4451 unit tests still pass.
The baseline meant 'replicated by a live subscription', but apply_changed_config persisted the raw configured request on validation success — including newly-added databases whose subscription does not exist yet (creation is deferred to the relation-changed gate). The creation gate re-derives previous from this peer key, so a phantom entry silenced the empty-table guard and re-subscribed with copy_data=true over a non-empty table on the config cycle (6 rows became 12). Persist baseline = configured ∩ live subscriptions, advanced where live replication state actually changes (_on_relation_changed creation/drop loops and the apply). canonical/postgresql-k8s-operator#982 comment 3019811325.
… set The auto/enforce guard compared against the applied-request baseline and the database-level bookkeeping, so a table added to an ALREADY-subscribed database silently skipped the guard (test_pg2_dynamic_error: the re-add never blocked and the unit hung active on the arm64 runners until the 180-minute spread kill). Read the subscription's actual per-table state from pg_subscription_rel: a table present there is truly replicated (skip the guard); a table absent is a new addition whose copy_data would duplicate local rows (enforce the guard). The baseline remains the fallback signal while no live subscription exists.
… subpublications pg_subscription_rel's relid join broke on the CI target (column s.relid does not exist). The subscription's own subpublications text[] column carries the publication names it was created with — join pg_publication_tables through those, which works on every supported version and needs no extra privileges.
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
2 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.
Issue
Last PR of the logical replication migration series (after #269): unit coverage for the ported module, and the handler wiring into the library charms.
Solution
PostgreSQLLogicalReplicationinAbstractPostgreSQLCharm(the composition root) and hands itsreplication_slotsview to the config manager; the test charms gain the twological-replicationrelations in their metadata.Checklist