API presign multipart upload experimenal support for S3 #7246

nopcoder · 2024-01-04T15:43:20Z

OpenAPI s3 presign multipart upload experimental for s3 base on the following proposal:

https://github.com/treeverse/lakeFS/blob/master/design/accepted/openapi-multipart-upload.md

Close #7253

github-actions · 2024-01-04T15:44:32Z

♻️ PR Preview 8b45119 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2024-01-04T15:53:07Z

E2E Test Results - DynamoDB Local - Local Block Adapter

itaiad200

Thank you for adding this so quickly! Please add tests, other than that mostly minor comments

itaiad200 · 2024-01-04T16:42:17Z

api/swagger.yml

+        Initiates a multipart upload and returns an upload ID with presigned URLs for each part (optional).
+        Part numbers starts with 1. Each part except the last one has minimum size depends on the underlying blockstore implementation.
+        For example working with S3 blockstore, minimum size is 5MB (excluding the last part).
+      responses:


Need 400 too. For example, 'parts' is 0

Added the missing 400 status code for all routes.
The SDK will fail the request in case of bad request.
The number of parts for create is optional, in order to support additional API to provide presigned parts URL request for streaming.

itaiad200 · 2024-01-04T16:43:59Z

api/swagger.yml

+        required: true
+        schema:
+          type: string
+      - in: query


Why is the path needed when uploadID is given?
Can 2 mpus be available for the same part?
isn't uploadID unique between different paths?

We require path like S3 protocol requires key. Same for upload ID, even though it seems unique, s3 protocol requires bucket and key on each api call.

When we actually work with physical address (which we also require) we use the path for permission check on create and complete.
For each part the request is signed with all the information which include the upload id and the path.

There can be two mpu to the same logical address which means that there can be two parts upload in parallel. In our case because we provide different physical addresses these parts will have different addresses.
If you will try to provide such path to different upload request the complete will not find this part. Hope I understood the concern.

itaiad200 · 2024-01-04T16:44:56Z

api/swagger.yml

+          application/json:
+            schema:
+              $ref: "#/components/schemas/CompletePresignMultipartUpload"
+      responses:


400 is needed. for example. wrong parts order

itaiad200 · 2024-01-04T16:45:29Z

api/swagger.yml

+          application/json:
+            schema:
+              $ref: "#/components/schemas/AbortPresignMultipartUpload"
+      responses:


400 is needed. For example, uploadID mismatch with path

itaiad200 · 2024-01-04T16:51:56Z

pkg/graveler/ref/manager.go

@@ -609,6 +609,9 @@ func newCache(cfg CacheConfig) cache.Cache {
 }

 func (m *Manager) SetLinkAddress(ctx context.Context, repository *graveler.RepositoryRecord, physicalAddress string) error {
+	if physicalAddress == "" {


Was this a bug? If so, can you add tests for it?

just input validation to make sure we will not store value for empty path.

itaiad200 · 2024-01-04T17:42:42Z

pkg/api/helpers/upload.go

+const DefaultUploadPartSize = MinUploadPartSize
+
+// DefaultUploadConcurrency is the default number of goroutines to spin up when uploading a multipart upload
+const DefaultUploadConcurrency = 5


Is it enough? Seems too low for me

The is the default aws sdk uses - I'll make it configurable for lakectl fs upload

If it's AWS defaults then it's good enough

pkg/api/helpers/upload.go

itaiad200 · 2024-01-04T17:49:28Z

pkg/api/helpers/upload.go

+		if uploader != nil {
+			stats, err = uploader.Upload(ctx)
+		} else {
+			stats, err = clientUploadPreSignHelper(ctx, client, repoID, branchID, objPath, metadata, contentType, contents, contentLength)
+		}


This is confusing. uploader is PreSignUploader. If it's nil, use clientUploadPreSignHelper instead. Can you rename vars or use comment to help clarify?

Sure, I'll update the name or integrate them together.

Integrated presign upload of single object into uploader functionality.

itaiad200 · 2024-01-04T17:50:02Z

pkg/api/helpers/upload.go

+const MinUploadPartSize int64 = 1024 * 1024 * 5
+
+// DefaultUploadPartSize is the default part size to buffer chunks of a payload into
+const DefaultUploadPartSize = MinUploadPartSize


Is this optimal?

These are AWS SDK defaults

Co-authored-by: itaiad200 <itaiad200@gmail.com>

itaiad200

LGTM
Only concern is with the upload file different flows test coverage. Given the urgency, it could be handled in a separate PR.

N-o-Z

Looks awesome!

Some comments and also missing crucial tests for the uploader

N-o-Z · 2024-01-07T09:00:18Z

pkg/config/config.go

@@ -253,6 +253,7 @@ type Config struct {
 			PreSignedExpiry               time.Duration `mapstructure:"pre_signed_expiry"`
 			DisablePreSigned              bool          `mapstructure:"disable_pre_signed"`
 			DisablePreSignedUI            bool          `mapstructure:"disable_pre_signed_ui"`
+			DisablePreSignedMultipart     bool          `mapstructure:"disable_pre_signed_multipart"`


Can we put this under "experimental" or "beta" please?

Added configuration reference doc line with experimental - didn't want to configure it outside the block adapter.
Let me know if you think we have better place.

N-o-Z · 2024-01-07T09:05:15Z

pkg/api/controller.go

+		return
+	}
+
+	repo, err := c.Catalog.GetRepository(ctx, repository)


You can skip this - BranchExists will already do that for you

Glad it is cached, we need it for BlockAdapter.ResolveNamespace

pkg/api/controller.go

cmd/lakectl/cmd/fs_upload.go

N-o-Z · 2024-01-07T09:37:56Z

pkg/api/helpers/upload.go

+func (u *presignUpload) initMultipart(ctx context.Context) (*apigen.PresignMultipartUpload, error) {
+	// adjust part size
+	u.partSize = DefaultUploadPartSize
+	if u.size/u.partSize >= int64(MaxUploadParts) {


This is kind of awkward. I think we should enforce that at least (MaxUploadParts + x) * u.partSize > u.size, where x is a constant size.
Otherwise, this can lead to bad performance

I've used the same logic the s3 sdk uses to calculate the part size the client will use. It uses default part size and try to use it until the max number of parts allowed.
In the case the size can't fit the parts it will adjust the size.
The cli usually gives a way to configure the part size, but for now I kept it as the default which is the min size.

N-o-Z · 2024-01-07T09:50:31Z

pkg/block/s3/adapter.go

@@ -422,6 +431,42 @@ func (a *Adapter) GetPreSignedURL(ctx context.Context, obj block.ObjectPointer,
 	return req.URL, expiry, nil
 }

+func (a *Adapter) GetPresignUploadPartURL(ctx context.Context, obj block.ObjectPointer, uploadID string, partNumber int) (string, error) {
+	if a.disablePreSigned {


Suggested change

if a.disablePreSigned {

if a.disablePreSigned || a.disablePreSignedMultipart {

was thinking about this one - if I like to block the presign because of the multipart capability or not. but I kept it bound to the feature it is based on. so if we support presign you can have part presign, in case you need it for some other feature.

nopcoder · 2024-01-07T20:53:25Z

LGTM Only concern is with the upload file different flows test coverage. Given the urgency, it could be handled in a separate PR.

added esti tests for presin multipart upload.
will add lakectl tests in separate pr.

N-o-Z

Some questions / requests regarding tests

N-o-Z · 2024-01-08T08:49:08Z

esti/presign_multipart_test.go

+			require.NotNil(t, resp.JSON201)
+			require.NotEmpty(t, resp.JSON201.UploadId)
+			require.NotEmpty(t, resp.JSON201.PhysicalAddress)
+			if tt.parts != nil {


Correct me if I'm wrong but this code is never reached

Relevant for no_parts case

N-o-Z · 2024-01-08T08:53:33Z

esti/presign_multipart_test.go

+		})
+		require.NoError(t, err, "CompletePresignMultipartUpload should succeed")
+		require.Equalf(t, http.StatusOK, resp.StatusCode(), "CompletePresignMultipartUpload status code mismatch: %s - %s", resp.Status(), resp.Body)
+	})


Where do we validate that the object actually exists?

N-o-Z · 2024-01-08T09:04:49Z

esti/presign_multipart_test.go

@@ -0,0 +1,246 @@
+package esti


Can we add a test for u.size / u.partSize >= maxParts?

:D sorry - I can manually test this one but it will be a big file for a test.

N-o-Z

LGTM!

API multipart upload experimenal support for s3

d9ef33f

nopcoder self-assigned this Jan 4, 2024

nopcoder requested a review from a team January 4, 2024 15:44

nopcoder changed the title ~~API multipart upload experimenal support for s3~~ API presign multipart upload experimenal support for s3 Jan 4, 2024

Add missing bad request 400

62612d7

nopcoder added area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Jan 4, 2024

itaiad200 requested changes Jan 4, 2024

View reviewed changes

nopcoder and others added 5 commits January 5, 2024 11:59

Update pkg/block/s3/adapter.go

c0a3294

Co-authored-by: itaiad200 <itaiad200@gmail.com>

Update pkg/api/controller.go

fafc506

Co-authored-by: itaiad200 <itaiad200@gmail.com>

Update pkg/api/helpers/upload.go

cd54fe9

Co-authored-by: itaiad200 <itaiad200@gmail.com>

Update pkg/api/helpers/upload.go

bc02ce7

Co-authored-by: itaiad200 <itaiad200@gmail.com>

apply code review changes

2bdc6fd

nopcoder changed the title ~~API presign multipart upload experimenal support for s3~~ API presign multipart upload experimenal support for S3 Jan 6, 2024

nopcoder requested a review from itaiad200 January 6, 2024 10:47

itaiad200 approved these changes Jan 6, 2024

View reviewed changes

fix presigned multipart support

3a7ab8e

N-o-Z requested changes Jan 7, 2024

View reviewed changes

nopcoder added 2 commits January 7, 2024 22:49

validation and system test code

614c899

docs: update config reference

a6236a5

nopcoder requested a review from N-o-Z January 7, 2024 20:56

N-o-Z requested changes Jan 8, 2024

View reviewed changes

check entry was created

8b45119

nopcoder requested a review from N-o-Z January 8, 2024 10:15

N-o-Z approved these changes Jan 8, 2024

View reviewed changes

nopcoder merged commit 11c07ff into master Jan 8, 2024
37 checks passed

nopcoder deleted the feature/api-presign-multipart-upload branch January 8, 2024 10:26

	if a.disablePreSigned {
	if a.disablePreSigned \|\| a.disablePreSignedMultipart {

API presign multipart upload experimenal support for S3 #7246

API presign multipart upload experimenal support for S3 #7246

Conversation

nopcoder commented Jan 4, 2024 • edited Loading

github-actions bot commented Jan 4, 2024 • edited Loading

github-actions bot commented Jan 4, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

itaiad200 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

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

itaiad200 left a comment

Choose a reason for hiding this comment

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

nopcoder commented Jan 7, 2024

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

nopcoder Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

nopcoder commented Jan 4, 2024 •

edited

Loading

github-actions bot commented Jan 4, 2024 •

edited

Loading

nopcoder Jan 8, 2024 •

edited

Loading