crimson/osd/osd_operations/client_requests: we don't support rados locator keys by xxhdx1985126 · Pull Request #56025 · ceph/ceph

xxhdx1985126 · 2024-03-07T08:27:49Z

Fixes: https://tracker.ceph.com/issues/64782
Signed-off-by: Xuehan Xu xuxuehan@qianxin.com

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

cyx1231st · 2024-03-08T09:09:33Z

I'm not quite following the purpose of the object-locator-key, and not sure why it was initially missing during the design of onode key for Crimson (is it intentional or not).

From https://tracker.ceph.com/projects/ceph/wiki/Rados_namespaces, is it an optional abstraction layer between namespace and oid? Should Crimson support it as well?

xxhdx1985126 · 2024-03-11T08:58:20Z

I'm not quite following the purpose of the object-locator-key, and not sure why it was initially missing during the design of onode key for Crimson (is it intentional or not).

From https://tracker.ceph.com/projects/ceph/wiki/Rados_namespaces, is it an optional abstraction layer between namespace and oid? Should Crimson support it as well?

RADOS maps objects to pgs by the objects' locator keys, by default, the key is empty and the object name is treated as the key.

cyx1231st · 2024-03-12T01:49:59Z

RADOS maps objects to pgs by the objects' locator keys, by default, the key is empty and the object name is treated as the key.

It seems to me that we can replace/reuse the oid field by the locator key if the locator is valid, and store the oid in the onode value in that case. So there is no need to add another string field in the onode key?

xxhdx1985126 · 2024-03-12T02:04:22Z

RADOS maps objects to pgs by the objects' locator keys, by default, the key is empty and the object name is treated as the key.

It seems to me that we can replace/reuse the oid field by the locator key if the locator is valid, and store the oid in the onode value in that case. So there is no need to add another string field in the onode key?

Yeah, will change it.

xxhdx1985126 · 2024-03-14T06:29:34Z

@cyx1231st Please take a look again, thanks:-)

cyx1231st

Initial comments.

cyx1231st · 2024-03-15T08:17:28Z

src/crimson/os/seastore/onode_manager/staged-fltree/stages/key_layout.h

+  }
+  string_view_masked_t key_masked() const {
+    // TODO(cross-node string dedup)
+    return oid_masked();


It doesn't seem that we need both key() and oid() here, and the same to some other places around the onode index changes.

IIUC with this PR, the oid field of the onode tree index is replaced by the concept of effective_key of hobject_t, and there is no need to distingush key and oid from onode indexing perspective.

And if that can be aligned, how about splitting out the renaming efforts (oid -> effective_key) as a separate commit?

But in the onode deltas, I think we need to remenber both oid and key, which is why I kept the oid() method

But in the onode deltas, I think we need to remenber both oid and key

Can you elaborate more about why need to remember both?

cyx1231st · 2024-03-15T08:19:01Z

src/crimson/os/seastore/onode_manager/staged-fltree/stages/key_layout.h

    gen_t gen;
    ceph::decode(gen, delta);
-    return key_hobj_t(ghobject_t(
-        shard_id_t(shard), pool, crush, nspace, oid, snap, gen));


It should be fine to adjust the constructor of ghobject_t directly. IIUC it is the only user of the specific constructor.

cyx1231st · 2024-03-15T08:28:24Z

src/crimson/os/seastore/onode_manager/staged-fltree/fltree_onode_manager.cc

+	onode->create_default_layout(trans, hoid.hobj.oid.name);
+      } else {
+	onode->create_default_layout(trans, key);
+      }


Is it correct that the onode value needs to store the oid only if the locator is valid (!key.empty()), otherwise, there is no need to store or even reserve the value space?

Is it correct that the onode value needs to store the oid only if the locator is valid (!key.empty()), otherwise, there is no need to store or even reserve the value space?

Yes, but I think the size of onode_layout_t has to be a constant value, is this right?

I suppose the reason why we were not aware of the issue is because that it's rare key.empty() is false, so we mostly don't need the space to store the oid in the value.

Yes, but I think the size of onode_layout_t has to be a constant value, is this right?

I think we might be able to store it in xattr omap with a special internal-only label. So oid becomes really optional. What do you think?

I think we might be able to store it in xattr omap with a special internal-only label. So oid becomes really optional. What do you think?

I think we need a way to determine whether the "oid" field in the key is actually the oid or locator key, at least when listing objects, so that we can reconstruct the ghobject_t correctly. If we put oid in xattr, we'll need to search the omap tree for each of the objects that are listed. This seems to be a potential performance issue.

cyx1231st · 2024-03-18T02:09:26Z

From @athanatos's feedback, there is no user of the locator field, probably we should:

return -ENOTSUPP for any incoming IO with a locator set in crimson-osd
skip this test for crimson

Quoted:

Unfortunately, supporting locator correctly would require effective-key in addition to oid -> (shard, pool, reversed-hash, ns, effective-key, oid, snap, gen). However, there are no users of locator that I'm aware of at all. Once upon a time, it was used to ensure that multipart-upload parts would end up in the same PG so that they could be clone_range'd into the same object via librados, but that's not how multipart upload works anymore and we no longer support clone_range via librados.

We probably ought to remove support for it from librados entirely.

xxhdx1985126 · 2024-03-18T02:27:05Z

From @athanatos's feedback, there is no user of the locator field, probably we should:

return -ENOTSUPP for any incoming IO with a locator set in crimson-osd

skip this test for crimson

Quoted:

Unfortunately, supporting locator correctly would require effective-key in addition to oid -> (shard, pool, reversed-hash, ns, effective-key, oid, snap, gen). However, there are no users of locator that I'm aware of at all. Once upon a time, it was used to ensure that multipart-upload parts would end up in the same PG so that they could be clone_range'd into the same object via librados, but that's not how multipart upload works anymore and we no longer support clone_range via librados.
We probably ought to remove support for it from librados entirely.

Yeah, will do.

xxhdx1985126 · 2024-03-18T06:46:42Z

@cyx1231st Done, please take a look again, thanks.

src/test/pybind/test_rados.py

cyx1231st · 2024-03-18T07:20:22Z

src/crimson/osd/osd_operations/client_request.cc

+  // TODO: gate the crosscore sending
+  return get_foreign_connection().send_with_throttling(std::move(reply));
+}
+


Is this a meanless change?

No, reply_op_error() returns deduced type, so it has to be defined before used. So I moved its definition before with_pg_int()

nit: I think that returning seastar::future<> is more readable and avoids the issue you encountered.

src/crimson/osd/osd_operations/client_request.cc

xxhdx1985126 · 2024-04-02T03:20:03Z

jenkins test make check

Matan-B · 2024-04-02T06:33:13Z

https://shaman.ceph.com/builds/ceph/wip-matanb-crimson-testing-apr-2/

xxhdx1985126 · 2024-04-07T10:17:30Z

jenkins test make check

xxhdx1985126 · 2024-04-10T02:38:56Z

The following tests FAILED:
22 - run-tox-cephadm (Failed)

jenkins test make check

xxhdx1985126 · 2024-04-10T02:39:12Z

jenkins test make check

xxhdx1985126 · 2024-04-10T03:06:04Z

@Matan-B I think the issue should have been fixed, please test it again, thanks:-)

Matan-B · 2024-04-10T11:26:09Z

@Matan-B I think the issue should have been fixed, please test it again, thanks:-)

https://shaman.ceph.com/builds/ceph/wip-matanb-crimson-testing-april-17

Matan-B

https://pulpito.ceph.com/matan-2024-04-21_07:41:30-crimson-rados-wip-matanb-crimson-only-testing-april-17-distro-crimson-smithi/7666494

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:=================================== FAILURES ===================================
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:____________________________ TestIoctx.test_locator ____________________________

xxhdx1985126 · 2024-04-21T09:48:12Z

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:=================================== FAILURES ===================================
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:____________________________ TestIoctx.test_locator ____________________________
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:self = <test_rados.TestIoctx object at 0x7f170bb6b8b0>
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:../../../clone.client.0/src/test/pybind/test_rados.py:719:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:

Seems that the test branch lacks this PR's commit b3754ac

xxhdx1985126 · 2024-04-21T09:50:22Z

jenkins test make check

Matan-B · 2024-04-21T10:34:10Z

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:=================================== FAILURES ===================================
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:____________________________ TestIoctx.test_locator ____________________________
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:self = <test_rados.TestIoctx object at 0x7f170bb6b8b0>
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:../../../clone.client.0/src/test/pybind/test_rados.py:719:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:

Seems that the test branch lacks this PR's commit b3754ac

I think that it's there, please see:
https://github.com/ceph/ceph-ci/commits/wip-matanb-crimson-only-testing-april-17/

xxhdx1985126 · 2024-04-21T11:51:59Z

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:=================================== FAILURES ===================================
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:____________________________ TestIoctx.test_locator ____________________________
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:self = <test_rados.TestIoctx object at 0x7f170bb6b8b0>
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:../../../clone.client.0/src/test/pybind/test_rados.py:719:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:

Seems that the test branch lacks this PR's commit b3754ac

I think that it's there, please see: https://github.com/ceph/ceph-ci/commits/wip-matanb-crimson-only-testing-april-17/

I don't quite understand, but, looking at the error log above, the code doesn't have the change of commit b3754ac

Matan-B · 2024-04-21T12:00:20Z

Seems that the test branch lacks this PR's commit b3754ac

I think that it's there, please see: https://github.com/ceph/ceph-ci/commits/wip-matanb-crimson-only-testing-april-17/

I don't quite understand, but, looking at the error log above, the code doesn't have the change of commit b3754ac

https://qa-proxy.ceph.com/teuthology/matan-2024-04-21_07:41:30-crimson-rados-wip-matanb-crimson-only-testing-april-17-distro-crimson-smithi/7666494/teuthology.log

From the log:

2024-04-21T07:53:48.724 INFO:teuthology.packaging:sha1: b52414e0249b7db43b05804d90a0d3f7a8047037

The sha1 is represents this branch: https://github.com/ceph/ceph-ci/commits/wip-matanb-crimson-only-testing-april-17/
which includes ceph/ceph-ci@1e8f16a (cherry-pick of b3754ac).

xxhdx1985126 · 2024-04-21T12:44:31Z

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')

I know, but the code observed (shown below) in the log is wrong, maybe later commits overwrote this part of the code:

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')

Matan-B · 2024-04-21T12:47:30Z

2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')

I know, but the code observed (shown below) in the log is wrong, maybe later commits overwrote this part of the code:
2024-04-21T08:04:59.490 INFO:tasks.workunit.client.0.smithi186.stdout:
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: def test_locator(self):
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout: self.ioctx.set_locator_key("bar")
2024-04-21T08:04:59.491 INFO:tasks.workunit.client.0.smithi186.stdout:> self.ioctx.write('foo', b'contents1')

2024-04-21T08:00:18.769 INFO:tasks.workunit.client.0.smithi186.stderr:+ python3 -m pytest -v /home/ubuntu/cephtest/clone.client.0/qa/workunits/rados/../../../src/test/pybind/test_rados.py -m 'not (wait or tier or ec or bench or stats)'

Found the issue, I was testing against main and I should have tested against the PR as a suite. Retesting.

https://pulpito.ceph.com/matan-2024-04-21_12:49:32-crimson-rados-wip-matanb-crimson-only-testing-april-17-distro-crimson-smithi/

xxhdx1985126 requested a review from cyx1231st March 7, 2024 08:27

xxhdx1985126 requested a review from a team as a code owner March 7, 2024 08:27

github-actions bot added crimson tests labels Mar 7, 2024

xxhdx1985126 force-pushed the wip-seastore-onode-loc-key branch from 5b5f330 to c35b95b Compare March 7, 2024 08:42

cyx1231st requested a review from athanatos March 8, 2024 09:09

xxhdx1985126 mentioned this pull request Mar 13, 2024

qa/suites/crimson-rados: add seastore #56150

Merged

14 tasks

xxhdx1985126 force-pushed the wip-seastore-onode-loc-key branch from c35b95b to 6eb0778 Compare March 14, 2024 06:24

cyx1231st reviewed Mar 15, 2024

View reviewed changes

xxhdx1985126 force-pushed the wip-seastore-onode-loc-key branch from 6eb0778 to 0f71bf1 Compare March 18, 2024 06:45

xxhdx1985126 changed the title ~~crimson/os/seastore/onode_manager: store objects' location key in the onode tree~~ crimson/osd/osd_operations/client_requests: we don't support rados locator keys Mar 18, 2024

cyx1231st reviewed Mar 18, 2024

View reviewed changes

xxhdx1985126 force-pushed the wip-seastore-onode-loc-key branch from 0f71bf1 to 6be84df Compare March 18, 2024 09:12

athanatos approved these changes Mar 18, 2024

View reviewed changes

liu-chunmei approved these changes Mar 18, 2024

View reviewed changes

cyx1231st approved these changes Mar 19, 2024

View reviewed changes

cyx1231st added the needs-crimson-qa label Mar 19, 2024

Matan-B added the wip-matan-testing label Mar 25, 2024

github-actions bot added the core label Apr 2, 2024

xxhdx1985126 added the DNM label Apr 3, 2024

xxhdx1985126 force-pushed the wip-seastore-onode-loc-key branch from 2c1975e to 776c2b3 Compare April 7, 2024 06:29

github-actions bot removed the needs-rebase label Apr 7, 2024

Matan-B requested changes Apr 21, 2024

View reviewed changes

Matan-B removed wip-matan-testing needs-crimson-qa labels Apr 21, 2024

Matan-B added needs-crimson-qa and removed needs-crimson-qa labels Apr 21, 2024

Matan-B added the wip-matan-testing label Apr 21, 2024

Matan-B approved these changes Apr 21, 2024

View reviewed changes

Matan-B merged commit 226cba1 into ceph:main Apr 21, 2024

xxhdx1985126 deleted the wip-seastore-onode-loc-key branch April 23, 2024 02:53

Matan-B mentioned this pull request May 21, 2024

squid: crimson/osd/osd_operations/client_requests: we don't support rados locator keys #57597

Merged

Conversation

xxhdx1985126 commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

cyx1231st commented Mar 8, 2024

Uh oh!

xxhdx1985126 commented Mar 11, 2024

Uh oh!

cyx1231st commented Mar 12, 2024

Uh oh!

xxhdx1985126 commented Mar 12, 2024

Uh oh!

xxhdx1985126 commented Mar 14, 2024

Uh oh!

cyx1231st left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyx1231st Mar 15, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

cyx1231st commented Mar 18, 2024

Uh oh!

xxhdx1985126 commented Mar 18, 2024

Uh oh!

xxhdx1985126 commented Mar 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xxhdx1985126 commented Apr 2, 2024

Uh oh!

Matan-B commented Apr 2, 2024

Uh oh!

xxhdx1985126 commented Apr 7, 2024

Uh oh!

xxhdx1985126 commented Apr 10, 2024

Uh oh!

xxhdx1985126 commented Apr 10, 2024

Uh oh!

xxhdx1985126 commented Apr 10, 2024

Uh oh!

Matan-B commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

xxhdx1985126 commented Apr 21, 2024

Uh oh!

xxhdx1985126 commented Apr 21, 2024

Uh oh!

Matan-B commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xxhdx1985126 commented Apr 21, 2024

Uh oh!

Matan-B commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

xxhdx1985126 commented Mar 7, 2024 •

edited

Loading

cyx1231st Mar 15, 2024 •

edited

Loading

Matan-B commented Apr 10, 2024 •

edited

Loading

Matan-B commented Apr 21, 2024 •

edited

Loading

Matan-B commented Apr 21, 2024 •

edited

Loading

Matan-B commented Apr 21, 2024 •

edited

Loading