crimson/osd: put snapmapper's key-value pairs into dedicated objs #52267

xxhdx1985126 · 2023-06-30T04:35:57Z

Otherwise, PG::read_log_and_missing() will meet those key-values and won't know what to do with them. This is modeling what the classic osd is doing

Fixes: https://tracker.ceph.com/issues/61875
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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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

Matan-B · 2023-07-02T07:39:19Z

src/crimson/osd/pg.cc

+  assert(coll_ref);
+  t.touch(coll_ref->get_cid(), make_snapmapper_oid());


Why is this section not part of OSD::start() instead?

We should also check for existence and add logs if possible.

// make sure snap mapper object exists if (!store->exists(coll_ref, make_snapmapper_oid())) { logger().debug("init creating/touching snapmapper object"); .. t.touch(coll_ref->get_cid(), make_snapmapper_oid()); }

Since, in crimson, we previously store snap mapper in pgmeta objects which are pg specific, I made the snap mapper objects pg specific, too. I think you are suggesting that we make the snapmapper object shared across the whole OSD like what classic OSD is doing. Am I right?

Hmm. The reason classic places all the snap trimmer information in a single object is that otherwise we'd need to read and split/merge the objects during pg split/merge. This would require IO linear in the number of snapshots, and split/merge aren't otherwise dependent on the size of the PGs (for good reason -- they must occur during the map transition and block IO/peering on those PGs). Crimson will have different problems for split/merge -- specifically that seastore will want to maintain per-shard metadata trees, so we won't be able to merge PGs easily on different shards or split into a different shard. For now, I'd actually suggest a single OSD-wide object maintained by shard 0 (pgs on other shards will have to proxy IO to that shard). If this becomes a bottleneck, we can try something else later.

Hmm. The reason classic places all the snap trimmer information in a single object is that otherwise we'd need to read and split/merge the objects during pg split/merge. This would require IO linear in the number of snapshots, and split/merge aren't otherwise dependent on the size of the PGs (for good reason -- they must occur during the map transition and block IO/peering on those PGs). Crimson will have different problems for split/merge -- specifically that seastore will want to maintain per-shard metadata trees, so we won't be able to merge PGs easily on different shards or split into a different shard. For now, I'd actually suggest a single OSD-wide object maintained by shard 0 (pgs on other shards will have to proxy IO to that shard). If this becomes a bottleneck, we can try something else later.

@athanatos Hi, sam. I think the modification to the OSD-wide object and other modifications have to be in the same transaction, am I right? If so, it looks to me that we need to synchronize seastore transactions across multiple shards if they all modify the OSD-wide object, am I right?

Yeah, I forgot that the sharded objectstore abstraction doesn't allow transactions that span shards. A per-pg object seems fine, we'll figure out pg split/merge later.

Yeah, I forgot that the sharded objectstore abstraction doesn't allow transactions that span shards. A per-pg object seems fine, we'll figure out pg split/merge later.

Done:-)

Matan-B · 2023-07-02T07:51:42Z

src/crimson/osd/pg.h

+  ghobject_t make_snapmapper_oid() {
+    return ghobject_t(hobject_t(
+      sobject_t(
+       object_t("snapmapper"),
+       0)));
+  }


Let's move this to crimson/osd/osd_meta.cc where we already create the osdmap/superblock/final_pool_info oids.
This will require introducing void OSDMeta::create_snapmapper.

Since we are no longer using OSD-wide snapmapper object, this may not be necessary, am I right?

Matan-B · 2023-07-02T07:59:54Z

Can you please open a tracker so we could easily backport this?

xxhdx1985126 · 2023-07-03T02:02:11Z

Can you please open a tracker so we could easily backport this?

Yeah, will do.

xxhdx1985126 · 2023-07-18T06:09:32Z

jenkins test make check

xxhdx1985126 · 2023-07-26T03:57:36Z

jenkins test make check

Matan-B · 2023-08-24T09:42:13Z

crimson/admin/admin_socket: remove path file if it exists #53082
osd/PGLog: return a ready future when found missing objects #53032
crimson/osd/pg: check if backfill_state exists when judging objects' #53031
crimson/osd: put snapmapper's key-value pairs into dedicated objs #52267
qa/suites/crimson-rados: bring backfill testing #48052
crimson/osd/object_context: consider clones found as long as they're in SnapSet::clones #52204

https://pulpito.ceph.com/matan-2023-08-24_09:25:19-crimson-rados-wip-matanb-crimson-testing-24.8-v2-distro-crimson-smithi/

Matan-B

I suspect this PR is breaking our tests:

With this PR:
https://pulpito.ceph.com/matan-2023-08-24_09:25:19-crimson-rados-wip-matanb-crimson-testing-24.8-v2-distro-crimson-smithi/

Without:
https://pulpito.ceph.com/matan-2023-08-24_14:53:35-crimson-rados-wip-matanb-crimson-testing-24.8-v3-distro-crimson-smithi/

xxhdx1985126 · 2023-08-28T02:57:24Z

I suspect this PR is breaking our tests:

With this PR: https://pulpito.ceph.com/matan-2023-08-24_09:25:19-crimson-rados-wip-matanb-crimson-testing-24.8-v2-distro-crimson-smithi/

Without: https://pulpito.ceph.com/matan-2023-08-24_14:53:35-crimson-rados-wip-matanb-crimson-testing-24.8-v3-distro-crimson-smithi/

Looking into it:-)

xxhdx1985126 · 2023-08-28T03:05:54Z

@Matan-B It seems that qa-proxy.ceph.com doesn't work anymore, is there any other way to get the log? Thanks:-)

Matan-B · 2023-08-28T14:00:52Z

@Matan-B It seems that qa-proxy.ceph.com doesn't work anymore, is there any other way to get the log? Thanks:-)

It's back :)

The traceback from osds:

 0# gsignal in /lib64/libc.so.6
 1# abort in /lib64/libc.so.6
 2# 0x00007F7FD9B54D79 in /lib64/libc.so.6
 3# 0x00007F7FD9B7A426 in /lib64/libc.so.6
 4# seastar::sharded<crimson::osd::OSDSingletonState>::local() in ceph-osd
 5# 0x0000555740BCC5AF in ceph-osd
 6# 0x0000555740BD0886 in ceph-osd
 7# 0x0000555740BD0AFE in ceph-osd
 8# auto seastar::internal::future_invoke<seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&, std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >(seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&, std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&) in ceph-osd
 9# void seastar::futurize<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > >::satisfy_with_result_of<seastar::future<std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >::then_impl_nrvo<seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>, seastar::future<boost::intrusive_ptr<crimson::osd::PG> > >(seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&&)::{lambda(seastar::internal::promise_base_with_type<boost::intrusive_ptr<crimson::osd::PG> >&&, seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&, seastar::future_state<std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >&&)#1}::operator()(seastar::internal::promise_base_with_type<boost::intrusive_ptr<crimson::osd::PG> >&&, seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&, seastar::future_state<std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >&&) const::{lambda()#1}>(seastar::internal::promise_base_with_type<boost::intrusive_ptr<crimson::osd::PG> >&&, seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&&) in ceph-osd
10# seastar::continuation<seastar::internal::promise_base_with_type<boost::intrusive_ptr<crimson::osd::PG> >, seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>, seastar::future<std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >::then_impl_nrvo<seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>, seastar::future<boost::intrusive_ptr<crimson::osd::PG> > >(seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&&)::{lambda(seastar::internal::promise_base_with_type<boost::intrusive_ptr<crimson::osd::PG> >&&, seastar::noncopyable_function<seastar::future<boost::intrusive_ptr<crimson::osd::PG> > (std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > >&&)>&, seastar::future_state<std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >&&)#1}, std::tuple<seastar::future<std::tuple<pg_pool_t, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > > >, seastar::future<boost::intrusive_ptr<crimson::os::FuturizedCollection> > > >::run_and_dispose() in ceph-osd
11# 0x000055574E58B2A1 in ceph-osd

xxhdx1985126 · 2023-08-29T02:14:09Z

@Matan-B It seems that qa-proxy.ceph.com doesn't work anymore, is there any other way to get the log? Thanks:-)

It's back :)

I got "403 Forbidden" when accessing qa-proxy.ceph.com; what should I do? Thanks:-)

Matan-B · 2023-08-29T06:56:09Z

I got "403 Forbidden" when accessing qa-proxy.ceph.com; what should I do? Thanks:-)

Same for me, you can join the slack sepia channel for updates.

github-actions · 2023-12-06T10:01:30Z

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.

xxhdx1985126 · 2023-12-29T01:33:15Z

@Matan-B @athanatos Could you take a final look at this PR, please? Thansk:-)

Matan-B

LGTM, two nits from earlier discussions:

crimson/osd: put snapmapper's key-value pairs into dedicated objs #52267 (comment)
crimson/osd: put snapmapper's key-value pairs into dedicated objs #52267 (comment)

Matan-B · 2024-01-30T08:39:45Z

https://shaman.ceph.com/builds/ceph/wip-crimson-pg-snapmapper-obj-rebased

xxhdx1985126 · 2024-01-31T02:49:55Z

@Matan-B Hi, I've just fixed our testbed, and it seems that there's still problem with this PR, I'm looking into it. Sorry for this inconvenience.

xxhdx1985126 · 2024-02-19T01:51:16Z

@Matan-B @athanatos this PR has passed our teuthology test, here's the results:
results.log

athanatos · 2024-02-19T19:46:48Z

Pushed with wip-sjust-crimson-testing-2024-02-19

athanatos · 2024-02-20T02:00:22Z

http://pulpito.ceph.com/sjust-2024-02-20_01:50:29-crimson-rados-wip-sjust-crimson-testing-2024-02-19-distro-default-smithi/

xxhdx1985126 · 2024-02-20T02:03:09Z

@Matan-B @athanatos this PR has passed our teuthology test, here's the results: results.log

Please ignore this, I was testing the wrong build.

athanatos · 2024-02-20T02:06:48Z

@xxhdx1985126 Gotcha, the test run above may nonetheless be sufficient to merge once it completes.

Matan-B · 2024-02-20T08:22:18Z

Tests generally look ok although this looks odd:

2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjects
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsZeroInName
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsNS
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsStart
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsCursor
2024-02-20T02:14:52.053 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.EnumerateObjects
2024-02-20T02:14:52.053 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.EnumerateObjectsSplit
...

2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_list_objects_empty
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_list_objects
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_list_ns_objects
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_locator
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_write
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_write_no_comp_ref
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_append
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_write_full
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_writesame
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_stat
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_remove
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_execute

Haven't seen that before, I'll test main for comparison. @xxhdx1985126 Can you run these unit tests locally if you have this build? Thanks!

https://pulpito.ceph.com/matan-2024-02-20_08:26:01-crimson-rados-main-distro-crimson-smithi/

xxhdx1985126 · 2024-02-20T08:26:55Z

Tests generally look ok although this looks odd:

2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjects
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsZeroInName
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsNS
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsStart
2024-02-20T02:14:52.052 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.ListObjectsCursor
2024-02-20T02:14:52.053 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.EnumerateObjects
2024-02-20T02:14:52.053 INFO:tasks.workunit.client.0.smithi125.stdout:[  FAILED  ] LibRadosList.EnumerateObjectsSplit
...

2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_list_objects_empty
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_list_objects
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_list_ns_objects
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_locator
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_write
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_write_no_comp_ref
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_append
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_write_full
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_writesame
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_stat
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_remove
2024-02-20T02:17:48.956 INFO:tasks.workunit.client.0.smithi027.stdout:FAILED ../../../clone.client.0/src/test/pybind/test_rados.py::TestIoctx::test_aio_execute

Haven't seen that before, I'll test main for comparison. @xxhdx1985126 Can you run these unit tests locally if you have this build? Thanks!

https://pulpito.ceph.com/matan-2024-02-20_08:26:01-crimson-rados-main-distro-crimson-smithi/

Yeah, will try.

Matan-B · 2024-02-20T11:12:09Z

main looks clean, what were the local results?

xxhdx1985126 · 2024-02-20T12:16:27Z

I have the same error, will look into it.

xxhdx1985126 · 2024-02-20T12:25:52Z

2024-02-20T12:15:30.594 INFO:tasks.workunit.client.0.scephqa03.stdout:/da1/xxh/rpmbuild/BUILD/ceph-19.0.0-1585-g1dd88d8ee6b/src/test/librados/list.cc:38: Failure
2024-02-20T12:15:30.595 INFO:tasks.workunit.client.0.scephqa03.stdout:Expected equality of these values:
2024-02-20T12:15:30.595 INFO:tasks.workunit.client.0.scephqa03.stdout: std::string(entry)
2024-02-20T12:15:30.595 INFO:tasks.workunit.client.0.scephqa03.stdout: Which is: "snapmapper"
2024-02-20T12:15:30.596 INFO:tasks.workunit.client.0.scephqa03.stdout: "foo"

~~It seems that the errors are by design. Since we are creating per-pg snapmapper objects, they are supposed to be listed, which is not what the librados unittests expect.~~

~~So it seems that it's the unittests that should be adjusted. Will try it:-)~~

It seems that the snapmapper object is exposed, will try to hide it.

athanatos · 2024-02-20T16:41:36Z

@xxhdx1985126 Right, librados users should only ever see librados objects, OSD internal objects shouldn't be exposed.

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 · 2024-02-21T06:34:52Z

2024-02-21T06:31:10.547 INFO:teuthology.run:Summary data:
description: crimson-rados/basic/{clusters/fixed-2 crimson-supported-all-distro/centos_latest
crimson_qa_overrides deploy/ceph tasks/rados_api_tests}
duration: 3245.61913895607
flavor: crimson
owner: scheduled_teuthology@cephqa09
success: true

2024-02-21T06:31:10.548 DEBUG:teuthology.report:Pushing job info to http://10.57.38.110:8080/
2024-02-21T06:31:10.651 INFO:teuthology.run:pass

@Matan-B @athanatos snapmapper objects have been hidden, and the test succeeded, please take a look again

Matan-B · 2024-02-21T09:16:35Z

src/crimson/osd/ops_executer.cc

    -> PG::interruptible_future<std::tuple<std::vector<hobject_t>, hobject_t>> {
      auto& [objects, next] = ret;
+      auto is_snapmapper = [&pg](const hobject_t &obj) {
+	if (obj == pg.make_snapmapper_oid().hobj) {


Instead of adding pg ref parameter we can introduce static bool PG::is_snapmapper_oid().

static inline bool is_snapmapper_oid(const hobject_t &obj) { return (obj.hobj.oid.name == "snapmapper"); }

Thanks, will schedule test after make check passes

Otherwise, PG::read_log_and_missing() will meet those key-values and won't know what to do with them. This is modeling what the classic osd is doing Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

Matan-B · 2024-02-21T09:52:54Z

src/crimson/osd/ops_executer.cc

@@ -1443,7 +1459,8 @@ static PG::interruptible_future<> do_pgls_filtered(
                            lower_bound,
                            nspace,
                            osd_op.op.pgls.count,
-                            filter.get())
+                            filter.get(),
+			    pg)


no longer needed

Ah, yes, sorry, I thought it wouldn't need a recompilation...

Matan-B · 2024-02-21T09:53:00Z

src/crimson/osd/ops_executer.cc

@@ -1399,7 +1414,8 @@ static PG::interruptible_future<> do_pgls(
 			lower_bound,
 			nspace,
 			osd_op.op.pgls.count,
-			nullptr /* no filter */)
+			nullptr /* no filter */,
+			pg)


Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

Matan-B · 2024-02-22T09:42:15Z

https://shaman.ceph.com/builds/ceph/wip-matanb-crimson-testing-snapmapper
https://pulpito.ceph.com/matan-2024-02-22_09:43:33-crimson-rados-wip-matanb-crimson-testing-snapmapper-distro-crimson-smithi
Rerunning:

https://shaman.ceph.com/builds/ceph/wip-matanb-crimson-pg-snapmapper-obj
https://pulpito.ceph.com/matan-2024-03-05_16:16:59-crimson-rados-wip-matanb-crimson-pg-snapmapper-obj-distro-crimson-smithi/

Matan-B · 2024-03-06T09:00:15Z

https://pulpito.ceph.com/matan-2024-03-05_16:16:59-crimson-rados-wip-matanb-crimson-pg-snapmapper-obj-distro-crimson-smithi/

Both failures: https://tracker.ceph.com/issues/64009

xxhdx1985126 requested review from athanatos and Matan-B June 30, 2023 04:35

xxhdx1985126 requested a review from a team as a code owner June 30, 2023 04:35

github-actions bot added the crimson label Jun 30, 2023

Matan-B reviewed Jul 2, 2023

View reviewed changes

xxhdx1985126 force-pushed the wip-crimson-pg-snapmapper-obj branch from 666ec48 to 3deab19 Compare July 18, 2023 02:17

athanatos approved these changes Jul 26, 2023

View reviewed changes

Matan-B added the needs-crimson-qa label Aug 22, 2023

Matan-B added the wip-matan-testing label Aug 24, 2023

Matan-B requested changes Aug 24, 2023

View reviewed changes

Matan-B removed wip-matan-testing needs-crimson-qa labels Aug 24, 2023

xxhdx1985126 force-pushed the wip-crimson-pg-snapmapper-obj branch from 3deab19 to f5f718e Compare October 7, 2023 09:55

github-actions bot added the stale label Dec 6, 2023

xxhdx1985126 removed the stale label Dec 29, 2023

Matan-B reviewed Dec 31, 2023

View reviewed changes

Matan-B self-requested a review December 31, 2023 10:42

Matan-B added the needs-crimson-qa label Dec 31, 2023

crimson/os: add FuturizedStore::Shard::exists() interface

6f0803a

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 force-pushed the wip-crimson-pg-snapmapper-obj branch from a91c509 to 3c808f3 Compare February 21, 2024 03:21

xxhdx1985126 force-pushed the wip-crimson-pg-snapmapper-obj branch from 3c808f3 to 32408e9 Compare February 21, 2024 06:37

Matan-B reviewed Feb 21, 2024

View reviewed changes

crimson/osd: put snapmapper's key-value pairs into dedicated objs

e56e908

Otherwise, PG::read_log_and_missing() will meet those key-values and won't know what to do with them. This is modeling what the classic osd is doing Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 force-pushed the wip-crimson-pg-snapmapper-obj branch 2 times, most recently from 81c45ac to d0cfc58 Compare February 21, 2024 09:51

Matan-B reviewed Feb 21, 2024

View reviewed changes

crimson/osd/pg: filter out snapmapper objects when doing pgls/pgnls

8f7921e

Signed-off-by: Xuehan Xu <xuxuehan@qianxin.com>

xxhdx1985126 force-pushed the wip-crimson-pg-snapmapper-obj branch from d0cfc58 to 8f7921e Compare February 21, 2024 10:01

Matan-B merged commit b420380 into ceph:main Mar 6, 2024
11 checks passed

Matan-B mentioned this pull request Mar 24, 2024

crimson/osd/pg: use pgmeta_oid for snapmapper #56376

Closed

xxhdx1985126 deleted the wip-crimson-pg-snapmapper-obj branch April 23, 2024 02:49

Matan-B mentioned this pull request May 7, 2024

squid: crimson/osd: put snapmapper's key-value pairs into dedicated objs #57324

Open

		assert(coll_ref);
		t.touch(coll_ref->get_cid(), make_snapmapper_oid());

crimson/osd: put snapmapper's key-value pairs into dedicated objs #52267

crimson/osd: put snapmapper's key-value pairs into dedicated objs #52267

Conversation

xxhdx1985126 commented Jun 30, 2023 • edited

Contribution Guidelines

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxhdx1985126 Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matan-B commented Jul 2, 2023

xxhdx1985126 commented Jul 3, 2023

xxhdx1985126 commented Jul 18, 2023

xxhdx1985126 commented Jul 26, 2023

Matan-B commented Aug 24, 2023

Matan-B left a comment

Choose a reason for hiding this comment

xxhdx1985126 commented Aug 28, 2023

xxhdx1985126 commented Aug 28, 2023

Matan-B commented Aug 28, 2023 • edited

xxhdx1985126 commented Aug 29, 2023

Matan-B commented Aug 29, 2023

github-actions bot commented Dec 6, 2023

xxhdx1985126 commented Dec 29, 2023

Matan-B left a comment

Choose a reason for hiding this comment

Matan-B commented Jan 30, 2024

xxhdx1985126 commented Jan 31, 2024 • edited

xxhdx1985126 commented Feb 19, 2024

athanatos commented Feb 19, 2024

athanatos commented Feb 20, 2024

xxhdx1985126 commented Feb 20, 2024

athanatos commented Feb 20, 2024

Matan-B commented Feb 20, 2024 • edited

xxhdx1985126 commented Feb 20, 2024

Matan-B commented Feb 20, 2024

xxhdx1985126 commented Feb 20, 2024

xxhdx1985126 commented Feb 20, 2024 • edited

athanatos commented Feb 20, 2024

xxhdx1985126 commented Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxhdx1985126 Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matan-B commented Feb 22, 2024 • edited

Matan-B commented Mar 6, 2024

xxhdx1985126 commented Jun 30, 2023 •

edited

xxhdx1985126 Sep 27, 2023 •

edited

Matan-B commented Aug 28, 2023 •

edited

xxhdx1985126 commented Jan 31, 2024 •

edited

Matan-B commented Feb 20, 2024 •

edited

xxhdx1985126 commented Feb 20, 2024 •

edited

xxhdx1985126 commented Feb 21, 2024 •

edited

xxhdx1985126 Feb 21, 2024 •

edited

Matan-B commented Feb 22, 2024 •

edited