Skip to content

fix(backups): always reset the connectivity flag after a replica backup #1643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
marceloneppel wants to merge 1 commit into
base: 16/edge
Choose a base branch
from
Draft

fix(backups): always reset the connectivity flag after a replica backup #1643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions src/backups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,19 +831,21 @@ def _on_create_backup_action(self, event) -> None:
event.fail(error_message)
return

disabled_connectivity = False
if not self.charm.is_primary:
# Create a rule to mark the cluster as in a creating backup state and update
# the Patroni configuration.
self._change_connectivity_to_database(connectivity=False)
disabled_connectivity = True

self.charm.set_unit_status(MaintenanceStatus("creating backup"))
# Set flag due to missing in progress backups on JSON output
# (reference: https://github.com/pgbackrest/pgbackrest/issues/2007)
self.charm.update_config(is_creating_backup=True)

# Decide before any side effect whether this unit disables connectivity, so the
# reset in the finally below runs even if disabling or the re-render raises partway
# — a stale "off" leaves pg_hba rejecting peer/replica connections across restarts.
disabled_connectivity = not self.charm.is_primary
try:
if disabled_connectivity:
# Create a rule to mark the cluster as in a creating backup state and update
# the Patroni configuration.
self._change_connectivity_to_database(connectivity=False)

self.charm.set_unit_status(MaintenanceStatus("creating backup"))
# Set flag due to missing in progress backups on JSON output
# (reference: https://github.com/pgbackrest/pgbackrest/issues/2007)
self.charm.update_config(is_creating_backup=True)

command = [
"pgbackrest",
f"--stanza={self.stanza_name}",
Expand Down Expand Up @@ -906,11 +908,13 @@ def _on_create_backup_action(self, event) -> None:
else:
logger.info(f"Backup succeeded: with backup-id {datetime_backup_requested}")
event.set_results({"backup-status": "backup created"})

if disabled_connectivity:
# Remove the rule that marks the cluster as in a creating backup state
# and update the Patroni configuration.
self._change_connectivity_to_database(connectivity=True)
finally:
# Reset connectivity even if the backup raised or was interrupted — a stale
# "off" leaves pg_hba rejecting peer/replica connections across restarts.
if disabled_connectivity:
# Remove the rule that marks the cluster as in a creating backup state
# and update the Patroni configuration.
self._change_connectivity_to_database(connectivity=True)

self.charm.update_config(is_creating_backup=False)
self.charm.set_unit_status(ActiveStatus())
Expand Down
22 changes: 22 additions & 0 deletions src/charm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,9 @@ def _on_config_changed(self, event) -> None:
event.defer()
return

# Recover from a stale "connectivity: off" left by an interrupted backup before
# re-rendering the Patroni configuration below.
self.reset_stale_connectivity_flag()
try:
self._validate_config_options()
# update config on every run
Expand Down Expand Up @@ -1416,6 +1419,10 @@ def _on_postgresql_pebble_ready(self, event: WorkloadEvent) -> None:
event.defer()
return

# Recover from a stale "connectivity: off" left by an interrupted backup before
# the workload is configured/started.
self.reset_stale_connectivity_flag()

# Create the PostgreSQL data directory. This is needed on cloud environments
# where the volume is mounted with more restrictive permissions.
self._create_pgdata(container)
Expand Down Expand Up @@ -2152,6 +2159,21 @@ def is_connectivity_enabled(self) -> bool:
"""Return whether this unit can be connected externally."""
return self.unit_peer_data.get("connectivity", "on") == "on"

def reset_stale_connectivity_flag(self) -> None:
"""Reset a stale "connectivity: off" left by an interrupted backup.

The flag is written to the unit peer databag during replica backups and survives
restarts/upgrades; without this recovery a unit stuck "off" stays externally
disconnected (pg_hba rejects peer/replica connections). Skipped while any backup
is in progress cluster-wide to avoid un-hiding a unit mid-backup.
"""
if self.unit_peer_data.get("connectivity") != "off":
return
if self.patroni_manager.is_creating_backup:
return
logger.info("Resetting stale connectivity flag left by an interrupted backup")
self.unit_peer_data["connectivity"] = "on"

@property
def is_ldap_charm_related(self) -> bool:
"""Return whether this unit has an LDAP charm related."""
Expand Down
120 changes: 120 additions & 0 deletions tests/unit/test_backups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,126 @@ def test_change_connectivity_to_database(harness):
_update_config.assert_called_once()


def test_create_backup_resets_connectivity_when_backup_raises(harness):
# Regression: if the backup run is interrupted (raises an unexpected exception) on a
# replica, the connectivity flag must still be reset to "on" via the finally block —
# otherwise pg_hba keeps rejecting peer/replica connections across restarts/upgrades.
with (
patch("charm.PostgresqlOperatorCharm.update_config"),
patch(
"charm.PostgreSQLBackups._change_connectivity_to_database"
) as _change_connectivity_to_database,
patch(
"charm.PostgreSQLBackups._execute_command", side_effect=RuntimeError("boom")
) as _execute_command,
patch(
"charm.PostgresqlOperatorCharm.is_primary", new_callable=PropertyMock
) as _is_primary,
patch("charm.PostgreSQLBackups._upload_content_to_s3", return_value=True),
patch("backups.datetime"),
patch("ops.JujuVersion.from_environ"),
patch("charm.PostgreSQLBackups._retrieve_s3_parameters") as _retrieve_s3_parameters,
patch("charm.PostgreSQLBackups._can_unit_perform_backup", return_value=(True, None)),
):
# This unit is a replica, so connectivity is disabled before the backup runs.
_is_primary.return_value = False
_retrieve_s3_parameters.return_value = (
{
"bucket": "test-bucket",
"access-key": "test-access-key",
"secret-key": "test-secret-key",
"endpoint": "test-endpoint",
"path": "test-path",
"region": "test-region",
},
[],
)
mock_event = MagicMock()
mock_event.params = {"type": "full"}

# The backup run raises; the reset must still run.
with pytest.raises(RuntimeError):
harness.charm.backup._on_create_backup_action(mock_event)

# Connectivity disabled then re-enabled despite the exception.
_change_connectivity_to_database.assert_has_calls([
call(connectivity=False),
call(connectivity=True),
])
_execute_command.assert_called_once()


def test_create_backup_resets_connectivity_when_disabling_raises(harness):
# Regression: if disabling connectivity itself raises (after the databag was written),
# the reset must still run — the latch is decided before any side effect, so the finally
# fires even though disabled_connectivity was never set inside the try body.
with (
patch("charm.PostgresqlOperatorCharm.update_config"),
patch(
"charm.PostgreSQLBackups._change_connectivity_to_database",
side_effect=[RuntimeError("boom"), None],
) as _change_connectivity_to_database,
patch(
"charm.PostgresqlOperatorCharm.is_primary", new_callable=PropertyMock
) as _is_primary,
patch("charm.PostgreSQLBackups._upload_content_to_s3", return_value=True),
patch("backups.datetime"),
patch("ops.JujuVersion.from_environ"),
patch("charm.PostgreSQLBackups._retrieve_s3_parameters") as _retrieve_s3_parameters,
patch("charm.PostgreSQLBackups._can_unit_perform_backup", return_value=(True, None)),
):
_is_primary.return_value = False
_retrieve_s3_parameters.return_value = (
{
"bucket": "test-bucket",
"access-key": "test-access-key",
"secret-key": "test-secret-key",
"endpoint": "test-endpoint",
"path": "test-path",
"region": "test-region",
},
[],
)
mock_event = MagicMock()
mock_event.params = {"type": "full"}

# Disabling connectivity raises; the reset must still run.
with pytest.raises(RuntimeError):
harness.charm.backup._on_create_backup_action(mock_event)

# Disable (which raised) then re-enable despite the exception.
_change_connectivity_to_database.assert_has_calls([
call(connectivity=False),
call(connectivity=True),
])


def test_reset_stale_connectivity_flag(harness):
harness.charm.patroni_manager = MagicMock()
peer_rel_id = harness.model.get_relation(PEER_RELATION).id

# Stale "off" with no backup in progress: reset to "on".
harness.charm.patroni_manager.is_creating_backup = False
with harness.hooks_disabled():
harness.update_relation_data(peer_rel_id, harness.charm.unit.name, {"connectivity": "off"})
harness.charm.reset_stale_connectivity_flag()
assert harness.get_relation_data(peer_rel_id, harness.charm.unit)["connectivity"] == "on"

# While a backup is in progress cluster-wide: do not reset (stay "off").
with harness.hooks_disabled():
harness.update_relation_data(peer_rel_id, harness.charm.unit.name, {"connectivity": "off"})
harness.charm.patroni_manager.is_creating_backup = True
harness.charm.reset_stale_connectivity_flag()
assert harness.get_relation_data(peer_rel_id, harness.charm.unit)["connectivity"] == "off"

# Already "on": no-op (does not even consult the backup flag).
harness.charm.patroni_manager.is_creating_backup = False
with harness.hooks_disabled():
harness.update_relation_data(peer_rel_id, harness.charm.unit.name, {"connectivity": "on"})
harness.charm.reset_stale_connectivity_flag()
assert harness.get_relation_data(peer_rel_id, harness.charm.unit)["connectivity"] == "on"


def test_execute_command(harness):
with patch("ops.model.Container.exec") as _exec:
command = ["rm", "-r", "/var/lib/postgresql/16/main"]
Expand Down
Loading