rgw: skip empty check on non-owned buckets by zonegroup by clwluvw · Pull Request #60227 · ceph/ceph

clwluvw · 2024-10-09T21:35:46Z

Compare RGW's zonegroup with the bucket's zonegroup and only do the empty check when the bucket is owned by the RGW running the delete.

Fixes: https://tracker.ceph.com/issues/68190

clwluvw · 2024-10-27T16:34:36Z

Should we make this more generic to indexless buckets? (ca96feb)

github-actions · 2024-12-26T21:01:21Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2025-01-08T18:38:58Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

src/rgw/driver/rados/rgw_bucket.cc

src/rgw/driver/rados/rgw_rest_bucket.cc

cbodley · 2025-01-09T16:29:15Z

src/rgw/driver/rados/rgw_bucket.cc

+int RGWBucketAdminOp::remove_bucket(rgw::sal::Driver* driver, const rgw::SiteConfig& site,
+                                    RGWBucketAdminOpState& op_state,


the DELETE /admin/bucket api doesn't actually call through this function, so i think we'll need special logic for that. currently it just calls rgw_forward_request_to_master() then sal::Bucket::remove()

edit: or maybe the rest api should use RGWBucketAdminOp::remove_bucket() instead

hmm I couldn't get your point with that... why do we need DELETE /admin/bucket to comeback to RGWBucketAdminOp::remove_bucket again? Is it about those controls I added to RGWBucketAdminOp::remove_bucket()?

because RGWOp_Bucket_Remove doesn't call through RGWBucketAdminOp::remove_bucket(), it won't get redirected to the bucket's zonegroup so may remove the bucket without its data

we generally put code in RGWBucketAdminOp so it can shared by both the radosgw-admin commands and their /admin APIs. that's why it's surprising to me that RGWOp_Bucket_Remove doesn't use it

because RGWOp_Bucket_Remove doesn't call through RGWBucketAdminOp::remove_bucket(), it won't get redirected to the bucket's zonegroup so may remove the bucket without its data

Basically in RGWBucketAdminOp::remove_bucket() we don't redirect to the bucket's zonegroup but to stop if the zonegroups don't match: 1880739#diff-36d954c172f2e053a2942c640d14a6183efb6f19d0ad7c991762a769094f2b35R1449-R1455

we generally put code in RGWBucketAdminOp so it can shared by both the radosgw-admin commands and their /admin APIs. that's why it's surprising to me that RGWOp_Bucket_Remove doesn't use it

I agree this doesn't look efficient, we can merge these together, but I was wondering how to handle that if-clause to not stop for forwarded requests when zonegroup doesn't match and it's master zonegroup. as that is an admin API it's always about system_user so that can't be a good identifier I believe.

@cbodley I have integrated both into one and tried a workaround for detecting forwarded requests by checking for rgwx-zonegroup arg. Could you please have another look?

github-actions · 2025-03-14T18:02:13Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

src/common/options/rgw.yaml.in

cbodley · 2025-03-31T20:13:01Z

qa was mostly green in https://pulpito.ceph.com/cbodley-2025-03-28_17:53:04-rgw-wip-cbodley-testing-distro-default-smithi/, but the rgw/dbstore suite failed on test_bucket_delete_nonempty:

=================================== FAILURES ===================================
_________________________ test_bucket_delete_nonempty __________________________

    def test_bucket_delete_nonempty():
        key_names = ['foo']
        bucket_name = _create_objects(keys=key_names)
        client = get_client()

>       e = assert_raises(ClientError, client.delete_bucket, Bucket=bucket_name)

s3tests_boto3/functional/test_s3.py:1532:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

excClass = <class 'botocore.exceptions.ClientError'>
callableObj = <bound method ClientCreator._create_api_method.<locals>._api_call of <botocore.client.S3 object at 0x7faa2d922410>>
args = (), kwargs = {'Bucket': 'test-qod9ft1hu6wj1a69qj4n440z-105'}
excName = 'ClientError'

    def assert_raises(excClass, callableObj, *args, **kwargs):
        """
        Like unittest.TestCase.assertRaises, but returns the exception.
        """
        try:
            callableObj(*args, **kwargs)
        except excClass as e:
            return e
        else:
            if hasattr(excClass, '__name__'):
                excName = excClass.__name__
            else:
                excName = str(excClass)
>           raise AssertionError("%s not raised" % excName)
E           AssertionError: ClientError not raised

s3tests_boto3/functional/utils.py:19: AssertionError

if you need to debug that locally, vstart supports --rgw_store dbstore

clwluvw · 2025-03-31T20:36:41Z

the rgw/dbstore suite failed on test_bucket_delete_nonempty

This looks strange as the zoengroup id, which I take should be uuid is set to default

ceph/src/rgw/rgw_sal_dbstore.h

Line 283 in c3b9e1c

    
           std::unique_ptr<RGWZoneGroup> rzg = std::make_unique<RGWZoneGroup>("default", "default");

clwluvw · 2025-03-31T20:41:34Z

the rgw/dbstore suite failed on test_bucket_delete_nonempty

This looks strange as the zoengroup id, which I take should be uuid is set to default

ceph/src/rgw/rgw_sal_dbstore.h

Line 283 in c3b9e1c

std::unique_ptr<RGWZoneGroup> rzg = std::make_unique<RGWZoneGroup>("default", "default");

@cbodley - Would that make sense to skip this fix for dbstore based on the following comment?

ceph/src/rgw/rgw_sal_dbstore.cc

Line 1827 in c3b9e1c

/* XXX: for now only one zonegroup supported */

cbodley · 2025-03-31T20:46:19Z

@cbodley - Would that make sense to skip this fix for dbstore based on the following comment?

sure. it will probably be a long time before any other drivers support multisite. as long as we have multisite test coverage, that will force us to fix this in dbstore eventually

Compare RGW's zonegroup with bucket's zonegroup and only do the empty check when the bucket is owned by the RGW running the delete. Fixes: https://tracker.ceph.com/issues/68190 Signed-off-by: Seena Fallah <[email protected]>

To align same functionality for bucket deletion from both API and rgw-admin, use the same function from RGWBucketAdminOp. Signed-off-by: Seena Fallah <[email protected]>

Allow running `radosgw-admin bucket rm` from secondary zonegroup. This allows bucket deletion with `--purge-objects` and with `--bypass-gc` when deleting the bucket owned by non-master zonegroup. Signed-off-by: Seena Fallah <[email protected]>

clwluvw · 2025-03-31T20:53:09Z

fixed the dbstore case and also addressed @anthonyeleven's review on the doc.

cbodley · 2025-04-23T17:40:57Z

passed qa in https://pulpito.ceph.com/cbodley-2025-04-22_21:27:58-rgw-wip-cbodley-testing-distro-default-smithi
after rerun https://pulpito.ceph.com/cbodley-2025-04-23_13:53:21-rgw-wip-cbodley-testing-distro-default-smithi/

clwluvw requested a review from a team as a code owner October 9, 2024 21:35

github-actions bot added the rgw label Oct 9, 2024

clwluvw mentioned this pull request Oct 9, 2024

rgw: add support bucket replication between zonegroups #59911

Closed

adamemerson requested a review from smanjara October 11, 2024 18:27

clwluvw mentioned this pull request Dec 10, 2024

rgw/multisite: bucket instance cleanup after deletion #53799

Merged

14 tasks

github-actions bot added the stale label Dec 26, 2024

clwluvw removed the stale label Dec 27, 2024

clwluvw requested a review from cbodley January 8, 2025 17:43

github-actions bot added the needs-rebase label Jan 8, 2025

clwluvw force-pushed the zonegroup-delbucket branch from e3250b4 to 5cc5ed0 Compare January 8, 2025 18:42

github-actions bot removed the needs-rebase label Jan 8, 2025

clwluvw force-pushed the zonegroup-delbucket branch from 5cc5ed0 to 1785a30 Compare January 8, 2025 18:54

github-actions bot added the tests label Jan 8, 2025

cbodley reviewed Jan 8, 2025

View reviewed changes

src/rgw/driver/rados/rgw_bucket.cc Outdated Show resolved Hide resolved

clwluvw force-pushed the zonegroup-delbucket branch 2 times, most recently from 2d47f0a to ea0b0a1 Compare January 8, 2025 22:42

smanjara reviewed Jan 9, 2025

View reviewed changes

src/rgw/driver/rados/rgw_bucket.cc Show resolved Hide resolved

cbodley reviewed Jan 9, 2025

View reviewed changes

src/rgw/driver/rados/rgw_rest_bucket.cc Outdated Show resolved Hide resolved

clwluvw force-pushed the zonegroup-delbucket branch from ea0b0a1 to 1880739 Compare January 9, 2025 16:16

clwluvw requested a review from a team as a code owner January 9, 2025 16:16

github-actions bot added the documentation label Jan 9, 2025

cbodley reviewed Jan 9, 2025

View reviewed changes

clwluvw force-pushed the zonegroup-delbucket branch 3 times, most recently from 3bc710e to c1881c5 Compare January 13, 2025 17:08

clwluvw mentioned this pull request Feb 24, 2025

rgw: add support replication actions in policy #61962

Merged

anthonyeleven approved these changes Mar 26, 2025

View reviewed changes

src/common/options/rgw.yaml.in Outdated Show resolved Hide resolved

cbodley added the wip-cbodley-testing label Mar 28, 2025

cbodley removed needs-qa wip-cbodley-testing labels Mar 31, 2025

cbodley assigned clwluvw and unassigned cbodley Mar 31, 2025

clwluvw added 2 commits March 31, 2025 22:50

rgw: call RGWBucketAdminOp::remove_bucket by RGWOp_Bucket_Remove

9ae2d8c

To align same functionality for bucket deletion from both API and rgw-admin, use the same function from RGWBucketAdminOp. Signed-off-by: Seena Fallah <[email protected]>

clwluvw force-pushed the zonegroup-delbucket branch from 472c2c6 to eb9f569 Compare March 31, 2025 20:51

clwluvw force-pushed the zonegroup-delbucket branch from eb9f569 to c7694dd Compare March 31, 2025 20:52

cbodley added the needs-qa label Mar 31, 2025

cbodley added the wip-cbodley-testing label Apr 22, 2025

cbodley added TESTED ready-to-merge and removed needs-qa wip-cbodley-testing labels Apr 23, 2025

cbodley merged commit 83c9201 into ceph:main Apr 23, 2025
12 checks passed

clwluvw mentioned this pull request Apr 26, 2025

squid: rgw: skip empty check on non-owned buckets by zonegroup #62994

Merged

anoopcs9 mentioned this pull request Apr 30, 2025

CI test failures against ceph main for TestBucket/remove_non-existing_bucket ceph/go-ceph#1107

Closed

		int RGWBucketAdminOp::remove_bucket(rgw::sal::Driver* driver, const rgw::SiteConfig& site,
		RGWBucketAdminOpState& op_state,

Comments

Conversation

clwluvw commented Oct 9, 2024

Uh oh!

clwluvw commented Oct 27, 2024

Uh oh!

github-actions bot commented Dec 26, 2024

Uh oh!

github-actions bot commented Jan 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbodley Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clwluvw Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

cbodley Jan 9, 2025

Choose a reason for hiding this comment

Uh oh!

clwluvw Jan 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clwluvw Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 14, 2025

Uh oh!

Uh oh!

cbodley commented Mar 31, 2025

Uh oh!

clwluvw commented Mar 31, 2025

Uh oh!

clwluvw commented Mar 31, 2025

Uh oh!

cbodley commented Mar 31, 2025

Uh oh!

clwluvw commented Mar 31, 2025

Uh oh!

cbodley commented Apr 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbodley Jan 9, 2025 •

edited

Loading

clwluvw Jan 9, 2025 •

edited

Loading