prune --keep-all: alias for --keep-last <inf>, fixes #6656

[Michael-Girma]

Added a subparser argument that stores a large value for --keep-last when pruning.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.48%. Comparing base (16e7039) to head (2d0e590).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7462      +/-   ##
==========================================
+ Coverage   81.46%   81.48%   +0.02%     
==========================================
  Files          77       77              
  Lines       13520    13521       +1     
  Branches     2004     2004              
==========================================
+ Hits        11014    11018       +4     
+ Misses       1853     1851       -2     
+ Partials      653      652       -1     

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

[ThomasWaldmann]

Hehe, looks good. Considering the special infinity value, I guess a test for this would be good (just test the code does not crash and keeps all archives present, except old checkpoint archives).

[jdchristensen]

What happens if someone does --keep-all --keep-secondly=10? Does the 10 overwrite the infinite value? This might be surprising to the user since the options seem independent. (This is already an issue with --keep-last 100 --keep-secondly 10 as well, I believe.) Maybe --keep-last should be implemented separately from --keep-secondly? (I'd also be inclined to remove --keep-secondly in borg2.) (I'm not worried about the interaction between --keep-all and --keep-last, since --keep-all is documented to be equivalent to --keep-last <inf>.)

The above conflicts might happen because someone has a script that already has various --keep options, and adds --keep-all at the front, thinking that this would cause everything to be kept.

[Michael-Girma]

@jdchristensen, interesting insight. I'll take a look at the interactions between those flags. @ThomasWaldmann I did test that eye-catching infinite value locally (didn't test deeply, just created a few archives to see that it didn't crash). I can write up a unit test to see how well it works with keeping archives and removing checkpoints. I would love to hear your thoughts on @jdchristensen's remarks though.

[ThomasWaldmann]

Good catch! Issue #6269 applies here also, I guess.

[ThomasWaldmann]

If we have different argparse options, we could also use this:

https://docs.python.org/3/library/argparse.html#mutual-exclusion

[ThomasWaldmann]

I resolved the merge conflict and also had another look:

• Highlander is not needed for --keep-all - it can be given multiple times without causing an issue (because it is always using the same infinity value)
• but we should have the mutual exclusion for --keep-last, --keep-secondly, --keep-all.

@Michael-Girma Can you try to implement that and add a test?

[ThomasWaldmann]

@Michael-Girma did you see my last comment, can you fix it?

[Michael-Girma]

@ThomasWaldmann just seeing this. Will take a look tonight.

[ThomasWaldmann]

@Michael-Girma do you need help finishing this?

[Goddesen]

I'm looking at similar logic in #8775 and could finish this up pretty quick. Additionally I've been thinking to add handling of "inf" or "all" to the prune retention filters which could share some of this logic.

Thinking about what this PR practically solves, cleanup of checkpoints, should this flag maybe be something like prune --checkpoints as a cleanup command instead? Running prune but choosing to explicitly keep all archives does not seem very intuitive. The command could then throw an error if you provided any archive name filter or other flags that are related to selecting archives to keep.

[PhrozenByte]

Thinking about what this PR practically solves, cleanup of checkpoints, should this flag maybe be something like prune --checkpoints as a cleanup command instead? Running prune but choosing to explicitly keep all archives does not seem very intuitive.

I'd say both Yes and No. You're absolutely right that using prune --keep-all to actually just remove checkpoints is a little off. However, on the other hand, prune --checkpoints kinda contradicts the logic of all other options that tell Borg what to --keep-*, not what to remove. So, to throw in yet another alternative, what about remove --checkpoints? I honestly can't tell yet what option (prune --keep-all vs. prune --checkpoints vs. delete --checkpoints) is truly the best… I currently lean to prune, because removing old checkpoints is more a "cleaning up" thing than deleting single archives, and I also lean to prune --keep-all, because I think that keeping prune consistent is advantageous - users must read the docs in this regard anyway.

[Goddesen]

[...] I think that keeping prune consistent is advantageous - users must read the docs in this regard anyway.

Agreed on this. Maybe it could even belong together with compact, as that is already clearly communicated to be a cleanup command. It is already a bit confusing that prune also deals with cleaning up of invisible resources.

Whatever the case, I would like to have "all" and/or "inf" available as retention values and can jump on implementing it pretty quick.

[PhrozenByte]

Maybe it could even belong together with compact, as that is already clearly communicated to be a cleanup command. It is already a bit confusing that prune also deals with cleaning up of invisible resources.

True that. In the end it boils down to whether you consider a checkpoint an "archive" (from a user's perspective, not technically) - if it is an archive, removing it with compact would be unexpected since it is intended to only remove data that was previously marked for deletion by the user; if it is not, then it might be a reasonable thing to do. In the early days checkpoints weren't really hidden from the user and treated like regular archives, but nowadays (since Borg 1.1 I think?) they don't show up anywhere anyway (except list --consider-checkpoints)… AFAIK prune started handling checkpoints before compact existed.

I like the idea to let compact handle this very much: It better matches Borg 2.0. We have no checkpoints in Borg 2.0, just data that isn't referenced by any archive ("yet", to state the similarity with the checkpoint logic) and thus deleted by compact. Since there are no checkpoints in Borg 2.0, compact can't even make an exception if the (non-existing…) checkpoint is the (non-existing…) latest archive.

So, yeah, I feel like compact could be that "true best" solution for Borg 1.x: Remove the checkpoint handling from prune and let compact do the job instead. However, one tiny nit: Could this be considered a breaking change? 🤔 Adding it to Borg 1.4 feels even weirder and I'm not entirely sure whether Thomas wants to invest resources into a Borg 1.5, or rather focus on Borg 2.0.

So, in the end prune --keep-all might not be the best, but the most pragmatic solution.

WDYT? Other opinions?

Whatever the case, I would like to have "all" and/or "inf" available as retention values and can jump on implementing it pretty quick.

IMHO that's a reasonable addition no matter what 👍 I'd go with "all" (prune --keep-yearly "all" with #8775 just feels "right", "inf" feels rather technical).

[ThomasWaldmann]

I added some tests. The reuse of the "secondly" variable for miscellaneous purposes that should be mutually exclusive is problematic. I added one (failing) test that shows the problem @jdchristensen pointed out.

2 other considerations:

• maybe not worth a lot of effort to implement this, users could always use --keep-last=999999 also.
• borg2 does things very differently from borg1. We do not have .checkpoint archives anymore, so this feature would be for borg 1.x only. Guess I did that change in master after this PR was posted.

[ThomasWaldmann]

note that secondly is missing here!

[PhrozenByte]

I added some tests. The reuse of the "secondly" variable for miscellaneous purposes that should be mutually exclusive is problematic. I added one (failing) test that shows the problem @jdchristensen pointed out.

The concerns raised by Dan are very valid, thanks Dan, also thank you Thomas for making the problem more approachable by adding the test: IMO a user won't ever expect prune --keep-all --keep-secondly=2 to prune an archive.

I feel like this comes from me originally suggesting --keep-all to act like --keep-last 1000000000 in #6656. However, this suggestion was made from an user's perspective: I expect --keep-all to act like --keep-last 1000000000, but this doesn't mean it has to be implemented like that.

Let's take a short step back and think about how --keep-* options work from an user's perspective in general. If I start with prune --keep-daily 30 and make it prune --keep-daily 30 --keep-weekly 13, I tell Borg to keep more archives. --keep-* options are additive in a sense: Every time I add a --keep-* option I tell Borg to keep more. Adding --keep-all should do exactly the same: It just tells Borg to keep more - that "more" now means "everything" shouldn't make a difference. I therefore change what I originally said in #6656 and no longer think that enforcing mutual exclusivity of --keep-all with other --keep-* options is the right approach. So, prune --keep-all --keep-secondly=2 (or any other combination with any other --keep-* option) should be valid and just tell Borg to keep everything.

From this point on a technical solution immediately popped into my mind: Why not store the presence of --keep-all in a separate argument, which then causes Borg to reset all other --keep-* options to zero and args.secondly to infinity (as before, just not directly, but via this detour)? prune --keep-all --keep-secondly=2 would work just fine then. Basically like this:

    def build_parser_prune(self, subparsers, common_parser, mid_common_parser):
        # …
        subparser.add_argument(
            "--keep-all",
            dest="all",
            action="store_true",
            help="keep all archives (overrules all other --keep-* options)",
        )
        # …

    def do_prune(self, args, repository, manifest):
        # …
        if args.all:
            args.within = 0
            args.secondly = float("inf")
            args.minutely = 0
            args.hourly = 0
            args.daily = 0
            args.weekly = 0
            args.monthly = 0
            args.quarterly_13weekly = 0
            args.quarterly_3monthly = 0
            args.yearly = 0
        # …

Regarding Borg 2: I feel like that even if things end up making prune --keep-all a no-op with Borg 2, it still has the benefit of allowing users to easily disable pruning temporarily without being required to remove the retention policy. I don't see much harm if things end up with if args.all: return in Borg 2.