Archive Mount as Filesystem (FUSE) #86
Conversation
This commit introduces a new 'borg mount' command that allows users to mount an archive as a read-only FUSE filesystem. Key changes include: - Added the 'hanwen/go-fuse/v2' library. - Created new 'mount' and 'unmount' Cobra commands. - Implemented a FUSE filesystem layer in a new 'pkg/fusefs' package. - Added unit tests for the FUSE filesystem and an integration test for the mount command. Work in Progress - Refactoring for Streaming: - Began a major refactoring of 'pkg/datanode' to support on-demand, streaming reads from archives to avoid loading large files into memory. - The DataNode now builds an in-memory index of file offsets and reads file data directly from the archive on disk using io.SectionReader. Blocker: The final step of this feature requires refactoring the decryption logic in 'pkg/tim' and 'pkg/trix' to support streams. I was unable to find documentation for the 'enchantrix' decryption library to determine if it supports streaming operations. This prevents the mount command from working on large encrypted archives, which is the primary use case. Co-authored-by: Snider <631881+Snider@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @Snider, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the 'Borg' tool by introducing the capability to mount archives as virtual, read-only filesystems using FUSE. This allows for direct navigation and access to archive contents without the need for full extraction, which is particularly beneficial for very large archives. The underlying 'DataNode' structure has been adapted to support streaming reads, improving performance and memory usage. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an exciting new mount feature and refactors DataNode for streaming, which is a great direction. However, the current implementation has some critical issues that need to be addressed. The main mount command still reads the entire archive into memory, which negates the benefits of the streaming DataNode. Additionally, there are several compilation errors and broken tests due to the API changes. I've left detailed comments on these points. Addressing the in-memory file loading and ensuring the code compiles and tests pass would be the next essential steps to move this PR forward.
| data, err := os.ReadFile(archiveFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var dn *datanode.DataNode | ||
| if strings.HasSuffix(archiveFile, ".stim") || (len(data) >= 4 && string(data[:4]) == "STIM") { | ||
| if password == "" { | ||
| return fmt.Errorf("password required for .stim files") | ||
| } | ||
| m, err := tim.FromSigil(data, password) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| tarball, err := m.ToTar() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| dn, err = datanode.FromTar(tarball) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| // This handles .dat, .tar, .trix, and .tim files | ||
| dn, err = trix.FromTrix(data, password) | ||
| if err != nil { | ||
| // If FromTrix fails, try FromTar as a fallback for plain tarballs | ||
| if dn, err = datanode.FromTar(data); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block reads the entire archive into memory with os.ReadFile, which contradicts the goal of supporting large archives via streaming. The logic should be reworked to use os.Open and operate on the resulting io.ReaderAt file stream. Furthermore, there are compilation errors because functions like datanode.FromTar are called with []byte instead of an io.ReaderAt as required by the new API. The entire file processing logic needs to be refactored for streaming.
| dn := datanode.New() | ||
| dn.AddData("test.txt", []byte("hello")) | ||
| tarball, err := dn.ToTar() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
The test setup for creating a dummy archive is based on the old datanode API (New(), AddData(), ToTar()), which has been changed. datanode.New() now requires an io.ReaderAt, AddData() is a no-op, and ToTar() returns an error. This test will not compile. It needs to be updated to create a test tarball and then use the new streaming DataNode API.
| } | ||
| if mounted { | ||
| // As a fallback, try to unmount it manually. | ||
| server, _ := fuse.NewServer(nil, mountDir, nil) |
There was a problem hiding this comment.
fuse.NewServer returns a single value (*fuse.Server), but the code attempts to assign to two variables. This is a compilation error.
| server, _ := fuse.NewServer(nil, mountDir, nil) | |
| server := fuse.NewServer(nil, mountDir, nil) |
| Args: cobra.ExactArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| mountpoint := args[0] | ||
| server, _ := fuse.NewServer(nil, mountpoint, nil) |
There was a problem hiding this comment.
fuse.NewServer returns a single value (*fuse.Server), but the code attempts to assign to two variables. This is a compilation error.
| server, _ := fuse.NewServer(nil, mountpoint, nil) | |
| server := fuse.NewServer(nil, mountpoint, nil) |
| maxDepth = opts[0].MaxDepth | ||
| filter = opts[0].Filter | ||
| skipErrors = opts[0].SkipErrors | ||
| _ skipErrors = opts[0].SkipErrors |
There was a problem hiding this comment.
This line contains a syntax error due to the leading blank identifier _. To fix the compilation error, the _ should be removed.
| _ skipErrors = opts[0].SkipErrors | |
| skipErrors = opts[0].SkipErrors |
| dn := datanode.New() | ||
| dn.AddData("file1.txt", []byte("hello")) | ||
| dn.AddData("dir1/file2.txt", []byte("world")) |
There was a problem hiding this comment.
This test uses the old datanode API (New() with no arguments and AddData()), which will cause compilation errors with the new API. The test needs to be updated to create a DataNode from a test archive using the new streaming API.
| }() | ||
|
|
||
| // Give the FUSE server a moment to start up and stabilize | ||
| time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
Using time.Sleep to wait for the FUSE server to start can lead to flaky tests. A more robust approach would be to poll the mount point until it becomes active. The same issue exists on line 80 when waiting for the unmount.
This change introduces a new
borg mountcommand that allows users to mount an archive as a read-only FUSE filesystem. This is particularly useful for browsing large archives without extracting them. The implementation includes a streamingDataNodeto avoid loading large files into memory. However, the decryption logic for encrypted archives still needs to be refactored to support streaming, which is a current blocker.Fixes #46
PR created automatically by Jules for task 3959879505260208522 started by @Snider