-
-
Notifications
You must be signed in to change notification settings - Fork 822
soft delete / undelete #8515
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
soft delete / undelete #8515
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8515 +/- ##
==========================================
+ Coverage 81.47% 81.75% +0.28%
==========================================
Files 73 74 +1
Lines 13142 13240 +98
Branches 1927 1941 +14
==========================================
+ Hits 10707 10824 +117
+ Misses 1770 1753 -17
+ Partials 665 663 -2 ☔ View full report in Codecov by Sentry. |
19a8a35 to
edb5801
Compare
PhrozenByte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good 👍
I have some comments and suggestions about the docs, e.g. I recommend being very consistent in always calling them "soft-deleted archives".
Important: I neither did a code review, nor did I test any of the changes.
|
What do you think about also updating the |
This keeps the object, just renames it to "*.del".
|
I thought about it: I will remove all user docs of "soft-deleted" again from borg and only explain what's happening in Mentioning it all over the place isn't necessarily helpful, explaining it everywhere makes the docs longer than necessary. |
1ac9ca3 to
6dfd35e
Compare
We are only interested in archive metadata objects here, thus for most repo objects it is enough to read the repoobj's metadata and determine the object's type. Only if it is the right type of object, we need to read the full object (metadata and data).
Consider soft-deleted archives/ directory entries, but only create a new archives/ directory entry if: - there is no entry for that archive ID - there is no soft-deleted entry for that archive ID either Support running with or without --repair. Without --repair, it can be used to detect such inconsistencies and return with rc != 0. --repository-only contradicts --find-lost-archives.
6dfd35e to
72b1a8e
Compare
|
@PhrozenByte I did it a bit differently, but hopefully giving all the information that is useful to the user (and not just an implementation detail). |
c790938 to
142a739
Compare
PhrozenByte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove all user docs of "soft-deleted" again from borg and only explain what's happening in borg undelete and/or borg compact command, where it is somehow relevant.
Mentioning it all over the place isn't necessarily helpful, explaining it everywhere makes the docs longer than necessary.
I understand that you want to shorten the docs and remove unnecessary detail, not that you object documenting this distinction at all, right?
I totally agree with shortening the docs: reading over my suggestions again they indeed were a bit excessive (I have that tendency 😆). However, there's a big issue with trying not to mention the distinction at all: At some occurrences Borg must talk about them, because Borg is specifically dealing with them. Therefore the docs must deal with the distinction and the current docs try solving it in two ways: By either calling them "deleted archives", or by not mentioning them at all. Both IMHO is pretty problematic:
-
Borg shouldn't call them "deleted archives" - because they simply weren't deleted yet. This IMHO creates way more confusion, because, as a user, I know the common term "deleted" to mean "gone for good". This was fine before, because deletion actually meant deletion (
check --repairwas and is data recovery), but withundeleteit no longer does.Take email software like Thunderbird with default
.mboxstorage in comparison: Completely removing an email is called "Delete" in the UI, even though the email isn't actually deleted from the.mboxfile, but only after the.mboxfile is compacted separately. Thunderbird calls that action "Delete" nevertheless, because Thunderbird doesn't allow restoring that email. However, alternatively one can also "Trash" an email, meaning that the email isn't actually deleted, but just moved out of the user's sight.borg deletepreviously only did the first variant, but withundeleteit rather does the latter - and therefore the term must change as well.People know what "delete" means, and if it unexpectedly means something else, it creates confusion - not for you and me, but new users, not knowing yet what Borg is doing there. Thus, instead of (i.e. not additionally, just instead of) calling them "deleted archives", better call them "soft-deleted archives": it's more precise, doesn't cause such confusion and gives a nice "heads up" that there must be an easy way to get them back (i.e.
undelete). -
I agree that the docs don't have to explain what "soft-deleted" means all over the place, especially if nothing special is happening. From an users perspective one expects "archive" (i.e. without an adjective) to mean a normal/not-deleted archive, i.e. the archives one sees with e.g.
list. Calling them "normal/not-deleted" implies speciality that isn't there, so I agree that it causes confusion to call them anything but just "archive". However, if Borg (also) considers soft-deleted archives, it should mention this - and only then. Otherwise one just can't properly assess (both "not understanding" and "understanding wrong") what Borg will do from the docs.For example, in the
checkdocs, "archive" meant both normal and soft-deleted archives in the main section, but just normal archive in the repair section (see line comment there). This sends the user on the wrong track within the main section, and prevents the user from knowing what will happen in the repair section, thus causing confusion. A very short and simple clarification is sufficient to completely fix that.If you just don't like the term "soft-deletion" I'd like to emphasize that there are alternatives. For example, many UI's use "trash" (
delete) and "untrash" (undelete).
This naturally also means that one must explain what "soft-deleted archive" means at some point - and instead of excessively explaining it all over the place like I did in my previous suggestion, it's sufficient to just use the term "soft-delete" in prune and delete once respectively, together with the already existing mention of undelete. This makes it absolutely clear what "soft-deletion" means and allows users to understand it anywhere else, too. Simple and precise.
| metadata that doesn't match with an archive directory entry, it means that an | ||
| entry was lost. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| metadata that doesn't match with an archive directory entry, it means that an | |
| entry was lost. | |
| metadata that doesn't match with an archive directory entry (including | |
| soft-deleted archives), it means that an entry was lost. |
IMHO it's particularly important to clarify this here, because without I'd assume that --find-lost-archives only considers the archives shown with list, meaning that --find-lost-archives also recovers soft-deleted archives - which it intentionally doesn't.
As said in the main review comment the "normal/not-deleted archive" clarification from the previous suggestion isn't necessary.
|
|
||
| if deleted: | ||
| filters_group.add_argument( | ||
| "--deleted", dest="deleted", action="store_true", help="consider only deleted archives." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "--deleted", dest="deleted", action="store_true", help="consider only deleted archives." | |
| "--deleted", dest="deleted", action="store_true", help="consider only soft-deleted archives." |
Reopening, see main review comment.
| if self.manifest.archives.exists_id(archive_id, deleted=False): | ||
| logger.debug(f"We already have an archives directory entry for {name} {archive_id_hex}.") | ||
| elif self.manifest.archives.exists_id(archive_id, deleted=True): | ||
| logger.debug(f"We already have a deleted archives directory entry for {name} {archive_id_hex}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logger.debug(f"We already have a deleted archives directory entry for {name} {archive_id_hex}.") | |
| logger.debug(f"We already have a soft-deleted archives directory entry for {name} {archive_id_hex}.") |
Reopening, see main review comment.
| logger.warning(f"{len(self.reappeared_chunks)} previously missing objects re-appeared!" + run_repair) | ||
| set_ec(EXIT_WARNING) | ||
|
|
||
| logger.info("Cleaning archives directory from deleted archives...") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logger.info("Cleaning archives directory from deleted archives...") | |
| logger.info("Cleaning archives directory from soft-deleted archives...") |
See main review comment.
| After compacting it is not possible anymore to use ``borg undelete`` to recover | ||
| previously deleted archives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| After compacting it is not possible anymore to use ``borg undelete`` to recover | |
| previously deleted archives. | |
| After compacting it is no longer possible to use ``borg undelete`` to recover | |
| soft-deleted archives. |
See main review comment.
| - interrupted backups (maybe retry the backup first before running compact!) | ||
| - backup of source files that had an I/O error in the middle of their contents | ||
| and that were skipped due to this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - interrupted backups (maybe retry the backup first before running compact!) | |
| - backup of source files that had an I/O error in the middle of their contents | |
| and that were skipped due to this. | |
| - interrupted backups (maybe retry the backup first before running compact) | |
| - backup of source files that had an I/O error in the middle of their contents | |
| and that were skipped due to this |
Unify punctuation.
| - interrupted backups (maybe retry the backup first before running compact!) | ||
| - backup of source files that had an I/O error in the middle of their contents | ||
| and that were skipped due to this. | ||
| - corruption of the repository (e.g. the archives directory having lost entries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - corruption of the repository (e.g. the archives directory having lost entries) | |
| - corruption of the repository (e.g. the archives directory having lost | |
| entries, see notes below) |
| @@ -64,8 +64,11 @@ def build_parser_delete(self, subparsers, common_parser, mid_common_parser): | |||
| """ | |||
| This command deletes archives from the repository. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This command deletes archives from the repository. | |
| This command soft-deletes archives from the repository. |
See main review comment. It's really the only necessary change to delete and prune thanks to the later mention of undelete.
| The prune command prunes a repository by deleting all archives not matching | ||
| any of the specified retention options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The prune command prunes a repository by deleting all archives not matching | |
| any of the specified retention options. | |
| The prune command prunes a repository by soft-deleting all archives not | |
| matching any of the specified retention options. |
See main review comment. It's really the only necessary change to delete and prune thanks to the later mention of undelete.
|
|
||
| Important: Undeleting archives is only possible before compacting. | ||
| Once ``borg compact`` has run, all disk space occupied only by the | ||
| deleted archives will be freed and undelete is not possible anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| deleted archives will be freed and undelete is not possible anymore. | |
| soft-deleted archives will be freed and undelete is not possible | |
| anymore. |
See main review comment.
|
I suggest you apply all your suggestions in a separate PR. I already mentioned that applying suggestions via the github UI doesn't always work. Then it sometimes also causes |
Follow-up to borgbackup#8515 Refer to borgbackup#8515 (review) for details.
Follow-up to borgbackup#8515 Refer to borgbackup#8515 (review) for details.
Follow-up to borgbackup#8515 Refer to borgbackup#8515 (review) for details.
Follow-up to borgbackup#8515 Refer to borgbackup#8515 (review) for details.
Original PR #8515 by ThomasWaldmann Original: borgbackup/borg#8515
Original PR #8515 by ThomasWaldmann Original: borgbackup/borg#8515
Original PR #8515 by ThomasWaldmann Original: borgbackup/borg#8515
No description provided.