Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 3 additions & 6 deletions src/backups.py
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ def _on_s3_credentials_checks(self, event: CredentialsChangedEvent) -> bool:
logger.debug("_on_s3_credential_changed early exit: no connection info")
return False

if "cluster_initialised" not in self.charm.app_peer_data:
if not self.charm.is_cluster_initialised:
logger.debug("Cannot set pgBackRest configurations, PostgreSQL has not yet started.")
event.defer()
return False
Expand All @@ -681,10 +681,7 @@ def _on_s3_credentials_checks(self, event: CredentialsChangedEvent) -> bool:
return False

# Prevents S3 change in the middle of restoring backup and patroni / pgbackrest errors caused by that.
if (
"restoring-backup" in self.charm.app_peer_data
or "restore-to-time" in self.charm.app_peer_data
):
if self.charm.is_cluster_restoring_backup or self.charm.is_cluster_restoring_to_time:
logger.info("Cannot change S3 configuration during restore")
event.defer()
return False
Expand Down Expand Up @@ -1014,7 +1011,7 @@ def _on_restore_action(self, event): # noqa: C901
)
except ApiError as e:
# If previous PITR restore was unsuccessful, there are no such endpoints.
if "restore-to-time" not in self.charm.app_peer_data:
if not self.charm.is_cluster_restoring_to_time:
error_message = f"Failed to remove previous cluster information with error: {e!s}"
logger.error(f"Restore failed: {error_message}")
event.fail(error_message)
Expand Down
44 changes: 32 additions & 12 deletions src/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,26 @@ def is_cluster_initialised(self) -> bool:
"""Returns whether the cluster is already initialised."""
return "cluster_initialised" in self.app_peer_data

@property
def is_cluster_restoring_backup(self) -> bool:
"""Returns whether the cluster is restoring a backup."""
return "restoring-backup" in self.app_peer_data

@property
def is_cluster_restoring_to_time(self) -> bool:
"""Returns whether the cluster is restoring a backup to a specific time."""
return "restore-to-time" in self.app_peer_data

@property
def is_unit_departing(self) -> bool:
"""Returns whether the unit is departing."""
return "departing" in self.unit_peer_data

@property
def is_unit_stopped(self) -> bool:
"""Returns whether the unit is stopped."""
return "stopped" in self.unit_peer_data

@property
def postgresql(self) -> PostgreSQL:
"""Returns an instance of the object used to interact with the database."""
Expand Down Expand Up @@ -456,7 +476,7 @@ def _on_peer_relation_departed(self, event: RelationDepartedEvent) -> None:
if not self.unit.is_leader() or event.departing_unit == self.unit:
return

if "cluster_initialised" not in self._peers.data[self.app]:
if not self.is_cluster_initialised:
logger.debug(
"Deferring on_peer_relation_departed: Cluster must be initialized before members can leave"
)
Expand Down Expand Up @@ -521,7 +541,7 @@ def _on_peer_relation_changed(self, event: HookEvent) -> None: # noqa: C901
"""Reconfigure cluster members."""
# The cluster must be initialized first in the leader unit
# before any other member joins the cluster.
if "cluster_initialised" not in self._peers.data[self.app]:
if not self.is_cluster_initialised:
if self.unit.is_leader():
if self._initialize_cluster(event):
logger.debug("Deferring on_peer_relation_changed: Leader initialized cluster")
Expand Down Expand Up @@ -566,7 +586,7 @@ def _on_peer_relation_changed(self, event: HookEvent) -> None: # noqa: C901

services = container.pebble.get_services(names=[self._postgresql_service])
if (
("restoring-backup" in self.app_peer_data or "restore-to-time" in self.app_peer_data)
(self.is_cluster_restoring_backup or self.is_cluster_restoring_to_time)
and len(services) > 0
and not self._was_restore_successful(container, services[0])
):
Expand Down Expand Up @@ -769,7 +789,7 @@ def _add_members(self, event) -> None:

# Reconfiguration can be successful only if the cluster is initialised
# (i.e. first unit has bootstrap the cluster).
if "cluster_initialised" not in self._peers.data[self.app]:
if not self.is_cluster_initialised:
return

try:
Expand Down Expand Up @@ -941,7 +961,7 @@ def _on_postgresql_pebble_ready(self, event: WorkloadEvent) -> None:
# Otherwise, each unit will create a different cluster and
# any update in the members list on the units won't have effect
# on fixing that.
if not self.unit.is_leader() and "cluster_initialised" not in self._peers.data[self.app]:
if not self.unit.is_leader() and not self.is_cluster_initialised:
logger.debug(
"Deferring on_postgresql_pebble_ready: Not leader and cluster not initialized"
)
Expand Down Expand Up @@ -1171,8 +1191,8 @@ def _has_non_restore_waiting_status(self) -> bool:
"""Returns whether the unit is in a waiting state and there is no restore process ongoing."""
return (
isinstance(self.unit.status, WaitingStatus)
and "restoring-backup" not in self.app_peer_data
and "restore-to-time" not in self.app_peer_data
and not self.is_cluster_restoring_backup
and not self.is_cluster_restoring_to_time
)

def _on_get_password(self, event: ActionEvent) -> None:
Expand Down Expand Up @@ -1420,9 +1440,9 @@ def _on_update_status(self, _) -> None:
return

if (
"restoring-backup" not in self.app_peer_data
and "restore-to-time" not in self.app_peer_data
and "stopped" not in self.unit_peer_data
not self.is_cluster_restoring_backup
and not self.is_cluster_restoring_to_time
and not self.is_unit_stopped
and services[0].current != ServiceStatus.ACTIVE
):
logger.warning(
Expand All @@ -1438,7 +1458,7 @@ def _on_update_status(self, _) -> None:
return

if (
"restoring-backup" in self.app_peer_data or "restore-to-time" in self.app_peer_data
self.is_cluster_restoring_backup or self.is_cluster_restoring_to_time
) and not self._was_restore_successful(container, services[0]):
return

Expand All @@ -1454,7 +1474,7 @@ def _on_update_status(self, _) -> None:

def _was_restore_successful(self, container: Container, service: ServiceInfo) -> bool:
"""Checks if restore operation succeeded and S3 is properly configured."""
if "restore-to-time" in self.app_peer_data and all(self.is_pitr_failed(container)):
if self.is_cluster_restoring_to_time and all(self.is_pitr_failed(container)):
logger.error(
"Restore failed: database service failed to reach point-in-time-recovery target. "
"You can launch another restore with different parameters"
Expand Down
11 changes: 4 additions & 7 deletions src/relations/async_replication.py
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ def is_primary_cluster(self) -> bool:
return self.charm.app == self.get_primary_cluster()

def _on_async_relation_broken(self, _) -> None:
if self.charm._peers is None or "departing" in self.charm._peers.data[self.charm.unit]:
if self.charm._peers is None or self.charm.is_unit_departing:
logger.debug("Early exit on_async_relation_broken: Skipping departing unit.")
return

Expand Down Expand Up @@ -509,11 +509,11 @@ def _on_async_relation_changed(self, event: RelationChangedEvent) -> None:
if not self._stop_database(event):
return

if not all(
if not (self.charm.is_unit_stopped or self._is_following_promoted_cluster()) or not all(
"stopped" in self.charm._peers.data[unit]
or self.charm._peers.data[unit].get("unit-promoted-cluster-counter")
== self._get_highest_promoted_cluster_counter_value()
for unit in {*self.charm._peers.units, self.charm.unit}
for unit in self.charm._peers.units
Comment on lines +512 to +516
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review these lines carefully. I think I preserved the original logic

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so too.

):
self.charm.unit.status = WaitingStatus(
"Waiting for the database to be stopped in all units"
Expand Down Expand Up @@ -692,10 +692,7 @@ def _set_app_status(self) -> None:

def _stop_database(self, event: RelationChangedEvent) -> bool:
"""Stop the database."""
if (
"stopped" not in self.charm._peers.data[self.charm.unit]
and not self._is_following_promoted_cluster()
):
if not self.charm.is_unit_stopped and not self._is_following_promoted_cluster():
if not self.charm.unit.is_leader() and not self.container.exists(POSTGRESQL_DATA_PATH):
logger.debug("Early exit on_async_relation_changed: following promoted cluster.")
return False
Expand Down
17 changes: 4 additions & 13 deletions src/relations/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,7 @@ def _on_relation_changed(self, event: RelationChangedEvent) -> None:
Generate password and handle user and database creation for the related application.
"""
# Check for some conditions before trying to access the PostgreSQL instance.
if (
"cluster_initialised" not in self.charm._peers.data[self.charm.app]
or not self.charm._patroni.member_started
):
if not self.charm.is_cluster_initialised or not self.charm._patroni.member_started:
logger.debug(
"Deferring on_relation_changed: Cluster not initialized or patroni not running"
)
Expand Down Expand Up @@ -271,10 +268,7 @@ def _on_relation_departed(self, event: RelationDepartedEvent) -> None:
Remove unit name from allowed_units key.
"""
# Check for some conditions before trying to access the PostgreSQL instance.
if (
"cluster_initialised" not in self.charm._peers.data[self.charm.app]
or not self.charm._patroni.member_started
):
if not self.charm.is_cluster_initialised or not self.charm._patroni.member_started:
logger.debug(
"Deferring on_relation_departed: Cluster not initialized or patroni not running"
)
Expand Down Expand Up @@ -309,17 +303,14 @@ def _on_relation_departed(self, event: RelationDepartedEvent) -> None:
def _on_relation_broken(self, event: RelationBrokenEvent) -> None:
"""Remove the user created for this relation."""
# Check for some conditions before trying to access the PostgreSQL instance.
if (
"cluster_initialised" not in self.charm._peers.data[self.charm.app]
or not self.charm._patroni.member_started
):
if not self.charm.is_cluster_initialised or not self.charm._patroni.member_started:
logger.debug(
"Deferring on_relation_broken: Cluster not initialized or patroni not running"
)
event.defer()
return

if "departing" in self.charm._peers.data[self.charm.unit]:
if self.charm.is_unit_departing:
logger.debug("Early exit on_relation_broken: Skipping departing unit")
return

Expand Down
9 changes: 3 additions & 6 deletions src/relations/postgresql_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,7 @@ def _on_database_requested(self, event: DatabaseRequestedEvent) -> None:
Generate password and handle user and database creation for the related application.
"""
# Check for some conditions before trying to access the PostgreSQL instance.
if (
"cluster_initialised" not in self.charm._peers.data[self.charm.app]
or not self.charm._patroni.member_started
):
if not self.charm.is_cluster_initialised or not self.charm._patroni.member_started:
logger.debug(
"Deferring on_database_requested: Cluster must be initialized before database can be requested"
)
Expand Down Expand Up @@ -159,7 +156,7 @@ def _on_relation_broken(self, event: RelationBrokenEvent) -> None:
# Check for some conditions before trying to access the PostgreSQL instance.
if (
not self.charm._peers
or "cluster_initialised" not in self.charm._peers.data[self.charm.app]
or not self.charm.is_cluster_initialised
or not self.charm._patroni.member_started
):
logger.debug(
Expand All @@ -170,7 +167,7 @@ def _on_relation_broken(self, event: RelationBrokenEvent) -> None:

self._update_unit_status(event.relation)

if "departing" in self.charm._peers.data[self.charm.unit]:
if self.charm.is_unit_departing:
logger.debug("Early exit on_relation_broken: Skipping departing unit")
return

Expand Down
6 changes: 4 additions & 2 deletions tests/unit/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,9 @@ def test_on_peer_relation_departed(harness):
patch(
"charm.PostgresqlOperatorCharm._get_endpoints_to_remove"
) as _get_endpoints_to_remove,
patch("charm.PostgresqlOperatorCharm._peers", new_callable=PropertyMock) as _peers,
patch(
"charm.PostgresqlOperatorCharm.app_peer_data", new_callable=PropertyMock
) as _app_peer_data,
patch(
"charm.PostgresqlOperatorCharm._get_endpoints_to_remove", return_value=sentinel.units
) as _get_endpoints_to_remove,
Expand All @@ -680,7 +682,7 @@ def test_on_peer_relation_departed(harness):
harness.charm._on_peer_relation_departed(event)
event.defer.assert_called_once_with()

_peers.return_value.data = {harness.charm.app: {"cluster_initialised": True}}
_app_peer_data.return_value = {"cluster_initialised": True}
harness.charm._on_peer_relation_departed(event)
_get_endpoints_to_remove.assert_called_once_with()
_remove_from_endpoints.assert_called_once_with(sentinel.units)
Expand Down
Loading