Choose WAL-G backup by start time when cloning and deleting #1

simonklb · 2024-05-29T15:23:21Z

Fixes https://github.com/elastisys/compliantkubernetes-postgresql/issues/241

Also fixes the following upstream issues AFAIK (more investigation needed to verify):

Bump wal-g v3:
- Bump wal-g to v3 zalando/spilo#986
Restore issues:
- WAL-E Clone should not use last_modified for recovery target time evaluation zalando/spilo#576
- Wrong timestamp taken during base backup recovery zalando/spilo#652
Clean up issues:

Does not fix when Spilo relies on wal-g backup-list LATEST:

Relying on modification time may lead to unexpected results (issues with backup-list, and backup-fetch .. LATEST) wal-g/wal-g#694
Sort backups by the creation time in generic backup-list command wal-g/wal-g#1108

Breaks cloning from backup that does not include metadata.json (for example backup created with wal-e):

backup-list --detail fails on missing metadata.json wal-g/wal-g#556

OlleLarsson · 2024-05-31T11:28:36Z

Thanks! Will take a look early next week!

OlleLarsson

LGTM!

davidumea

👍

simonklb · 2024-06-18T15:56:57Z

This fixes the immediate problem of cleaning up backups based on start time instead of modified time: fcb00b1

Note that this is not at all backwards compatible with wal-e, so don't merge this! ⚠️

However, when working on this I found this: wal-g/wal-g#816 which suggests that there is already built-in support for this. Also, using delete retain instead of delete before could also slim down the script even further as it currently tries to calculate how many backups to retain on it's own instead of relying on wal-g.

simonklb · 2024-06-20T12:56:02Z

Here's a new patch that makes use of wal-g delete retain --after: bb77d67
PTAL!

Diff it with ignore whitespace and it'll look less intimidating. :)

simonklb · 2024-06-20T16:12:52Z

FWIW the Spilo tests passes with these changes cherry-picked on master and with USE_WALG=true.

However, without USE_WALG=true some tests fail logs similar to this:

2024-06-20 16:08:59,805 INFO: Trying s3://testbucket/spilo/demo/wal/12/ for clone
INFO: 2024/06/20 16:08:59.822165 List backups from storages: [default]
INFO: 2024/06/20 16:08:59.823516 No backups found
2024-06-20 16:08:59,825 INFO: Trying s3://testbucket/spilo/demo/wal/11/ for clone
INFO: 2024/06/20 16:08:59.840049 List backups from storages: [default]
ERROR: 2024/06/20 16:08:59.841804 object 'base_000000010000000000000005_00000208/metadata.json' not found in storage
2024-06-20 16:08:59,843 ERROR: Clone failed

This is expected because if the backups are created with wal-e the metadata.json file is never created, so if we want to try to get this merged upstream we'll probably have to work around this as well or query if wal-e support is planned to be dropped in the near future.

Note that this is should only be an issue if you are trying to migrate from wal-e to wal-g. Using wal-e exclusively should still work.

OlleLarsson · 2024-06-24T13:20:26Z

...
This is expected because if the backups are created with wal-e the metadata.json file is never created, so if we want to try to get this merged upstream we'll probably have to work around this as well or query if wal-e support is planned to be dropped in the near future.

Note that this is should only be an issue if you are trying to migrate from wal-e to wal-g. Using wal-e exclusively should still work.

I think it makes sense to ask if they are planning on removing wal-e support.

OlleLarsson · 2024-06-24T13:43:44Z

Ugh, running into some issues building the image from this branch, seem to fail on building wal-g 🤔

[100%] Built target brotli
+ bash link_libsodium.sh

gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
The command '/bin/sh -c bash /builddeps/dependencies.sh' returned a non-zero code: 2

simonklb · 2024-06-24T13:49:24Z

Ugh, running into some issues building the image from this branch, seem to fail on building wal-g 🤔

[100%] Built target brotli
+ bash link_libsodium.sh

gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
The command '/bin/sh -c bash /builddeps/dependencies.sh' returned a non-zero code: 2

wal-g/wal-g#1738

simonklb · 2024-06-24T14:06:16Z

Blocked by: wal-g/wal-g#1738

Temp fix to allow for testing while we wait for the fix to be released: 8416ba1

davidumea · 2024-07-03T11:48:05Z

Blocked by: wal-g/wal-g#1738

Temp fix to allow for testing while we wait for the fix to be released: 8416ba1

I saw that the fix on garry-t's fork was merged into upstream wal-g. Should we switch to using upstream repo, or sync and create a branch on our fork instead?

simonklb · 2024-07-03T12:49:16Z

Blocked by: wal-g/wal-g#1738
Temp fix to allow for testing while we wait for the fix to be released: 8416ba1

I saw that the fix on garry-t's fork was merged into upstream wal-g. Should we switch to using upstream repo, or sync and create a branch on our fork instead?

Waiting for a release to materialize: wal-g/wal-g#1739 (comment)

This reverts commit 8416ba1.

simonklb · 2024-08-21T14:30:40Z

A wal-g release has been created with the fix. Upstream Spilo has brought it in.

I've made some changes to better conform with Spilo master. If you think the changes look good I'll give it a try to get it merged upstream as well.

annielzy · 2024-08-26T20:04:49Z

postgres-appliance/bootstrap/clone_with_wale.py

@@ -70,20 +72,25 @@ def choose_backup(backup_list, recovery_target_time):

    match_timestamp = match = None
    for backup in backup_list:
-        last_modified = parse(backup['last_modified'])
+        last_modified = parse(backup['start_time' if os.getenv('USE_WALG_RESTORE') == 'true' else 'last_modified'])


Hi Simon, should this be finish_time instead of start_time of the backup to compare with the recovery_target_time? Postgres requires that the recovery target "must be after the ending time of the base backup" (https://www.postgresql.org/docs/13/continuous-archiving.html).

Thanks for bringing this up!

The rational behind picking start_time over finish_time is that one backup might have been started first but finished second. I'm actually not sure how likely parallel backups are, if they are possible at all. Please let me know if you know more on this matter!

Either way, after having read the documentation you linked to I think you are absolutely correct. It does not make sense to pick a base backup that has not yet finished before the "recovery target".

272fbf1

simonklb · 2024-11-18T16:13:51Z

postgres-appliance/bootstrap/clone_with_wale.py

@@ -57,8 +60,7 @@ def fix_output(output):
    started = None
    for line in output.decode('utf-8').splitlines():
        if not started:
-            started = re.match(r'^name\s+last_modified\s+', line)
-            started = re.match(r'^name\s+last_modified\s+', line) or re.match(r'^name\s+modified\s+', line)
+            started = re.match(r'^(backup_)?name\s+(last_)?modified\s+', line)


Make sure this matches up with the fix upstream so that we don't reintroduce the bug: zalando#1026

simonklb · 2024-11-18T16:14:43Z

postgres-appliance/scripts/wale_restore.sh

@@ -43,7 +43,7 @@ ATTEMPT=0
 server_version="-1"
 while true; do
    [[ -z $wal_segment_backup_start ]] && wal_segment_backup_start=$($WAL_E backup-list 2> /dev/null \
-        | sed '0,/^name\s*\(last_\)\?modified\s*/d' | sort -bk2 | tail -n1 | awk '{print $3;}' | sed 's/_.*$//')
+        | sed '0,/^(backup_\)?name\s*\(last_\)\?modified\s*/d' | sort -bk2 | tail -n1 | awk '{print $3;}' | sed 's/_.*$//')


Make sure this matches up with the fix upstream so that we don't reintroduce the bug: zalando#1026

Zash · 2024-11-26T13:52:13Z

postgres-appliance/scripts/wale_restore.sh

@@ -43,7 +43,7 @@ ATTEMPT=0
 server_version="-1"
 while true; do
    [[ -z $wal_segment_backup_start ]] && wal_segment_backup_start=$($WAL_E backup-list 2> /dev/null \
-        | sed '0,/^name\s*\(last_\)\?modified\s*/d' | sort -bk2 | tail -n1 | awk '{print $3;}' | sed 's/_.*$//')
+        | sed '0,/^(backup_\)?name\s*\(last_\)\?modified\s*/d' | sort -bk2 | tail -n1 | awk '{print $3;}' | sed 's/_.*$//')


Hm, missing \ ?

Suggested change

| sed '0,/^(backup_\)?name\s*$last_$\?modified\s*/d' | sort -bk2 | tail -n1 | awk '{print $3;}' | sed 's/_.*$//')

| sed '0,/^$backup_$?name\s*$last_$\?modified\s*/d' | sort -bk2 | tail -n1 | awk '{print $3;}' | sed 's/_.*$//')

Zash · 2024-11-26T13:55:46Z

Hey @elastisys/goto-postgresql is the target branch still correct? (as opposed to needing a rebase)

OlleLarsson · 2024-11-26T14:00:08Z

Hey @elastisys/goto-postgresql is the target branch still correct? (as opposed to needing a rebase)

It should now be based in this branch

Zash · 2024-12-09T09:45:26Z

New PR rebasing these changes (since it was opened from @simonklb 's private repo):

Choose WAL-G backup by start time when cloning and deleting #3

simonklb requested review from OlleLarsson, davidumea and lucianvlad May 30, 2024 07:57

OlleLarsson approved these changes Jun 5, 2024

View reviewed changes

davidumea approved these changes Jun 10, 2024

View reviewed changes

simonklb force-pushed the simonklb/choose-backup-by-start-time branch from fcb00b1 to bb77d67 Compare June 20, 2024 12:54

simonklb changed the title ~~Choose WAL-G backup by start time when cloning~~ Choose WAL-G backup by start time when cloning and deleting Jun 20, 2024

simonklb requested review from OlleLarsson and davidumea June 20, 2024 14:25

simonklb added 2 commits June 20, 2024 16:36

Choose WAL-G backup by start time when cloning

a4f11b0

Use wal-g delete retain to clean up old backups

30b4d13

simonklb force-pushed the simonklb/choose-backup-by-start-time branch from bb77d67 to 30b4d13 Compare June 20, 2024 14:36

OlleLarsson approved these changes Jun 24, 2024

View reviewed changes

tmpfix

8416ba1

simonklb added 3 commits August 21, 2024 15:47

Revert "tmpfix"

65a49d7

This reverts commit 8416ba1.

Bump wal-g v3.0.3

c8fcca2

Conform with upstream changes

899bc94

simonklb requested a review from OlleLarsson August 21, 2024 14:29

annielzy reviewed Aug 26, 2024

View reviewed changes

Pick base backup based on finish_time instead of start_time

272fbf1

simonklb requested a review from annielzy August 27, 2024 07:12

Another upstream change required for WAL-G v3

c788be7

simonklb commented Nov 18, 2024

View reviewed changes

Zash self-assigned this Nov 26, 2024

Zash reviewed Nov 26, 2024

View reviewed changes

Zash mentioned this pull request Dec 4, 2024

Choose WAL-G backup by start time when cloning and deleting #3

Merged

Zash closed this Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Choose WAL-G backup by start time when cloning and deleting #1

Choose WAL-G backup by start time when cloning and deleting #1

simonklb commented May 29, 2024 •

edited

Loading

OlleLarsson commented May 31, 2024

OlleLarsson left a comment

davidumea left a comment

simonklb commented Jun 18, 2024 •

edited

Loading

simonklb commented Jun 20, 2024 •

edited

Loading

simonklb commented Jun 20, 2024 •

edited

Loading

OlleLarsson commented Jun 24, 2024

OlleLarsson commented Jun 24, 2024

simonklb commented Jun 24, 2024

simonklb commented Jun 24, 2024

davidumea commented Jul 3, 2024

simonklb commented Jul 3, 2024

simonklb commented Aug 21, 2024

annielzy Aug 26, 2024

simonklb Aug 27, 2024

simonklb Nov 18, 2024

simonklb Nov 18, 2024

Zash Nov 26, 2024

Zash commented Nov 26, 2024 •

edited by OlleLarsson

Loading

OlleLarsson commented Nov 26, 2024 •

edited

Loading

Zash commented Dec 9, 2024

	\| sed '0,/^(backup_\)?name\s\(last_\)\?modified\s/d' \| sort -bk2 \| tail -n1 \| awk '{print $3;}' \| sed 's/_.*$//')
	\| sed '0,/^\(backup_\)?name\s\(last_\)\?modified\s/d' \| sort -bk2 \| tail -n1 \| awk '{print $3;}' \| sed 's/_.*$//')

Choose WAL-G backup by start time when cloning and deleting #1

Choose WAL-G backup by start time when cloning and deleting #1

Conversation

simonklb commented May 29, 2024 • edited Loading

OlleLarsson commented May 31, 2024

OlleLarsson left a comment

Choose a reason for hiding this comment

davidumea left a comment

Choose a reason for hiding this comment

simonklb commented Jun 18, 2024 • edited Loading

simonklb commented Jun 20, 2024 • edited Loading

simonklb commented Jun 20, 2024 • edited Loading

OlleLarsson commented Jun 24, 2024

OlleLarsson commented Jun 24, 2024

simonklb commented Jun 24, 2024

simonklb commented Jun 24, 2024

davidumea commented Jul 3, 2024

simonklb commented Jul 3, 2024

simonklb commented Aug 21, 2024

annielzy Aug 26, 2024

Choose a reason for hiding this comment

simonklb Aug 27, 2024

Choose a reason for hiding this comment

simonklb Nov 18, 2024

Choose a reason for hiding this comment

simonklb Nov 18, 2024

Choose a reason for hiding this comment

Zash Nov 26, 2024

Choose a reason for hiding this comment

Zash commented Nov 26, 2024 • edited by OlleLarsson Loading

OlleLarsson commented Nov 26, 2024 • edited Loading

Zash commented Dec 9, 2024

simonklb commented May 29, 2024 •

edited

Loading

simonklb commented Jun 18, 2024 •

edited

Loading

simonklb commented Jun 20, 2024 •

edited

Loading

simonklb commented Jun 20, 2024 •

edited

Loading

Zash commented Nov 26, 2024 •

edited by OlleLarsson

Loading

OlleLarsson commented Nov 26, 2024 •

edited

Loading