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

forget: Add --max-age policy to set hard cutoff for removing snapshots #1735

Merged
merged 5 commits into from
May 14, 2018
Merged

forget: Add --max-age policy to set hard cutoff for removing snapshots #1735

merged 5 commits into from
May 14, 2018

Conversation

mholt
Copy link
Contributor

@mholt mholt commented Apr 23, 2018

What is the purpose of this change? What does it change?

Adds a --max-age flag to the forget command so that snapshots can be deleted simply if they are older than a certain duration.

This is useful for keeping backup sizes under control and for a simple policy of "keeping deleted files for a certain time" i.e. a retention period.

It's possible that there is already a way to do it with existing policy flags and I am just not seeing it. If that's the case, feel free to teach me, and we can close this. :)

Still need to add docs... and tests.... not sure how to add a test for this though. But I did try it. :P Let me know if you have any ideas for that.

Was the change discussed in an issue or in the forum before?

I asked on the forum here: https://forum.restic.net/t/deleting-all-snapshots-older-than-a-certain-timestamp/611?u=matt

But then I tried implementing it which took only a few minutes, so here we are!

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@codecov-io
Copy link

codecov-io commented Apr 23, 2018

Codecov Report

Merging #1735 into master will decrease coverage by 5%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
- Coverage   51.91%   46.91%   -5.01%     
==========================================
  Files         148      148              
  Lines       11729    11733       +4     
==========================================
- Hits         6089     5504     -585     
- Misses       4693     5335     +642     
+ Partials      947      894      -53
Impacted Files Coverage Δ
internal/restic/snapshot_policy.go 97.26% <100%> (-0.04%) ⬇️
cmd/restic/cmd_forget.go 42.22% <83.33%> (+0.68%) ⬆️
internal/backend/azure/azure.go 0% <0%> (-82.15%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-78.45%) ⬇️
internal/backend/b2/b2.go 0% <0%> (-77.73%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-73.61%) ⬇️
internal/backend/swift/config.go 36.95% <0%> (-54.35%) ⬇️
internal/archiver/archiver.go 64.04% <0%> (-0.22%) ⬇️
internal/backend/rclone/backend.go 72.25% <0%> (+1.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2aa6b49...351ecab. Read the comment docs.

@fd0
Copy link
Member

fd0 commented Apr 24, 2018

Adds a --max-age flag to the forget command so that snapshots can be deleted simply if they are older than a certain duration.

I'm okay with that in general, but I think we should consider a different name for the option. In the spirit of the other options, what about --keep-all-within or --keep-newer-than?

It's possible that there is already a way to do it with existing policy flags and I am just not seeing it. If that's the case, feel free to teach me, and we can close this. :)

I don't think this is possible yet.

Can we please define what this option should do? It feels to me that this is an alternative to the other policy settings, instead of "keep 14 daily backups, delete everything else" you can then say "keep all snapshots newer than a year, delete everything else". So it would basically be incompatible to the other keep options...

I'd expect at least to return an error to the user that this option is not compatible with the other ones. Or is it?

Let's talk specifics: Let's say I have one snapshot for each day of the year 2017. At January 1st, 2018, I run restic forget --max-age 1m. What would that mean exactly? Delete all snapshots except the ones in December?

For all combinations with the other --keep-* options I'm not so sure: When I run forget --keep-weekly 4, then restic keeps four snapshots for the four weeks of December, and deletes everything. But what about forget --keep-weekly 4 --max-age 1y. What would that do? Or forget --keep-weekly 4 --max-age 1w?

Still need to add docs... and tests.... not sure how to add a test for this though. But I did try it. :P Let me know if you have any ideas for that.

Tests need to be added here: https://github.com/restic/restic/blob/master/internal/restic/snapshot_policy_test.go#L176

@mholt
Copy link
Contributor Author

mholt commented Apr 24, 2018

Can we please define what this option should do? It feels to me that this is an alternative to the other policy settings, instead of "keep 14 daily backups, delete everything else" you can then say "keep all snapshots newer than a year, delete everything else". So it would basically be incompatible to the other keep options...

I don't think it has to be. It can work alongside them and act as an extra filter; of the snapshots kept, do they fall within the timeframe specified? If not, forget; otherwise keep. It honestly sounds simple to me, but let me know if I'm missing something obvious.

Let's talk specifics: Let's say I have one snapshot for each day of the year 2017. At January 1st, 2018, I run restic forget --max-age 1m. What would that mean exactly? Delete all snapshots except the ones in December?

Well, 1m is 1 minute. :) I know you mean 1 month, but time.ParseDuration only accepts up to hour units. Not awesome, but it can work (for my needs especially). To answer your question, assuming 1m == 1 month, yes, I think that's what it means. Keep all the snapshots newer than 1 month old.

For all combinations with the other --keep-* options I'm not so sure: When I run forget --keep-weekly 4, then restic keeps four snapshots for the four weeks of December, and deletes everything. But what about forget --keep-weekly 4 --max-age 1y. What would that do? Or forget --keep-weekly 4 --max-age 1w?

Simple: it should keep all the weekly snapshots that remain (4 in this case) that are less than 1 year or 1 week old, respectively. It's just an extra filter, not incompatible with the existing ones.

I like your idea to rename to --keep-newer-than. I will make the change shortly, and write a test.

@mholt
Copy link
Contributor Author

mholt commented Apr 24, 2018

There is now a test case! Thanks for the pointer.

@fd0
Copy link
Member

fd0 commented Apr 25, 2018

Simple: it should keep all the weekly snapshots that remain (4 in this case) that are less than 1 year or 1 week old, respectively. It's just an extra filter, not incompatible with the existing ones.

Ah, that's where this feature (as proposed) is incompatible with the current settings: The options we already have (like --keep-weekly) tell restic which snapshots to keep. If there are several, then restic computes the union of all sets of snapshots to keep. That's easy to understand and reason about. If we have --keep-daily 3 and --keep-weekly 4, it'll keep the last 3 daily snapshots and up to three other snapshots so that for each of the last four weeks there is at least a snapshot. The youngest snapshot will probably match for both, as a daily and weekly snapshot, but that's fine since the final set of snapshots is just the union of the two sets.

If I understood you correctly (please speak up if I got it wrong), then --keep-younger-than would mean computing the set of snapshots to keep as usual, but then intersecting it with the set of snapshots younger than e.g. one week. In the example above, if I call restic forget --keep-daily 3 --keep-weekly 4 --keep-younger-than 1w, the first set would contain snapshots that are up to four weeks old. Which would then be deleted because of --keep-younger-than 1w. Is that correct?

I think that's not a good behavior. I fear it will confuse users. If I understood you correctly, you'd like to use --keep-younger-than exclusively, without the other options, so just make daily snapshots, then run forget --keep-younger-than 1y. Is that correct?

If so, I'd like to propose implementing this feature slightly different, in the same spirit as the other options (especially --keep-last): Process the keep options as usual, and then include the set of snapshots selected by --keep-younger-than in the union. In the example above (--keep-daily 3 --keep-weekly 4 --keep-younger-than 1w), this would mean that it'll keep the last three daily snapshots (probably superfluous here), four weekly snapshots, and everything that is younger than a week.

If you're only using forget --keep-younger-than 1y, it won't make a difference (since there is no other set of snapshots to intersect with), but I think it makes much more sense in combination with the other options.

There's another thing we need to take care of: By default, if there are no new snapshots (e.g. the backup command isn't run due to some fault), restic forget is written in a very conservative way so it won't delete any snapshots in this case. For example, let's say you have daily backups for a year, and then the restic backup isn't run but forget --keep-daily 365 still runs. As restic is instructed to keep the last 365 daily snapshots, it won't forget anything. As soon as there is a new daily snapshot (say, at March 1st), the oldest daily snapshot from January 1st is deleted.

With restic forget --keep-younger-than 1y, it'll happily remove daily snapshots although now new ones are created. I think that's not good.

What about changing the behavior here again slightly, so that the use case I assume you're mainly interested in will work in the same way, but it's more conservative: Don't use the current timestamp for the age calculation, but the timestamp of the youngest snapshot restic can find. If you do daily snapshots, this won't make a difference, but if your backup isn't run, restic won't delete old snapshots.

You see I spent some time thinking about this feature (during a walk), sorry for the long wall of text :) Getting forget right (and usable!) was surprisingly hard and complicated, at least in the same order of complexity as the repository format...

What do you think?

@fd0
Copy link
Member

fd0 commented Apr 25, 2018

Ah, and I'm happy to help with the implementation, once we agree on how the new option should work ;)

@fd0 fd0 added the PR:WIP label Apr 25, 2018
@mholt
Copy link
Contributor Author

mholt commented Apr 25, 2018

Oh... you know, you make some good points. I hadn't thought about unioning vs. intersecting, you're right, that's inconsistent.

I am intrigued by the idea of using the most recent snapshot date as the basis for the age calculation. I think I like that more than using the current timestamp; if someone stopped making backups, there might be a serious reason/distress why, and they might not want to delete their older snapshots just because time rolled forward.

Along these lines, a --keep-within flag might make the most sense, as you described. How would this be implemented? Since it's not a strict limit on the count of snapshots, I don't know how to use the existing bucket structure to fit it in and make it union...

@fd0
Copy link
Member

fd0 commented May 13, 2018

So, I went on a yak-shaving tour this morning, added a new type restic.Duration (which can track days, months and years) and completed this feature. Please have a look!

Sample, keep all snapshots within the last year, five months and 23 days of the latest snapshot:

$ restic forget --dry-run --keep-within 1y5m23d

If you agree this is a good change, we can merge it so it'll go into 0.9.0.

@fd0 fd0 added this to the 0.9.0 milestone May 13, 2018
@mholt
Copy link
Contributor Author

mholt commented May 13, 2018

Awesome. That looks like a great solution!

I approve. One question to think about: will the "m" notation ever conflict with the "m" of the standard duration type which means minutes? I doubt it personally since I don't see minutes being particularly useful in this context. But the docs clarify what it means, so that looks good to me!

@fd0 fd0 merged commit 375868e into restic:master May 14, 2018
fd0 added a commit that referenced this pull request May 14, 2018
forget: Add --max-age policy to set hard cutoff for removing snapshots
@fd0
Copy link
Member

fd0 commented May 14, 2018

cool, merged!

@mholt mholt deleted the forget-max-age branch July 31, 2018 23:33
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.

3 participants