Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.

Ensure max backup bug #2113

Open
alynlin opened this issue Aug 22, 2019 · 3 comments · Fixed by cbws/etcd-operator#35 · May be fixed by #2116
Open

Ensure max backup bug #2113

alynlin opened this issue Aug 22, 2019 · 3 comments · Fixed by cbws/etcd-operator#35 · May be fixed by #2116

Comments

@alynlin
Copy link

alynlin commented Aug 22, 2019

When etcd max revision reaches the next order of magnitude, the deleted expired file is the latest backup file.
@mdkrajewski
Copy link

@alynlin I'm seeing behavior where the backup being deleted to ensure a maximum number of backups isn't necessarily the oldest backup, resulting in the latest backup possibly being deleted instead, and leaving around much older backups. Is this what you are describing?

I see in backup_manager.go's EnsureMaxBackup() that it is sorting based on file name, which is not reliable for this purpose, as the etcd max kv store revision number is inserted before the date.

For my local build, I will try changing this line in backup_manager.go, to put rev at the end of s3Path, and probably submit a PR:

s3Path = fmt.Sprintf(s3Path+"_v%d_%s", rev, now.Format("2006-01-02-15:04:05"))

@mdkrajewski
Copy link

On second thought, I'm going to try submitting a solution that fixes the sorting instead of changing the format of the backup paths, to make the fix backwards-compatible with current deployments.

mdkrajewski pushed a commit to mdkrajewski/etcd-operator that referenced this issue Aug 29, 2019
etcd-backup-operator: fixed sorting of etcd backups to ensure deletio…
e1538c0 
…n of only the oldest backups when rotating

When rotating backups based on a maximum number allowed, newer backups were being deleted when the number of digits in an etcd store revision increased.
E.g., when "v99" became "v100", the newer "v100" backup was being deleted, because the "1" in "100" was sorted to come before the first "9" in "99".
This new sorting method relies on the timestamp in each backup path, rather than the revision number.
The reason the timestamp is given precedence over the revision when sorting is because the revision could potentially go backwards when an older backup is restored.
See my comment in pkg/backup/util/util.go's SortableBackupPaths type for more details.

Fixes coreos#2113
@mdkrajewski mdkrajewski linked a pull request Aug 29, 2019 that will close this issue
Open
@mdkrajewski
Copy link

@alynlin I've submitted a PR (#2116). Let me know what you think! :)

mdkrajewski pushed a commit to mdkrajewski/etcd-operator that referenced this issue Aug 29, 2019
etcd-backup-operator: fixed sorting of etcd backups to ensure deletio…
9145a6f 
…n of only the oldest backups when rotating

When rotating backups based on a maximum number allowed, newer backups were being deleted when the number of digits in an etcd store revision increased.
E.g., when "v99" became "v100", the newer "v100" backup was being deleted, because the "1" in "100" was sorted to come before the first "9" in "99".
This new sorting method relies on the timestamp in each backup path, rather than the revision number.
The reason the timestamp is given precedence over the revision when sorting is because the revision could potentially go backwards when an older backup is restored.
See my comment in pkg/backup/util/util.go's SortableBackupPaths type for more details.

Fixes coreos#2113
@Marlinc Marlinc mentioned this issue Nov 11, 2019
Closed
@mdkrajewski mdkrajewski mentioned this issue Nov 18, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@alynlin @mdkrajewski