[DPE-4479][DPE-4625] Make list-backups action conformant + type check (#522)

* add list backup extra info + type check

* fix unit test

* revert unit test time

* add libpq connection string

* Revert "add libpq connection string"

This reverts commit a6d3c56.

Author: Lucas Gameiro

src/backups.py

@@ -342,11 +342,43 @@ def result():
 
     def _format_backup_list(self, backup_list) -> str:
         """Formats provided list of backups as a table."""
-        backups = ["{:<21s} | {:<12s} | {:s}".format("backup-id", "backup-type", "backup-status")]
-        backups.append("-" * len(backups[0]))
-        for backup_id, backup_type, backup_status in backup_list:
+        s3_parameters, _ = self._retrieve_s3_parameters()
+        backups = [
+            "Storage bucket name: {:s}".format(s3_parameters["bucket"]),
+            "Backups base path: {:s}/backup/\n".format(s3_parameters["path"]),
+            "{:<20s} | {:<12s} | {:<8s} | {:<20s} | {:<23s} | {:<20s} | {:<20s} | {:s}".format(
+                "backup-id",
+                "type",
+                "status",
+                "reference-backup-id",
+                "LSN start/stop",
+                "start-time",
+                "finish-time",
+                "backup-path",
+            ),
+        ]
+        backups.append("-" * len(backups[2]))
+        for (
+            backup_id,
+            backup_type,
+            backup_status,
+            reference,
+            lsn_start_stop,
+            start,
+            stop,
+            path,
+        ) in backup_list:
             backups.append(
-                "{:<21s} | {:<12s} | {:s}".format(backup_id, backup_type, backup_status)
+                "{:<20s} | {:<12s} | {:<8s} | {:<20s} | {:<23s} | {:<20s} | {:<20s} | {:s}".format(
+                    backup_id,
+                    backup_type,
+                    backup_status,
+                    reference,
+                    lsn_start_stop,
+                    start,
+                    stop,
+                    path,
+                )
             )
         return "\n".join(backups)
 
@@ -368,11 +400,29 @@ def _generate_backup_list_output(self) -> str:
         backups = json.loads(output)[0]["backup"]
         for backup in backups:
             backup_id, backup_type = self._parse_backup_id(backup["label"])
+            backup_reference = "None"
+            if backup["reference"]:
+                backup_reference, _ = self._parse_backup_id(backup["reference"][-1])
+            lsn_start_stop = f'{backup["lsn"]["start"]} / {backup["lsn"]["stop"]}'
+            time_start, time_stop = (
+                datetime.strftime(datetime.fromtimestamp(stamp), "%Y-%m-%dT%H:%M:%SZ")
+                for stamp in backup["timestamp"].values()
+            )
+            backup_path = f'/{self.stanza_name}/{backup["label"]}'
             error = backup["error"]
             backup_status = "finished"
             if error:
                 backup_status = f"failed: {error}"
-            backup_list.append((backup_id, backup_type, backup_status))
+            backup_list.append((
+                backup_id,
+                backup_type,
+                backup_status,
+                backup_reference,
+                lsn_start_stop,
+                time_start,
+                time_stop,
+                backup_path,
+            ))
         return self._format_backup_list(backup_list)
 
     def _list_backups(self, show_failed: bool, parse=True) -> OrderedDict[str, str]:
@@ -634,6 +684,17 @@ def _on_create_backup_action(self, event) -> None:  # noqa: C901
             event.fail(error_message)
             return
 
+        if (
+            backup_type in ["differential", "incremental"]
+            and len(self._list_backups(show_failed=False)) == 0
+        ):
+            error_message = (
+                f"Invalid backup type: {backup_type}. No previous full backup to reference."
+            )
+            logger.error(f"Backup failed: {error_message}")
+            event.fail(error_message)
+            return
+
         logger.info(f"A {backup_type} backup has been requested on unit")
         can_unit_perform_backup, validation_message = self._can_unit_perform_backup()
         if not can_unit_perform_backup:

tests/integration/helpers.py

@@ -1150,8 +1150,8 @@ async def backup_operations(
     action = await ops_test.model.units.get(replica).run_action("list-backups")
     await action.wait()
     backups = action.results.get("backups")
-    # 2 lines for header output, 1 backup line ==> 3 total lines
-    assert len(backups.split("\n")) == 3, "full backup is not outputted"
+    # 5 lines for header output, 1 backup line ==> 6 total lines
+    assert len(backups.split("\n")) == 6, "full backup is not outputted"
     await ops_test.model.wait_for_idle(status="active", timeout=1000)
 
     # Write some data.
@@ -1177,8 +1177,8 @@ async def backup_operations(
     action = await ops_test.model.units.get(replica).run_action("list-backups")
     await action.wait()
     backups = action.results.get("backups")
-    # 2 lines for header output, 2 backup lines ==> 4 total lines
-    assert len(backups.split("\n")) == 4, "differential backup is not outputted"
+    # 5 lines for header output, 2 backup lines ==> 7 total lines
+    assert len(backups.split("\n")) == 7, "differential backup is not outputted"
     await ops_test.model.wait_for_idle(status="active", timeout=1000)
 
     # Write some data.

tests/unit/test_backups.py

@@ -602,49 +602,98 @@ def test_execute_command(harness):
 
 
 def test_format_backup_list(harness):
-    # Test when there are no backups.
-    tc.assertEqual(
-        harness.charm.backup._format_backup_list([]),
-        """backup-id             | backup-type  | backup-status
-----------------------------------------------------""",
-    )
+    with patch(
+        "charms.data_platform_libs.v0.s3.S3Requirer.get_s3_connection_info"
+    ) as _get_s3_connection_info:
+        # Test when there are no backups.
+        _get_s3_connection_info.return_value = {
+            "bucket": " /test-bucket/ ",
+            "access-key": " test-access-key ",
+            "secret-key": " test-secret-key ",
+            "path": " test-path/ ",
+        }
+        assert (
+            harness.charm.backup._format_backup_list([])
+            == """Storage bucket name: test-bucket
+Backups base path: /test-path/backup/
 
-    # Test when there are backups.
-    backup_list = [
-        ("2023-01-01T09:00:00Z", "full", "failed: fake error"),
-        ("2023-01-01T10:00:00Z", "full", "finished"),
-    ]
-    tc.assertEqual(
-        harness.charm.backup._format_backup_list(backup_list),
-        """backup-id             | backup-type  | backup-status
-----------------------------------------------------
-2023-01-01T09:00:00Z  | full         | failed: fake error
-2023-01-01T10:00:00Z  | full         | finished""",
-    )
+backup-id            | type         | status   | reference-backup-id  | LSN start/stop          | start-time           | finish-time          | backup-path
+-----------------------------------------------------------------------------------------------------------------------------------------------------------"""
+        )
+
+        # Test when there are backups.
+        backup_list = [
+            (
+                "2023-01-01T09:00:00Z",
+                "full",
+                "failed: fake error",
+                "None",
+                "0/3000000 / 0/5000000",
+                "2023-01-01T09:00:00Z",
+                "2023-01-01T09:00:05Z",
+                "a/b/c",
+            ),
+            (
+                "2023-01-01T10:00:00Z",
+                "full",
+                "finished",
+                "None",
+                "0/5000000 / 0/7000000",
+                "2023-01-01T10:00:00Z",
+                "2023-01-01T010:00:07Z",
+                "a/b/d",
+            ),
+        ]
+        assert (
+            harness.charm.backup._format_backup_list(backup_list)
+            == """Storage bucket name: test-bucket
+Backups base path: /test-path/backup/
+
+backup-id            | type         | status   | reference-backup-id  | LSN start/stop          | start-time           | finish-time          | backup-path
+-----------------------------------------------------------------------------------------------------------------------------------------------------------
+2023-01-01T09:00:00Z | full         | failed: fake error | None                 | 0/3000000 / 0/5000000   | 2023-01-01T09:00:00Z | 2023-01-01T09:00:05Z | a/b/c
+2023-01-01T10:00:00Z | full         | finished | None                 | 0/5000000 / 0/7000000   | 2023-01-01T10:00:00Z | 2023-01-01T010:00:07Z | a/b/d"""
+        )
 
 
 def test_generate_backup_list_output(harness):
-    with patch("charm.PostgreSQLBackups._execute_command") as _execute_command:
+    with (
+        patch(
+            "charms.data_platform_libs.v0.s3.S3Requirer.get_s3_connection_info"
+        ) as _get_s3_connection_info,
+        patch("charm.PostgreSQLBackups._execute_command") as _execute_command,
+    ):
+        _get_s3_connection_info.return_value = {
+            "bucket": " /test-bucket/ ",
+            "access-key": " test-access-key ",
+            "secret-key": " test-secret-key ",
+            "path": " test-path/ ",
+        }
         # Test when no backups are returned.
         _execute_command.return_value = (0, '[{"backup":[]}]', "")
-        tc.assertEqual(
-            harness.charm.backup._generate_backup_list_output(),
-            """backup-id             | backup-type  | backup-status
-----------------------------------------------------""",
+        assert (
+            harness.charm.backup._generate_backup_list_output()
+            == """Storage bucket name: test-bucket
+Backups base path: /test-path/backup/
+
+backup-id            | type         | status   | reference-backup-id  | LSN start/stop          | start-time           | finish-time          | backup-path
+-----------------------------------------------------------------------------------------------------------------------------------------------------------"""
         )
 
         # Test when backups are returned.
         _execute_command.return_value = (
             0,
-            '[{"backup":[{"label":"20230101-090000F","error":"fake error"},{"label":"20230101-100000F","error":null}]}]',
+            '[{"backup":[{"label":"20230101-090000F","error":"fake error","reference":null,"lsn":{"start":"0/3000000","stop":"0/5000000"},"timestamp":{"start":1719866711,"stop":1719866714}}]}]',
             "",
         )
-        tc.assertEqual(
-            harness.charm.backup._generate_backup_list_output(),
-            """backup-id             | backup-type  | backup-status
-----------------------------------------------------
-2023-01-01T09:00:00Z  | full         | failed: fake error
-2023-01-01T10:00:00Z  | full         | finished""",
+        assert (
+            harness.charm.backup._generate_backup_list_output()
+            == """Storage bucket name: test-bucket
+Backups base path: /test-path/backup/
+
+backup-id            | type         | status   | reference-backup-id  | LSN start/stop          | start-time           | finish-time          | backup-path
+-----------------------------------------------------------------------------------------------------------------------------------------------------------
+2023-01-01T09:00:00Z | full         | failed: fake error | None                 | 0/3000000 / 0/5000000   | 2024-07-01T20:45:11Z | 2024-07-01T20:45:14Z | /None.postgresql/20230101-090000F"""
         )
 
 
@@ -1230,8 +1279,18 @@ def test_on_create_backup_action(harness):
         mock_event.fail.assert_called_once()
         mock_event.set_results.assert_not_called()
 
+        # Test when the backup is of type diff/incr when there's no previous full backup.
+        mock_event.reset_mock()
+        mock_event.params = {"type": "differential"}
+        _upload_content_to_s3.return_value = True
+        _is_primary.return_value = True
+        harness.charm.backup._on_create_backup_action(mock_event)
+        mock_event.fail.assert_called_once()
+        mock_event.set_results.assert_not_called()
+
         # Test when the backup fails.
         mock_event.reset_mock()
+        mock_event.params = {"type": "full"}
         _upload_content_to_s3.return_value = True
         _is_primary.return_value = True
         _execute_command.return_value = (1, "", "fake error")