From d491aad64fe6235480323376a56ddcce721d5291 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 10 Jul 2026 19:57:28 -0300 Subject: [PATCH 1/6] fix(charm): release storage on scale-down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Juju 4.0 storage-detaching runs before the relation-departed and stop hooks and nothing else stops the snap, so a scaled-down unit's storage stayed mounted and Juju could not unmount it. The teardown fix only stops the snap when the whole app is going away, leaving single-unit removals leaked. Stopping the snap frees the storage, but once stopped the unit can no longer be removed from the pysyncobj raft cluster (the leader can no longer resolve its member IP), leaking a ghost member; a node also cannot remove itself from its own endpoint. So the departing unit drops itself from raft via a surviving peer before stopping. When several units are removed at once the first peer tried may itself be departing, so it iterates peers until a reachable node accepts the removal — on a minority scale-down a survivor always remains and quorum holds. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 34 +++++++++++++++-- tests/unit/test_charm.py | 81 ++++++++++++++++++++++++++++++---------- 2 files changed, 91 insertions(+), 24 deletions(-) diff --git a/src/charm.py b/src/charm.py index e0d5606dc3..860012d498 100755 --- a/src/charm.py +++ b/src/charm.py @@ -2633,11 +2633,13 @@ def _install_snap_package( raise def _on_storage_detaching(self, _) -> None: - """Stop the workload so Juju can unmount the storage on app teardown.""" - # On scale-down the surviving cluster still needs this unit's Patroni to - # remove it from raft; only stop when the whole app is going away. + """Stop the workload so Juju can unmount the storage on teardown or scale-down.""" + # storage-detaching runs before relation-departed/stop on Juju 4.0 and nothing + # else stops the snap. On scale-down drop this unit from raft via a surviving + # peer first (a node can't remove itself, and once stopped the leader can no + # longer resolve its member IP to remove it), then stop so Juju can unmount. if self.app.planned_units() > 0: - return + self._remove_from_raft_via_peer() self._observer.stop_observer() self._rotate_logs.stop_log_rotation() try: @@ -2647,6 +2649,30 @@ def _on_storage_detaching(self, _) -> None: except snap.SnapError: logger.exception("Failed to stop charmed-postgresql snap services") + def _remove_from_raft_via_peer(self) -> None: + """Drop this departing unit from raft using a surviving peer's endpoint. + + pysyncobj rejects a node removing itself from its own endpoint, so the + removal is issued against a peer. When several units are removed together the + first peer tried may itself be departing (snap already stopped), so try each + peer until one reachable raft node accepts the removal — any live node routes + it to the leader, and on scale-down at least one survivor stays up. + """ + if not self._peers: + return + for unit in self._peers.units: + peer_ip = self._get_unit_ip(unit) + if not peer_ip: + continue + try: + self._patroni.remove_raft_member( + f"{self.state.unit_ip}:{RAFT_PORT}", remote_address=f"{peer_ip}:{RAFT_PORT}" + ) + return + except RemoveRaftMemberFailedError: + continue + logger.warning("Failed to remove departing unit from raft: no reachable peer") + def _is_storage_attached(self) -> bool: """Returns if storage is attached.""" try: diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 29e0d84100..860ff04b2f 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -131,34 +131,75 @@ def test_on_storage_detaching(harness): patch("charm.ClusterTopologyObserver.stop_observer") as _stop_observer, patch("charm.RotateLogs.stop_log_rotation") as _stop_log_rotation, patch.object(harness.charm.app, "planned_units") as _planned_units, + patch("charm.Patroni.remove_raft_member") as _remove_raft_member, + patch( + "charm.PostgresqlOperatorCharm._get_unit_ip", return_value="2.2.2.2" + ) as _get_unit_ip, + patch( + "single_kernel_postgresql.core.state.CharmState.unit_ip", + new_callable=PropertyMock, + return_value="1.1.1.1", + ), ): _selected_snap = _snap_cache.return_value.__getitem__.return_value + rel_id = harness.model.get_relation(PEER_RELATION).id + + def _reset(): + for _m in (_stop_observer, _stop_log_rotation, _selected_snap, _remove_raft_member): + _m.reset_mock() - # Scale-down (units remain): the surviving cluster still needs this unit's - # Patroni reachable to remove it from raft, so the workload must NOT be - # stopped here. + # Scale-down but no surviving peer to ask: skip the raft removal, yet still + # stop so Juju can unmount the departing unit's storage. _planned_units.return_value = 2 + storage_id = harness.add_storage("archive", attach=True)[0] + harness.detach_storage(storage_id) + _remove_raft_member.assert_not_called() + _selected_snap.stop.assert_called_once_with(disable=True) + _reset() + + # Scale-down with a surviving peer: a node cannot remove itself from its own + # raft endpoint, so drop this unit from raft via the peer, then stop. + harness.add_relation_unit(rel_id, f"{harness.charm.app.name}/1") storage_id = harness.add_storage("data", attach=True)[0] harness.detach_storage(storage_id) - _stop_observer.assert_not_called() - _stop_log_rotation.assert_not_called() - _selected_snap.stop.assert_not_called() + _remove_raft_member.assert_called_once_with("1.1.1.1:2222", remote_address="2.2.2.2:2222") + _stop_observer.assert_called_once_with() + _stop_log_rotation.assert_called_once_with() + _selected_snap.stop.assert_called_once_with(disable=True) + _reset() + + # Concurrent scale-down where the first peer tried is itself a departing unit + # whose raft endpoint is already down (removal fails against it): fall through to + # a still-live survivor and drop this member via that peer's endpoint instead of + # leaking it. Distinct peer IPs prove the retry lands on the live survivor. + harness.add_relation_unit(rel_id, f"{harness.charm.app.name}/2") + _get_unit_ip.side_effect = ["2.2.2.2", "3.3.3.3"] + _remove_raft_member.side_effect = [RemoveRaftMemberFailedError, None] + storage_id = harness.add_storage("logs", attach=True)[0] + harness.detach_storage(storage_id) + assert _remove_raft_member.call_count == 2 + _remove_raft_member.assert_called_with("1.1.1.1:2222", remote_address="3.3.3.3:2222") + _selected_snap.stop.assert_called_once_with(disable=True) + _get_unit_ip.side_effect = None + _reset() + + # Scale-down where every peer's raft endpoint is unreachable: log and still + # stop rather than blocking the storage release. + _remove_raft_member.side_effect = RemoveRaftMemberFailedError + storage_id = harness.add_storage("temp", attach=True)[0] + harness.detach_storage(storage_id) + assert _remove_raft_member.call_count == 2 + _selected_snap.stop.assert_called_once_with(disable=True) + _reset() - # Full teardown (no units remain): release the storage by stopping the - # background processes and disabling (so a mid-teardown restart can't - # re-grab the mount) all the snap services. + # Full teardown (no units remain): stop, no raft removal needed. _planned_units.return_value = 0 - for storage_name in ("archive", "data", "logs", "temp"): - _stop_observer.reset_mock() - _stop_log_rotation.reset_mock() - _selected_snap.reset_mock() - - storage_id = harness.add_storage(storage_name, attach=True)[0] - harness.detach_storage(storage_id) - - _stop_observer.assert_called_once_with() - _stop_log_rotation.assert_called_once_with() - _selected_snap.stop.assert_called_once_with(disable=True) + storage_id = harness.add_storage("data", attach=True)[0] + harness.detach_storage(storage_id) + _remove_raft_member.assert_not_called() + _stop_observer.assert_called_once_with() + _stop_log_rotation.assert_called_once_with() + _selected_snap.stop.assert_called_once_with(disable=True) def test_patroni_scrape_config(harness): From 7a5b32440162fabec0568dce949471b195f99a0a Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Fri, 10 Jul 2026 20:11:03 -0300 Subject: [PATCH 2/6] test(integration): cover storage release on scale-down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only full-app teardown was exercised for #1550; the scale-down (remove-unit) path — where the departing unit must drop itself from raft via a peer before stopping — had no integration coverage. Add a remove-two-at-once scenario (minority removal, so quorum holds) that asserts the storage is released and the survivors stay active, reusing its deployment for the teardown test to avoid a second cluster. Signed-off-by: Marcelo Henrique Neppel --- tests/integration/test_storage_detaching.py | 37 +++++++++++++++++---- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_storage_detaching.py b/tests/integration/test_storage_detaching.py index a3dabfa5d3..0b23647c5d 100644 --- a/tests/integration/test_storage_detaching.py +++ b/tests/integration/test_storage_detaching.py @@ -17,22 +17,47 @@ @pytest.mark.abort_on_fail -def test_storage_released_on_removal(juju: JujuFixture, charm): - """Graceful removal must release the storage so teardown completes. +def test_storage_released_on_scale_down(juju: JujuFixture, charm): + """Scale-down (remove-unit) must release the departing units' storage. - Regression test for canonical/postgresql-operator#1550: without stopping the - workload in the ``storage-detaching`` hook the snap keeps the storage mounts - busy, so Juju's unmount fails ("target is busy") and teardown hangs. + Regression test for canonical/postgresql-operator#1550 on the scale-down + path. Removing two units at once also exercises dropping each departing unit + from raft via a surviving peer before it stops (the first peer tried may + itself be departing); a minority removal keeps quorum, so the survivors must + settle back to active. """ juju.ext.model.deploy( charm, - num_units=1, + num_units=4, config={"profile": "testing"}, storage=ROOTFS_STORAGE, force=True, ) juju.ext.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") + doomed = [unit.name for unit in juju.ext.model.applications[DATABASE_APP_NAME].units][-2:] + logger.info("Removing %s at once; the two survivors must keep quorum", doomed) + for unit in doomed: + juju.ext.model.destroy_unit(unit, destroy_storage=True) + juju.ext.model.wait_for_idle( + apps=[DATABASE_APP_NAME], status="active", wait_for_exact_units=2, timeout=15 * 60 + ) + + detaching = [ + storage for storage in juju.ext.model.list_storage() if storage["life"] == "detaching" + ] + assert not detaching, f"storage stuck detaching after scale-down: {detaching}" + + +@pytest.mark.abort_on_fail +def test_storage_released_on_removal(juju: JujuFixture): + """Full removal must release the storage so teardown completes. + + Regression test for canonical/postgresql-operator#1550: without stopping the + workload in the ``storage-detaching`` hook the snap keeps the storage mounts + busy, so Juju's unmount fails ("target is busy") and teardown hangs. Reuses + the two-unit cluster the scale-down test above leaves behind. + """ logger.info("Removing %s and waiting for a clean teardown", DATABASE_APP_NAME) juju.ext.model.remove_application( DATABASE_APP_NAME, From 7ef9b022ee763a666b4ae2c037da065c5bd2f6df Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 14 Jul 2026 11:53:57 -0300 Subject: [PATCH 3/6] test(integration): poll for storage to settle after scale-down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit destroy_unit returns once the removal is queued; the storage detach/destroy finishes asynchronously on a separate provisioner, so a one-shot check right after wait_for_idle could catch a transient detaching state. Poll until no storage is stuck detaching (the #1550 symptom) — the timeout doubles as the failure signal for a snap-held mount that never unmounts. Signed-off-by: Marcelo Henrique Neppel --- tests/integration/test_storage_detaching.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_storage_detaching.py b/tests/integration/test_storage_detaching.py index 0b23647c5d..62a2924f04 100644 --- a/tests/integration/test_storage_detaching.py +++ b/tests/integration/test_storage_detaching.py @@ -3,6 +3,7 @@ # See LICENSE file for licensing details. import logging +import time import pytest @@ -43,9 +44,14 @@ def test_storage_released_on_scale_down(juju: JujuFixture, charm): apps=[DATABASE_APP_NAME], status="active", wait_for_exact_units=2, timeout=15 * 60 ) - detaching = [ - storage for storage in juju.ext.model.list_storage() if storage["life"] == "detaching" - ] + # destroy_unit returns once the removal is queued; the storage detach/destroy + # finishes asynchronously on a separate provisioner, so poll until no storage is + # stuck "detaching" — the #1550 symptom is a snap-held mount that never leaves it. + deadline = time.monotonic() + 3 * 60 + detaching = [s for s in juju.ext.model.list_storage() if s["life"] == "detaching"] + while detaching and time.monotonic() < deadline: + time.sleep(10) + detaching = [s for s in juju.ext.model.list_storage() if s["life"] == "detaching"] assert not detaching, f"storage stuck detaching after scale-down: {detaching}" From d3de03d5e959401fd73b96a4649a36bfde2e2fc0 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 14 Jul 2026 16:35:13 -0300 Subject: [PATCH 4/6] test(integration): tolerate Juju 4.0 context-canceled race on concurrent removal Removing two units at once on Juju 4.0 can transiently error a departing unit on a canceled hook ('context canceled' as the uniter tears it down) and stall its removal, so the test's wait-for-2-survivors never settled on CI runners (it passed on a bare-metal box, so it is a timing race, not a logic bug). Resolve the departing unit's transient error and keep waiting; a survivor erroring or storage staying stuck still fails the test. The resolve is best-effort and only catches jubilant's CLIError (the unit changed state between the status check and the call). Signed-off-by: Marcelo Henrique Neppel --- tests/integration/test_storage_detaching.py | 54 ++++++++++++++++----- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/tests/integration/test_storage_detaching.py b/tests/integration/test_storage_detaching.py index 62a2924f04..26ae76fece 100644 --- a/tests/integration/test_storage_detaching.py +++ b/tests/integration/test_storage_detaching.py @@ -5,6 +5,7 @@ import logging import time +import jubilant import pytest from .adapters import JujuFixture @@ -36,22 +37,51 @@ def test_storage_released_on_scale_down(juju: JujuFixture, charm): ) juju.ext.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") - doomed = [unit.name for unit in juju.ext.model.applications[DATABASE_APP_NAME].units][-2:] - logger.info("Removing %s at once; the two survivors must keep quorum", doomed) - for unit in doomed: + doomed = {unit.name for unit in juju.ext.model.applications[DATABASE_APP_NAME].units[-2:]} + logger.info("Removing %s at once; the two survivors must keep quorum", sorted(doomed)) + for unit in sorted(doomed): juju.ext.model.destroy_unit(unit, destroy_storage=True) - juju.ext.model.wait_for_idle( - apps=[DATABASE_APP_NAME], status="active", wait_for_exact_units=2, timeout=15 * 60 + + # Concurrent removal on Juju 4.0 can transiently error a departing unit on a canceled + # hook ("context canceled" as the uniter tears it down) and stall its removal. Resolve + # those on the departing units — benign, they are going away — and keep waiting; a + # survivor erroring is a real failure. The #1550 symptom is storage stuck "detaching". + resolved: set[str] = set() + deadline = time.monotonic() + 15 * 60 + while time.monotonic() < deadline: + units = juju.status().apps[DATABASE_APP_NAME].units or {} + for name, unit in units.items(): + if not unit.is_error: + continue + if name in doomed: + if name not in resolved: + logger.info( + "resolving transient error on departing %s: %s", + name, + unit.workload_status.message, + ) + try: + juju.cli("resolve", name) + except jubilant.CLIError: + # Benign race: the unit's state changed between the status check + # and resolve (already removed, or no longer in error). The poll + # keeps waiting; a genuinely stuck removal fails on the timeout. + logger.debug("resolve no-op for %s (state changed)", name) + resolved.add(name) + else: + raise AssertionError( + f"survivor {name} errored during scale-down: {unit.workload_status.message}" + ) + if len(units) <= 2 and all(u.is_active for u in units.values()): + break + time.sleep(10) + units = juju.status().apps[DATABASE_APP_NAME].units or {} + assert len(units) == 2, f"expected 2 survivors after scale-down, got {sorted(units)}" + assert all(u.is_active for u in units.values()), ( + f"survivors not active: {[(n, u.workload_status.current) for n, u in units.items()]}" ) - # destroy_unit returns once the removal is queued; the storage detach/destroy - # finishes asynchronously on a separate provisioner, so poll until no storage is - # stuck "detaching" — the #1550 symptom is a snap-held mount that never leaves it. - deadline = time.monotonic() + 3 * 60 detaching = [s for s in juju.ext.model.list_storage() if s["life"] == "detaching"] - while detaching and time.monotonic() < deadline: - time.sleep(10) - detaching = [s for s in juju.ext.model.list_storage() if s["life"] == "detaching"] assert not detaching, f"storage stuck detaching after scale-down: {detaching}" From 866d10e118ae7edb5d42acd0789e976916d11d58 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Tue, 14 Jul 2026 17:31:37 -0300 Subject: [PATCH 5/6] docs(charm): fix storage-detaching ordering comment The storage-detaching hook runs before relation-departed/stop on all Juju versions, not just 4.0; what is 4.0-specific is that 3.6 force-removes still-Dying storage (masking the stuck mount) while 4.0 leaves it to unmount. The comment conflated the two. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/charm.py b/src/charm.py index 860012d498..75b77f6062 100755 --- a/src/charm.py +++ b/src/charm.py @@ -2634,10 +2634,11 @@ def _install_snap_package( def _on_storage_detaching(self, _) -> None: """Stop the workload so Juju can unmount the storage on teardown or scale-down.""" - # storage-detaching runs before relation-departed/stop on Juju 4.0 and nothing - # else stops the snap. On scale-down drop this unit from raft via a surviving - # peer first (a node can't remove itself, and once stopped the leader can no - # longer resolve its member IP to remove it), then stop so Juju can unmount. + # storage-detaching runs before the relation-departed/stop hooks and nothing + # else stops the snap, so stop it here or the mount stays busy. (3.6 force-removes + # still-Dying storage and masks this; 4.0 leaves it for Juju to unmount.) On + # scale-down drop this unit from raft via a surviving peer first — a node can't + # remove itself, and once stopped the leader can no longer resolve its member IP. if self.app.planned_units() > 0: self._remove_from_raft_via_peer() self._observer.stop_observer() From a8ad7d903a28e8c1a6545fa884f80390ecb82502 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Mon, 20 Jul 2026 17:19:11 -0300 Subject: [PATCH 6/6] fix(raft): skip stuck-raft flags on scale-down self-removal On scale-down a departing unit removes itself from raft via a surviving peer. With the default set_raft_flags=True, a transient no-quorum read from the polled peer makes remove_raft_member set a stuck-raft flag and return without removing: the member is left as a ghost, the peer fall-through stops early, and a spurious "Raft majority loss" BlockedStatus is written on a unit that is already leaving. set_raft_flags=False lets that read raise instead, so the loop falls through to the next reachable peer. This mirrors the self-removal already performed in _on_raft_reconnect. Signed-off-by: Marcelo Henrique Neppel --- src/charm.py | 4 +++- tests/unit/test_charm.py | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/charm.py b/src/charm.py index 75b77f6062..807551eb4c 100755 --- a/src/charm.py +++ b/src/charm.py @@ -2667,7 +2667,9 @@ def _remove_from_raft_via_peer(self) -> None: continue try: self._patroni.remove_raft_member( - f"{self.state.unit_ip}:{RAFT_PORT}", remote_address=f"{peer_ip}:{RAFT_PORT}" + f"{self.state.unit_ip}:{RAFT_PORT}", + remote_address=f"{peer_ip}:{RAFT_PORT}", + set_raft_flags=False, ) return except RemoveRaftMemberFailedError: diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 860ff04b2f..4e15fcde0e 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -162,7 +162,9 @@ def _reset(): harness.add_relation_unit(rel_id, f"{harness.charm.app.name}/1") storage_id = harness.add_storage("data", attach=True)[0] harness.detach_storage(storage_id) - _remove_raft_member.assert_called_once_with("1.1.1.1:2222", remote_address="2.2.2.2:2222") + _remove_raft_member.assert_called_once_with( + "1.1.1.1:2222", remote_address="2.2.2.2:2222", set_raft_flags=False + ) _stop_observer.assert_called_once_with() _stop_log_rotation.assert_called_once_with() _selected_snap.stop.assert_called_once_with(disable=True) @@ -178,7 +180,9 @@ def _reset(): storage_id = harness.add_storage("logs", attach=True)[0] harness.detach_storage(storage_id) assert _remove_raft_member.call_count == 2 - _remove_raft_member.assert_called_with("1.1.1.1:2222", remote_address="3.3.3.3:2222") + _remove_raft_member.assert_called_with( + "1.1.1.1:2222", remote_address="3.3.3.3:2222", set_raft_flags=False + ) _selected_snap.stop.assert_called_once_with(disable=True) _get_unit_ip.side_effect = None _reset()