Skip to content
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.

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

[DPE-4453] Fix scale up with S3 and TLS relations #480

Merged
merged 1 commit into from
May 31, 2024
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
21 changes: 21 additions & 0 deletions src/backups.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,22 @@ def _are_backup_settings_ok(self) -> Tuple[bool, Optional[str]]:

return True, None

@property
def _can_initialise_stanza(self) -> bool:
"""Validates whether this unit can initialise a stanza."""
# Don't allow stanza initialisation if this unit hasn't started the database
# yet and either hasn't joined the peer relation yet or hasn't configured TLS
# yet while other unit already has TLS enabled.
if not self.charm._patroni.member_started and (
(len(self.charm._peers.data.keys()) == 2)
or (
"tls" not in self.charm.unit_peer_data
and any("tls" in unit_data for _, unit_data in self.charm._peers.data.items())
)
):
return False
return True

def _can_unit_perform_backup(self) -> Tuple[bool, Optional[str]]:
"""Validates whether this unit can perform a backup."""
if self.charm.is_blocked:
Expand Down Expand Up @@ -512,6 +528,11 @@ def _on_s3_credential_changed(self, event: CredentialsChangedEvent):
logger.debug("Cannot set pgBackRest configurations, missing configurations.")
return

if not self._can_initialise_stanza:
logger.debug("Cannot initialise stanza yet.")
Copy link
Member

Choose a reason for hiding this comment

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

Should this be debug level only? It feels more important than this to me, maybe info level? Up to you!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think DEBUG is okay for this case. Like the other messages we already have for similar situations, which don't provide immediate value for the user but only when troubleshooting, I think it's okay as it is.

event.defer()
return

# Verify the s3 relation only on the primary.
if not self.charm.is_primary:
return
Expand Down
53 changes: 50 additions & 3 deletions tests/unit/test_backups.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,39 @@ def test_are_backup_settings_ok(harness):
)


def test_can_initialise_stanza(harness):
with patch("charm.Patroni.member_started", new_callable=PropertyMock) as _member_started:
# Test when Patroni or PostgreSQL hasn't started yet
# and the unit hasn't joined the peer relation yet.
_member_started.return_value = False
tc.assertEqual(
harness.charm.backup._can_initialise_stanza,
False,
)

# Test when the unit hasn't configured TLS yet while other unit already has TLS enabled.
harness.add_relation_unit(
harness.model.get_relation(PEER).id, f"{harness.charm.app.name}/1"
)
with harness.hooks_disabled():
harness.update_relation_data(
harness.model.get_relation(PEER).id,
f"{harness.charm.app.name}/1",
{"tls": "enabled"},
)
tc.assertEqual(
harness.charm.backup._can_initialise_stanza,
False,
)

# Test when everything is ok to initialise the stanza.
_member_started.return_value = True
tc.assertEqual(
harness.charm.backup._can_initialise_stanza,
True,
)


@patch_network_get(private_address="1.1.1.1")
def test_can_unit_perform_backup(harness):
with (
Expand Down Expand Up @@ -926,6 +959,9 @@ def test_on_s3_credential_changed(harness):
patch(
"charm.PostgresqlOperatorCharm.is_primary", new_callable=PropertyMock
) as _is_primary,
patch(
"charm.PostgreSQLBackups._can_initialise_stanza", new_callable=PropertyMock
) as _can_initialise_stanza,
patch(
"charm.PostgreSQLBackups._render_pgbackrest_conf_file"
) as _render_pgbackrest_conf_file,
Expand Down Expand Up @@ -953,31 +989,42 @@ def test_on_s3_credential_changed(harness):
{"cluster_initialised": "True"},
)
_render_pgbackrest_conf_file.return_value = False
_is_primary.return_value = False
harness.charm.backup.s3_client.on.credentials_changed.emit(
relation=harness.model.get_relation(S3_PARAMETERS_RELATION, s3_rel_id)
)
_defer.assert_not_called()
_render_pgbackrest_conf_file.assert_called_once()
_can_initialise_stanza.assert_not_called()
_create_bucket_if_not_exists.assert_not_called()
_can_use_s3_repository.assert_not_called()
_initialise_stanza.assert_not_called()

# Test when it's not possible to initialise the stanza in this unit.
_render_pgbackrest_conf_file.return_value = True
_can_initialise_stanza.return_value = False
harness.charm.backup.s3_client.on.credentials_changed.emit(
relation=harness.model.get_relation(S3_PARAMETERS_RELATION, s3_rel_id)
)
_defer.assert_called_once()
_can_initialise_stanza.assert_called_once()
_is_primary.assert_not_called()

# Test that followers will not initialise the bucket
harness.charm.unit.status = ActiveStatus()
_render_pgbackrest_conf_file.reset_mock()
_can_initialise_stanza.return_value = True
_is_primary.return_value = False
with harness.hooks_disabled():
harness.update_relation_data(
peer_rel_id,
harness.charm.app.name,
{"cluster_initialised": "True"},
)
_render_pgbackrest_conf_file.return_value = True

harness.charm.backup.s3_client.on.credentials_changed.emit(
relation=harness.model.get_relation(S3_PARAMETERS_RELATION, s3_rel_id)
)
_render_pgbackrest_conf_file.assert_called_once()
_is_primary.assert_called_once()
_create_bucket_if_not_exists.assert_not_called()
tc.assertIsInstance(harness.charm.unit.status, ActiveStatus)
_can_use_s3_repository.assert_not_called()
Expand Down
Loading