[#275] Change logic abort multipart upload #275

Merged
alexvanin merged 1 commit from :feature/change_logic_abort_multipart into master 2023-12-27 11:08:59 +00:00
Member

When abort multipart upload, the following situation may occur: when deleting multipart upload nodes from the wooden service, they may not immediately synchronize with other nodes. Because of this, for example, the ListParts operation can return a list of downloaded parts, although the multipart download was interrupted and should no longer exist.

The solution to the problem is to change the logic of node deletion during Abort Multipart. It is proposed not to delete all nodes of the tree associated with multipart upload, but to leave the root node with the interrupt flag enabled. When reading the multipart upload root node, this flag is checked: if it is enabled, then we do not go to poll the nodes of the tree service to receive parts upload.

Signed-off-by: Roman Loginov r.loginov@yadro.com

When abort multipart upload, the following situation may occur: when deleting multipart upload nodes from the wooden service, they may not immediately synchronize with other nodes. Because of this, for example, the ListParts operation can return a list of downloaded parts, although the multipart download was interrupted and should no longer exist. The solution to the problem is to change the logic of node deletion during Abort Multipart. It is proposed not to delete all nodes of the tree associated with multipart upload, but to leave the root node with the interrupt flag enabled. When reading the multipart upload root node, this flag is checked: if it is enabled, then we do not go to poll the nodes of the tree service to receive parts upload. Signed-off-by: Roman Loginov <r.loginov@yadro.com>
r.loginov self-assigned this 2023-12-04 07:55:37 +00:00
r.loginov force-pushed feature/change_logic_abort_multipart from 37c9c5b11d to 51acb2462e 2023-12-04 07:58:47 +00:00 Compare
r.loginov changed title from [##] Change logic abort multipart upload to [#275] Change logic abort multipart upload 2023-12-04 08:07:46 +00:00
r.loginov changed title from [#275] Change logic abort multipart upload to WIP: [#275] Change logic abort multipart upload 2023-12-04 08:22:39 +00:00
r.loginov force-pushed feature/change_logic_abort_multipart from 51acb2462e to 86f29fff60 2023-12-04 09:06:58 +00:00 Compare
r.loginov changed title from WIP: [#275] Change logic abort multipart upload to [#275] Change logic abort multipart upload 2023-12-04 09:07:23 +00:00
r.loginov requested review from storage-services-committers 2023-12-04 09:12:06 +00:00
r.loginov requested review from storage-services-developers 2023-12-04 09:12:07 +00:00
Member

Do we have the same problem with CompleteMultipartUpload?

Do we have the same problem with `CompleteMultipartUpload`?
dkirillov reviewed 2023-12-04 13:09:23 +00:00
@ -195,2 +195,4 @@
}
if multipartInfo.Aborted {
return "", fmt.Errorf("%w", s3errors.GetAPIError(s3errors.ErrNoSuchUpload))
Member

Can we encapsulate this check into GetMultipartUpload method in TreeService interface?

Can we encapsulate this check into `GetMultipartUpload` method in `TreeService` interface?
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-12-04 13:13:03 +00:00
@ -566,3 +574,3 @@
}
return n.treeService.DeleteMultipartUpload(ctx, p.Bkt, multipartInfo.ID)
if err = n.treeService.DeleteMultipartUpload(ctx, p.Bkt, multipartInfo.ID); err != nil {
Member

Probably we can pass multipartInfo to DeleteMultiaprtUpload and encapsulate creating new node with aborted attribute there

Probably we can pass `multipartInfo` to `DeleteMultiaprtUpload` and encapsulate creating new node with aborted attribute there
dkirillov marked this conversation as resolved
r.loginov force-pushed feature/change_logic_abort_multipart from 86f29fff60 to 267a8f4996 2023-12-05 06:06:58 +00:00 Compare
Author
Member

Do we have the same problem with CompleteMultipartUpload?

Yes, this problem is also present for CompleteMultipartUpload.

> Do we have the same problem with `CompleteMultipartUpload`? Yes, this problem is also present for CompleteMultipartUpload.
dkirillov reviewed 2023-12-05 06:29:30 +00:00
@ -1259,6 +1271,7 @@ func metaFromMultipart(info *data.MultipartInfo, fileName string) map[string]str
info.Meta[uploadIDKV] = info.UploadID
info.Meta[ownerKV] = info.Owner.EncodeToString()
info.Meta[createdKV] = strconv.FormatInt(info.Created.UTC().UnixMilli(), 10)
info.Meta[finished] = strconv.FormatBool(info.Finished)
Member

Actually we can set this attribute only if it's true

Actually we can set this attribute only if it's true
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-12-05 06:31:10 +00:00
@ -1010,3 +1015,3 @@
continue
}
if info.UploadID == uploadID {
if info.UploadID == uploadID && !info.Finished {
Member

Probably we can return layer.ErrNodeNotFound if uploadID matched but MultipartUpload has already finished

Probably we can return `layer.ErrNodeNotFound` if `uploadID` matched but MultipartUpload has already finished
dkirillov marked this conversation as resolved
r.loginov force-pushed feature/change_logic_abort_multipart from 267a8f4996 to 83f9d03b4b 2023-12-05 08:43:06 +00:00 Compare
dkirillov reviewed 2023-12-05 09:26:04 +00:00
CHANGELOG.md Outdated
@ -60,6 +60,7 @@ This document outlines major changes between releases.
- Generalise config param `use_default_xmlns_for_complete_multipart` to `use_default_xmlns` so that use default xmlns for all requests (#221)
- Set server IdleTimeout and ReadHeaderTimeout to `30s` and allow to configure them (#220)
- Return `ETag` value in quotes (#219)
- Change the logic of working with a tree service in AbortMultipartUpload (#275)
Member

in AbortMultipartUpload

in AbortMultipartUpload/CompleteMultipartUpload?

Or probably we can name this as using tombstone when delete multipart upload

> in AbortMultipartUpload in AbortMultipartUpload/CompleteMultipartUpload? Or probably we can name this as using tombstone when delete multipart upload
dkirillov marked this conversation as resolved
dkirillov approved these changes 2023-12-05 09:27:20 +00:00
dkirillov left a comment
Member

LGTM

LGTM
r.loginov force-pushed feature/change_logic_abort_multipart from 83f9d03b4b to f0186b7e23 2023-12-05 09:47:41 +00:00 Compare
r.loginov force-pushed feature/change_logic_abort_multipart from f0186b7e23 to a6ef98c4ce 2023-12-05 09:49:57 +00:00 Compare
dkirillov approved these changes 2023-12-05 09:58:43 +00:00
r.loginov force-pushed feature/change_logic_abort_multipart from a6ef98c4ce to 42d96d3a12 2023-12-08 14:17:05 +00:00 Compare
mbiryukova reviewed 2023-12-08 14:37:33 +00:00
@ -992,6 +997,9 @@ func (c *Tree) GetMultipartUpload(ctx context.Context, bktInfo *data.BucketInfo,
continue
}
if info.UploadID == uploadID {
if info.Finished {
Member

Maybe we should do this check in GetMultipartUploadsByPrefix too

Maybe we should do this check in `GetMultipartUploadsByPrefix` too
mbiryukova marked this conversation as resolved
r.loginov changed title from [#275] Change logic abort multipart upload to WIP: [#275] Change logic abort multipart upload 2023-12-08 14:39:40 +00:00
r.loginov force-pushed feature/change_logic_abort_multipart from 42d96d3a12 to 1184b83e8a 2023-12-08 16:20:58 +00:00 Compare
r.loginov changed title from WIP: [#275] Change logic abort multipart upload to [#275] Change logic abort multipart upload 2023-12-08 16:28:07 +00:00
dkirillov approved these changes 2023-12-11 06:22:30 +00:00
mbiryukova approved these changes 2023-12-11 07:47:34 +00:00
r.loginov force-pushed feature/change_logic_abort_multipart from 1184b83e8a to 94017462ad 2023-12-22 13:58:38 +00:00 Compare
alexvanin reviewed 2023-12-25 08:55:43 +00:00
@ -248,0 +249,4 @@
finished, _ := treeNode.Get(finishedKV)
if flag, err := strconv.ParseBool(finished); err == nil {
multipartInfo.Finished = flag
}
Owner

What happens when this code is running during multipart upload started by previous version of the gateway?

Previous version won't set finishedKV flag, so ParseBool returns error and multipartInfo.Finished stays false. Shouldn't it be true in such cases?

What happens when this code is running during multipart upload started by previous version of the gateway? Previous version won't set `finishedKV` flag, so `ParseBool` returns error and `multipartInfo.Finished` stays `false`. Shouldn't it be `true` in such cases?
Owner

As we found out, this code isn't triggered when accessing multipart upload finished by previous version of the gateway, so it works as expected.

As we found out, this code isn't triggered when accessing multipart upload finished by previous version of the gateway, so it works as expected.
alexvanin marked this conversation as resolved
r.loginov force-pushed feature/change_logic_abort_multipart from 94017462ad to 6f9ee3da76 2023-12-27 10:07:06 +00:00 Compare
alexvanin merged commit 6f9ee3da76 into master 2023-12-27 11:08:59 +00:00
alexvanin deleted branch feature/change_logic_abort_multipart 2023-12-27 11:09:00 +00:00
alexvanin added this to the v0.29.0 milestone 2024-05-27 10:30:49 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#275
No description provided.