Skip to content

Commit

Permalink
[MISC] Clean-up duplicated Patroni config reloads (#786)
Browse files Browse the repository at this point in the history
  • Loading branch information
sinclert-canonical authored Nov 27, 2024
1 parent b9f5aa5 commit 773c938
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 25 deletions.
7 changes: 0 additions & 7 deletions src/backups.py
Original file line number Diff line number Diff line change
Expand Up @@ -602,8 +602,6 @@ def check_stanza(self) -> bool:
# for that or else the s3 initialization sequence will fail.
for attempt in Retrying(stop=stop_after_attempt(6), wait=wait_fixed(10), reraise=True):
with attempt:
if self.charm._patroni.member_started:
self.charm._patroni.reload_patroni_configuration()
self._execute_command(["pgbackrest", f"--stanza={self.stanza_name}", "check"])
self.charm._set_active_status()
except Exception as e:
Expand Down Expand Up @@ -642,9 +640,6 @@ def coordinate_stanza_fields(self) -> None:
})

self.charm.update_config()
if self.charm._patroni.member_started:
self.charm._patroni.reload_patroni_configuration()

break

@property
Expand Down Expand Up @@ -739,8 +734,6 @@ def _on_s3_credential_changed(self, event: CredentialsChangedEvent) -> bool | No

def _on_s3_credential_changed_primary(self, event: HookEvent) -> bool:
self.charm.update_config()
if self.charm._patroni.member_started:
self.charm._patroni.reload_patroni_configuration()

try:
self._create_bucket_if_not_exists()
Expand Down
18 changes: 0 additions & 18 deletions tests/unit/test_backups.py
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,6 @@ def test_check_stanza(harness):
with (
patch("charm.PostgresqlOperatorCharm.update_config"),
patch("backups.wait_fixed", return_value=wait_fixed(0)),
patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started,
patch("charm.Patroni.reload_patroni_configuration") as _reload_patroni_configuration,
patch("charm.PostgreSQLBackups._execute_command") as _execute_command,
patch("charm.PostgresqlOperatorCharm._set_active_status") as _set_active_status,
Expand All @@ -782,12 +781,10 @@ def test_check_stanza(harness):
f"--stanza={harness.charm.backup.stanza_name}",
"check",
]
_member_started.return_value = False
_execute_command.side_effect = ExecError(
command=stanza_check_command, exit_code=1, stdout="", stderr="fake error"
)
assert not harness.charm.backup.check_stanza()
_member_started.assert_called()
_reload_patroni_configuration.assert_not_called()
_set_active_status.assert_not_called()
_s3_initialization_set_failure.assert_called_once_with(
Expand All @@ -796,10 +793,8 @@ def test_check_stanza(harness):

_execute_command.reset_mock()
_s3_initialization_set_failure.reset_mock()
_member_started.return_value = True
_execute_command.side_effect = None
assert harness.charm.backup.check_stanza()
_reload_patroni_configuration.assert_called_once()
_execute_command.assert_called_once()
_set_active_status.assert_called_once()
_s3_initialization_set_failure.assert_not_called()
Expand All @@ -816,7 +811,6 @@ def test_check_stanza(harness):
def test_coordinate_stanza_fields(harness):
with (
patch("charm.PostgresqlOperatorCharm.update_config") as _update_config,
patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started,
patch("charm.Patroni.reload_patroni_configuration") as _reload_patroni_configuration,
):
peer_rel_id = harness.model.get_relation(PEER).id
Expand Down Expand Up @@ -852,7 +846,6 @@ def test_coordinate_stanza_fields(harness):

# Test with clear values.
harness.charm.backup.coordinate_stanza_fields()
_member_started.assert_not_called()
assert harness.get_relation_data(peer_rel_id, harness.charm.app) == {}
assert harness.get_relation_data(peer_rel_id, harness.charm.unit) == {}
assert harness.get_relation_data(peer_rel_id, new_unit) == {}
Expand All @@ -862,7 +855,6 @@ def test_coordinate_stanza_fields(harness):
harness.update_relation_data(peer_rel_id, new_unit_name, peer_data_primary_error)
harness.charm.backup.coordinate_stanza_fields()
_update_config.assert_not_called()
_member_started.assert_not_called()
assert harness.get_relation_data(peer_rel_id, harness.charm.app) == {}
assert harness.get_relation_data(peer_rel_id, harness.charm.unit) == {}
assert harness.get_relation_data(peer_rel_id, new_unit) == peer_data_primary_error
Expand All @@ -874,26 +866,22 @@ def test_coordinate_stanza_fields(harness):
)
harness.charm.backup.coordinate_stanza_fields()
_update_config.assert_not_called()
_member_started.assert_not_called()
assert harness.get_relation_data(peer_rel_id, harness.charm.app) == peer_data_leader_start
assert harness.get_relation_data(peer_rel_id, harness.charm.unit) == {}
assert harness.get_relation_data(peer_rel_id, new_unit) == peer_data_primary_error

# Leader should sync fail result from the primary.
_member_started.return_value = False
with harness.hooks_disabled():
harness.set_leader()
harness.charm.backup.coordinate_stanza_fields()
_update_config.assert_called_once()
_member_started.assert_called_once()
_reload_patroni_configuration.assert_not_called()
assert harness.get_relation_data(peer_rel_id, harness.charm.app) == peer_data_leader_error
assert harness.get_relation_data(peer_rel_id, harness.charm.unit) == {}
assert harness.get_relation_data(peer_rel_id, new_unit) == peer_data_primary_error

# Test with successful result from the primary.
_update_config.reset_mock()
_member_started.return_value = True
with harness.hooks_disabled():
harness.update_relation_data(peer_rel_id, harness.charm.app.name, peer_data_clean)
harness.update_relation_data(
Expand All @@ -903,7 +891,6 @@ def test_coordinate_stanza_fields(harness):
harness.update_relation_data(peer_rel_id, new_unit_name, peer_data_primary_ok)
harness.charm.backup.coordinate_stanza_fields()
_update_config.assert_called_once()
_reload_patroni_configuration.assert_called_once()
assert harness.get_relation_data(peer_rel_id, harness.charm.app) == peer_data_leader_ok
assert harness.get_relation_data(peer_rel_id, harness.charm.unit) == {}
assert harness.get_relation_data(peer_rel_id, new_unit) == peer_data_primary_ok
Expand Down Expand Up @@ -1070,7 +1057,6 @@ def test_on_s3_credential_changed(harness):
def test_on_s3_credential_changed_primary(harness):
with (
patch("charm.PostgresqlOperatorCharm.update_config"),
patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started,
patch("charm.Patroni.reload_patroni_configuration") as _reload_patroni_configuration,
patch(
"charm.PostgreSQLBackups._create_bucket_if_not_exists"
Expand All @@ -1089,10 +1075,8 @@ def test_on_s3_credential_changed_primary(harness):
):
mock_event = MagicMock()

_member_started.return_value = False
_create_bucket_if_not_exists.side_effect = ValueError()
assert not harness.charm.backup._on_s3_credential_changed_primary(mock_event)
_member_started.assert_called_once()
_reload_patroni_configuration.assert_not_called()
_create_bucket_if_not_exists.assert_called_once()
_s3_initialization_set_failure.assert_called_once_with(
Expand All @@ -1101,11 +1085,9 @@ def test_on_s3_credential_changed_primary(harness):
_can_use_s3_repository.assert_not_called()

_s3_initialization_set_failure.reset_mock()
_member_started.return_value = True
_create_bucket_if_not_exists.side_effect = None
_can_use_s3_repository.return_value = (False, ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE)
assert not harness.charm.backup._on_s3_credential_changed_primary(mock_event)
_reload_patroni_configuration.assert_called_once()
_can_use_s3_repository.assert_called_once()
_s3_initialization_set_failure.assert_called_once_with(
ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE
Expand Down

0 comments on commit 773c938

Please sign in to comment.