From a9044f70815a7176f97fd5c5f341c4f302e4ae26 Mon Sep 17 00:00:00 2001 From: Dragomir Penev <6687393+dragomirp@users.noreply.github.com> Date: Tue, 12 Sep 2023 03:23:26 +0300 Subject: [PATCH] [DPE-2546] Split stanza create and stanza check (#240) * Split stanza create and stanza check * Remove force param for pgbackrest server * Code review tweaks --- src/backups.py | 16 ++++++--- src/charm.py | 4 ++- tests/unit/test_backups.py | 73 +++++++++++++++++++++++++++----------- 3 files changed, 67 insertions(+), 26 deletions(-) diff --git a/src/backups.py b/src/backups.py index 5c9927bf7d..be386ee4a0 100644 --- a/src/backups.py +++ b/src/backups.py @@ -299,12 +299,21 @@ def _initialise_stanza(self) -> None: self.charm.unit.status = BlockedStatus(FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) return + self.start_stop_pgbackrest_service() + # Store the stanza name to be used in configurations updates. - self.charm.app_peer_data.update({"stanza": self.stanza_name}) + self.charm.app_peer_data.update({"stanza": self.stanza_name, "init-pgbackrest": "True"}) + + def check_stanza(self) -> None: + """Runs the pgbackrest stanza validation.""" + if not self.charm.unit.is_leader() or "init-pgbackrest" not in self.charm.app_peer_data: + return # Update the configuration to use pgBackRest as the archiving mechanism. self.charm.update_config() + self.charm.unit.status = MaintenanceStatus("checking stanza") + try: # Check that the stanza is correctly configured. for attempt in Retrying(stop=stop_after_attempt(5), wait=wait_fixed(3)): @@ -317,13 +326,14 @@ def _initialise_stanza(self) -> None: # If the check command doesn't succeed, remove the stanza name # and rollback the configuration. self.charm.app_peer_data.update({"stanza": ""}) + self.charm.app_peer_data.pop("init-pgbackrest", None) self.charm.update_config() logger.exception(e) self.charm.unit.status = BlockedStatus(FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) return - return + self.charm.app_peer_data.pop("init-pgbackrest", None) @property def _is_primary_pgbackrest_service_running(self) -> bool: @@ -372,8 +382,6 @@ def _on_s3_credential_changed(self, event: CredentialsChangedEvent): self._initialise_stanza() - self.start_stop_pgbackrest_service() - def _on_create_backup_action(self, event) -> None: """Request that pgBackRest creates a backup.""" can_unit_perform_backup, validation_message = self._can_unit_perform_backup() diff --git a/src/charm.py b/src/charm.py index 1cefb9a763..91dd2a91bc 100755 --- a/src/charm.py +++ b/src/charm.py @@ -464,6 +464,8 @@ def _on_peer_relation_changed(self, event: HookEvent) -> None: self.postgresql_client_relation.update_read_only_endpoint() + self.backup.check_stanza() + # Start or stop the pgBackRest TLS server service when TLS certificate change. if not self.backup.start_stop_pgbackrest_service(): # Ping primary to start its TLS server. @@ -474,7 +476,7 @@ def _on_peer_relation_changed(self, event: HookEvent) -> None: event.defer() return else: - self.unit_peer_data.pop("start-tls.server", None) + self.unit_peer_data.pop("start-tls-server", None) if not self.is_blocked: self.unit.status = ActiveStatus() diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index 5c72598f4e..9115d5aa0b 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -517,35 +517,73 @@ def test_initialise_stanza( # Assert there is no stanza name in the application relation databag. self.assertEqual(self.harness.get_relation_data(self.peer_rel_id, self.charm.app), {}) - # Test when the stanza creation succeeds, but the archiving is not working correctly - # (pgBackRest check command fails). + # Test when the archiving is working correctly (pgBackRest check command succeeds). _execute_command.reset_mock() - _execute_command.side_effect = [ - None, - ExecError( - command=stanza_creation_command, exit_code=1, stdout="", stderr="fake error" - ), + _update_config.reset_mock() + _member_started.reset_mock() + _reload_patroni_configuration.reset_mock() + _execute_command.side_effect = None + self.charm.backup._initialise_stanza() + self.assertEqual( + self.harness.get_relation_data(self.peer_rel_id, self.charm.app), + {"stanza": "None.patroni-postgresql-k8s", "init-pgbackrest": "True"}, + ) + self.assertIsInstance(self.charm.unit.status, MaintenanceStatus) + + @patch("charm.Patroni.reload_patroni_configuration") + @patch("charm.Patroni.member_started", new_callable=PropertyMock) + @patch("backups.wait_fixed", return_value=wait_fixed(0)) + @patch("charm.PostgresqlOperatorCharm.update_config") + @patch("charm.PostgreSQLBackups._execute_command") + def test_check_stanza( + self, _execute_command, _update_config, _, _member_started, _reload_patroni_configuration + ): + # Set peer data flag + with self.harness.hooks_disabled(): + self.harness.update_relation_data( + self.peer_rel_id, + self.charm.app.name, + {"init-pgbackrest": "True"}, + ) + + # Test when the unit is not the leader. + self.charm.backup.check_stanza() + _execute_command.assert_not_called() + + # Set the unit as leader + with self.harness.hooks_disabled(): + self.harness.set_leader() + + stanza_check_command = [ + "pgbackrest", + f"--stanza={self.charm.backup.stanza_name}", + "check", ] + # Test when the archiving is not working correctly (pgBackRest check command fails). + _execute_command.side_effect = ExecError( + command=stanza_check_command, exit_code=1, stdout="", stderr="fake error" + ) _member_started.return_value = True - self.charm.backup._initialise_stanza() + self.charm.backup.check_stanza() self.assertEqual(_update_config.call_count, 2) - self.assertEqual(self.harness.get_relation_data(self.peer_rel_id, self.charm.app), {}) self.assertEqual(_member_started.call_count, 5) self.assertEqual(_reload_patroni_configuration.call_count, 5) self.assertIsInstance(self.charm.unit.status, BlockedStatus) self.assertEqual(self.charm.unit.status.message, FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE) # Test when the archiving is working correctly (pgBackRest check command succeeds). + with self.harness.hooks_disabled(): + self.harness.update_relation_data( + self.peer_rel_id, + self.charm.app.name, + {"init-pgbackrest": "True"}, + ) _execute_command.reset_mock() _update_config.reset_mock() _member_started.reset_mock() _reload_patroni_configuration.reset_mock() _execute_command.side_effect = None - self.charm.backup._initialise_stanza() - self.assertEqual( - self.harness.get_relation_data(self.peer_rel_id, self.charm.app), - {"stanza": self.charm.backup.stanza_name}, - ) + self.charm.backup.check_stanza() _update_config.assert_called_once() _member_started.assert_called_once() _reload_patroni_configuration.assert_called_once() @@ -576,7 +614,6 @@ def test_is_primary_pgbackrest_service_running( self.assertEqual(self.charm.backup._is_primary_pgbackrest_service_running, True) _execute_command.assert_called_once() - @patch("charm.PostgreSQLBackups.start_stop_pgbackrest_service") @patch("charm.PostgreSQLBackups._initialise_stanza") @patch("charm.PostgreSQLBackups.can_use_s3_repository") @patch("charm.PostgreSQLBackups._create_bucket_if_not_exists") @@ -589,7 +626,6 @@ def test_on_s3_credential_changed( _create_bucket_if_not_exists, _can_use_s3_repository, _initialise_stanza, - _start_stop_pgbackrest_service, ): # Test when the cluster was not initialised yet. self.relate_to_s3_integrator() @@ -601,7 +637,6 @@ def test_on_s3_credential_changed( _create_bucket_if_not_exists.assert_not_called() _can_use_s3_repository.assert_not_called() _initialise_stanza.assert_not_called() - _start_stop_pgbackrest_service.assert_not_called() # Test when the cluster is already initialised, but the charm fails to render # the pgBackRest configuration file due to missing S3 parameters. @@ -621,7 +656,6 @@ def test_on_s3_credential_changed( _create_bucket_if_not_exists.assert_not_called() _can_use_s3_repository.assert_not_called() _initialise_stanza.assert_not_called() - _start_stop_pgbackrest_service.assert_not_called() # Test when the charm render the pgBackRest configuration file, but fails to # access or create the S3 bucket. @@ -654,7 +688,6 @@ def test_on_s3_credential_changed( ) _can_use_s3_repository.assert_not_called() _initialise_stanza.assert_not_called() - _start_stop_pgbackrest_service.assert_not_called() # Test when it's not possible to use the S3 repository due to backups from another cluster. _create_bucket_if_not_exists.reset_mock() @@ -668,7 +701,6 @@ def test_on_s3_credential_changed( _create_bucket_if_not_exists.assert_called_once() _can_use_s3_repository.assert_called_once() _initialise_stanza.assert_not_called() - _start_stop_pgbackrest_service.assert_not_called() # Test when the stanza can be initialised and the pgBackRest service can start. _can_use_s3_repository.reset_mock() @@ -678,7 +710,6 @@ def test_on_s3_credential_changed( ) _can_use_s3_repository.assert_called_once() _initialise_stanza.assert_called_once() - _start_stop_pgbackrest_service.assert_called_once() @patch("charm.PostgresqlOperatorCharm.update_config") @patch("charm.PostgreSQLBackups._change_connectivity_to_database")