Skip to content

[DPE-10625] select valid backup entry in restore integration test PG14#1840

Draft
taurus-forever wants to merge 1 commit into
mainfrom
alutay/restore-backup-selection
Draft

[DPE-10625] select valid backup entry in restore integration test PG14#1840
taurus-forever wants to merge 1 commit into
mainfrom
alutay/restore-backup-selection

Conversation

@taurus-forever

@taurus-forever taurus-forever commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Backport of canonical/postgresql-k8s-operator#1552.

Issue

test_restore_on_new_cluster in test_backups_gcp.py was failing with AssertionError: restore hasn't succeeded because the charm logged Cannot restore to the timeline without restore-to-time parameter. The test used a hardcoded positional index (backups.split("\n")[-3]) to select a backup ID, assuming the last two entries are always restore/timeline records. When the output contains a different number of such entries, the selected line is itself a timeline entry — invalid for a plain restore.

Solution

Replace the positional index with a semantic parse of the pipe-delimited list-backups output. The action column (second |-delimited field) is "full backup", "differential backup", or "incremental backup" for real backups and "restore" for timeline entries. Filter to lines where that column contains "backup":

real_backup_lines = [
    line
    for line in backups.splitlines()
    for parts in [line.split("|")]
    if len(parts) > 1 and "backup" in parts[1].lower()
]
assert real_backup_lines, f"No valid backup entry found in list-backups output:\n{backups}"
backup_id = real_backup_lines[-1].split()[0]

Also improves the restore assertion message to include action.results for easier debugging.

Other backup helpers reviewed and unchanged:

  • backup_helpers.py lines 177/222: positional indices are safe — called before any restores exist, guarded by a strict line-count assertion.
  • _get_most_recent_backup: intentionally returns the most recent entry including timeline records; used by PITR tests to verify new timelines were created.

Checklist

  • I have added or updated any relevant documentation.
  • I have cleaned any remaining cloud resources from my accounts.
Original prompt

A GitHub Actions job is failing in repository canonical/postgresql-k8s-operator for the integration test tests/integration/test_backups_gcp.py::test_restore_on_new_cluster.

Failing job URL: https://github.com/canonical/postgresql-k8s-operator/actions/runs/26546494621/job/78200057802
Workflow file: .github/workflows/ci.yaml
Relevant test file: tests/integration/test_backups_gcp.py
Failing commit/ref: 0a0c5cd996bd36c237475a582b2d4d938f38b51a

Observed failure from logs:

  • The test repeatedly runs the restore action.
  • The charm logs show: Restore failed: Cannot restore to the timeline without restore-to-time parameter
  • The test eventually fails with AssertionError: restore hasn't succeeded and a tenacity.RetryError.

The likely cause is fragile parsing of the list-backups action output in test_restore_on_new_cluster. The current test assumes the third line from the end is always a valid backup:

# Last two entries are 'action: restore', that cannot be used without restore-to-time parameter
most_recent_real_backup = backups.split("\n")[-3]
backup_id = most_recent_real_backup.split()[0]

This positional assumption appears incorrect when the backup listing contains additional restore/timeline-related entries or formatting differences, causing the test to select an invalid backup ID that requires restore-to-time.

Please create a pull request that fixes the test by making backup selection robust.

Requirements:

  1. Update tests/integration/test_backups_gcp.py.
  2. Replace the positional backups.split("\n")[-3] logic with parsing that:
    • ignores blank lines,
    • excludes restore/timeline entries that cannot be restored without restore-to-time,
    • selects a valid backup entry deterministically.
  3. Improve the assertion/message around failed restore attempts so debugging is easier (for example, include action.results in the assertion failure).
  4. Keep the fix minimal and focused on this failure.
  5. Use an explicit commit message explaining the issue, e.g. that the GCP restore integration test was selecting an invalid backup entry from list-backups output and could trigger timeline restores without restore-to-time.

Please open a PR with a concise title reflecting the fix.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Please find a solution for this failing job. Use the logs, job definition, and any referenced files where the failure occurred. Keep your response focused on the solution and include code suggestions when appropriate.

Assistant: The failure is in test_restore_on_new_cluster and the actionable fix is in the test logic, not the workflow.

Root cause

The restore action is being called with a backup entry that requires restore-to-time, but the test assumes the third line from the end is always a valid full backup:

The logs show the restore repeatedly failing with:

  • Restore failed: Cannot restore to the timeline without restore-to-time parameter
  • final assertion: AssertionError: restore hasn't succeeded

That matches this fragile selection logic:

# Last two entries are 'action: restore', that cannot be used without restore-to-time parameter
most_recent_real_backup = backups.split("\n")[-3]
backup_id = most_recent_real_backup.split()[0]

If the output format changes, contains extra blank lines, or has more restore-related entries than expected, [-3] can still select an invalid entry.

Solution

Parse the list-backups output and choose a real backup entry explicitly, excluding restore/timeline entries instead of relying on positional indexing.

Suggested code change

Replace lines 136–138 with filtered parsing like this:

logger.info("restoring the backup")

backup_lines = [line.strip() for line in backups.splitlines() if line.strip()]

# Ignore entries that represent restore actions / timeline restores
real_backup_lines = [
    line for line in backup_lines
    if "action: restore" not in line.lower()
    and "timeline" not in line.lower()
]

assert real_backup_lines, f"No valid backup entry found in list-backups output:\n{backups}"

backup_id = real_backup_lines[-1].split()[0]

action = await ops_test.model.units.get(f"{database_app_name}/0").run_action(
    "restore", **{"backup-id": backup_id}
)
await action.wait()
restore_status = action.results.get("restore-status")
assert restore_status, f"restore hasn't succeeded: {action.results}"

Better hardening

If the backup list format is structured enough, prefer selecting only entries that match a known backup type, for example full/differential/incremental backups, instead of using substring exclusions. For example:

real_backup_lines = [
    lin...

</details>

Assisted-by: Claude:claude-5-fable

test_restore_on_new_cluster selected a backup ID from the list-backups
output by positional index (backups.split("\n")[-3]), assuming the last
two entries are always restore/timeline records. When the output
contains a different number of such entries, the selected line is
itself a timeline entry, which fails with "Cannot restore to the
timeline without restore-to-time parameter".

Parse the pipe-delimited output instead and select only lines whose
action column contains "backup" (full/differential/incremental),
and include action.results in the restore assertion message.

Backport of canonical/postgresql-k8s-operator#1552.

Assisted-by: Claude:claude-5-fable
@taurus-forever taurus-forever added the bug Something isn't working as expected label Jul 9, 2026
@github-actions github-actions Bot added the Libraries: Out of sync The charm libs used are out-of-sync label Jul 9, 2026
@taurus-forever taurus-forever changed the title [DPE-10625] select valid backup entry in restore integration test [DPE-10625] select valid backup entry in restore integration test PG14 Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working as expected Libraries: Out of sync The charm libs used are out-of-sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant