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

Fix max backup deletion #2116

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mdkrajewski
Copy link

etcd-backup-operator: fixed sorting of etcd backups to ensure deletion of only the oldest backups when rotating

Issue: 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".

Fix: 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 #2113

…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
@alaypatel07
Copy link
Collaborator

Thanks, @mdkrajewski for the PR. I just glanced through the PR a quick question: since the backup file names are stored in the format of s3Path+"_v%d_%s", would exploding the file path string with _ and sorting based on the last string in the list (time) work? Would that be a more maintainable way of achieving the same thing?

Sorry if I missed anything, will dig into the details soon

@mdkrajewski
Copy link
Author

...would exploding the file path string with _ and sorting based on the last string in the list (time) work? Would that be a more maintainable way of achieving the same thing?

@alaypatel07 Thanks for your comment! Your idea should work, and I think produce a little less code, and should be a little faster. However, one of my goals was to apply some semantics to the paths, so the utility function knows that it's comparing timestamps - just in case something happens to that filename after the fact, and to be somewhat resilient in case someone changes the format used by BackupManager. So in that respect, which approach is more maintainable might be a matter of perspective. I could go either way, though. If people such as yourself feel that my approach is too fancy, I could do what you suggest.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure max backup bug
2 participants