rgw: fix bug deleting a non-existent object also generates a delete marker by kxkxk · Pull Request #48400 · ceph/ceph

kxkxk · 2022-10-08T07:21:53Z

fix:https://tracker.ceph.com/issues/56992
Signed-off-by: Yuxuan Yang xuanyu.yang@sincerecloud.com
I don't think rgw should create a delete marker for a non-existent object

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

src/rgw/rgw_op.cc

ivancich

I'm also wondering if this is where we should code this. I have a merged PR where we would not add a delete marker after an existing delete marker (see: https://github.com/ceph/ceph/pull/41897/files). There is existing code is src/cls/rgw/cls_rgw.cc in function rgw_bucket_link_olh, which loads in the OLH. In this area (

ceph/src/cls/rgw/cls_rgw.cc

Line 1763 in ee321e8

) if olh_found is false, we have a case where we're trying to add a delete marker when there is no object.

And that would put this adjacent to the stacked delete marker checks.

What do you think?

src/rgw/rgw_op.cc

kxkxk

I modified this part of the code so that the function is called correctly. My intention is that the bucket does not create delete markers for objects that are not created. This will be achieved by determining whether OLH exists or not.

ivancich

Thank you!

ivancich · 2023-01-30T20:28:58Z

src/cls/rgw/cls_rgw.cc

      return ret;
    }
+
+    // if the OLH not exist, error out


Maybe add "does" as in "if the OLH does not exist, error out".

Ok, thanks for pointing out

src/cls/rgw/cls_rgw.cc

kxkxk

Correct comment errors

ivancich · 2023-01-31T06:51:58Z

I just realized this is 10 commits. Are you familiar with how to squash all this down to a single commit? If you'd like some guidance, just let me know. I've added a DNM (do not merge) tag so prevent merge until the squash. But QA can proceed.

Also when you're down to one commit, it needs to be signed, as the CI is currently complaining about an unsigned commit.

Thanks!

kxkxk · 2023-01-31T08:23:30Z

I just realized this is 10 commits. Are you familiar with how to squash all this down to a single commit? If you'd like some guidance, just let me know. I've added a DNM (do not merge) tag so prevent merge until the squash. But QA can proceed.

Also when you're down to one commit, it needs to be signed, as the CI is currently complaining about an unsigned commit.

Thanks!

Got it.
Thanks for your advice!

ivancich · 2023-01-31T18:28:17Z

I just realized this is 10 commits. Are you familiar with how to squash all this down to a single commit? If you'd like some guidance, just let me know. I've added a DNM (do not merge) tag so prevent merge until the squash. But QA can proceed.
Also when you're down to one commit, it needs to be signed, as the CI is currently complaining about an unsigned commit.
Thanks!

Got it. Thanks for your advice!

Great, thank you. I don't know which method you used, but my tool for this is git rebase -i HEAD~_N_, where N is the number of commits. There are special occasions where I use git rebase -ir .... And then when I'm in the editor, I mark selected commits with f.

ivancich · 2023-01-31T18:32:04Z

@kxkxk, I updated the tracker for backports to quincy and pacific. Please edit if you think it should be different. Thanks!

kxkxk · 2023-02-01T10:23:30Z

@kxkxk, I updated the tracker for backports to quincy and pacific. Please edit if you think it should be different. Thanks!

It is available on both versions.

kxkxk

Thanks! I think it's ready.

cbodley · 2023-03-13T15:04:31Z

this failed qa on the s3test case test_versioning_multi_object_delete_with_marker_create:

=================================== FAILURES ===================================
____________ test_versioning_multi_object_delete_with_marker_create ____________

    @pytest.mark.fails_on_dbstore
    def test_versioning_multi_object_delete_with_marker_create():
        bucket_name = get_new_bucket()
        client = get_client()

        check_configure_versioning_retry(bucket_name, "Enabled", "Enabled")

        key = 'key'

        response = client.delete_object(Bucket=bucket_name, Key=key)
>       delete_marker_version_id = response['VersionId']
E       KeyError: 'VersionId'

s3tests_boto3/functional/test_s3.py:7531: KeyError

i tested amazon's behavior with the following boto script, and see that they allow this creation of delete markers:

client.delete_object(Bucket='test-nonexistent-delete-marker', Key='nonexistent-key')
response = client.list_object_versions(Bucket='test-nonexistent-delete-marker')
print(response['DeleteMarkers'])

[{'Owner': {'ID': '...'}, 'Key': 'nonexistent-key', 'VersionId': 'H8Czx7yv25IqznMOQbbU4bU.iHKfSqSn', 'IsLatest': True, 'LastModified': datetime.datetime(2023, 3, 13, 14, 55, 48, tzinfo=tzutc())}]

kxkxk · 2023-03-13T17:57:16Z

this failed qa on the s3test case test_versioning_multi_object_delete_with_marker_create:
=================================== FAILURES ===================================
____________ test_versioning_multi_object_delete_with_marker_create ____________

    @pytest.mark.fails_on_dbstore
    def test_versioning_multi_object_delete_with_marker_create():
        bucket_name = get_new_bucket()
        client = get_client()

        check_configure_versioning_retry(bucket_name, "Enabled", "Enabled")

        key = 'key'

        response = client.delete_object(Bucket=bucket_name, Key=key)
>       delete_marker_version_id = response['VersionId']
E       KeyError: 'VersionId'

s3tests_boto3/functional/test_s3.py:7531: KeyError
i tested amazon's behavior with the following boto script, and see that they allow this creation of delete markers:
client.delete_object(Bucket='test-nonexistent-delete-marker', Key='nonexistent-key')
response = client.list_object_versions(Bucket='test-nonexistent-delete-marker')
print(response['DeleteMarkers'])
[{'Owner': {'ID': '...'}, 'Key': 'nonexistent-key', 'VersionId': 'H8Czx7yv25IqznMOQbbU4bU.iHKfSqSn', 'IsLatest': True, 'LastModified': datetime.datetime(2023, 3, 13, 14, 55, 48, tzinfo=tzutc())}]

It should be that I misunderstood the AWS API documentation related to this. I don't understand the point of creating delete markers for non-existent objects. Maybe we should keep pace with Amazon?

NarrativeBias · 2023-05-06T17:57:15Z

@kxkxk, mind checking something semi-related to this issue for me?
Those delete markers for non-existent objects are generally successfully cleaned up by the ExpiredObjectDeleteMarker lifecycle however when I create that marker by sending two delete requests and process that lifecycle - there are leftover objects for those markers in the data pool.

github-actions · 2023-07-05T18:02:09Z

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 · 2023-08-04T19:01:28Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

github-actions bot added the rgw label Oct 8, 2022

mattbenjamin requested a review from ivancich October 9, 2022 21:14

mattbenjamin reviewed Oct 9, 2022

View reviewed changes

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

ivancich requested changes Oct 10, 2022

View reviewed changes

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

src/rgw/rgw_op.cc Outdated Show resolved Hide resolved

kxkxk commented Jan 10, 2023

View reviewed changes

kxkxk requested a review from ivancich January 29, 2023 06:52

ivancich approved these changes Jan 30, 2023

View reviewed changes

ivancich added needs-qa feature labels Jan 30, 2023

kxkxk commented Jan 31, 2023

View reviewed changes

ivancich added the DNM label Jan 31, 2023

kxkxk force-pushed the fix-delete-marker branch from 124ad2a to 0d6787d Compare January 31, 2023 09:46

fix delete_marker bug

d0193ac

kxkxk force-pushed the fix-delete-marker branch from 0d6787d to d0193ac Compare January 31, 2023 10:00

ivancich removed the DNM label Jan 31, 2023

kxkxk commented Feb 16, 2023

View reviewed changes

cbodley added pending-discussion and removed needs-qa labels Mar 13, 2023

Merge branch 'ceph:main' into fix-delete-marker

b5f9cce

kxkxk requested a review from a team as a code owner March 13, 2023 15:50

github-actions bot added the stale label Jul 5, 2023

github-actions bot closed this Aug 4, 2023

Conversation

kxkxk commented Oct 8, 2022

Checklist

Uh oh!

Uh oh!

Uh oh!

ivancich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kxkxk left a comment

Choose a reason for hiding this comment

Uh oh!

ivancich left a comment

Choose a reason for hiding this comment

Uh oh!

ivancich Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

kxkxk Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kxkxk left a comment

Choose a reason for hiding this comment

Uh oh!

ivancich commented Jan 31, 2023

Uh oh!

kxkxk commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivancich commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivancich commented Jan 31, 2023

Uh oh!

kxkxk commented Feb 1, 2023

Uh oh!

kxkxk left a comment

Choose a reason for hiding this comment

Uh oh!

cbodley commented Mar 13, 2023

Uh oh!

kxkxk commented Mar 13, 2023

Uh oh!

NarrativeBias commented May 6, 2023

Uh oh!

github-actions bot commented Jul 5, 2023

Uh oh!

github-actions bot commented Aug 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kxkxk commented Jan 31, 2023 •

edited

Loading

ivancich commented Jan 31, 2023 •

edited

Loading