Navigation Menu

Skip to content
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

Jump to bottom

quincy: rgw: fix multipart upload object leaks due to re-upload #51976

Merged
merged 6 commits into from Jan 12, 2024
Merged

quincy: rgw: fix multipart upload object leaks due to re-upload #51976

merged 6 commits into from Jan 12, 2024

Conversation

trociny
Copy link
Contributor

@trociny trociny commented Jun 9, 2023 •
edited

backport tracker: https://tracker.ceph.com/issues/59065

backport of #49709 #50667
parent tracker: https://tracker.ceph.com/issues/16767

this backport was staged using ceph-backport.sh version 16.0.0.6848
find the latest version at https://github.com/ceph/ceph/blob/main/src/script/ceph-backport.sh

@trociny trociny added this to the quincy milestone Jun 9, 2023
@trociny trociny removed the DNM label Jun 12, 2023
@trociny trociny force-pushed the wip-59065-quincy branch 2 times, most recently from 0af1b9a to e0cb791 Compare June 12, 2023 07:55
@trociny
Copy link
Contributor Author

trociny commented Jun 12, 2023

jenkins retest this please

mattbenjamin and others added 5 commits June 13, 2023 07:18
@mattbenjamin @trociny
rgw: setup for inclusion of rgw_basic_types in rgw cls
bd73889 
In downstream 4.1 version this change moved rgw_pool, rgw_bucket,
and some related types--but these have already moved on more
recent branches.

Include rgw_basic_types.h only from cls_rgw_types.h (review).

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
Signed-off-by: Daniel Gryniewicz <dang@redhat.com>
(cherry picked from commit 9ee09fc)
@mattbenjamin @trociny
rgw: define a hierarchy of basic type headers
647f2af 
Defines a hierarchy of "simple" data types ensured to be
safe to include in any context above the zipper line, and
also from CLS.

The following headers are currently defined to contain basic
types only:

 rewrite src/rgw/rgw_basic_types.h (70%) // includes the whole hierarchy
 create mode 100644 src/rgw/rgw_acl_types.h
 create mode 100644 src/rgw/rgw_bucket_types.h
 create mode 100644 src/rgw/rgw_obj_types.h
 create mode 100644 src/rgw/rgw_placement_types.h
 create mode 100644 src/rgw/rgw_pool_types.h
 create mode 100644 src/rgw/rgw_quota_types.h
 create mode 100644 src/rgw/rgw_user_types.h
 create mode 100644 src/rgw/rgw_zone_types.h

This commit consolidates the following original commits:

* rgw: move RGWUploadPartInfo to rgw_basic_types.{h,cc}
* rgw: move rgw_obj_key to rgw_basic_types.{h,cc}
* rgw: move rgw_placement_rule to rgw_basic_types.{h,cc}
* rgw: move rgw_obj to rgw_basic_types.{h,cc}
* rgw: include rgw_compression_types.h in rgw_basic_types.{h,cc}
* rgw: move rgw_raw_obj to rgw_basic_types.{h,cc}
* rgw: rgw_multi.h: remove unused RGWMPObj forward decl

and the following cleanups from review:

* rgw: remove stray comments
* rgw: move rgw_obj_manifest.h inclusion to top of rgw_basic_types.h
* rgw: nit: indentation
* rgw: remove this line
* rgw: move rgw_bucket_shard to rgw_bucket_types.h, cleanup
* rgw: fix rgw_quota.h

Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
Signed-off-by: Daniel Gryniewicz <dang@redhat.com>
(cherry picked from commit c30449b)

Conflicts:
	src/rgw/driver/rados/rgw_user.cc (different file location,
                                          necessary changes already there)
	src/rgw/rgw_basic_types.h (rgw_bucket_key, rgw_bucket: had to remove manually,
                                   no idea why automatic merge failed,
                                   the structs seem the same)
	src/rgw/rgw_common.h (missing comment about RGWObjVersionTracker:
                              had to remove rgw_raw_obj manually;
                              rgw_obj: had to remove manually,
                              no idea why automatic merge failed,
                              the structs seem the same)
	src/rgw/rgw_quota.h (struct RGWQuota missing)
	src/rgw/rgw_zone.h (different file location,
                            struct RGWZonePlacementInfo v8 vs v7,
                            struct RGWZone v7 vs v8)
	src/rgw/rgw_zone_features.h (zone features not available)
@yima77 @trociny
rgw: fix multipart upload object leaks due to re-upload
9b74dab 
This commit fixes the object leaks when an mp part object
is re-uploaded. Details of the fix are:

1. Upon re-upload, remember the prefix used in previous
part upload in a new field "past_prefixes" in RGWUploadPartInfo.
2. Create a new CLS function for updating part info
in the metadata object.
3. Utilize the new CLS function during mp upload.
4. At the upload conclusion (compete/abort), clean up
the part objects that are not used for the final
assembly, thus preventing the object leak.

Fixes: https://tracker.ceph.com/issues/16767
Signed-off-by: Yixin Jin <yjin77@yahoo.ca>
(cherry picked from commit 2ac63de)

Conflicts:
	src/rgw/rgw_sal_rados.cc (trivial)
        src/cls/rgw/cls_rgw.cc (use std::set 'count' instead of 'contains',
                                cls_get_object_info not available)
@yima77 @trociny
rgw: Fix mp upload obj leak due to reupload
f17d379 
1. Replace the ugly dynamic_cast with the call
of obj_to_raw() to access the raw meta obj.

Fixes: https://tracker.ceph.com/issues/16767
Signed-off-by: Yixin Jin <yjin77@yahoo.ca>
(cherry picked from commit 4cc0db6)
@yima77 @trociny
rgw: Turn sync part_info_update CLS call to an async one
d82fa08 
1. Use librados::ObjectWriteOperation to implement
the async CLS call.
2. Change the call to use the async CLS call.
3. Style update to be compliant

Signed-off-by: Yixin Jin <yjin77@yahoo.ca>
(cherry picked from commit 9996b02)
cbodley
cbodley requested changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the backport. this is blocked on test coverage in #50667, so we'll need to get that working and included here for validation

@rhamon
Copy link

rhamon commented Jul 24, 2023

With #50667 merged, can this move forward to a release?

@ivancich @trociny
qa/rgw: test that multipart re-upload does not leave any orphans
9e783a7 
Runs a boto script that reuploads one part multiple times before
completing and then we check for any orphans.

Original boto script contributed by Matt Benjamin
<mbenjami@redhat.com> on top of which modifications were made.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
(cherry picked from commit aeffd1b)
@trociny
Copy link
Contributor Author

trociny commented Jul 25, 2023

I have cherry-picked the commit from #50667 here.

@rhamon thank you for the remainder

@pr0ton11
Copy link

Hi guys
Are there any blockers preventing this from being merged? It would be awesome if this could be merged ASAP as we are awaiting this backport for our upgrade schedule.
Thanks you

@dang dang added needs-qa and removed tests labels Nov 15, 2023
@dang
Copy link
Contributor

dang commented Nov 15, 2023

jenkins test api

@dang
Copy link
Contributor

dang commented Nov 15, 2023

jenkins test make check

@prazumovsky
Copy link

Hello! What is the current status of the backport fix? We're observing it on our clusters and waiting for quincy backport.

@cbodley
Copy link
Contributor

cbodley commented Dec 1, 2023

Hello! What is the current status of the backport fix? We're observing it on our clusters and waiting for quincy backport.

hey @prazumovsky, since this didn't make the 17.2.7 release, it will be tested when we're ready to do 17.2.8. that will happen after 18.2.1 and 16.2.15

@cbodley
Copy link
Contributor

cbodley commented Dec 1, 2023

jenkins test make check

@cbodley
Copy link
Contributor

cbodley commented Jan 11, 2024

jenkins test make check

@yuriw yuriw merged commit 56daea9 into ceph:quincy Jan 12, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbodley cbodley cbodley approved these changes

@yima77 yima77 yima77 approved these changes

@ivancich ivancich Awaiting requested review from ivancich

@mattbenjamin mattbenjamin Awaiting requested review from mattbenjamin

Assignees
No one assigned
Labels
build/ops needs-qa rgw wip-yuri2-testing
Projects
None yet
Milestone
quincy
10 participants
@trociny @rhamon @pr0ton11 @dang @prazumovsky @cbodley @yima77 @yuriw @mattbenjamin @ivancich