Prune by either int or interval for all retention policies

[Goddesen]

This PR adds optional interval handling for all retention filter flags of the prune command, previously only available on --keep-within. E.g. prune --keep-hourly=7d will keep hourly archives for the last 7 days regardless of count. Adds --keep which acts as a combination of --keep-last and --keep-within.

Implements #8637.

I opted to make the existing flags handle both ints and intervals instead of adding new flags as there are already so many flags for this command. Simplified some prune filtering: With the default filter function now also handling intervals, prune_within is no longer needed as a special case.

I added a library to freeze time in testing, let me know if that's not wanted and I'll figure out something else. It's such a hassle to deal with timestamps relative to now() in test. The tests should be fairly comprehensive in checking both their timely filter (hourly/yearly, etc.) and the new inclusive timestamp check. I did not add new helper tests for prune_split as this function is not used anywhere other than prune_cmd and isn't really a helper.

TODOs:

• New complicated example using intervals, showing how they overlap and interact with simple ints. Test that confirms this example (currently no test at all for overlapping intervals).
• Docs once exact implementation is decided.

TODOs from comments:

• Figure out default values and their interaction with the must pass at least one flag check.

---

Notes:

• New retention flag --keep: Merges functionality of --keep-within and --keep-last with new int-or-interval handling.
• --keep-last is no longer an alias of --keep-secondly and thus keeps archives made on the same second. It now fits together with --keep-within and new flag --keep.
• Intervals now support 0 seconds. While working with int_or_interval and creating timedelta objects it seemed weird to restrict input like this. Not extremely useful unless you want to prune on archives in the same second as they were created, but it seemed logical when setting up tests to verify new --keep-last behavior.. Technically breaking, but will likely not meaningfully affect any real scenarios.
• Timestamp comparisons for retention intervals are now inclusive on seconds. Matches (my personal) human intuition: If it is currently xx:xx:10 and there's an archive at xx:xx:05 then --keep-within 5S should cover that archive. Easy change to make when already altering the filtering logic. Technically breaking, but will likely not meaningfully affect any real scenarios.

---

This has been a pet peeve of mine in the pruning command for a long time. In my mind the most clean backup regime keeps all backups for a short time (allows catching small errors quickly), then hourly backups for a reasonable time (say, 7 days), then daily backups for a little longer and then finally weekly/monthly as storage permits. This was not previously possible, requiring for example --keep-within 7d --keep-hourly 168 --keep-daily 14 --keep-weekly -1 for an approximation. But keeping around 168 archives for a machine that's only running a few hours a day seems mighty excessive. So here we are :)
With this implemented my ideal retention for my primary laptop with archives every 15m looks something like --keep 3d --keep-hourly 7d --keep-daily 30d --keep-weekly -1.

[PhrozenByte]

This is loosely related to #8715 as well.

I like the idea very much (I didn't and can't review the code, but I'm happy to help with the docs if desired). Borg's current retention policy IMHO is rather hard to understand for beginners (but very safe, because it usually keeps more than what users expect) and an interval-based approach feels more intuitive to me. However, special care is necessary to not cause unexpected behaviour, especially with frequent backups (e.g. daily), combined with overlapping rules (e.g. including all of within, daily, weekly, monthly, and yearly), and some missing backups (and thus the need to sometimes keep the "next best" to later fulfil less frequent rules). As far as I remember there were some issues with this in the early days of Borg that at least caused multiple major docs overhauls (not sure whether the behaviour actually changed substantially, but the docs definitely did because many users understood things wrong). Maybe Thomas can give some insights about the challenges back then?

[ThomasWaldmann]

Thanks for the PR!

Some minor stuff I found...

[Goddesen]

It seems the GHA test runner did not like the previous tzinfo=None change in helpers_test.py. Replaced with an implementation in MockArchive more alike to the one for real archives, let's see if that plays nicely.

[PhrozenByte]

New retention flag --keep: Merges functionality of --keep-within and --keep-last with new int-or-interval handling.

I'd vote for removing --keep-within and --keep-last in favour of the new --keep option with Borg 2.0 for consistency with other --keep-* options. If this gets backported to Borg 1.x, they should be deprecated. WDYT?

Timestamp comparisons for retention intervals are now inclusive on seconds. Matches (my personal) human intuition: If it is currently xx:xx:10 and there's an archive at xx:xx:05 then --keep-within 5S should cover that archive. Easy change to make when already altering the filtering logic.

IMHO this must be consistent with #8776. Question is what we agree on. I honestly believe differently: If it is xx:xx:10 now and what to cover the last five seconds, that's exclusive the xx:xx:05 second, because I also count archives that are created at xx:xx:10 (i.e. seconds 10, 9, 8, 7, and 6). WDYT?

[ThomasWaldmann]

If you want to be able to backport this to 1.4-maint, you must not break compatibility in this PR, but deprecating some options that can be replaced by a better new option can be done here.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.08%. Comparing base (2129d81) to head (dfcee1d).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8775      +/-   ##
==========================================
+ Coverage   82.00%   82.08%   +0.07%     
==========================================
  Files          74       74              
  Lines       13374    13390      +16     
  Branches     1979     1977       -2     
==========================================
+ Hits        10968    10991      +23     
+ Misses       1746     1743       -3     
+ Partials      660      656       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Goddesen]

If you want to be able to backport this to 1.4-maint, you must not break compatibility in this PR, but deprecating some options that can be replaced by a better new option can be done here.

How do you do separation of functionality like this when a breaking 2.0 change is to be backported with deprecations instead? The code might turn out very different, especially with changes introduced on master.

[Goddesen]

IMHO this must be consistent with #8776. Question is what we agree on. I honestly believe differently: If it is xx:xx:10 now and what to cover the last five seconds, that's exclusive the xx:xx:05 second, because I also count archives that are created at xx:xx:10 (i.e. seconds 10, 9, 8, 7, and 6). WDYT?

I don't think #8776 is relevant, as it matches absolute timestamps based on a pattern (like how the prune period grouping functions work) and does not deal with relative intervals where this consideration is needed. I am not familiar with the code being touched in that PR but I don't think there is overlap with the work here.

I had a whole comment here written out defending the inclusive check based on comparing timestamps only using seconds-granularity. In that scenario this makes sense. Having taken a closer look I see that archive.save explicitly saves timestamps with microseconds and the second-precision timestamps I have been seeing have all been test values. Good thing I checked. In that case the change from > to >= only makes for one microsecond of difference. I'm comfortable reverting this.

[Goddesen]

Comments fixed, inclusive timestamp change reverted with tests made slightly more robust. If that's it for implementation comments I'll extend the documentation with an involved example like the retention policy I have described in the PR description and write a test for it.

I am still not sure how to go about doing breaking change on 2.0 and backporting change with deprecations to 1.*. @ThomasWaldmann ?

[ThomasWaldmann]

Easiest is to backport a non-breaking change and then do the breaking change in a separate PR afterwards (and not backport that).

There are some code structure differences between master and 1.4-maint though, so even backporting the first PR might not be trivial. As 1.4.x is a stable release, we need to be very careful to not break anything there. That sometimes can mean "no backport" if the change is too big or too risky.

[Goddesen]

Last explicit default removed.

There are some code structure differences between master and 1.4-maint though, so even backporting the first PR might not be trivial. As 1.4.x is a stable release, we need to be very careful to not break anything there. That sometimes can mean "no backport" if the change is too big or too risky.

The changiest change introduced here is allowing intervals to be of length 0. Since this is an extension of functionality I think it's a fairly safe backport.

Am I correct in assuming I just put entries for --keep-within and --keep-last in deprecations in preprocess_args in archiver/__init__.py?

[PhrozenByte]

In regards to backporting this to 1.4-maint and breaking changes:

I kinda feel like I accidentally caused some confusion, so: Do we even have breaking changes right now?

The only "breaking" change is that --keep-within 0 works now. Since that is undocumented anyway, I personally wouldn't consider that a breaking change. The only breaking change would be to drop --keep-within and --keep-last in favour of the new --keep. I still strongly vote for doing that. However, just for 2.0: We can safely keep them for 1.4 and mark them as deprecated. They're just aliases for --keep with this PR, but don't really change functionality, do they?

[Goddesen]

The only "breaking" change is that --keep-within 0 works now.

Ah I forgot there is one more thing: --keep-last is no longer an alias for --keep-secondly. A little more breaky than the interval change I think, but I struggle to believe this behavior is heavily relied upon in the wild? That is, unless you read the docs about secondly pruning and then decide to just use --keep-last for some reason.

But if you want, it's pretty easy to just not introduce that change now, and do it in the breaking PR instead.

[PhrozenByte]

Ah I forgot there is one more thing: --keep-last is no longer an alias for --keep-secondly.

There's only a difference if someone actually managed to create multiple archives within the same second, right? I'd say that's similar to 0 intervals: Sure, it's a breaking change in theory, but in practice?

Anyway, I don't think we really have a problem here. If we decide on dropping --keep-last (and --keep-within for this matter) with Borg 2.x anyway, we can simply let --keep-last stay an alias for --keep-secondly (whose behaviour hasn't changed, right?) with Borg 1.4. Users that want to try the "true" and new "last" with Borg 1.x can use the new --keep. Same for 0 intervals: Adding a check and explicitly rejecting 0 intervals with just Borg 1.4 isn't that much of a big deal, is it?

Not for me to decide, but I suggest dealing with that stuff in the backport PR.

I don't think #8776 is relevant, as it matches absolute timestamps based on a pattern (like how the prune period grouping functions work) and does not deal with relative intervals where this consideration is needed.

It does: It can also match all archives within e.g. a full month relative to "now" (and added just recently, also relative to arbitrary other timestamps). However, I agree that there's no practical difference due to the microseconds scale. I didn't consider that. Yet I'd still vote for consistency. I just skimmed through @c-herz's work in #8776 and noticed that matching exclusively causes some problems there as well (minor, yet there are some). IMHO the consistency question still somewhat matters, so how about we simply decide on always matching inclusively? I'm thus also tagging @c-herz, WDYT?

[Goddesen]

After some consideration I think this new retention interval should also account for the oldest rule that was implemented for the retention count flags. I'll get back to this.

[ThomasWaldmann]

before doing further changes, please rebase on current master branch to get the cython workaround. otherwise, builds will fail.

[ThomasWaldmann]

Guess I'ld like to merge this for next beta, can we finish this until then?

[Goddesen]

I'll endeavor to get something done soon then. Factorio has been a distraction 😄

[ThomasWaldmann]

btw, i dissected the borg.testsuite.helpers_test monster module into a borg.testsuite.helpers package with modules corresponding to modules in borg.helpers.

[ThomasWaldmann]

ping?

shall I help here a bit by rebasing this on current master branch and resolve the conflicts?

[Goddesen]

I'm still here! I pondered myself into a corner with how to best write a similarly comprehensive test and example for the interval code path. I've got to admit I've found it quite hard to iterate on tests with this test suite setup -- but I am not very familiar with Python and its ecosystem so that might be on me.

While thinking about this new large example and how it would relate to my own preferred backup scheme it has additionally become clear to me that it is a bit confusing if you specify --keep-daily 3d and you run daily backups immediately followed by a prune, the backup that occurred exactly three days ago will sometimes be included depending on the time taken to do that backup. I would like to experiment with some kind of fuzziness, maybe each --keep-* rule defines the interval to last for that entire time period (so my 3d example will consider every backup made on that day; if --keep-hourly was used, consider every archive in the hour that covers 3d ago). But maybe the current implementation is good enough for a feature merge and beta release and can be further refined in another PR.

I have rebased on master and moved my changes to the split helper tests. I have squashed all my commits and made some alterations for the rebase to make sense as best I could. I will try to keep thinking about that comprehensive example + tests.

[Goddesen]

Fixed lint.