Skip to content

[DPE-4479][DPE-4625] Make backups behavior correspond to specification #542

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 3, 2024
Merged

[DPE-4479][DPE-4625] Make backups behavior correspond to specification #542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d16acda
add list-backups new format
Jul 1, 2024
6b2106c
add create backup check + unit test
Jul 1, 2024
6253714
Merge remote-tracking branch 'origin/main' into lucas/list-backups
Jul 2, 2024
8ae0815
fix integration test
Jul 2, 2024
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
71 changes: 66 additions & 5 deletions src/backups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,11 +266,43 @@ def _execute_command(

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)

Expand All @@ -284,11 +316,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])
Copy link
Author

Choose a reason for hiding this comment

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

Because pgbackrest returns a list of references, we pick only the most recent one to represent.
For example, if an incremental backup A depends on the previous differential backup B, which depends on a previous full backup C, the reference list for A will be [C, B] --> last one, most recent B will be shown

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]:
Expand Down Expand Up @@ -514,6 +564,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:
Expand Down
8 changes: 4 additions & 4 deletions tests/integration/test_backups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,8 @@ async def test_backup_and_restore(ops_test: OpsTest, cloud_configs: Tuple[Dict,
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.
Expand All @@ -197,8 +197,8 @@ async def test_backup_and_restore(ops_test: OpsTest, cloud_configs: Tuple[Dict,
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.
Expand Down
110 changes: 84 additions & 26 deletions tests/unit/test_backups.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,48 +542,97 @@ def test_execute_command(harness):


def test_format_backup_list(harness):
# Test when there are no backups.
assert (
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"),
]
assert (
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 = ('[{"backup":[]}]', None)
assert (
harness.charm.backup._generate_backup_list_output()
== """backup-id | backup-type | backup-status
----------------------------------------------------"""
== """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 = (
'[{"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}}]}]',
None,
)
assert (
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"""
== """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.patroni-postgresql-k8s/20230101-090000F"""
)


Expand Down Expand Up @@ -1140,6 +1189,15 @@ 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"}
Expand Down