Repository navigation
prune: Use calendar-aware retention intervals - #10541
NIne-WIngEd wants to merge 1 commit into
Conversation
ThomasWaldmann
left a comment
There was a problem hiding this comment.
Thanks! The approach is right and matches what #9929 asked for: calculate_relative_offset() replaces the 31-day/365-day interval(), relative_time_marker_validator does the parsing, and the dead interval() is gone. One real problem needs fixing before merge; the rest are nits.
Must fix: the interval-order check now depends on the run date
Calendar intervals are converted to a timedelta at the reference timestamp, and _validate_prune_args() then compares those values. So whether a rule combination is "effectively useless" changes from day to day, and a scheduled prune job fails on some days (rc 4) and works on others:
| args | --from |
result |
|---|---|---|
--keep-weekly 4w --keep-monthly 1m |
2026-03-10 | error: weekly='28 days, 0:00:00' vs monthly='28 days, 0:00:00' |
| same | 2026-04-10 | ok |
--keep-weekly 13w --keep-monthly 3m |
2026-05-01 | error: 91 days vs 89 days |
| same | 2026-05-31 | ok |
test_prune_calendar_interval_order and the docs ("1m can be shorter than, equal to, or longer than 30d depending on the month") describe this as intended, but it is bad for cron users.
Suggestion: make the check independent of the date:
- both intervals calendar units (m/y): compare in months (so
12mvs1ystill errors), - both fixed units: compare in seconds,
- mixed: error only if the shorter rule's minimum possible length is >= the longer rule's maximum possible length (a month is 28..31 days, a year 365..366 days).
A smaller alternative: for mixed units, log a warning instead of raising.
Nits
- The error message shows the converted value (
weekly='28 days, 0:00:00') instead of what the user typed (4w,1m). The original strings are available before the conversion, so it could show those. do_pruneoverwrites the rule values inargswith the convertedtimedelta. That works, but a separate dict of converted values would be cleaner and would make the error message fix above easy.test_prune_interval_rolling_schedule_oldest_retentionnow has two blank lines inside the function body where the comment was removed; black (pre-commit) reformats it.- That test also lost its comment explaining why
01-01is the expected oldest archive. With the range changed from 95 to 93 days, an updated comment would be better than none. - Minor, not new: the reference timestamp from
datetime.now().astimezone()has a fixed UTC offset, so1macross a DST change ends up one hour off in local wall-clock time. The old code andrepo-list --olderbehave the same; not a blocker.
Looks good
- The docs example (
prune-example-interval.txt) and the expected test output agree: 2025-12-31 now fills the 2025 yearly slot. - Out-of-range values (
99999y, huged) are caught and reported as aCommandErrorbefore anything is deleted, with a test. 0m/0ycorrectly count as zero-length intervals.- The new boundary tests cover month ends, leap years, microseconds and timezone offsets.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10541 +/- ##
==========================================
- Coverage 89.36% 89.34% -0.03%
==========================================
Files 104 104
Lines 20082 20071 -11
Branches 3152 3151 -1
==========================================
- Hits 17947 17932 -15
- Misses 1478 1483 +5
+ Partials 657 656 -1 ☔ View full report in Codecov by Harness. |
jdchristensen
left a comment
There was a problem hiding this comment.
I just took a quick look at the docs, not the implementation. I think the documented behaviour seems reasonable.
| 30m31 | ||
|
|
||
| April May June | ||
| 1 2 3 4 5 1 2 3w 1d 2d 3 4x |
There was a problem hiding this comment.
I think the calendar view would be a lot easier to understand if the days without backups were also marked. E.g. they could have a suffix "n". Or those dates could just not be displayed. On the line above this comment, this idea would apply to "June 3".
| (or February 29 in a leap year). | ||
|
|
||
| Weeks, days, hours, minutes, and seconds keep their fixed-length meanings. | ||
| For example, ``1w`` is seven days. Plain counts and ``all`` are unchanged. |
There was a problem hiding this comment.
"Plain counts and all are unchanged." This reads like a description of how to current implementation differs from an earlier one, but the docs should just document the current version.
There was a problem hiding this comment.
All in all: Great work, thanks! ❤️
As always: I haven't checked the code, just the docs.
the interval-order check now depends on the run date
Do we actually need the lo_hi_mismatch check at all? I agree that e.g. --keep-daily 30d --keep-weekly 7d is effectively useless, but what harm does it do? It simply won't match anything. Just as other rules might not match anything as well. In fact, allowing this makes temporarily changing a retention policy easier, because one doesn't have to remove and later restore other rules.
So I'd argue we could simply remove this check altogether, but keep the example in the docs.
| example, the 2026 yearly slot is consumed by earlier rules. December 31 | ||
| fills the 2025 slot, and November 15 is retained as the oldest archive. |
There was a problem hiding this comment.
| example, the 2026 yearly slot is consumed by earlier rules. December 31 | |
| fills the 2025 slot, and November 15 is retained as the oldest archive. | |
| example, 2025-12-31 fills the first slot, and 2025-11-15 fills the second slot | |
| as the oldest remaining archive, preserving rolling backup semantics. |
- "YYYY slot" suggests an interval-based rule, so it's better to refer to the slots by number ("first slot").
- The absence of a yearly candidate for 2026 isn't due to earlier rules, but to 2026 being the current year.
- Use the
YYYY-MM-DDdate format for consistency.
However, more importantly, the original point of this paragraph has been lost. It was meant to illustrate that --keep-yearly 2 doesn't count 2025-12-31, because that archive was already kept by the previous --keep-monthly rule. With calendar-aware retention intervals, --keep-monthly 5m no longer matches 2025-12-31.
We could change the example to --keep-monthly 6m instead. That way, 2025-12-31 is matched by --keep-monthly again, leaving no "true" candidates for --keep-yearly.
Alternatively, this paragraph might simply be removed and/or merged with the --keep-weekly paragraphs. It's somewhat redundant, because we mix count-based and interval-based rules there already anyway. Just a short sentence about --keep-yearly keeping 2025-11-15 as the oldest archive to preserve rolling backup semantics should remain.
| is clamped to its last day: one month before March 31 is February 28 | ||
| (or February 29 in a leap year). |
There was a problem hiding this comment.
| is clamped to its last day: one month before March 31 is February 28 | |
| (or February 29 in a leap year). | |
| is clamped to its last day: one month before 2024-03-31 is 2024-02-29 (due | |
| to 2024 being a leap year), and one year before 2024-02-29 is 2023-02-28. |
Use the YYYY-MM-DD date format for consistency. 2024 could be used to illustrate a leap year.
| Weeks, days, hours, minutes, and seconds keep their fixed-length meanings. | ||
| For example, ``1w`` is seven days. Plain counts and ``all`` are unchanged. | ||
|
|
||
| When combining interval-based pruning rules, their lengths are compared at | ||
| the same reference timestamp. Thus, ``1m`` can be shorter than, equal to, | ||
| or longer than ``30d`` depending on the month. |
There was a problem hiding this comment.
| Weeks, days, hours, minutes, and seconds keep their fixed-length meanings. | |
| For example, ``1w`` is seven days. Plain counts and ``all`` are unchanged. | |
| When combining interval-based pruning rules, their lengths are compared at | |
| the same reference timestamp. Thus, ``1m`` can be shorter than, equal to, | |
| or longer than ``30d`` depending on the month. | |
| Weeks, days, hours, minutes, and seconds have a fixed length. For example, | |
| ``1w`` is 7 days, and ``1d`` matches ``86400s``. Years and months are | |
| interpreted relative to a reference timestamp and can vary in length. | |
| Thus, ``1m`` can be shorter than, equal to, or longer than 30 days, | |
| depending on the month. ``1y`` can match either ``365d`` or ``366d``, | |
| depending on the year, but it always matches 12 months. |
As Dan noted earlier, the first paragraph reads more like a changelog entry. I think we can merge these two paragraphs and mention years here as well.
Furthermore, timespans aren't only used by prune, so we shouldn't limit this explanation to that command. It might make sense to move the paragraph starting with "The borg prune --keep-* retention options […]" below this one, making it the last paragraph of this section.
I changed pruning to use calendar months and years instead of fixed day counts. The retention cutoff now uses Borg's existing calendar helper. I also added tests for month ends and leap years and updated the docs and pruning example.
Fixes #9929