Skip to content
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.

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

Exclude specific backends from pruning #262

Merged
merged 6 commits into from
Aug 27, 2023
Merged

Exclude specific backends from pruning #262

merged 6 commits into from
Aug 27, 2023

Conversation

MaxJa4
Copy link
Contributor

@MaxJa4 MaxJa4 commented Aug 25, 2023

Fixes #207

Content

New env variable: BACKUP_SKIP_BACKENDS_FROM_PRUNE
All mentioned storage backend names will be excluded from pruning.

Notes

Tests will be added after #261 is available.

Todo

  • Tests

@MaxJa4 MaxJa4 changed the title Skip backends while pruning Exclude specific backends from pruning Aug 25, 2023
@m90
Copy link
Member

m90 commented Aug 25, 2023

It's fine if you wanna wait for #261, but the way tests are written won't change with that PR. It will just be easier / more robust to run them, especially locally. It could well be that it still takes me a week or two to get that into a state that I'd wanna merge it in. Anyways, whatever you prefer, just wanted to mention it.

@MaxJa4
Copy link
Contributor Author

MaxJa4 commented Aug 25, 2023

Ok, good to know.
I saw that the following, which is in most run.sh files for testing, needs to be addressed first:
# TODO: find out if we can test actual deletion without having to wait for a day

Implementing #93 with pruning policies first could fix this issue, as it should enable us to prune all existing backups (e.g. when keep-n=1).
Should such a change be a separate PR or be added to this one? It's also about pruning, but technically a different topic (schedule instead of skipping prunes).

@m90
Copy link
Member

m90 commented Aug 25, 2023

I saw that the following, which is in most run.sh files for testing, needs to be addressed first:

I'm surprised I didn't think of it when I set all of this up, but right now I'd implement tests for pruning like this:

  • a first backup is taken
  • tests on file contents are done
  • the file is being backdated by ~14 days
  • another backup run is triggered, configured to use ~7 retention days
  • the old file should be gone, and a newer one should be there

Or am I missing something here.

Implementing #93 with pruning policies first could fix this issue, as it should enable us to prune all existing backups (e.g. when keep-n=1).

Question is what does implementing #93 even look like? Story time: v2 of this tool is a rewrite in Go as I did not manage to properly implement pruning in the v1 bash version, instead the tool would wipe all backups (#18). Ugh. Ever since then, I tend to believe this tool is better off with a pruning strategy that is very simple and hard to get wrong.

All I am trying to say is: if we were to enhance the current mechanism, I'd like to discuss and understand it in detail. Also, I don't think the current mechanism is a real show stopper.

@MaxJa4
Copy link
Contributor Author

MaxJa4 commented Aug 25, 2023

Sounds like a good solution to me

the file is being backdated by ~14 days

touch -r "file" -d '-14 days' -m "file" would make that easy and do exactly that.

All I am trying to say is: if we were to enhance the current mechanism, I'd like to discuss and understand it in detail. Also, I don't think the current mechanism is a real show stopper.

Since implementing #93 (in part) was mostly to fix the pruning test issue, I absolutely agree. Let's only do this when we know how and if we want to do that.

@MaxJa4
Copy link
Contributor Author

MaxJa4 commented Aug 25, 2023

It finally works. Azure has the issue, that it ignores the modifiedTime metadata of the file and just uses the upload date instead... so going back in time, upload another 'old' file, going forward to the present (set OS date) and then running backup again works.
Also added --quiet-pull to the first docker compose up to clean up the logs a little.

Will do the other tests (where applicable) tomorrow.

Really looking forward to your enhanced solution for running the tests, so I can just act -j test locally and it runs the GH action locally... right now it fails at the running-container count test. Will save a lot of time with test issues :)

@MaxJa4
Copy link
Contributor Author

MaxJa4 commented Aug 26, 2023

All test cases for storage backends include a pruning test case now. Manipulating time was a little different for each one, but nothing magical in hindsight.

Also added two test cases in /test/pruning for this PR for the skip-backend-while-pruning feature. It includes a local and a S3 backend. First, the S3 backend is skipped for pruning. Then, both backends (=all available) are skipped. As all skip-functionality is in script.go globally and equally to all backends, IMO just using S3 as an example here is enough.

@MaxJa4 MaxJa4 marked this pull request as ready for review August 26, 2023 19:20
test/cli/run.sh Show resolved Hide resolved
test/dropbox/run.sh Outdated Show resolved Hide resolved
test/local/run.sh Outdated Show resolved Hide resolved

// skipPrune returns true if the given backend name is contained in the
// list of skipped backends.
func skipPrune(name string, skippedBackends []string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the name should be as specific, as the logic in the function really is very generic. Maybe this could be containedEqualFold(item string, list []string)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about this too, also had this briefly inside util.go as it is very generic and could be used by any module.

Copy link
Member

@m90 m90 left a comment

Choose a reason for hiding this comment

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

Brilliant work once again, thanks!

@m90 m90 merged commit ad4e2af into offen:main Aug 27, 2023
@MaxJa4 MaxJa4 deleted the skip_backend_pruning branch August 27, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to only prune local archive
2 participants