neorados: Update exerciser/demo to use C++20 coroutines by adamemerson · Pull Request #48129 · ceph/ceph

adamemerson · 2022-09-15T20:49:23Z

Just to work the bugs out and give some demo code.

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

src/tools/neorados.cc

adamemerson · 2022-09-16T01:03:14Z

jenkins test make check

src/include/neorados/RADOS.hpp

yuvalif · 2022-09-19T16:46:52Z

isn't the neorados code currently used with the RGW?
shouldn't the neorados library support both a spawn/yield API and c++20 coroutines API?

cbodley · 2022-09-19T17:10:20Z

isn't the neorados code currently used with the RGW? shouldn't the neorados library support both a spawn/yield API and c++20 coroutines API?

it does support both as part of asio's Asynchronous Model, specifically through our handling of the CompletionToken template type

adamemerson · 2022-09-20T15:23:03Z

This looks mostly reasonable, I think? I'm not as familiar with how much background failure RBD has.

https://pulpito.ceph.com/aemerson-2022-09-20_03:59:09-rbd-wip-coro-test-distro-default-smithi/

yuvalif · 2022-09-20T16:38:08Z

do we have teuthology tests running ceph_test_neorados?

adamemerson · 2022-09-20T16:43:44Z

do we have teuthology tests running ceph_test_neorados?

No, not yet.

cbodley · 2022-09-21T14:51:21Z

This looks mostly reasonable, I think? I'm not as familiar with how much background failure RBD has.

https://pulpito.ceph.com/aemerson-2022-09-20_03:59:09-rbd-wip-coro-test-distro-default-smithi/

yikes, from those cli_generic.sh failures i see that rbd bench is causing osds to report a bunch of slow requests:

2022-09-20T06:18:21.142 INFO:tasks.ceph.osd.0.smithi171.stderr:2022-09-20T06:18:21.139+0000 7f9299790700 -1 osd.0 158 get_health_metrics reporting 256 slow ops, oldest is osd_op(client.10593.0:2055 3.3 3:db356461:test1::rbd_data.295e2a328d04.00000000000000f0:head [set-alloc-hint object_size 4194304 write_size 4194304,write 3420160~4096 [fadvise_random] in=4096b] snapc 0=[] RETRY=4 ondisk+retry+write+known_if_redirected+supports_pool_eio e158)
2022-09-20T06:18:21.448 INFO:tasks.ceph.osd.1.smithi171.stderr:2022-09-20T06:18:21.443+0000 7f52fc777700 -1 osd.1 158 get_health_metrics reporting 256 slow ops, oldest is osd_op(client.10593.0:2007 3.2 3:50ad0aa8:test1::rbd_data.295e2a328d04.0000000000000060:head [set-alloc-hint object_size 4194304 write_size 4194304,write 1519616~4096 [fadvise_random] in=4096b] snapc 0=[] RETRY=3 ondisk+retry+write+known_if_redirected+supports_pool_eio e158)

do these failures look familiar @idryomov?

cbodley · 2022-10-06T15:20:58Z

@adamemerson i've been watching other runs of the rbd suite like https://pulpito.ceph.com/dis-2022-10-05_10:59:41-rbd-wip-dis-testing-distro-default-smithi/ and haven't seen any of those cli_generic.sh failures. you might try a rebase and rerun to see if they persist?

adamemerson · 2022-10-07T13:34:28Z

@cbodley How's this?

http://pulpito.ceph.com/aemerson-2022-10-06_18:51:28-rbd-wip-coro-test-distro-default-smithi/

cbodley · 2022-10-07T13:56:20Z

looks much better! for the one test_librbd.sh failure, it got stuck on TestLibRBD.ConcurentOperations and never finished. not sure what's going on there, but maybe that's related to https://tracker.ceph.com/issues/53440? cc @idryomov

cbodley · 2022-10-18T14:29:27Z

@idryomov a friendly ping to review the rbd suite results

adamemerson · 2023-01-09T16:45:53Z

jenkins test api

yuvalif · 2023-01-22T11:11:17Z

@adamemerson any status update?

adamemerson · 2023-01-23T16:13:48Z

@adamemerson any status update?

@idryomov Says he will include it in his next test run of RBD PRs.

cbodley · 2023-03-06T19:01:38Z

@adamemerson i know that you've been carrying these commits in your wip-coro-after-reef branch. now that reef has forked, i think we should start by getting just this PR reviewed/tested/merged. i don't want to ask other components to review everything in #49737. but merging this PR could unblock all of the rgw-specific in your feature branch. what do you think?

cbodley · 2023-03-06T19:03:36Z

i also see that the rbd suite is looking very green for reef, so that should make it easier to validate the results

adamemerson · 2023-03-06T19:19:25Z

@adamemerson i know that you've been carrying these commits in your wip-coro-after-reef branch. now that reef has forked, i think we should start by getting just this PR reviewed/tested/merged. i don't want to ask other components to review everything in #49737. but merging this PR could unblock all of the rgw-specific in your feature branch. what do you think?

That sounds fine to me, I think.

cbodley · 2023-03-10T21:59:51Z

src/include/neorados/RADOS.hpp

+namespace fmt {
+template <>
+struct formatter<neorados::Object> : ostream_formatter {};
+
+template <>
+struct formatter<neorados::IOContext> : ostream_formatter {};
+
+template <>
+struct formatter<neorados::Op> : ostream_formatter {};
+} // namespace fmt


this appears to be the cause of centos9 build failures in shaman:

src/neorados/RADOS.cc:38: src/include/neorados/RADOS.hpp:1024:56: error: expected class-name before ‘{’ token 1024 | struct formatter<neorados::Object> : ostream_formatter {}; | ^ src/include/neorados/RADOS.hpp:1027:59: error: expected class-name before ‘{’ token 1027 | struct formatter<neorados::IOContext> : ostream_formatter {}; | ^ src/include/neorados/RADOS.hpp:1030:52: error: expected class-name before ‘{’ token 1030 | struct formatter<neorados::Op> : ostream_formatter {}; | ^

edit: same for the jammy build

cbodley · 2023-03-12T14:43:52Z

passed the rgw suite in https://pulpito.ceph.com/cbodley-2023-03-11_01:08:10-rgw-wip-coro-test-distro-default-smithi/

the rbd suite in https://pulpito.ceph.com/cbodley-2023-03-11_01:12:31-rbd-wip-coro-test-distro-default-smithi/ showed a number of failures in cli_generic.sh, rbd_mirror_stress.sh and rbd_mirror_ha.sh

Previously we had versions of all the calls that took an int64_t and optional key and namespace. This didn't really offer much benefit and doubled the maintenance burden for changing anything. As such just make the IOContext constructor non-explicit, and make all its mutators return the same IOContext so people can supply them builder-style if they want to. Also don't wrap things in 'optional' when the underlying library doesn't. The callers just end up having to wrap and unwrap with value-or repeatedly. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

This form of completion handling is compatible with C++20 Completions and generally more flexible stuff. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

The lock will continue to be held over the 'dispatch' with C++20 coroutines. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Apparently in some cases we were using references to objects on the caller stack that happen to no longer exist if the caller's stack goes away. C++20 Coroutines tickled this bug. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Use `min` rather than `max` when deciding how much we need to read in `read()`. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

Just as a demonstration to see how well they work and how to put things together with them. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

cbodley · 2023-03-21T20:16:12Z

the 'ceph API tests' check fails to build with clang without the boost 1.81 changes:

src/tools/neorados.cc:68:5: error: no template named 'awaitable' in namespace 'boost::asio'
ba::awaitable<R::IOContext> lookup_pool(R::RADOS& r, const std::string& pname)
~~~~^

cbodley · 2023-03-22T18:15:30Z

the rbd suite in https://pulpito.ceph.com/cbodley-2023-03-11_01:12:31-rbd-wip-coro-test-distro-default-smithi/ showed a number of failures in cli_generic.sh, rbd_mirror_stress.sh and rbd_mirror_ha.sh

the --rerun was queued for so long that shaman had deleted its packages. i scheduled a new rbd suite on yesterday's builds

cbodley · 2023-03-22T19:33:09Z

the 'ceph API tests' check fails to build with clang without the boost 1.81 changes

@adamemerson do we need to pull in the boost 1.81 commits from #49737?

cbodley · 2023-03-23T13:51:50Z

https://pulpito.ceph.com/cbodley-2023-03-22_18:12:21-rbd-wip-coro-test-distro-default-smithi/ shows the same cli_generic.sh, rbd_mirror_stress.sh and rbd_mirror_ha.sh failures as the previous run; they might be caused by this pr, but i wouldn't know how to debug further

github-actions · 2023-06-01T23:01:28Z

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.

adamemerson requested review from cbodley and yuvalif September 15, 2022 20:49

adamemerson requested a review from a team as a code owner September 15, 2022 20:49

github-actions bot added build/ops rbd tests labels Sep 15, 2022

cbodley reviewed Sep 15, 2022

View reviewed changes

src/tools/neorados.cc Outdated Show resolved Hide resolved

adamemerson force-pushed the wip-coro-test branch from c89bc3b to 7fe5019 Compare September 15, 2022 23:33

yuvalif reviewed Sep 19, 2022

View reviewed changes

src/include/neorados/RADOS.hpp Outdated Show resolved Hide resolved

adamemerson force-pushed the wip-coro-test branch from 7fe5019 to c8321ec Compare September 20, 2022 02:09

adamemerson force-pushed the wip-coro-test branch from c8321ec to 6f92249 Compare September 20, 2022 20:58

adamemerson requested review from cbodley and yuvalif September 20, 2022 20:58

adamemerson force-pushed the wip-coro-test branch from 6f92249 to 789f411 Compare September 20, 2022 21:00

cbodley approved these changes Sep 20, 2022

View reviewed changes

yuvalif approved these changes Sep 21, 2022

View reviewed changes

cbodley requested a review from idryomov September 21, 2022 14:51

yuvalif mentioned this pull request Sep 28, 2022

[DNM] neorados: add more unit tests #48284

Closed

14 tasks

adamemerson force-pushed the wip-coro-test branch from 789f411 to 9c7e5ab Compare October 7, 2022 13:33

adamemerson removed the needs-qa label Jan 9, 2023

cbodley added the needs-qa label Jan 9, 2023

yuvalif mentioned this pull request Jan 22, 2023

Neorados CLS version #48234

Merged

14 tasks

cbodley force-pushed the wip-coro-test branch from 4d297db to a3133df Compare March 10, 2023 21:37

cbodley reviewed Mar 10, 2023

View reviewed changes

cbodley added the wip-cbodley-testing label Mar 11, 2023

cbodley removed wip-cbodley-testing needs-qa labels Mar 12, 2023

adamemerson added 6 commits March 21, 2023 14:04

neorados: Switch to async_initiate

7e0ff7a

This form of completion handling is compatible with C++20 Completions and generally more flexible stuff. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

neorados: Don't call dispatch inside with_osdmap

0018cdf

The lock will continue to be held over the 'dispatch' with C++20 coroutines. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

neorados: Ensure we don't have dangling references

c24db83

Apparently in some cases we were using references to objects on the caller stack that happen to no longer exist if the caller's stack goes away. C++20 Coroutines tickled this bug. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

neorados: Fix logic error in exerciser

e429563

Use `min` rather than `max` when deciding how much we need to read in `read()`. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

neorados: Use C++20 Coroutines in exerciser/demo

3bc8694

Just as a demonstration to see how well they work and how to put things together with them. Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

adamemerson force-pushed the wip-coro-test branch from a3133df to 3bc8694 Compare March 21, 2023 18:53

github-actions bot added the stale label Jun 1, 2023

adamemerson closed this Jun 22, 2023

cbodley mentioned this pull request Jul 18, 2023

neorados: Learning from experience #52495

Merged

14 tasks

Conversation

adamemerson commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

adamemerson commented Sep 16, 2022

Uh oh!

Uh oh!

yuvalif commented Sep 19, 2022

Uh oh!

cbodley commented Sep 19, 2022

Uh oh!

adamemerson commented Sep 20, 2022

Uh oh!

yuvalif commented Sep 20, 2022

Uh oh!

adamemerson commented Sep 20, 2022

Uh oh!

cbodley commented Sep 21, 2022

Uh oh!

cbodley commented Oct 6, 2022

Uh oh!

adamemerson commented Oct 7, 2022

Uh oh!

cbodley commented Oct 7, 2022

Uh oh!

cbodley commented Oct 18, 2022

Uh oh!

adamemerson commented Jan 9, 2023

Uh oh!

yuvalif commented Jan 22, 2023

Uh oh!

adamemerson commented Jan 23, 2023

Uh oh!

cbodley commented Mar 6, 2023

Uh oh!

cbodley commented Mar 6, 2023

Uh oh!

adamemerson commented Mar 6, 2023

Uh oh!

cbodley Mar 10, 2023 • 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!

cbodley commented Mar 12, 2023

Uh oh!

cbodley commented Mar 21, 2023

Uh oh!

cbodley commented Mar 22, 2023

Uh oh!

cbodley commented Mar 22, 2023

Uh oh!

cbodley commented Mar 23, 2023

Uh oh!

github-actions bot commented Jun 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamemerson commented Sep 15, 2022 •

edited

Loading

cbodley Mar 10, 2023 •

edited

Loading