-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crimson/net: support connections in multiple shards #51916
Conversation
481fbe3
to
53d3bcc
Compare
make check looks good, retry more times and check teuthology |
This comment was marked as off-topic.
This comment was marked as off-topic.
The failed ceph_assert() doesn't look correct and is actually possible in the test case. |
53d3bcc
to
922f8e7
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Looks the crimson build is always 404: https://shaman.ceph.com/builds/ceph/wip-yingxin-msgr-multi-core-crimson-only//crimson/345576/ |
This is an issue with all crimson builds from the last few days, I have notified the sepia team. |
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
jenkins test make check |
unrelated failure:
|
You can try scheduling a build without |
922f8e7
to
4e4dcd2
Compare
Saw an unrelated issue in OSD.2, https://pulpito.ceph.com/yingxin-2023-06-09_02:09:26-crimson-rados-wip-yingxin-msgr-multi-core-crimson-3-distro-default-smithi/7299712/ OSD.2 hits a heap-use-after-free issue when printing PG. The AddressSanitizer report is interleaved with debug logs, try to recover here:
|
Connection is switching between STANDBY/CONNECTION too frequently, seems reconnect waiting is missing in this case. |
4e4dcd2
to
25de6df
Compare
|
I have not encountered that issue before. Are you sure it's unrelated? If so, can you please open a tracker w/ the logs? |
@athanatos @cyx1231st, |
This PR implements multi-core messenger, but it hasn't enabled the multi-core feature to crimson OSD. So Crimson OSD is still running with the single-core-mode messenger, and the interactions between OSD and msgr are not (supposed to be) modified. Please also refer to how this PR changes the OSD-part code, there aren't many. It seems to me that printing PG after destructing it is unrelated to messenger internal refactorings. More likely to be a side-effect due to unexpected messenger behavior. For example with #51916 (comment) the connection will immediately retry without backoff, which is wrong but should not cause the PG AddressSanitizer issue above.
I'm not sure if we can reproduce the issue after correcting the msgr behavior. And the log for OSD.2 is 530MB. I'll paste the link to the log and open the tracker tomorrow. |
Sure, I'll schedule the follow-up tests based on the 2 PRs. There are still TODOs #51916 (comment), so it's fine to take some time. |
Thank you. They are still not ready so you can rebase after they will be merged. |
|
To prevent the previous shard-states racing on the shared data structures after switching to a new shard-states. Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Users may need to know the new connection shard prior to message dispatching. Otherwise, there will be no chance for user to do any related preparations. This is still a placeholder before multi-core messegner is enabled. Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Note that it is inevitable that the user can mark down the connection while the protocol is trying to move the connection to another core. In that case, the implementation should tolerate the racing, which finally needs to cleanup resources correctly and dispatch reasonable events. Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
…-handler interfaces Otherwise, calling io-handler interfaces may result in wrong core/order. This needs to take special care to handle preemptive cases such as closing and replacing. Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
… to be cross-core Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
It's meaningless to dispatch the initial acceptance always from the msgr core. Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
1be8b3f
to
1e8a39f
Compare
rebased to catch up with main |
I think we should merge at 1. given the size of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than my above nit.
OK, for 1, teuthology still sees inconsistent failures, it takes time to investigate and fix. I'll work on 2 at the same time as the tool is ready. |
Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
Changeset: crimson/net: cleanup, rename is_fixed_cpu |
Changeset: crimson/osd/heartbeat: relax the order of replacement reset and accept Identified and fixed an issue that causes regression during heartbeat racing, which causes some failures in teutholoy test. |
With the new implementation in messenger, the order of replacement reset and accept events cannot be determined because they are from different connections. Modify the heatbeat logic to tolerate the both cases. Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
9bd4d85
to
a870946
Compare
Test results: https://pulpito.ceph.com/matan-2023-06-29_09:53:04-crimson-rados-wip-yingxin-msgr-multi-core-crimson-10-distro-default-smithi/ 3 out of 60 are failed: 7321111-osd.1.log
73211112-osd.0.log
7321120-osd.2.log
@athanatos @Matan-B They look like to be the same issue and not related to this PR. |
I'm inclined to go ahead and merge -- I'll do it in the morning if no one disagrees (or beats me to it :) ). |
This PR implements multi-shard support in Crimson Messenger, following up #49420 and #50835.
Generally, all handshakes in protocol v2 are done in a dedicated shard for a Crimson Messenger as before, hopefully this will make further integrations with Crimson OSD simpler, because the dependent logic (such as the authentication) in OSD is not sharded, and the internal connection registration as well as replacements can remain atomic. Apart from that, the events and message dispatching are distributed to all the available shards per connection, determined by where the shard
seastar::connected_socket
lives. Due to the limitation thatconnected_socket
cannot be moved to a different shard once established, the working connection shard can only available with the connected and accepted events. For lossless connection, this means that the working connection shard can be changed during connction recovery and upon reconnect and reaccept.Also see #49420 (comment).
TODOs:
Integrate multi-shard Crimson Messenger with perf-crimson-msgr;crimson/tools/perf_crimson_msgr: integrate multi-core msgr with various improvements #52091Contribution 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
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