-
Notifications
You must be signed in to change notification settings - Fork 362
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
Fix: Get mtime from storage server #8329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This obviously advances us on a user issue, so approving. However see comments on s3.Adapter.Put for why I do not consider this to be a solution yet.
We will also need a separate solution for LinkPhysicalAddress, as generated by lakeFSFS. Opening an issue to do that as well.
pkg/block/adapter.go
Outdated
@@ -150,11 +150,22 @@ type BlockstoreMetadata struct { | |||
Region *string | |||
} | |||
|
|||
type PutResponse struct { | |||
Mtime *time.Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we keep changing this, and capitalization is tricky. Let's open an issue to change it (where we can, e.g. not in OpenAPI) to a better name? io/fs
uses ModTime, maybe we should too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand your comment - modify struct parameter to ModTime
? 👍
if err != nil { | ||
return nil, err | ||
} | ||
return &block.PutResponse{}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it would make sense to take the mtime from the filesystem. Unfortunately lakeFS on "local" NFS is a thing, and this code will run into time sync issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to deal with the more urgent issue first and have fallbacks.
I'll open a separate issue for that
} | ||
return nil | ||
mtime := getServerTimeFromResponseMetadata(resp.ResultMetadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT this is still incorrect 😭 . This code ends up taking the time in the server Date
header. But I could not find anywhere where AWS say that this is the actual "Last-Modified" time of the object on S3. Indeed, it might not be.
- The claim that "but it will be later" does not settle the matter. Right now a user has issues with If-Unmodified-Since, where obviously reporting a later time is better. But S3 get-object also supports If-Modified-Since, which swings in exactly the opposite direction.
- The claim that "time on the AWS gateway will be close to time on the S3 backend" is much stronger. But still obviously not enough - AWS do not consider the 2 to be equivalent, see e.g. copy-object which returns a LastModified XML element in addition to the standard Date header.
I would much prefer to have an extra head-object call here when needed; obviously whether to do that would have to depend on some PutOpts so we don't do it and drop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a compromise we took under consideration. By definition lakeFS mtime will always be explicitly >= storage mtime. A good example for that is modifying metadata in lakeFS where we modify the mtime in lakeFS but storage mtime remains unmodified. We do not expect equality.
Closes #8328
Change Description
Background
Put Object: Try to use storage mtime or server time as best effort and provide it to lakeFS as creation date
Bug Fix
A better fix for lakeFS mtime < storage mtime
Testing Details
Tested manually
Breaking Change?
No