diff --git a/src/charm.py b/src/charm.py index 0b9626453f..ae6bc848db 100755 --- a/src/charm.py +++ b/src/charm.py @@ -413,6 +413,10 @@ def __init__(self, *args): # incorrectly # https://canonical-charm-refresh.readthedocs-hosted.com/latest/add-to-charm/status/#implementation self.framework.observe(self.on.collect_unit_status, self._reconcile_refresh_status) + for storage_name in self.meta.storages: + self.framework.observe( + self.on[storage_name].storage_detaching, self._on_storage_detaching + ) self.cluster_name = self.app.name self._member_name = self.unit.name.replace("/", "-") self._certs_path = "/usr/local/share/ca-certificates" @@ -2563,6 +2567,21 @@ 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. + if self.app.planned_units() > 0: + return + self._observer.stop_observer() + self._rotate_logs.stop_log_rotation() + try: + # Disable too, so a mid-teardown restart of the unit can't re-enable the + # services and re-grab the storage mounts before Juju finishes unmounting. + snap.SnapCache()[charm_refresh.snap_name()].stop(disable=True) + except snap.SnapError: + logger.exception("Failed to stop charmed-postgresql snap services") + def _is_storage_attached(self) -> bool: """Returns if storage is attached.""" try: diff --git a/src/rotate_logs.py b/src/rotate_logs.py index 39cdafc11e..f6dd621e26 100644 --- a/src/rotate_logs.py +++ b/src/rotate_logs.py @@ -5,6 +5,7 @@ import logging import os +import signal import subprocess from typing import TYPE_CHECKING @@ -56,3 +57,14 @@ def start_log_rotation(self): self._charm.unit_peer_data.update({"rotate-logs-pid": f"{pid}"}) logging.info(f"Started rotate logs process with PID {pid}") + + def stop_log_rotation(self) -> None: + """Stop the running rotate logs process if we have previously started it.""" + if stored_pid := self._charm.unit_peer_data.get("rotate-logs-pid"): + pid = int(stored_pid) + try: + os.kill(pid, signal.SIGINT) + logging.info(f"Stopped rotate logs process with PID {pid}") + self._charm.unit_peer_data.update({"rotate-logs-pid": ""}) + except OSError: + pass diff --git a/tests/integration/adapters.py b/tests/integration/adapters.py index 1892a63814..64cacb23f0 100644 --- a/tests/integration/adapters.py +++ b/tests/integration/adapters.py @@ -379,7 +379,8 @@ def destroy_unit( def list_storage(self, filesystem: bool = False, volume: bool = False) -> list[TStorageInfo]: """Lists storage details.""" raw = self._juju.cli("list-storage", "--format", "json") - json_ = json.loads(raw) + # Juju 4.0 prints nothing (not `{}`) for a model with no storage. + json_ = json.loads(raw) if raw.strip() else {} ret = [] for storage_key, storage_details in json_.get("storage", {}).items(): ret.append({"key": storage_key, **storage_details}) diff --git a/tests/integration/test_storage_detaching.py b/tests/integration/test_storage_detaching.py new file mode 100644 index 0000000000..a3dabfa5d3 --- /dev/null +++ b/tests/integration/test_storage_detaching.py @@ -0,0 +1,47 @@ +#!/usr/bin/env python3 +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +import logging + +import pytest + +from .adapters import JujuFixture +from .jubilant_helpers import DATABASE_APP_NAME + +logger = logging.getLogger(__name__) + +# rootfs (machine-scoped) is the only pool that reproduces the stuck unmount on +# Juju 4.0: the detachable lxd pool destroys the container before detaching. +ROOTFS_STORAGE = dict.fromkeys(("archive", "data", "logs", "temp"), "rootfs") + + +@pytest.mark.abort_on_fail +def test_storage_released_on_removal(juju: JujuFixture, charm): + """Graceful 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. + """ + juju.ext.model.deploy( + charm, + num_units=1, + config={"profile": "testing"}, + storage=ROOTFS_STORAGE, + force=True, + ) + juju.ext.model.wait_for_idle(apps=[DATABASE_APP_NAME], status="active") + + logger.info("Removing %s and waiting for a clean teardown", DATABASE_APP_NAME) + juju.ext.model.remove_application( + DATABASE_APP_NAME, + block_until_done=True, + destroy_storage=True, + 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 removal: {detaching}" diff --git a/tests/spread/test_storage_detaching.py/task.yaml b/tests/spread/test_storage_detaching.py/task.yaml new file mode 100644 index 0000000000..512faae976 --- /dev/null +++ b/tests/spread/test_storage_detaching.py/task.yaml @@ -0,0 +1,11 @@ +summary: test_storage_detaching.py +environment: + TEST_MODULE: test_storage_detaching.py + # #1550 only reproduces on Juju 4.0 (3.6 masks it by removing still-Dying storage), + # so juju40 is the real regression test. Run juju36 too as a no-regression guard: + # stopping/disabling the snap in storage-detaching must not break 3.6 teardown. + CONCIERGE_JUJU_CHANNEL/juju40: 4.0/stable +execute: | + tox run -e integration -- "tests/integration/$TEST_MODULE" --model testing --alluredir="$SPREAD_TASK/allure-results" +artifacts: + - allure-results diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 7a9db219e5..c4fa1a8ec8 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -125,6 +125,42 @@ def test_on_install(harness): assert isinstance(harness.model.unit.status, WaitingStatus) +def test_on_storage_detaching(harness): + with ( + patch("charm.snap.SnapCache") as _snap_cache, + 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, + ): + _selected_snap = _snap_cache.return_value.__getitem__.return_value + + # 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. + _planned_units.return_value = 2 + 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() + + # 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() + + 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) + + def test_patroni_scrape_config(harness): result = harness.charm.patroni_scrape_config() diff --git a/tests/unit/test_rotate_logs.py b/tests/unit/test_rotate_logs.py index 70b870f021..e399e9d2d4 100644 --- a/tests/unit/test_rotate_logs.py +++ b/tests/unit/test_rotate_logs.py @@ -1,5 +1,6 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. +import signal from unittest.mock import Mock, PropertyMock, patch import pytest @@ -96,3 +97,33 @@ def test_start_log_rotation_already_running(harness): harness.charm.rotate_logs.start_log_rotation() _kill.assert_called_once_with(1234, 0) _popen.assert_called_once() + + +def test_stop_log_rotation(harness): + with ( + patch("os.kill") as _kill, + patch.object(MockCharm, "_peers", new_callable=PropertyMock) as _peers, + ): + # Nothing is done when the peer relation (and so the stored PID) is gone. + _peers.return_value = None + harness.charm.rotate_logs.stop_log_rotation() + _kill.assert_not_called() + + # Nothing is done when there is no process running. + _peers.return_value = Mock(data={harness.charm.unit: {}}) + harness.charm.rotate_logs.stop_log_rotation() + _kill.assert_not_called() + + # The process is killed and the stored PID is cleared. + peer_data = {"rotate-logs-pid": "1"} + _peers.return_value = Mock(data={harness.charm.unit: peer_data}) + harness.charm.rotate_logs.stop_log_rotation() + _kill.assert_called_once_with(1, signal.SIGINT) + assert peer_data["rotate-logs-pid"] == "" + _kill.reset_mock() + + # A dead process doesn't break the script. + _peers.return_value = Mock(data={harness.charm.unit: {"rotate-logs-pid": "1"}}) + _kill.side_effect = OSError + harness.charm.rotate_logs.stop_log_rotation() + _kill.assert_called_once_with(1, signal.SIGINT)