diff --git a/src/charm.py b/src/charm.py index e0d5606dc3..807551eb4c 100755 --- a/src/charm.py +++ b/src/charm.py @@ -2633,11 +2633,14 @@ 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 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: - return + self._remove_from_raft_via_peer() self._observer.stop_observer() self._rotate_logs.stop_log_rotation() try: @@ -2647,6 +2650,32 @@ 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}", + set_raft_flags=False, + ) + 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/integration/test_storage_detaching.py b/tests/integration/test_storage_detaching.py index a3dabfa5d3..26ae76fece 100644 --- a/tests/integration/test_storage_detaching.py +++ b/tests/integration/test_storage_detaching.py @@ -3,7 +3,9 @@ # See LICENSE file for licensing details. import logging +import time +import jubilant import pytest from .adapters import JujuFixture @@ -17,22 +19,81 @@ @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", sorted(doomed)) + for unit in sorted(doomed): + juju.ext.model.destroy_unit(unit, destroy_storage=True) + + # 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()]}" + ) + + 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}" + + +@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, diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 29e0d84100..4e15fcde0e 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -131,34 +131,79 @@ 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("data", attach=True)[0] + storage_id = harness.add_storage("archive", 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_not_called() + _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. - _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() + # 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) + _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) + _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", set_raft_flags=False + ) + _selected_snap.stop.assert_called_once_with(disable=True) + _get_unit_ip.side_effect = None + _reset() - storage_id = harness.add_storage(storage_name, attach=True)[0] - harness.detach_storage(storage_id) + # 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() - _stop_observer.assert_called_once_with() - _stop_log_rotation.assert_called_once_with() - _selected_snap.stop.assert_called_once_with(disable=True) + # Full teardown (no units remain): stop, no raft removal needed. + _planned_units.return_value = 0 + 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):