[DPE-3258] Check system identifier in stanza (#318)

marceloneppel · web-flow · commit e509cedf3411 · 2024-01-24T10:22:31.000-03:00
* Check system identifier in stanza and update tests

Signed-off-by: Marcelo Henrique Neppel &lt;marcelo.neppel@canonical.com&gt;

* Fix integration test

Signed-off-by: Marcelo Henrique Neppel &lt;marcelo.neppel@canonical.com&gt;

---------

Signed-off-by: Marcelo Henrique Neppel &lt;marcelo.neppel@canonical.com&gt;
diff --git a/poetry.lock b/poetry.lock
diff --git a/src/backups.py b/src/backups.py
@@ -147,7 +147,23 @@ def can_use_s3_repository(self) -> Tuple[bool, Optional[str]]:
 
         if self.charm.unit.is_leader():
             for stanza in json.loads(stdout):
-                if stanza.get("name") != self.charm.app_peer_data.get("stanza", self.stanza_name):
+                return_code, system_identifier_from_instance, error = self._execute_command(
+                    [
+                        f'/snap/charmed-postgresql/current/usr/lib/postgresql/{self.charm._patroni.get_postgresql_version().split(".")[0]}/bin/pg_controldata',
+                        POSTGRESQL_DATA_PATH,
+                    ]
+                )
+                if return_code != 0:
+                    raise Exception(error)
+                system_identifier_from_instance = [
+                    line
+                    for line in system_identifier_from_instance.splitlines()
+                    if "Database system identifier" in line
+                ][0].split(" ")[-1]
+                system_identifier_from_stanza = str(stanza.get("db")[0]["system-id"])
+                if system_identifier_from_instance != system_identifier_from_stanza or stanza.get(
+                    "name"
+                ) != self.charm.app_peer_data.get("stanza", self.stanza_name):
                     # Prevent archiving of WAL files.
                     self.charm.app_peer_data.update({"stanza": ""})
                     self.charm.update_config()
diff --git a/tests/integration/test_backups.py b/tests/integration/test_backups.py
@@ -227,14 +227,27 @@ async def test_backup(ops_test: OpsTest, cloud_configs: Tuple[Dict, Dict]) -> No
 async def test_restore_on_new_cluster(ops_test: OpsTest, github_secrets) -> None:
     """Test that is possible to restore a backup to another PostgreSQL cluster."""
     charm = await ops_test.build_charm(".")
+    previous_database_app_name = f"{DATABASE_APP_NAME}-gcp"
     database_app_name = f"new-{DATABASE_APP_NAME}"
+    await ops_test.model.deploy(charm, application_name=previous_database_app_name)
     await ops_test.model.deploy(
         charm,
         application_name=database_app_name,
         series=CHARM_SERIES,
     )
+    await ops_test.model.relate(previous_database_app_name, S3_INTEGRATOR_APP_NAME)
     await ops_test.model.relate(database_app_name, S3_INTEGRATOR_APP_NAME)
     async with ops_test.fast_forward():
+        logger.info(
+            "waiting for the database charm to become blocked due to existing backups from another cluster in the repository"
+        )
+        await wait_for_idle_on_blocked(
+            ops_test,
+            previous_database_app_name,
+            2,
+            S3_INTEGRATOR_APP_NAME,
+            ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE,
+        )
         logger.info(
             "waiting for the database charm to become blocked due to existing backups from another cluster in the repository"
         )
@@ -246,6 +259,10 @@ async def test_restore_on_new_cluster(ops_test: OpsTest, github_secrets) -> None
             ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE,
         )
 
+    # Remove the database app with the same name as the previous one (that was used only to test
+    # that the cluster becomes blocked).
+    await ops_test.model.remove_application(previous_database_app_name, block_until_done=True)
+
     # Run the "list backups" action.
     unit_name = f"{database_app_name}/0"
     logger.info("listing the available backups")
diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py
@@ -154,9 +154,15 @@ def test_can_unit_perform_backup(
     @patch("charm.Patroni.reload_patroni_configuration")
     @patch("charm.Patroni.member_started", new_callable=PropertyMock)
     @patch("charm.PostgresqlOperatorCharm.update_config")
+    @patch("charm.Patroni.get_postgresql_version", return_value="14.10")
     @patch("charm.PostgreSQLBackups._execute_command")
     def test_can_use_s3_repository(
-        self, _execute_command, _update_config, _member_started, _reload_patroni_configuration
+        self,
+        _execute_command,
+        _get_postgresql_version,
+        _update_config,
+        _member_started,
+        _reload_patroni_configuration,
     ):
         # Define the stanza name inside the unit relation data.
         with self.harness.hooks_disabled():
@@ -178,13 +184,13 @@ def test_can_use_s3_repository(
             (False, FAILED_TO_INITIALIZE_STANZA_ERROR_MESSAGE),
         )
 
-        # Test when the unit is a replica and there is a backup from another cluster
-        # in the S3 repository.
-        _execute_command.return_value = (
+        # Test when the unit is a replica.
+        pgbackrest_info_same_cluster_backup_output = (
             0,
-            f'[{{"name": "another-model.{self.charm.cluster_name}"}}]',
+            f'[{{"db": [{{"system-id": "12345"}}], "name": "{self.charm.backup.stanza_name}"}}]',
             "",
         )
+        _execute_command.return_value = pgbackrest_info_same_cluster_backup_output
         self.assertEqual(
             self.charm.backup.can_use_s3_repository(),
             (True, None),
@@ -196,10 +202,80 @@ def test_can_use_s3_repository(
             {"stanza": self.charm.backup.stanza_name},
         )
 
-        # Test when the unit is the leader and the workload is running.
+        # Test when the unit is the leader and the workload is running,
+        # but an exception happens when retrieving the cluster system id.
         _member_started.return_value = True
+        _execute_command.side_effect = [
+            pgbackrest_info_same_cluster_backup_output,
+            (1, "", "fake error"),
+        ]
         with self.harness.hooks_disabled():
             self.harness.set_leader()
+        with self.assertRaises(Exception):
+            self.assertEqual(
+                self.charm.backup.can_use_s3_repository(),
+                (False, ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE),
+            )
+        _update_config.assert_not_called()
+        _member_started.assert_not_called()
+        _reload_patroni_configuration.assert_not_called()
+
+        # Test when the cluster system id can be retrieved, but it's different from the stanza system id.
+        pgbackrest_info_other_cluster_system_id_backup_output = (
+            0,
+            f'[{{"db": [{{"system-id": "12345"}}], "name": "{self.charm.backup.stanza_name}"}}]',
+            "",
+        )
+        other_instance_system_identifier_output = (
+            0,
+            "Database system identifier:           67890",
+            "",
+        )
+        _execute_command.side_effect = [
+            pgbackrest_info_other_cluster_system_id_backup_output,
+            other_instance_system_identifier_output,
+        ]
+        with self.harness.hooks_disabled():
+            self.harness.update_relation_data(
+                self.peer_rel_id,
+                self.charm.app.name,
+                {"stanza": self.charm.backup.stanza_name},
+            )
+        self.assertEqual(
+            self.charm.backup.can_use_s3_repository(),
+            (False, ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE),
+        )
+        _update_config.assert_called_once()
+        _member_started.assert_called_once()
+        _reload_patroni_configuration.assert_called_once()
+
+        # Assert that the stanza name is not present in the unit relation data anymore.
+        self.assertEqual(self.harness.get_relation_data(self.peer_rel_id, self.charm.app), {})
+
+        # Test when the cluster system id can be retrieved, but it's different from the stanza system id.
+        _update_config.reset_mock()
+        _member_started.reset_mock()
+        _reload_patroni_configuration.reset_mock()
+        pgbackrest_info_other_cluster_name_backup_output = (
+            0,
+            f'[{{"db": [{{"system-id": "12345"}}], "name": "another-model.{self.charm.cluster_name}"}}]',
+            "",
+        )
+        same_instance_system_identifier_output = (
+            0,
+            "Database system identifier:           12345",
+            "",
+        )
+        _execute_command.side_effect = [
+            pgbackrest_info_other_cluster_name_backup_output,
+            same_instance_system_identifier_output,
+        ]
+        with self.harness.hooks_disabled():
+            self.harness.update_relation_data(
+                self.peer_rel_id,
+                self.charm.app.name,
+                {"stanza": self.charm.backup.stanza_name},
+            )
         self.assertEqual(
             self.charm.backup.can_use_s3_repository(),
             (False, ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE),
@@ -222,6 +298,10 @@ def test_can_use_s3_repository(
                 self.charm.app.name,
                 {"stanza": self.charm.backup.stanza_name},
             )
+        _execute_command.side_effect = [
+            pgbackrest_info_same_cluster_backup_output,
+            other_instance_system_identifier_output,
+        ]
         self.assertEqual(
             self.charm.backup.can_use_s3_repository(),
             (False, ANOTHER_CLUSTER_REPOSITORY_ERROR_MESSAGE),
@@ -240,7 +320,10 @@ def test_can_use_s3_repository(
                 self.charm.app.name,
                 {"stanza": self.charm.backup.stanza_name},
             )
-        _execute_command.return_value = (0, f'[{{"name": "{self.charm.backup.stanza_name}"}}]', "")
+        _execute_command.side_effect = [
+            pgbackrest_info_same_cluster_backup_output,
+            same_instance_system_identifier_output,
+        ]
         self.assertEqual(
             self.charm.backup.can_use_s3_repository(),
             (True, None),
