Skip to content
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

feat(server): support rclone as storage backend #2832

Merged
merged 1 commit into from
Jun 19, 2023
Merged

feat(server): support rclone as storage backend #2832

merged 1 commit into from
Jun 19, 2023

Conversation

cycneuramus
Copy link
Contributor

@cycneuramus cycneuramus commented Jun 17, 2023

Closes #2831, ref #1899, #1683.

I'm not entirely sure about the minimal approach taken here, but it does work: remote filesystems such as rclone FUSE mounts can now handle the file operation that was blocking support.

@vercel
Copy link

vercel bot commented Jun 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Jun 18, 2023 0:00am

@alextran1502
Copy link
Contributor

alextran1502 commented Jun 18, 2023

Can you please outline what tests have been done for these changes in terms of usage and verification of the move command? What is the behavior when there is a file that exists?

The current behavior is that if there is an existing file, check by file name. The current file will append +x to the file name and then move it there. Is this change still conform to that behavior?

@cycneuramus
Copy link
Contributor Author

Thanks for taking a look, and sorry about the barebones nature of this PR. I'll try for better clarity. But first I think I need a better understanding of the intended behavior here. I can't seem to find the part of the source code that appends +x to the file name before the move operation, either in the parts related to the moveFile function or in the mv library that it calls. Could you point me in the direction of the code that achieves the behavior you describe?

@alextran1502
Copy link
Contributor

You can find it in server/src/domain/storage-template/storage-template.core.ts under getTemplatePath method

@cycneuramus
Copy link
Contributor Author

Yes, the current behavior should remain unchanged.

Basically, the only effective change introduced by this PR is to call the mv library with clobber: true instead of false in the moveFile method.

When clobber is set to false, as is the current behavior, mv will attempt to move files by hard linking to avoid overwriting existing files. Hard links, however, are not supported on FUSE mounted file systems such as rclone remotes. This is why the storage migration job, for example, will fail on such remotes with the ENOSYS: function not implemented, link error.

Now, since the point of clobber: false is to avoid overwriting existing files (mv would normally throw an error in such cases) I added a manual check for existing files to the moveFile method in order to preserve the same behavior.

The relevant test cases to the storage and asset services are passing, and I have been testing my image library in the dev container—both with and without rclone mounted storage backends—where everything works as expected re. storage migrations and file handling.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested it, but reading through the related issues, the research about the mv library and the proposed solution I'm fine with this. It looks like it is a pretty minimal change on our side to support a wider range of underlying file systems.

LGTM

@alextran1502 alextran1502 merged commit 296c77a into immich-app:main Jun 19, 2023
18 checks passed
@cycneuramus cycneuramus deleted the rclone branch June 19, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Rclone not supported because of one move command
3 participants