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: support geesefs by allowing to skip utimes #14103

Closed
wants to merge 2 commits into from

Conversation

denysvitali
Copy link

This fixes #14005 by adding an environment variable that allows to skip the utimes call on filesystems that do not support it.

Thanks to this fix, it's possible to use an S3 storage in Immich via the geesefs FUSE filesystem.

This could indirectly fix #4445 too (or at least provide a workaround).

Copy link
Contributor

github-actions bot commented Nov 12, 2024

Label error. Requires exactly 1 of: changelog:.*. Found: documentation, 🗄️server

@github-actions github-actions bot added documentation Improvements or additions to documentation 🗄️server labels Nov 12, 2024
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 think I'd rather see utime errors be ignored

@zackpollard
Copy link
Contributor

Personally, and as was mentioned in the issue when it was opened, we don't want to support non-POSIX filesystems. This is a rabbit hole that we don't really want to get into, I can see it ending with us adding these kind of patches for a bunch of things filesystems should be providing.

@denysvitali
Copy link
Author

Personally, and as was mentioned in the issue when it was opened, we don't want to support non-POSIX filesystems. This is a rabbit hole that we don't really want to get into, I can see it ending with us adding these kind of patches for a bunch of things filesystems should be providing.

Supporting some FUSE filesystems (especially S3) might be a quick win. Personally I think this one line change won't hurt and might help more users in the same situation.

@zackpollard
Copy link
Contributor

Personally, and as was mentioned in the issue when it was opened, we don't want to support non-POSIX filesystems. This is a rabbit hole that we don't really want to get into, I can see it ending with us adding these kind of patches for a bunch of things filesystems should be providing.

Supporting some FUSE filesystems (especially S3) might be a quick win. Personally I think this one line change won't hurt and might help more users in the same situation.

We have plenty of people using S3 without needing to make this concession, I am not familiar with those setups specifically, however I imagine their mount tool just ignores utime updates instead of throwing errors.

@bo0tzz
Copy link
Member

bo0tzz commented Nov 12, 2024

Personally I think this one line change won't hurt

Even if it doesn't, it opens the door to more special cases to support misbehaving filesystems and that's not a road we want to go down.

@jrasm91 jrasm91 closed this Nov 12, 2024
@denysvitali
Copy link
Author

denysvitali commented Nov 12, 2024

FYI - I fixed this on my side by patching the fs library:

const fs = require('fs');
// Patch the async version
fs.utimes = function(path, atime, mtime, callback) {
    // Just call callback successfully without doing the operation
    if (callback) process.nextTick(callback);
    return Promise.resolve();
};

// Patch the sync version
fs.utimesSync = function(path, atime, mtime) {
    // Do nothing and return
    return;
};

// Patch the promises version if it exists
if (fs.promises) {
    fs.promises.utimes = fs.utimes;
}
console.log('Patched fs.utimes to be no-op');

Then NODE_OPTIONS can be set to --require /patch/fs-patch.js.

This way, we don't make modifications to immich's source code but still get geesefs support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation 🗄️server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] object storage Upload fails when utime is not available
4 participants