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

Improve error logs for unsupported operations: File Overwrite, Random Write, Directory Shadowing, Unlink (without mount option) #699

Merged
merged 5 commits into from
Jan 17, 2024

Conversation

sauraank
Copy link
Contributor

@sauraank sauraank commented Jan 12, 2024

Description of change

Added clear message for the operations that are unsupported but did not had easy to understand logs. Added links to semantics for deletion and allowed filenames to be visible using Mountpoint. But, with links there is a risk that they can get broken once the content is moved somewhere else.
I will add another PR for troubleshooting page and down level the error noise to debug! in other Pull Requests.

With this new change, following are error logs that we receive:
For directory shadowing of file with same name, ls give following output -

WARN readdir{req=5 ino=1 fh=2 offset=17}: mountpoint_s3::inode::readdir::ordered: file 'out' (full key "out") is omitted because another directory 'out' exist with the same name.

For file deletion without using --allow-delete flag, give following in logs -

WARN unlink{req=8 parent=1 name="ini.txt"}: mountpoint_s3::fuse: unlink failed: Deletes are disabled. Use '--allow-delete' mount option to enable it.

Out of Write gives -

WARN write{req=52 ino=49 fh=3 offset=512 length=512 name="new.txt"}: mountpoint_s3::fuse: write failed: upload error: out of order write NOT supported by Mountpoint, aborting the upload; expected offset 0 but got 512

UPDATE: Overwriting a file is now supported, so the error message is the one set in PR #487 -

WARN file overwrite is disabled by default, you need to remount with --allow-overwrite flag and open the file in truncate mode (O_TRUNC) to overwrite it

Also changed the logging in case of finish writing to update status of Inode, but the Inode WriteStatus is in Invalid state. Earlier it was InodeNotWritable which is same error for File Overwrite when its flag is not enabled. Also, it did not completely explained that WriteStatus of the Inode is not valid for writing.

Relevant issues:
#533

Does this change impact existing behavior?

No. It only improves the error logging as mentioned above.

No, there is no breaking change.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:05 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 12, 2024 02:06 — with GitHub Actions Inactive
@sauraank sauraank marked this pull request as ready for review January 12, 2024 02:08
@sauraank sauraank changed the title Improve error logs for unsupported operations: File Overwrite, Random Write, Directory Shadowing, Unlink Improve error logs for unsupported operations: File Overwrite, Random Write, Directory Shadowing, Unlink( without mount option) Jan 12, 2024
@sauraank sauraank changed the title Improve error logs for unsupported operations: File Overwrite, Random Write, Directory Shadowing, Unlink( without mount option) Improve error logs for unsupported operations: File Overwrite, Random Write, Directory Shadowing, Unlink (without mount option) Jan 12, 2024
Copy link
Contributor

@monthonk monthonk left a comment

Choose a reason for hiding this comment

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

@sauraank thanks for improving the logs! I have put a few comments.

mountpoint-s3/src/inode.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/inode.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/upload.rs Outdated Show resolved Hide resolved
WriteStatus::Remote => Err(InodeError::InodeNotWritable(inode.err())),
WriteStatus::Remote => Err(InodeError::InodeNotWritable(
inode.err(),
"Writing to existing file is NOT supported by Mountpoint.".to_owned(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than adding a message here, I'd prefer having a specific InodeError case for each situation.

mountpoint-s3/src/fs.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 12:54 — with GitHub Actions Inactive
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Copy link
Contributor

@monthonk monthonk left a comment

Choose a reason for hiding this comment

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

LGTM

@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests January 17, 2024 14:41 — with GitHub Actions Inactive
@sauraank sauraank added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
@sauraank sauraank added this pull request to the merge queue Jan 17, 2024
Merged via the queue into awslabs:main with commit 924b86c Jan 17, 2024
24 checks passed
@sauraank sauraank deleted the error_improvement branch January 17, 2024 17:34
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.

3 participants