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

Support for writing to Google Cloud Storage buckets #853

Open
thundergolfer opened this issue Apr 16, 2024 · 10 comments
Open

Support for writing to Google Cloud Storage buckets #853

thundergolfer opened this issue Apr 16, 2024 · 10 comments
Labels
CRT dependency requires CRT work enhancement New feature or request

Comments

@thundergolfer
Copy link

Tell us more about this new feature.

I've attempting to use mount-s3 to read and write a Google Cloud Storage bucket and I'm hitting this error in the application:

Traceback (most recent call last):
  File "/pkg/modal/_container_io_manager.py", line 459, in handle_input_exception
    yield
  File "/pkg/modal/_container_entrypoint.py", line 128, in run_input
    res = imp_fun.fun(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/gcp_gcs.py", line 27, in f
    pathlib.Path("/my-mount/demo.txt").write_text("Hello world!")
  File "/usr/local/lib/python3.11/pathlib.py", line 1078, in write_text
    with self.open(mode='w', encoding=encoding, errors=errors, newline=newline) as f:
OSError: [Errno 5] Input/output error

The application is attempting to do this:

pathlib.Path("/my-mount/demo.txt").write_text("Hello world!")

The logs from mount-s3 quite clearly show the problem:

2024-04-16T01:00:04.232452Z  INFO setattr{req=60 ino=6}: mountpoint_s3::fs: fs:setattr with ino 6 flags None atime None mtime None
2024-04-16T01:00:04.232647Z DEBUG fuser::request: FUSE( 62) ino 0x0000000000000006 WRITE fh FileHandle(7), offset 0, size 12, write flags 0x0
2024-04-16T01:00:04.232662Z DEBUG write{req=62 ino=6 fh=7 offset=0 length=12 pid=994828 name="demo.txt"}:put_object{id=19 bucket="modal-scratch" key="demo.txt"}: mountpoint_s3_client::s3_crt_client::put_object: new request
2024-04-16T01:00:04.354448Z DEBUG write{req=62 ino=6 fh=7 offset=0 length=12 pid=994828 name="demo.txt"}:put_object{id=19 bucket="modal-scratch" key="demo.txt"}: mountpoint_s3_client::s3_crt_client: CRT request failed request_type=CreateMultipartUpload crt_error=Some(Error(14343, "aws-c-s3: AWS_ERROR_S3_INVALID_RESPONSE_STATUS, Invalid response status from request")) http_status=411 range=None duration=120.297306ms ttfb=Some(108.322147ms) request_id=<unknown>
2024-04-16T01:00:04.354565Z  WARN write{req=62 ino=6 fh=7 offset=0 length=12 pid=994828 name="demo.txt"}:put_object{id=19 bucket="modal-scratch" key="demo.txt"}: mountpoint_s3_client::s3_crt_client: meta request failed duration=120.518195ms request_result=MetaRequestResult { response_status: 411, crt_error: Error(14343, "aws-c-s3: AWS_ERROR_S3_INVALID_RESPONSE_STATUS, Invalid response status from request"), error_response_headers: Some(Headers { inner: 0x7f6170013c80 }), error_response_body: Some("<!DOCTYPE html>\n<html lang=en>\n  <meta charset=utf-8>\n  <meta name=viewport content=\"initial-scale=1, minimum-scale=1, width=device-width\">\n  <title>Error 411 (Length Required)!!1</title>\n  <style>\n    *{margin:0;padding:0}html,code{font:15px/22px arial,sans-serif}html{background:#fff;color:#222;padding:15px}body{margin:7% auto 0;max-width:390px;min-height:180px;padding:30px 0 15px}* > body{background:url(//www.google.com/images/errors/robot.png) 100% 5px no-repeat;padding-right:205px}p{margin:11px 0 22px;overflow:hidden}ins{color:#777;text-decoration:none}a img{border:0}@media screen and (max-width:772px){body{background:none;margin-top:0;max-width:none;padding-right:0}}#logo{background:url(//www.google.com/images/branding/googlelogo/1x/googlelogo_color_150x54dp.png) no-repeat;margin-left:-5px}@media only screen and (min-resolution:192dpi){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat 0% 0%/100% 100%;-moz-border-image:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) 0}}@media only screen and (-webkit-min-device-pixel-ratio:2){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat;-webkit-background-size:100% 100%}}#logo{display:inline-block;height:54px;width:150px}\n  </style>\n  <a href=//www.google.com/><span id=logo aria-label=Google></span></a>\n  <p><b>411.</b> <ins>That’s an error.</ins>\n  <p>POST requests require a <code>Content-length</code> header.  <ins>That’s all we know.</ins>\n") }
2024-04-16T01:00:04.354708Z  WARN write{req=62 ino=6 fh=7 offset=0 length=12 pid=994828 name="demo.txt"}: mountpoint_s3::fuse: write failed: upload error: put request failed: Client error: Internal S3 client error: input stream was dropped: receiving from an empty and closed channel

According to https://cloud.google.com/storage/docs/xml-api/reference-headers Google's XML API for cloud storage requires the Content-Length header that is apparently not being passed.

From a quick scan through the code I don't see anywhere that this header is set.
@thundergolfer thundergolfer added the enhancement New feature or request label Apr 16, 2024
@thundergolfer thundergolfer mentioned this issue Apr 16, 2024
Merged
2 tasks
@arsh
Copy link
Contributor

arsh commented Apr 16, 2024

Hi, thanks for the feature request. We do not support third-party services. However, we are happy to make changes and/or welcome contributions, provided that they are not specific to another service and, most importantly, do not require separate testing.

@arsh arsh added the wontfix This will not be worked on label Apr 16, 2024
@arsh arsh closed this as not planned Won't fix, can't repro, duplicate, stale Apr 16, 2024
@thundergolfer
Copy link
Author

Hey @arsh thanks for your reply.

It sounds like the required change (1) would require changes specific to another service, GCS, and (2) would require separate testing. The wontfix label accords with this.

Makes sense to me. If we really want this functionality it sounds like we should fork 👍

@arsh arsh reopened this Apr 17, 2024
@arsh
Copy link
Contributor

arsh commented Apr 17, 2024

One thing that @jamesbornholt pointed out is that this might be a small change in the underlying S3 client Mountpoint uses[1]. Specifically, to put the object in S3, we're creating a multi-part upload and that involves several HTTP requests to create it, upload part, and complete it. It appears that the create multi-part upload is not setting the Content-Length header [2] while the others are.

[1] https://github.com/awslabs/aws-c-s3
[2] https://github.com/awslabs/aws-c-s3/blob/main/source/s3_request_messages.c#L249

@thundergolfer
Copy link
Author

Yes, sorry, that is possibly the only problem. I should've called this out more obviously in the issue, but it's in the error message:

<p>POST requests require a <code>Content-length</code> header. <ins>That’s all we know.</ins>\n

@thundergolfer
Copy link
Author

I'll submit a PR to there then, thanks!

@jamesbornholt jamesbornholt removed the wontfix This will not be worked on label Apr 17, 2024
@jamesbornholt
Copy link
Member

Just to add that several of our other SDKs already send Content-Length: 0 for CreateMultipartUpload (I tested boto3), so I'd expect adding this to work fine in terms of S3 compatibility. We'd be happy to merge the fix once it's in the CRT library that @arsh pointed to.

@arsh arsh added the CRT dependency requires CRT work label Apr 17, 2024
@thundergolfer thundergolfer mentioned this issue Apr 29, 2024
Closed
@thundergolfer
Copy link
Author

thundergolfer commented May 7, 2024 •
edited

@jamesbornholt https://github.com/awslabs/aws-c-s3/releases/tag/v0.5.8 is now released with the Content-Type change.

I had a look at doing the submodule bump myself, but it seems like something best for an experienced contributor 🙂.

@jamesbornholt
Copy link
Member

Yep, I think we're just waiting for awslabs/aws-c-auth#237 and then we'll pull in all the CRT updates together so we can do just one new Mountpoint release.

@monthonk monthonk mentioned this issue May 10, 2024
Merged
@jamesbornholt
Copy link
Member

Quick update on this one: we were planning to release this as part of 1.7.0 on Friday (#876) but we found an issue with a different change while testing the release. The team's going to pick that up again this coming week. Sorry for the delay!

@thundergolfer
Copy link
Author

No worries, thanks for the update :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CRT dependency requires CRT work enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@arsh @jamesbornholt @thundergolfer