-
-
Notifications
You must be signed in to change notification settings - Fork 752
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
Add Dry-Run Flags to borg extract Command (#8564) #8575
base: master
Are you sure you want to change the base?
Conversation
BTW, don't add |
Got it, I’ll ensure temporary files are omitted moving forward. Could you please share more details or context about the errors above? This would help me better understand and address the issue. |
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.
some more feedback.
The failing |
… flags - Ensure only matched items are processed and logged during extraction - Log excluded items with a `-` flag during dry-run
… exclusion flags - Verified that excluded items are not listed during non-dry-run extraction.
Changed the code and now everything seems to be running as expected at my end and borg extract finishes with rc 0. Just wanted to ask if the errors could be because of me working on MacOS and not having pyfuse3? |
The errors are unrelated to macOS and not having pyfuse3 - I also work on a macbook. If one want to test or work on the mount / FUSE code in borg, one would need macFUSE installed and use llfuse (not pyfuse3). But this is not required here. |
@ThomasWaldmann Please review. |
Used an in-line filter to make sure the items are displayed in the order they appear in the archive. Thank you so much for bearing with me as I learn to navigate the codebase!
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 is getting tricky.
src/borg/archiver/extract_cmd.py
Outdated
@@ -56,32 +56,53 @@ def do_extract(self, args, repository, manifest, archive): | |||
else: | |||
pi = None | |||
|
|||
for item in archive.iter_items(filter, preload=True): | |||
for item in archive.iter_items(preload=True): |
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.
Hmm, this develops to be a bit more tricky than I initially thought it is.
If you leave away the filter here (which at first looks like a good idea to get ALL items yielded), it will preload the file content chunks of ALL yielded items (not just the ones we might extract).
That's not necessary and it is even harmful, leading to a memory leak if one does not retrieve these chunks, see comment in unpack_many
.
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.
@ThomasWaldmann When you use the filter, only items that match the filter criteria are processed / "pass-through" which prevents us from logging any excluded items. Hence, I proposed the "two-pass" solution but that would display excludes either at the beginning or the end.
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.
Sure. So there is no good solution yet and it is not easy due to the preloading requirements.
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 am working right now on decoupling preloading and item iteration to help you with that.
After that is merged, you can rebase your work on these changes.
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.
Thank you so much. I really appreciate it!
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.
#8592 was just merged into master.
Please update your local master branch and rebase your PR branch onto that.
As you can see there, archive.iter_items()
does not support preloading anymore.
At the places where preloading is needed, I inserted a new call that does the preloading.
Be careful: the preload call must only be done if the item chunks will be fetched later (e.g. if the item is extracted, because it is included / not excluded). The borg extract
code fetches the content chunks even if it is in dry-run mode, because it is documented to do all steps of the extraction except writing content to disk files (so people can use it as a practical test whether the extraction would work without needing the space to do the extraction).
Summary
This pull request addresses the issue with the
borg extract --dry-run --list
command not displaying the flags as seen inborg create
andborg recreate
commands.