Use time of create MPU on backend storage as time of MPU #8311

arielshaqed · 2024-10-27T15:58:09Z

This time is either reported by the block adapter during completion, or it can be read by stat'ting the underlying object after complete-MPU.

Fixes #8303.

github-actions · 2024-10-27T16:07:55Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-10-27T16:08:17Z

E2E Test Results - Quickstart

This time is either reported by the block adapter during creation, or it can be read by stat'ting the underlying object after complete-MPU. Use the latter method on S3.

Bump timeSlippage accordingly.

Azure and GCS do _not_ use the time of create-MPU. The important thing is that the two share the same time. Anything else confuses not only people but also programs. For instance DataBricks with presigned URLs can get confused by getting one time from lakeFS and another from underlying storage.

arielshaqed · 2024-10-29T08:35:05Z

Not a customer issue

This does not match any report by a customer of an inconsistency. It's just a relevant cleanup.

N-o-Z

Thanks!
Very elegant - some minor comments

N-o-Z · 2024-10-29T16:13:12Z

pkg/api/controller.go

@@ -384,6 +384,12 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht

 	writeTime := time.Now()
 	checksum := httputil.StripQuotesAndSpaces(mpuResp.ETag)
+	if mpuResp.MTime != nil {
+		// Anything else can be _really_ wrong when the storage layer assigns the time
+		// of MPU creation.  For instance, the S3 block adapter takes sure to return an


Suggested change

// of MPU creation. For instance, the S3 block adapter takes sure to return an

// of MPU creation. For instance, the S3 block adapter makes sure to return an

N-o-Z · 2024-10-29T16:14:01Z

pkg/api/controller.go

@@ -384,6 +384,12 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht

 	writeTime := time.Now()
 	checksum := httputil.StripQuotesAndSpaces(mpuResp.ETag)
+	if mpuResp.MTime != nil {


Can we put line 387 right above this line

N-o-Z · 2024-10-29T16:15:37Z

pkg/gateway/operations/putobject.go

@@ -309,7 +309,8 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	// write metadata
 	metadata := amzMetaAsMetadata(req)
 	contentType := req.Header.Get("Content-Type")
-	err = o.finishUpload(req, blob.Checksum, blob.PhysicalAddress, blob.Size, true, metadata, contentType)
+	// BUG(ariels): Read MTime from upload!


I think we still haven't decided if this should be fixed or not - anyway it's not related to the changes in this PR (let's track it in an issue instead?)

N-o-Z · 2024-10-29T16:17:06Z

pkg/block/s3/adapter.go

@@ -779,6 +779,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP
 	etag := strings.Trim(aws.ToString(resp.ETag), `"`)
 	return &block.CompleteMultiPartUploadResponse{
 		ETag:             etag,
+		MTime:            headResp.LastModified,


N-o-Z · 2024-10-29T16:21:35Z

esti/multipart_test.go

+
+	// On S3 the object should have Last-Modified time at around time of MPU creation.
+	// Ensure lakeFS fails the test if it fakes it by using the current time.
+	time.Sleep(2 * timeResolution)


Never too happy about hardcoded sleeps in tests but seems inevitable.
Perhaps scope it to blockstoreType == s3?

I completely understand. Don't like it but did it, thanks!

arielshaqed

Thanks!

Pulling once Esti passes again...

arielshaqed · 2024-10-30T08:26:59Z

esti/multipart_test.go

+
+	// On S3 the object should have Last-Modified time at around time of MPU creation.
+	// Ensure lakeFS fails the test if it fakes it by using the current time.
+	time.Sleep(2 * timeResolution)


I completely understand. Don't like it but did it, thanks!

arielshaqed · 2024-10-30T08:32:26Z

pkg/api/controller.go

@@ -384,6 +384,12 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht

 	writeTime := time.Now()
 	checksum := httputil.StripQuotesAndSpaces(mpuResp.ETag)
+	if mpuResp.MTime != nil {
+		// Anything else can be _really_ wrong when the storage layer assigns the time
+		// of MPU creation.  For instance, the S3 block adapter takes sure to return an


arielshaqed · 2024-10-30T08:32:35Z

pkg/block/s3/adapter.go

@@ -779,6 +779,7 @@ func (a *Adapter) CompleteMultiPartUpload(ctx context.Context, obj block.ObjectP
 	etag := strings.Trim(aws.ToString(resp.ETag), `"`)
 	return &block.CompleteMultiPartUploadResponse{
 		ETag:             etag,
+		MTime:            headResp.LastModified,


arielshaqed · 2024-10-30T08:33:45Z

pkg/gateway/operations/putobject.go

@@ -309,7 +309,8 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	// write metadata
 	metadata := amzMetaAsMetadata(req)
 	contentType := req.Header.Get("Content-Type")
-	err = o.finishUpload(req, blob.Checksum, blob.PhysicalAddress, blob.Size, true, metadata, contentType)
+	// BUG(ariels): Read MTime from upload!


arielshaqed added bug Something isn't working area/API Improvements or additions to the API area/gateway Changes to the gateway include-changelog PR description should be included in next release changelog labels Oct 27, 2024

arielshaqed force-pushed the bug/8303-use-adapter-time-for-mpu branch 3 times, most recently from 84c7bbd to 2ad899c Compare October 27, 2024 16:04

arielshaqed added 3 commits October 28, 2024 18:14

Use time of create MPU on backend storage as time of MPU

a394f87

This time is either reported by the block adapter during creation, or it can be read by stat'ting the underlying object after complete-MPU. Use the latter method on S3.

[golangci-lint] fix repeated assignment

762e418

[bug] S3 gateway protocol reports time with 1-second precision

fc7c766

Bump timeSlippage accordingly.

arielshaqed force-pushed the bug/8303-use-adapter-time-for-mpu branch 6 times, most recently from 7f17008 to bb70230 Compare October 29, 2024 08:08

arielshaqed force-pushed the bug/8303-use-adapter-time-for-mpu branch from bb70230 to a732383 Compare October 29, 2024 08:20

arielshaqed marked this pull request as ready for review October 29, 2024 08:34

arielshaqed requested a review from N-o-Z October 29, 2024 08:35

N-o-Z approved these changes Oct 29, 2024

View reviewed changes

arielshaqed commented Oct 30, 2024

View reviewed changes

arielshaqed enabled auto-merge (squash) October 30, 2024 08:35

[CR] Sleep only on S3 in multipart test, and fix some comments

5dd27ca

arielshaqed force-pushed the bug/8303-use-adapter-time-for-mpu branch from edf1449 to 5dd27ca Compare October 30, 2024 15:16

arielshaqed merged commit c2e44a0 into master Oct 30, 2024
38 checks passed

arielshaqed deleted the bug/8303-use-adapter-time-for-mpu branch October 30, 2024 15:31

arielshaqed mentioned this pull request Oct 31, 2024

Upload Object: Align lakeFS object mtime with block adapter mtime as much as possible #8328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use time of create MPU on backend storage as time of MPU #8311

Use time of create MPU on backend storage as time of MPU #8311

arielshaqed commented Oct 27, 2024 •

edited

Loading

github-actions bot commented Oct 27, 2024

github-actions bot commented Oct 27, 2024

arielshaqed commented Oct 29, 2024

N-o-Z left a comment

N-o-Z Oct 29, 2024

arielshaqed Oct 30, 2024

N-o-Z Oct 29, 2024

N-o-Z Oct 29, 2024

arielshaqed Oct 30, 2024

N-o-Z Oct 29, 2024

arielshaqed Oct 30, 2024

N-o-Z Oct 29, 2024

arielshaqed Oct 30, 2024

arielshaqed left a comment

arielshaqed Oct 30, 2024

arielshaqed Oct 30, 2024

arielshaqed Oct 30, 2024

arielshaqed Oct 30, 2024

	// of MPU creation. For instance, the S3 block adapter takes sure to return an
	// of MPU creation. For instance, the S3 block adapter makes sure to return an

Use time of create MPU on backend storage as time of MPU #8311

Use time of create MPU on backend storage as time of MPU #8311

Conversation

arielshaqed commented Oct 27, 2024 • edited Loading

github-actions bot commented Oct 27, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Oct 27, 2024

E2E Test Results - Quickstart

arielshaqed commented Oct 29, 2024

Not a customer issue

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed commented Oct 27, 2024 •

edited

Loading