(PPS-411): Add bucket selection support for multi part upload #123

BinamB · 2023-08-15T15:04:40Z

Jira Ticket: PXP-xxxx

Remove this line if you've changed the title to (PXP-xxxx): <title>

Fence PR (uc-cdis/fence#1112)

New Features

Add bucket selection support for multi part upload
Add bucket selection support for single part upload
Add bucket selection support for upload-multiple
Add multi-part support for upload-multiple

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

coveralls · 2023-08-15T15:16:50Z

Pull Request Test Coverage Report for Build 7589691499

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 17.83%

Totals
Change from base Build 5868373361:	-0.1%
Covered Lines:	498
Relevant Lines:	2793

💛 - Coveralls

Avantol13 · 2023-08-23T16:04:22Z

gen3-client/commonUtils/commonUtils.go

@@ -105,6 +105,7 @@ type RetryObject struct {
 	GUID         string
 	RetryCount   int
 	Multipart    bool
+	Bucket *string


Go conventions prefer the type to be all lined up

Also, why not just use a string instead of a string pointer?

using just string was giving me some weird errors when I tried to build it. Im not too familiar with it but I can change it to a string and give it another Go. 🥁

Avantol13 · 2023-08-23T16:05:03Z

gen3-client/g3cmd/retry-upload.go

@@ -79,6 +81,10 @@ func retryUpload(failedLogMap map[string]commonUtils.RetryObject) {
 		log.Printf("Sleep for %.0f seconds\n", GetWaitTime(ro.RetryCount).Seconds())
 		time.Sleep(GetWaitTime(ro.RetryCount)) // exponential wait for retry

+		if ro.Bucket !=nil || *ro.Bucket != "" {


I think there should be a space != nil

Also this is a little confusing. I think if Bucket was just a string like GUID this would be a little easier.

Avantol13 · 2023-08-23T16:13:25Z

gen3-client/g3cmd/utils.go

@@ -38,6 +38,7 @@ type ManifestObject struct {
 // InitRequestObject represents the payload that sends to FENCE for getting a singlepart upload presignedURL or init a multipart upload for new object file
 type InitRequestObject struct {
 	Filename string `json:"file_name"`
+	Bucket *string `json:"bucket,omitempty"`


Why a string pointer and not just a string?

Avantol13 · 2023-08-23T16:19:50Z

gen3-client/g3cmd/retry-upload.go

@@ -53,6 +53,8 @@ func retryUpload(failedLogMap map[string]commonUtils.RetryObject) {
 	var guid string
 	var presignedURL string
 	var err error
+	var bucketPtr string


This is suffixed with Ptr but it doesn't look like it's a pointer? It's just a string

Avantol13 · 2023-08-23T16:21:03Z

gen3-client/g3cmd/utils.go

@@ -57,13 +58,15 @@ type MultipartUploadRequestObject struct {
 	Key        string `json:"key"`
 	UploadID   string `json:"uploadId"`
 	PartNumber int    `json:"partNumber"`
+	Bucket *string `json:"bucket,omitempty"`


Same comment here. The pattern for other variables here seem to be to use the raw type (string, int) rather than a pointer

Avantol13 · 2023-08-23T16:21:31Z

gen3-client/g3cmd/utils.go

 }

 // MultipartCompleteRequestObject represents the payload that sends to FENCE for completeing a multipart upload
 type MultipartCompleteRequestObject struct {
 	Key      string                `json:"key"`
 	UploadID string                `json:"uploadId"`
 	Parts    []MultipartPartObject `json:"parts"`
+	Bucket *string `json:"bucket,omitempty"`


Same comment re: pointers

Avantol13 · 2023-08-23T16:22:13Z

gen3-client/g3cmd/utils.go

 	request := new(jwt.Request)
 	configure := new(jwt.Configure)
 	function := new(jwt.Functions)

 	function.Config = configure
 	function.Request = request

-	multipartCompleteObject := MultipartCompleteRequestObject{Key: key, UploadID: uploadID, Parts: parts}
+	var bucketPtr *string


I think if you just passed through the bucketName as a string you wouldn't have to do all this every time

bwalsh · 2023-10-24T15:08:18Z

gen3-client/g3cmd/upload-multiple.go

-				if batch {
+				if forceMultipart {
+					uploadPath := []string{furObjects[i].FilePath}
+					_, multipartFilePaths := validateFilePath(uploadPath, forceMultipart)


validateFilePath returns two arrays (singlepartFilePaths, multipartFilePaths). It looks as if the singlepartFilePaths array is ignored. @BinamB

bwalsh · 2023-10-24T15:15:41Z

gen3-client/g3cmd/upload-multiple.go

 					if len(batchFURObjects) < workers {
 						batchFURObjects = append(batchFURObjects, furObject)
 					} else {
-						batchUpload(gen3Interface, batchFURObjects, workers, respCh, errCh)
+						batchUpload(gen3Interface, batchFURObjects, workers, respCh, errCh, bucketName)


Also, I don't believe multipartUpload gets called at all if part of a batch? @BinamB

lbeckman314 · 2023-11-02T20:36:50Z

Hey @BinamB thank you for your work adding support for small files with the multipart upload! The files are being uploaded successfully to the bucket which is fantastic, unfortunately I'm encountering a couple of potential issues during this process (that might be due to my testing setup) that could interfere with potential end users:

My Testing Setup

Testing directory and files:

.
├── DATA
│   ├── file-1MB.1.txt
│   ├── file-1MB.2.txt
│   ├── file-1MB.3.txt
│   ├── ...
│   └── file-1MB.99.txt
└── file-manifest.json

2 directories, 101 files

Command:

gen3-client upload-multiple --profile=development \
    --manifest=./file-manifest.json \
    --bucket aced-ohsu-development \
    --upload-path . \
    --force-multipart \
    --batch \
    --numparallel=10

File manifest sample (for one file):

[
  {
    "object_id": "bda45761-f6d5-543c-82ab-e459b94b96c1",
    "file_name": "DATA/file-1MB.1.txt",
    "size": 1048576,
    "modified": "2023-11-02T11:11:10.707559",
    "md5": "b6d81b360a5672d80c27430f39153e2c",
  }
 ]

Issue: Slow file validation

The upload process tries to validate files that aren't in the file manifest which can result in a somewhat long time (a few minutes) to start the actual upload. Could this be due to the following logic in upload-multiple.go?

The filePaths variable includes files that are not in the file manifest (but are in the upload path), resulting in attempts to validate/upload extra files:

upload-multiple.go#L87-L95

for i, furObject := range furObjects {
    if forceMultipart {
        uploadPath, _ := commonUtils.GetAbsolutePath(uploadPath)
        filePaths, err := commonUtils.ParseFilePaths(uploadPath, false)  <-- Includes non-manifest files?
        if err != nil {
            log.Fatalf("Error when parsing file paths: " + err.Error())
        }

        singlepartFilePaths, multipartFilePaths := validateFilePath(filePaths, forceMultipart)

Issue: Extra files uploaded

The upload process started much quicker after removing all files in the testing directory that are not included in the file manifest.

However it seems like the file manifest itself was uploaded judging by the resulting output?

2023/11/02 12:49:15 File "<TESTING DIRECTORY>/file-manifest.json" has been found in local submission history and has been skipped for preventing duplicated submissions.

Confirmed by adding a "Secret" file to the upload path (but not to the manifest):

echo "Secret password" > DATA/secret.txt

Rerunning the gen3-client command uploaded this "extra" file:

~/.gen3/logs/development_succeeded_log.json:

...
"DATA/secret.txt": "PREFIX/bf46fc01-0d27-43ca-b014-0af89334d94d"
...

Issue: Erroneous "Skipped" log after successful upload

The test files themselves were uploaded successfully, but the resulting output included the following error for each file (despite removing all upload logs prior to the test: rm ~/.gen3/logs/development_succeeded_log.json):

2023/11/02 12:49:15 File "<TESTING DIRECTORY>/file-1MB.99.txt" has been found in local submission history and has been skipped for preventing duplicated submissions.

Potential Tester Error?

Let me know if I'm missing something big or small in my testing setup! I've been known to miss the forest for the trees so it could be something obvious, can also provide any additional info on the testing process. Thank you!

Avantol13 · 2023-12-06T19:59:37Z

gen3-client/g3cmd/upload-multiple.go

-			}
+			if forceMultipart {
+				uploadPath, _ := commonUtils.GetAbsolutePath(uploadPath)
+				// filePaths, err := commonUtils.ParseFilePaths(uploadPath, false)


let's not leave dead code, if we don't need this, remove it

paulineribeyre

i forgot to post comments from when i started reviewing this

gen3-client/commonUtils/commonUtils.go

gen3-client/g3cmd/upload-multiple.go

…o feat/bucket_param_multipart

* Use file_name provided in manifest * Use file_name provided in manifest * Ensure file_name used in url * Ensure bucket passed to GeneratePresignedURL

…o feat/bucket_param_multipart

gen3-client/g3cmd/upload-multiple.go

gen3-client/g3cmd/upload-single.go

gen3-client/g3cmd/upload.go

gen3-client/g3cmd/upload-multiple.go

paulineribeyre · 2024-01-17T22:54:32Z

gen3-client/g3cmd/utils.go

-		endPointPostfix := commonUtils.FenceDataUploadEndpoint + "/" + furObject.GUID
-		msg, err := g3.DoRequestWithSignedHeader(&profileConfig, endPointPostfix, "", nil)
+        if furObject.PresignedURL == "" {
+               endPointPostfix := commonUtils.FenceDataUploadEndpoint + "/" + furObject.GUID + "?file_name=" + url.QueryEscape(furObject.Filename)


how did this ever work before? the file_name param is required in the fence data upload endpoint 🤔

Im not sure. I followed the code and there are cases for a nil body but it ends here and still sends this request somehow
https://github.com/uc-cdis/cdis-data-client/blob/master/gen3-client/jwt/functions.go#L66

gen3-client/g3cmd/utils.go

already reviewed and approved by someone else

Add bucket param for multipart upload

1a47c08

github-actions bot added @gen3-client skip-quay-build labels Aug 15, 2023

BinamB changed the title ~~Add bucket param for multipart upload~~ (feat): Add bucket param for multipart upload Aug 15, 2023

Merge branch 'master' into feat/bucket_param_multipart

aef7457

BinamB changed the title ~~(feat): Add bucket param for multipart upload~~ (feat): Add bucket selection support for multi part upload Aug 15, 2023

BinamB mentioned this pull request Aug 15, 2023

(PPS-411): Add bucket selection support for multi part upload uc-cdis/fence#1112

Merged

1 task

lbeckman314 mentioned this pull request Aug 15, 2023

Test with wasabi S3 ACED-IDP/gen3-helm#25

Closed

BinamB marked this pull request as ready for review August 16, 2023 19:58

Avantol13 previously requested changes Aug 23, 2023

View reviewed changes

BinamB added 3 commits August 24, 2023 16:04

Resolve comments

823d38c

Add bucket param for single file upload

d5cf1fd

add support for upload-single

c2a0463

lbeckman314 mentioned this pull request Sep 13, 2023

Update initial support for specifying data bucket #124

Closed

Get upload-multiple + multipart working

0ef7d5e

BinamB changed the title ~~(feat): Add bucket selection support for multi part upload~~ (PPS-411): Add bucket selection support for multi part upload Oct 18, 2023

bwalsh suggested changes Oct 24, 2023

View reviewed changes

Fix small files not working with multipart

88d3692

Fix upload

884c4e0

Avantol13 reviewed Dec 6, 2023

View reviewed changes

chore(comment): insignificant commit to force test reruns

2b7aa28

paulineribeyre reviewed Dec 6, 2023

View reviewed changes

gen3-client/commonUtils/commonUtils.go Show resolved Hide resolved

gen3-client/g3cmd/upload-multiple.go Show resolved Hide resolved

gen3-client/g3cmd/upload-multiple.go Outdated Show resolved Hide resolved

BinamB and others added 6 commits January 8, 2024 09:43

Refactor upload-multiple

0f4c428

Merge remote-tracking branch 'origin/feat/bucket_param_multipart' int…

9caeeed

…o feat/bucket_param_multipart

Use file_name provided in manifest (#125)

e8ca046

* Use file_name provided in manifest * Use file_name provided in manifest * Ensure file_name used in url * Ensure bucket passed to GeneratePresignedURL

Fix batch

aede88a

fix utils

1cc4ba5

Merge remote-tracking branch 'origin/feat/bucket_param_multipart' int…

04947e5

…o feat/bucket_param_multipart

paulineribeyre self-requested a review January 8, 2024 18:12

fix utils.go

87487fd

paulineribeyre reviewed Jan 17, 2024

View reviewed changes

cleanup+remove validateFilePath

15ef337

paulineribeyre self-requested a review January 22, 2024 16:42

paulineribeyre approved these changes Jan 22, 2024

View reviewed changes

BinamB merged commit b9b3605 into master Jan 22, 2024
8 checks passed

BinamB deleted the feat/bucket_param_multipart branch January 22, 2024 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PPS-411): Add bucket selection support for multi part upload #123

(PPS-411): Add bucket selection support for multi part upload #123

BinamB commented Aug 15, 2023 •

edited

Loading

coveralls commented Aug 15, 2023 •

edited

Loading

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

BinamB Aug 24, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

Avantol13 Aug 23, 2023

bwalsh Oct 24, 2023

bwalsh Oct 24, 2023

lbeckman314 commented Nov 2, 2023 •

edited

Loading

Avantol13 Dec 6, 2023

paulineribeyre left a comment

paulineribeyre Jan 17, 2024

BinamB Jan 18, 2024

(PPS-411): Add bucket selection support for multi part upload #123

(PPS-411): Add bucket selection support for multi part upload #123

Conversation

BinamB commented Aug 15, 2023 • edited Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

coveralls commented Aug 15, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7589691499

💛 - Coveralls

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

lbeckman314 commented Nov 2, 2023 • edited Loading

My Testing Setup

Issue: Slow file validation

Issue: Extra files uploaded

Issue: Erroneous "Skipped" log after successful upload

Potential Tester Error?

Choose a reason for hiding this comment

paulineribeyre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BinamB commented Aug 15, 2023 •

edited

Loading

coveralls commented Aug 15, 2023 •

edited

Loading

lbeckman314 commented Nov 2, 2023 •

edited

Loading