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.

Sign up for GitHub

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

Jump to bottom

Provide ErrorMetadata to crate::fuse layer with errors from the client #882

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Provide ErrorMetadata to crate::fuse layer with errors from the client #882

wants to merge 14 commits into from

Conversation

vladem
Copy link
Contributor

@vladem vladem commented May 17, 2024 •
edited

Note

The majority of the changes (646 lines) are for the updated Cargo.lock caused by the addition of httpmock dev dependency

Description of change

  1. ProvideErrorMetadata trait aims to provide additional information with errors. After this change, code in fuse.rs can access error_code, http_code, s3_error_code and some other fields for errors occurring on S3FuseFilesystem::lookup() (for other fuse operations we currently return no metadata). Currently, we don't use this information in any way apart from the tests.
  2. There are tests added for common lookup errors: forbidden, throttled, not found, unhandled error from s3;

Relevant issues: None

What is not in this change

  1. Emission of serialised errors in a separate file (for reference see the expected approach here: Logging errors in a structured format #894);
  2. Additional data with errors on fuse operations other then lookup() (e.g. throttling on read() will have no error_code or http_code);
  3. S3-response data for HeadObjectError::NotFound and ListObjectsError::NoSuchBucket;
  4. Returning mount errors in a way suitable for logging (e.g. forbidden on the initial ListObjectsV2), which will likely require replacing anyhow::Result with a type implementing ProvideErrorMetadata trait.

Does this change impact existing behavior?

This does not change any behaviour visible to customer.

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).

@vladem vladem temporarily deployed to PR integration tests May 17, 2024 16:52 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 17, 2024 16:52 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 17, 2024 16:52 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 17, 2024 16:52 — with GitHub Actions Inactive
@vladem vladem had a problem deploying to PR integration tests May 17, 2024 16:52 — with GitHub Actions Failure
@vladem vladem temporarily deployed to PR integration tests May 17, 2024 16:52 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 17, 2024 16:52 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:02 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:02 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:02 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:03 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:03 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:03 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 18, 2024 10:03 — with GitHub Actions Inactive
passaro
passaro reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@passaro passaro left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few suggestions.

I was unsure about returning the metadata by reference, by I see this the approach in the Rust SDK, so it may be worth to adopt as well.

Before finalizing, can you try and implement the propagation for another type of error, so we can get a better picture?

mountpoint-s3-client/src/error_metadata.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/fs/error.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/fs/error.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/inode.rs Outdated Show resolved Hide resolved
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests May 23, 2024 12:27 — with GitHub Actions Waiting
@vladem vladem temporarily deployed to PR integration tests May 23, 2024 20:22 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 23, 2024 20:22 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 23, 2024 20:22 — with GitHub Actions Inactive
@vladem vladem had a problem deploying to PR integration tests May 23, 2024 20:22 — with GitHub Actions Failure
@vladem vladem temporarily deployed to PR integration tests May 23, 2024 20:22 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 23, 2024 20:22 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 23, 2024 20:22 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests May 24, 2024 09:19 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 6, 2024 17:51 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 6, 2024 17:51 — with GitHub Actions Inactive
@vladem vladem requested a review from passaro June 6, 2024 18:21
passaro
passaro reviewed Jun 10, 2024
View reviewed changes
mountpoint-s3-client/src/error_metadata.rs Outdated Show resolved Hide resolved
mountpoint-s3-client/src/s3_crt_client.rs Outdated Show resolved Hide resolved
mountpoint-s3-client/src/s3_crt_client.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/inode.rs Show resolved Hide resolved
mountpoint-s3-client/src/s3_crt_client.rs Show resolved Hide resolved
simplify client_error constructor, rename metadata fields, flatten er…
20c2716 
…ror parser calls

Signed-off-by: Vladislav Volodkin <vlaad@amazon.co.uk>
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 13:11 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
@vladem vladem requested a deployment to PR integration tests June 11, 2024 14:25 — with GitHub Actions Waiting
allow constructing metadata on the flight in ProvideErrorMetadata
2ec7627 
Signed-off-by: Vladislav Volodkin <vlaad@amazon.co.uk>
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem temporarily deployed to PR integration tests June 11, 2024 14:37 — with GitHub Actions Inactive
@vladem vladem requested a review from passaro June 11, 2024 14:55
passaro
passaro reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@passaro passaro left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few minor suggestions.

mountpoint-s3-client/src/s3_crt_client.rs
try_parse_canceled_request(request_result).or_else(|| try_parse_no_credentials(request_result))
}),
0 => {
if let Some(err) = try_parse_throttled(request_result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is fine, but inconsistent with the 400 case. I'd leave it as a chain of .or_else.

mountpoint-s3-client/src/s3_crt_client.rs
#[error("Forbidden: {0}")]
Forbidden(String),
Forbidden(String, Box<ClientErrorMetadata>),
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I'd just simplify this case as well and add optional code and status here

mountpoint-s3-client/src/s3_crt_client.rs
}
Self::Forbidden(_, metadata) => (**metadata).clone(),
Self::Throttled => {
// CRT does not set S3 response data for error codes other then AWS_ERROR_S3_INVALID_RESPONSE_STATUS
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not sure this comment is relevant anymore. Maybe some of it (the first two lines?) could make more sense in try_parse_throttled.

mountpoint-s3/src/inode.rs
E: ProvideErrorMetadata + std::error::Error + Send + Sync + 'static,
{
let metadata = ErrorMetadata {
client_error_meta: err.meta().clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to clone, right?

mountpoint-s3/src/inode/readdir.rs
@@ -353,7 +353,7 @@ impl RemoteIter {
self.full_path.as_str(),
)
.await
.map_err(|e| InodeError::ClientError(anyhow::Error::new(e)))?;
.map_err(|e| InodeError::client_error(e, None, &self.bucket, &self.full_path))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: should we just add a context string here as well? And simply make it non-optional?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@passaro passaro passaro left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vladem @passaro