-
Notifications
You must be signed in to change notification settings - Fork 170
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
Regression between 1.4.0 and 1.6.0 when overwriting files with --allow-overwrite enabled #862
Comments
Hey @thundergolfer, thank you for opening the bug report. The first thing I'd note is that v1.4.0 is recalled, and we would not recommend using it. Regarding the error message you received: v1.4.0 introduced file overwrites. One downside with the recall is that the behavior you were able to achieve - opening a file for both reading and writing simultaneously - is reverted with v1.4.1+ and is not permitted. We don't plan to allow that behavior to occur, although if that's important for you then I'd recommend opening a feature request for it so we can learn about your use case. That being said, I was not able to reproduce the issue you are experiencing with v1.6.0. I was able to run this code fine: # using Python 3.8.19
from pathlib import Path
TEST_FILE_PATH: Path = Path("/mnt/s3/test.txt")
for i in range(50):
TEST_FILE_PATH.write_text(f"Writing text for iteration {i}") I note in your example that the simple reproduction only appears to write to the file. Is that script definitely able to reproduce the issue for you? Could there be another application that has this file open for reading? |
If this is an issue, please do open a bug report for it with some reproduction steps. |
Hi @dannycjones, adding context regarding
We filed an issue here and solved here. 1.6.0 includes a fix for that issue that's why we upgraded. |
Thanks for the response @dannycjones. Interesting that you can't repro. Prompted me to check if it was due to container runtime and it is! If we run the program with Appendix This is the entirety of the Modal program for posterity. from pathlib import Path
from uuid import uuid4
import modal
MOUNT_PATH: Path = Path("/mnt/s3_bucket")
S3_BUCKET_TEST_PATH: Path = MOUNT_PATH / "synmon/test_file.txt"
app = modal.App(
volumes={
MOUNT_PATH: modal.CloudBucketMount(
"modal-s3mount-test-bucket",
secret=modal.Secret.from_name("cloud-bucket-mount-secret", environment_name="main"),
)
},
image=modal.Image.debian_slim(python_version="3.11"),
)
@app.function()
def write() -> str:
data = uuid4().hex
S3_BUCKET_TEST_PATH.write_text(data)
return data
if __name__ == "__main__":
with app.run():
write_data = write.remote() |
@dannycjones I've done some more investigation into the behavior difference between gVisor and runc. A gVisor trace of the reproduction program shows that it's only make 1
From mount-s3's debug logs though, I can see:
Full logs snippet (gvisor)
With
|
The issue is that gVisor's gofer client (pkg/fsimpl/gofer/) opens the file I think we should change that logic to open regular files with O_PATH only. Directories and FIFOs can continue to be opened with O_RDONLY. |
@dannycjones Interested to know what the consequences of violating the concurrent read and write restriction would be. Is it just an inconsistent (empty) read? I've looked through these PRs because didn't gather the consequence:
|
Yeah, the reason we forbid this is that S3 doesn't allow reading from an inflight multi-part upload. So if you try reading concurrently, you'll observe the results of the new writes as long as they're still in the page cache, but then start seeing the old object again if they ever get evicted or dropped from cache. |
Ok opened google/gvisor#10389 in gvisor repo @ayushr2. @jamesbornholt happy for this to be closed on your end 🙂 . |
Glad that you have a path forward. I'm closing this issue as it's expected behavior in mountpoint. |
cc @luiscape
Mountpoint for Amazon S3 version
mount-s3 1.6.0
AWS Region
No response
Describe the running environment
Linux ip-10-1-1-198 5.15.0-1058-aws #64~20.04.1-Ubuntu SMP Tue Apr 9 11:12:27 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Mountpoint options
What happened?
This trivial program produces the error:
If
S3_BUCKET_TEST_PATH
already exists we get this error:The key error log from mount-s3 is this:
If I downgrade to 1.4.0 this problem goes away. Version 1.5.0 also seems to have this bug.
We upgraded to 1.6.0 because 1.4.0 had a bug where listing the root of the mount twice failed.
Relevant log output
The text was updated successfully, but these errors were encountered: