Don't pass the --stats flag to borg prune when using Borg 2.0 #1010

New issue

Closed

opened 2025-02-27 00:44:00 +00:00 by witten · 9 comments

witten commented 2025-02-27 00:44:00 +00:00

Owner

Copy link

What I'm trying to do and why

When using Borg 2.0 and running borgmatic with --stats (which implicitly runs prune), the user receives an error because Borg 2.0 no longer supports prune --stats: https://borgbackup.readthedocs.io/en/latest/usage/prune.html

It appears to have been removed in this commit, which suggests using compact for stats instead: git.zknt.org/mirror/borg@4c052cd65d (diff-a08f625915)

Also see: https://github.com/borgmatic-collective/docker-borgmatic/issues/395

Steps to reproduce

Use Borg 2.0 and run borgmatic --stats or a variant thereof.

Actual behavior

borg: error: unrecognized arguments: --stats

Expected behavior

borgmatic runs the default actions and doesn't pass --stats to prune (when using Borg 2).

Other notes / implementation ideas

Probably the thing to do here is add a feature toggle such that --stats gets passed to prune on Borg 1 but not 2.

It'd also be a good idea to update the CLI docs to say "Borg 1 only" on prune --stats.

borgmatic version

1.9.12

borgmatic installation method

No response

Borg version

2.0.0.b13

Python version

No response

Database version (if applicable)

No response

Operating system and version

No response

### What I'm trying to do and why When using Borg 2.0 and running `borgmatic` with `--stats` (which implicitly runs `prune`), the user receives an error because Borg 2.0 no longer supports `prune --stats`: https://borgbackup.readthedocs.io/en/latest/usage/prune.html It appears to have been removed in this commit, which suggests using compact for stats instead: https://git.zknt.org/mirror/borg/commit/4c052cd65d6f9e98ab6cae45ae15849e59dea686#diff-a08f625915d1815ca2a9b7d06a60630aeecc4695 Also see: https://github.com/borgmatic-collective/docker-borgmatic/issues/395 ### Steps to reproduce Use Borg 2.0 and run `borgmatic --stats` or a variant thereof. ### Actual behavior ``` borg: error: unrecognized arguments: --stats ``` ### Expected behavior borgmatic runs the default actions and doesn't pass `--stats` to `prune` (when using Borg 2). ### Other notes / implementation ideas Probably the thing to do here is add a feature toggle such that `--stats` gets passed to `prune` on Borg 1 but not 2. It'd also be a good idea to update the CLI docs to say "Borg 1 only" on `prune --stats`. ### borgmatic version 1.9.12 ### borgmatic installation method _No response_ ### Borg version 2.0.0.b13 ### Python version _No response_ ### Database version (if applicable) _No response_ ### Operating system and version _No response_

witten added the

good first issue

label 2025-02-27 00:46:50 +00:00

gb-123-git commented 2025-02-27 08:20:35 +00:00

Copy link

Additional Info :
Please Pass --stats to borg compact in-case borg compact allows it. (I am a little confused here whether it supports it or not since at one place it is mentioned it supports it but in the official documents it is not there.)

Additional Info : Please Pass --stats to `borg compact` in-case `borg compact` allows it. (I am a little confused here whether it supports it or not since at one place it is mentioned it supports it but in the official documents it is not there.)

witten commented 2025-02-27 16:44:15 +00:00

Author

Owner

Copy link

Yeah, unfortunately it doesn't appear that Borg 2 implements compact --stats: https://borgbackup.readthedocs.io/en/2.0.0b14/usage/compact.html ... That changelog might just be indicating that compact displays some stats by default without actually having a formal --stats flag. For instance, these "stats" are what I get when running Borg 2 just with compact --info (equivalent to borgmatic's --verbosity 1):

# ./borg2.0.0b14 compact --repo tmp/test2.borg --info
Enter passphrase for key /root/tmp/test2.borg: 
Starting compaction / garbage collection...
Getting object IDs present in the repository...
Computing object IDs used by archives...
Cleaning archives directory from soft-deleted archives...
Determining unused objects...
Deleting 0 unused objects...
Overall statistics, considering all 0 archives in this repository:
Source data size was 0 B in 0 files.
Repository size is 0 B in 0 objects.
Compaction saved 0 B.
Finished compaction / garbage collection...

Yeah, unfortunately it doesn't appear that Borg 2 implements `compact --stats`: https://borgbackup.readthedocs.io/en/2.0.0b14/usage/compact.html ... That changelog might just be indicating that compact displays some stats by default without actually having a formal `--stats` flag. For instance, these "stats" are what I get when running Borg 2 just with `compact --info` (equivalent to borgmatic's `--verbosity 1`): ``` # ./borg2.0.0b14 compact --repo tmp/test2.borg --info Enter passphrase for key /root/tmp/test2.borg: Starting compaction / garbage collection... Getting object IDs present in the repository... Computing object IDs used by archives... Cleaning archives directory from soft-deleted archives... Determining unused objects... Deleting 0 unused objects... Overall statistics, considering all 0 archives in this repository: Source data size was 0 B in 0 files. Repository size is 0 B in 0 objects. Compaction saved 0 B. Finished compaction / garbage collection... ```

120EE0980 commented 2025-03-13 02:06:51 +00:00

Contributor

Copy link

I want to work on this issue.

I want to work on this issue.

witten commented 2025-03-13 02:19:24 +00:00

Author

Owner

Copy link

Awesome, please have a look at the docs on how to get started with development! It might also save you time if you wanted to outline your approach to the ticket here to get feedback before you start writing code. But it's also small enough that you could probably just submit a PR if you don't mind the risk of re-work.

One thing that may be useful for this particular ticket is the existing feature flag support. That's how borgmatic typically provides different behavior for different versions of Borg. And here's an example of that in action.

Let me know if you have any questions!

Awesome, please have a look at the [docs on how to get started with development](https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/)! It might also save you time if you wanted to outline your approach to the ticket here to get feedback before you start writing code. But it's also small enough that you could probably just submit a PR if you don't mind the risk of re-work. One thing that may be useful for this particular ticket is the existing [feature flag](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/borg/feature.py) support. That's how borgmatic typically provides different behavior for different versions of Borg. And [here's an example](https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/901e668c760f6a17e4f02ef418ff65285bbca5ed/borgmatic/actions/compact.py#L30) of that in action. Let me know if you have any questions!

120EE0980 commented 2025-03-13 17:36:15 +00:00

Contributor

Copy link

Yes I have few things to say:

1. In feature flag support, FEATURE_TO_MINIMUM_BORG_VERSION is meant for when a feature is introduced, so I would like add new dictionary FEATURE_REMOVAL_BORG_VERSION to store PRUNE_STATS feature and its removal version 2.0.0b10(will add corresponding logic in available)(considering --stats is available since 1.1.0 - I guess there is no need of minimum version check)
2. This is where implementation goes as
   (('--stats',) if prune_arguments.stats and not dry_run and borgmatic.borg.feature.available(borgmatic.borg.feature.Feature.PRUNE_STATS, local_borg_version) else ())

Yes I have few things to say: 1. In [feature flag](https://projects.torsion.org/borgmatic-collective/borgmatic/src/branch/main/borgmatic/borg/feature.py) support, [FEATURE_TO_MINIMUM_BORG_VERSION](https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/901e668c760f6a17e4f02ef418ff65285bbca5ed/borgmatic/borg/feature.py#L22) is meant for when a feature is introduced, so I would like add new dictionary FEATURE_REMOVAL_BORG_VERSION to store PRUNE_STATS feature and its removal version 2.0.0b10(will add corresponding logic in [available](https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/901e668c760f6a17e4f02ef418ff65285bbca5ed/borgmatic/borg/feature.py#L39))(considering --stats is available since 1.1.0 - I guess there is no need of minimum version check) 2. [This](https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/901e668c760f6a17e4f02ef418ff65285bbca5ed/borgmatic/borg/prune.py#L78) is where implementation goes as `(('--stats',) if prune_arguments.stats and not dry_run and borgmatic.borg.feature.available(borgmatic.borg.feature.Feature.PRUNE_STATS, local_borg_version) else ())`

witten commented 2025-03-13 17:52:05 +00:00

Author

Owner

Copy link

That makes sense to me! For point number one though, I think you can simplify even further. Rather than introducing the concept of a "feature removal", you can continue using FEATURE_TO_MINIMUM_BORG_VERSION and add an entry to it called something like Feature.STATLESS_PRUNE or Feature.NO_PRUNE_STATS or whatever. The idea being that a Borg feature removal is really a feature change or addition just like anything else. Then you wouldn't have to update available() either.

Also, don't forget the automated tests piece of this (I'm think mostly for point number two), which should hopefully be pretty straight-forward. Thank you!

That makes sense to me! For point number one though, I think you can simplify even further. Rather than introducing the concept of a "feature removal", you can continue using `FEATURE_TO_MINIMUM_BORG_VERSION` and add an entry to it called something like `Feature.STATLESS_PRUNE` or `Feature.NO_PRUNE_STATS` or whatever. The idea being that a Borg feature removal is really a feature change or addition just like anything else. Then you wouldn't have to update `available()` either. Also, don't forget the automated tests piece of this (I'm think mostly for point number two), which should hopefully be pretty straight-forward. Thank you!

120EE0980 commented 2025-03-13 20:52:22 +00:00

Contributor

Copy link

Also, don't forget the automated tests piece of this (I'm think mostly for point number two), which should hopefully be pretty straight-forward. Thank you!

Is it about adding
flexmock(module.feature).should_receive('available').with_args( module.feature.Feature.NO_PRUNE_STATS, '1.2.3' ).and_return(True/False)
to all tests(in test_prune.py)(True/False based on test) with stats such as.Or is it required to write new tests? Please correct me if I am wrong(I am new to testing)

> Also, don't forget the automated tests piece of this (I'm think mostly for point number two), which should hopefully be pretty straight-forward. Thank you! Is it about adding `flexmock(module.feature).should_receive('available').with_args( module.feature.Feature.NO_PRUNE_STATS, '1.2.3' ).and_return(True/False)` to all tests(in test_prune.py)(True/False based on test) with stats [such as](https://projects.torsion.org/borgmatic-collective/borgmatic/src/commit/901e668c760f6a17e4f02ef418ff65285bbca5ed/tests/unit/borg/test_prune.py#L208).Or is it required to write new tests? Please correct me if I am wrong(I am new to testing)

witten commented 2025-03-13 21:05:39 +00:00

Author

Owner

Copy link

Adding that mock to the existing tests seems like a good idea, but you might need to add a single new test—since presumably the existing tests wouldn't vary the available(NO_PRUNE_STATS, ...) return value. For instance, the mock you add to all the existing tests might all return True, and then you could just add one more test that has the False return value mock and the corresponding expectation. (Or the other way around.)

Adding that mock to the existing tests seems like a good idea, but you might need to add a single new test—since presumably the existing tests wouldn't vary the `available(NO_PRUNE_STATS, ...)` return value. For instance, the mock you add to all the existing tests might all return `True`, and then you could just add one more test that has the `False` return value mock and the corresponding expectation. (Or the other way around.)

👍 1

120EE0980 referenced this issue from a pull request that will close it, 2025-03-14 05:56:16 +00:00

Add feature toggle to conditionally pass --stats to prune based on Borg version #1026

witten closed this issue 2025-03-15 16:59:51 +00:00

witten referenced this issue from a commit 2025-03-15 16:59:51 +00:00

Conditionally pass --stats to prune based on Borg version (#1010).

witten referenced this issue from a commit 2025-03-15 17:03:20 +00:00

Add Borg 2 "prune --stats" flag change to NEWS (#1010).

witten commented 2025-04-06 15:44:03 +00:00

Author

Owner

Copy link

Released in borgmatic 2.0.0!

Released in borgmatic 2.0.0!

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

parallel-borgmatic

command-hook-steps

2.1.7

2.1.6

2.1.5

2.1.4

2.1.3

2.1.2

2.1.1

2.1.0

2.0.13

2.0.12

2.0.11

2.0.10

2.0.9

2.0.8

2.0.7

2.0.6

2.0.5

2.0.4

2.0.3

2.0.2

2.0.1

2.0.0

1.9.14

1.9.13

1.9.12

1.9.11

1.9.10

1.9.9

1.9.8

1.9.7

1.9.6

1.9.5

1.9.4

1.9.3

1.9.2

1.9.1

1.9.0

1.8.14

1.8.13

1.8.12

1.8.11

1.8.10

1.8.9

1.8.8

1.8.7

1.8.6

1.8.5

1.8.4

1.8.3

1.8.2

1.8.1

1.8.0

1.7.15

1.7.14

1.7.13

1.7.12

1.7.11

1.7.10

1.7.9

1.7.8

1.7.7

1.7.6

1.7.5

1.7.4

1.7.3

1.7.2

1.7.1

1.7.0

1.6.6

1.6.5

1.6.4

1.6.3

1.6.2

1.6.1

1.6.0

1.5.24

1.5.23

1.5.22

1.5.21

1.5.20

1.5.19

1.5.18

1.5.17

1.5.16

1.5.15

1.5.14

1.5.13

1.5.12

1.5.11

1.5.10

1.5.9

1.5.8

1.5.7

1.5.7.dev0

1.5.6

1.5.5

1.5.4

1.5.3

1.5.2

1.5.1

1.5.0

1.4.22

1.4.21

1.4.20

1.4.19

1.4.18

1.4.17

1.4.16

1.4.15

1.4.14

1.4.13

1.4.12

1.4.11

1.4.10

1.4.9

1.4.8

1.4.7

1.4.6

1.4.5

1.4.4

1.4.3

1.4.2

1.4.1

1.4.0

1.3.26

1.3.25

1.3.24

1.3.23

1.3.22

1.3.21

1.3.20

1.3.19

1.3.18

1.3.17

1.3.16

1.3.15

1.3.14

1.3.13

1.3.12

1.3.11

1.3.10

1.3.9

1.3.8

1.3.7

1.3.6

1.3.5

1.3.4

1.3.3

1.3.2

1.3.1

1.3.0

1.2.18

1.2.17

1.2.16

1.2.15

1.2.14

1.2.13

1.2.12

1.2.11

1.2.10

1.2.9

1.2.8

1.2.7

1.2.6

1.2.5

1.2.4

1.2.3

1.2.2

1.2.1

1.2.0

1.1.15

1.1.14

1.1.13

1.1.12

1.1.11

1.1.10

1.1.9

1.1.7

1.1.6

1.1.5

1.1.4

1.1.3

1.1.2

1.1.1

1.1.0

1.0.3

1.0.2

1.0.1

1.0.0

0.1.8

0.1.7

0.1.6

0.1.5

0.1.4

0.1.3

0.1.2

0.1.1

0.1.0

0.0.7

0.0.6

0.0.5

0.0.4

0.0.3

0.0.2

Labels

Clear labels   

blocked

  

breaking

  

bug

  

data loss

  

design finalized

  

good first issue

  

new feature area

  

question / support

  

security

  

waiting for response

No labels

blocked

breaking

bug

data loss

design finalized

good first issue

new feature area

question / support

security

waiting for response

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

borgmatic-collective/borgmatic#1010

No description provided.