cls/rgw: fix rgw list-object-versions key-marker behavior to match s3 by samakshd · Pull Request #60068 · ceph/ceph

samakshd · 2024-10-01T01:26:12Z

The key-marker parameter in the list-object-versions operation is inclusive in RGW, while it's exclusive in S3. This commit updates RGW's behavior to make the key-marker parameter exclusive, ensuring consistent behavior between RGW and S3.

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

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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
jenkins test rook e2e

cbodley · 2024-10-01T13:03:09Z

thanks @samakshd. we have a lot of test cases for ListObjects and ListObjectVersions in https://github.com/ceph/s3-tests. can you please run those to make sure you haven't broken anything there? you can find instructions in the repo's README

we'll also want to add a new test case for this specific issue, since it wasn't caught before

samakshd · 2024-10-01T13:06:49Z

thanks @samakshd. we have a lot of test cases for ListObjects and ListObjectVersions in https://github.com/ceph/s3-tests. can you please run those to make sure you haven't broken anything there? you can find instructions in the repo's README

we'll also want to add a new test case for this specific issue, since it wasn't caught before

Sure, will do that. Although, I see that ceph API tests is failing during CI, is that expected?

cbodley · 2024-10-01T13:11:34Z

Although, I see that ceph API tests is failing during CI, is that expected?

no worries, 'ceph API tests' and 'make check' can be flaky. we'll get them green when this is ready to merge

adamemerson · 2024-10-01T20:27:23Z

jenkins test api

samakshd · 2024-10-04T07:25:05Z

Hi @cbodley @adamemerson , I have executed the S3-tests on the PR branch using following way:
$ cd build/
$ RGW=1 ../src/vstart.sh -n
$ ../qa/workunits/rgw/run-s3tests.sh

This internally runs the test using following command:
S3TEST_CONF=s3tests.conf.SAMPLE tox -- -m "not fails_on_rgw and not sse_s3 and not lifecycle_expiration and not test_of_sts and not webidentity_test" -v

Attaching the screenshot of result

Please let me know if this is the expected result or not.
Thanks.

cbodley · 2024-10-04T13:17:34Z

1 xfailed

@samakshd i don't think any should fail. can you tell which test case that was? the output should say above the DeprecationWarning stuff

samakshd · 2024-10-04T13:23:29Z

1 xfailed

@samakshd i don't think any should fail. can you tell which test case that was? the output should say above the DeprecationWarning stuff

Here is the corresponding log

s3tests/functional/test_s3_website.py::test_routing_generator XFAIL ([NOTRUN] yield tests were removed in pytest 4.0 - test_routing_generator will be ignored)

samakshd · 2024-10-04T18:33:09Z

It's giving same error on main branch also though @cbodley

…s with key marker The key-marker parameter in the list-object-versions operation is inclusive in RGW, while it's exclusive in S3. This commit adds the test to check the corresponding case. Issue ref: https://tracker.ceph.com/issues/68055 Fix PR: ceph/ceph#60068 Signed-off-by: Samaksh Dhingra <[email protected]>

samakshd · 2024-10-09T21:44:36Z

Hi @cbodley, I have added the corresponding test as you asked. I have tested it by building both main and my PR branch. It is giving expected results. Please let me know if any changes are required or any other tests have to be added. Thanks.

s3-tests PR: ceph/s3-tests#594

The key-marker parameter in the list-object-versions operation is inclusive in RGW, while it's exclusive in S3. This commit updates RGW's behavior to make the key-marker parameter exclusive, ensuring consistent behavior between RGW and S3. Fixes: https://tracker.ceph.com/issues/68055 Signed-off-by: Samaksh Dhingra <[email protected]>

…match s3 While testing on previous fix, I realized that it was not correct, hence corrected it now. Signed-off-by: Samaksh Dhingra <[email protected]>

samakshd · 2024-10-12T20:21:16Z

@cbodley I have created another PR with backup branch, in case the mess I created in this one is irreversible. Extremely sorry for creating such mess, didn't know that merge or rebase is not required after each commit, I thought since the main branch would have got stale, it would be better to rebase/merge the main and then push the commit.

Link for alt PR: #60280

FYI, I have run the S3 tests on the alt PR as well, and it they passed same as above with 1 xfailed (which I researched is an expected fail I guess with some library got deprecated as I mentioned above). I have added the tests for this bug as well with key marker and version id marker in my s3-tests PR. Please let me know if anything else is required from my side.

Thanks.

github-actions · 2024-12-13T11:01:37Z

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.

…s with key marker The key-marker parameter in the list-object-versions operation is inclusive in RGW, while it's exclusive in S3. This commit adds the test to check the corresponding case. Issue ref: https://tracker.ceph.com/issues/68055 Fix PR: ceph/ceph#60068 Signed-off-by: Samaksh Dhingra <[email protected]>

cbodley · 2024-12-13T15:26:29Z

@cbodley I have created another PR with backup branch, in case the mess I created in this one is irreversible. Extremely sorry for creating such mess, didn't know that merge or rebase is not required after each commit, I thought since the main branch would have got stale, it would be better to rebase/merge the main and then push the commit.

Link for alt PR: #60280

thanks, closing this one

samakshd requested a review from a team as a code owner October 1, 2024 01:26

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

meteorgan mentioned this pull request Oct 2, 2024

Tracking Issue: Ceph Rados ListObejctVersions is not compatible with S3 apache/opendal#5118

Open

ivancich added the bug-fix label Oct 3, 2024

samakshd mentioned this pull request Oct 9, 2024

s3tests_boto3/functional/test_s3.py: add test for list object versions with key marker ceph/s3-tests#594

Open

samakshd requested review from a team as code owners October 11, 2024 19:00

samakshd requested review from cloudbehl and ivoalmeida and removed request for a team October 11, 2024 19:00

github-actions bot added api-change build/ops cephadm cephfs Ceph File System common core labels Oct 11, 2024

github-actions bot added crimson dashboard documentation nfs pybind rbd tests labels Oct 11, 2024

samakshd force-pushed the list_object_versions_fix branch 2 times, most recently from 4888afc to 602a681 Compare October 11, 2024 19:40

Samaksh Dhingra and others added 2 commits October 11, 2024 19:46

cls/rgw: Correct fix for list-object-versions key-marker behavior to …

27e8468

…match s3 While testing on previous fix, I realized that it was not correct, hence corrected it now. Signed-off-by: Samaksh Dhingra <[email protected]>

Merge branch 'ceph:main' into list_object_versions_fix

cc243e2

idryomov removed the request for review from a team October 14, 2024 07:58

idryomov removed the rbd label Oct 14, 2024

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

cbodley closed this Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

cls/rgw: fix rgw list-object-versions key-marker behavior to match s3#60068

cls/rgw: fix rgw list-object-versions key-marker behavior to match s3#60068
samakshd wants to merge 3 commits intoceph:mainfrom
samakshd:list_object_versions_fix

samakshd commented Oct 1, 2024

Uh oh!

cbodley commented Oct 1, 2024

Uh oh!

samakshd commented Oct 1, 2024

Uh oh!

cbodley commented Oct 1, 2024

Uh oh!

adamemerson commented Oct 1, 2024

Uh oh!

samakshd commented Oct 4, 2024 •

edited

Loading

Uh oh!

cbodley commented Oct 4, 2024

Uh oh!

samakshd commented Oct 4, 2024

Uh oh!

samakshd commented Oct 4, 2024

Uh oh!

samakshd commented Oct 9, 2024

Uh oh!

samakshd commented Oct 12, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Dec 13, 2024

Uh oh!

cbodley commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

Conversation

samakshd commented Oct 1, 2024

Contribution Guidelines

Checklist

Uh oh!

cbodley commented Oct 1, 2024

Uh oh!

samakshd commented Oct 1, 2024

Uh oh!

cbodley commented Oct 1, 2024

Uh oh!

adamemerson commented Oct 1, 2024

Uh oh!

samakshd commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbodley commented Oct 4, 2024

Uh oh!

samakshd commented Oct 4, 2024

Uh oh!

samakshd commented Oct 4, 2024

Uh oh!

samakshd commented Oct 9, 2024

Uh oh!

samakshd commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 13, 2024

Uh oh!

cbodley commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samakshd commented Oct 4, 2024 •

edited

Loading

samakshd commented Oct 12, 2024 •

edited

Loading