RDMA builtin support #1209

pizhenwei · 2024-10-22T10:53:49Z

Hi,

There are several patches in this PR:

Abstract set/rewrite config bind option: bind option is a special config, socket and tls are using the same one. However RDMA uses the similar style but different one. Use a bit abstract work to make it flexible for both socket and RDMA. Even for QUIC in the future.
Introduce closeListener for connection type: closing socket by a simple syscall would be fine, RDMA has complex logic. Introduce connection type specific close listener method.
RDMA: Use valkey.conf style instead of module parameters: use --rdma-bind and --rdma-port style instead of module parameters. The module style config rdma.bind and rdma.port are removed.
RDMA: Support builtin: support make BUILD_RDMA=yes. module style is still kept for now. Once module style is decided to drop, I volunteer to do it for TLS and RDMA together.

Each patch has independent functionality, also been tested by CI and local commands, so request no-squash merge for this PR.

codecov · 2024-10-22T11:08:21Z

Codecov Report

Attention: Patch coverage is 54.54545% with 40 lines in your changes missing coverage. Please review.

Project coverage is 70.69%. Comparing base (fd58f8d) to head (ee34a17).
Report is 1 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/config.c	50.84%	29 Missing ⚠️
src/server.c	27.27%	8 Missing ⚠️
src/unix.c	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1209      +/-   ##
============================================
+ Coverage     70.62%   70.69%   +0.06%     
============================================
  Files           117      118       +1     
  Lines         63324    63385      +61     
============================================
+ Hits          44722    44809      +87     
+ Misses        18602    18576      -26

Files with missing lines	Coverage Δ
src/connection.c	`78.82% <100.00%> (+0.25%)`	⬆️
src/connection.h	`87.64% <100.00%> (+0.43%)`	⬆️
src/rdma.c	`100.00% <100.00%> (ø)`
src/server.h	`100.00% <ø> (ø)`
src/socket.c	`91.52% <100.00%> (+0.40%)`	⬆️
src/tls.c	`100.00% <ø> (ø)`
src/unix.c	`73.49% <0.00%> (-2.76%)`	⬇️
src/server.c	`87.44% <27.27%> (-0.24%)`	⬇️
src/config.c	`77.63% <50.84%> (-1.21%)`	⬇️

... and 12 files with indirect coverage changes

zuiderkwast · 2024-11-01T12:45:45Z

@valkey-io/core-team Please approve.

This adds make BUILD_RDMA=yes, so just like TLS it can be built as a module or as builtin. Just like for TLS, the default is no.

It is a step towards official (non-experimantal) RDMA support.

I already merged the RDMA fork-safety here: #1244. It makes sure the server doesn't crash if we try to talk to an RDMA client from a forked child process.

pizhenwei · 2024-11-08T08:51:17Z

@valkey-io/core-team Please approve.

This adds make BUILD_RDMA=yes, so just like TLS it can be built as a module or as builtin. Just like for TLS, the default is no.

It is a step towards official (non-experimantal) RDMA support.

I already merged the RDMA fork-safety here: #1244. It makes sure the server doesn't crash if we try to talk to an RDMA client from a forked child process.

Hi @valkey-io/core-team
The CMake building support has been merged, then we can drop module build option for TLS and RDMA together.

PingXie · 2024-11-09T07:32:23Z

The CMake building support has been merged, then we can drop module build option for TLS and RDMA together.

I vote for dropping the module packaging. Modularity is what we need and we have it here for RDMA.

zuiderkwast · 2024-11-09T13:45:13Z

I believe it's safe to drop the RDMA module because it's experimental, but I'm not sure about the TLS module. Maybe someone is using it? Maybe we should drop it in a major version (9.0?).

The potential benefit of module is that you don't need OpenSSL installed to start Valkey, as long as you don't load the module. If we ship binary releases or containers with bundled modules, they can be used also by those who don't have or want TLS. Examples are embedded system, IoT devices, etc. We've already talked about packaging binaries with buldled modules like (bloom, json).

zuiderkwast

I didn't review fully. Just the interface changes.

valkey.conf

pizhenwei · 2024-11-21T06:13:12Z

Hi @zuiderkwast
Please take a loot at the latest version, modify as your suggestion, and support builtin for cmake.

zuiderkwast

LGTM.

Let's wait for a core team majority decision before merge since it's an interface change.

Next time, can you avoid force-push and instead just push more commits? It's easier to see what's changed since last review. We squash-merge in the end anyway, normally.

valkey.conf

pizhenwei · 2024-11-21T08:24:35Z

Next time, can you avoid force-push and instead just push more commits? It's easier to see what's changed since last review. We squash-merge in the end anyway, normally.

OK.

zuiderkwast · 2024-11-21T08:26:36Z

@valkey-io/core-team This adds make BUILD_RDMA=yes and some configs. Please approve or vote 👍 or 👎.

The module style configs rdma.port and rdma.bind are removed. Even if BUILD_RDMA=module, the module is using the new, normal configs. That's why it is a breaking change, but we allow it because RDMA is experimental.

enjoy-binbin

Not a full review, but LGTM.

README.md

src/config.c

madolson

Directionally I'm inclined, I mostly just had some questions about the configs and documentation.

valkey.conf

pizhenwei · 2024-11-25T07:46:27Z

Hi, @madolson @zuiderkwast
Append a new commit to fix conflict against 33f42d7. Please take a look at the latest version. Thanks!

valkey-server is performance sensitive on RDMA, add man page section for deeply performance tuning. [Link](valkey-io/valkey#1209) for valkey changes. --------- Signed-off-by: zhenwei pi <[email protected]>

zuiderkwast · 2024-11-27T08:51:43Z

@pizhenwei The DCO is failing because some commits are written by some email and signed off with another email. Please fix it. Feel free to squash all commits to one if you want. See DCO job Details: https://github.com/valkey-io/valkey/pull/1209/checks?check_run_id=33460190673

@PingXie @soloestoy @hwware Can we have one more ACK to merge this one? (RDMA is still experimental so it's unclear if it even needs to be a major decision, but it's marked as one now.)

hwware · 2024-11-28T21:16:49Z

@valkey-io/core-team Please approve.

This adds make BUILD_RDMA=yes, so just like TLS it can be built as a module or as builtin. Just like for TLS, the default is no.

It is a step towards official (non-experimantal) RDMA support.

I already merged the RDMA fork-safety here: #1244. It makes sure the server doesn't crash if we try to talk to an RDMA client from a forked child process.

Yes, it is the way I recommend. Give user one more option. Thanks

Changes in this commit: - Originally, special config 'bind' is used for socket and TLS, however multiple addresses handling is also workable for RDMA(QUIC in the future). Abstract bind option as helper functions to apply more connection types. - rewriteConfigBindOption is a local function, declare it as 'static' function. - Socket family use close() syscall to close listener, however RDMA has another style. Use an abstract function handler instead of hard code syscall style. - Move 4 parameters from valkey-rdma.so to valkey-server, keep RDMA listener similar to TCP/TLS. Also prepare to build Valkey Over RDMA into builtin. - Support RDMA builtin and module together. To build a builtin version: $ make BUILD_RDMA=yes Or build it by cmake: $ cmake .. -DBUILD_RDMA=yes Finally, we have a builtin/module RDMA support with Valkey connection style config! Signed-off-by: zhenwei pi <[email protected]>

pizhenwei · 2024-11-29T09:30:57Z

Because of DCO check error, rebase against the latest code and squash into a single commit.

Since 4695d11("RDMA builtin support (valkey-io#1209)"), RDMA supports builtin. And module connection type may be removed in future. So run a builtin RDMA support for CI workflow. RDMA module is complied only in CI, keep it building check only until module connection type gets obsolete. Signed-off-by: zhenwei pi <[email protected]>

Since 4695d11 (#1209), RDMA supports builtin. And module connection type may be removed in future. So run a builtin RDMA support for CI workflow. RDMA module is complied only in CI, keep it building check only until module connection type gets obsolete. Signed-off-by: zhenwei pi <[email protected]>

Since 4695d11 (valkey-io#1209), RDMA supports builtin. And module connection type may be removed in future. So run a builtin RDMA support for CI workflow. RDMA module is complied only in CI, keep it building check only until module connection type gets obsolete. Signed-off-by: zhenwei pi <[email protected]>

pizhenwei force-pushed the rdma-build-yes branch from 35884e2 to 047c7ec Compare October 23, 2024 07:47

zuiderkwast added the major-decision-pending Major decision pending by TSC team label Nov 1, 2024

zuiderkwast reviewed Nov 20, 2024

View reviewed changes

valkey.conf Outdated Show resolved Hide resolved

valkey.conf Outdated Show resolved Hide resolved

valkey.conf Outdated Show resolved Hide resolved

pizhenwei force-pushed the rdma-build-yes branch 2 times, most recently from b8378df to 3bdd658 Compare November 21, 2024 05:23

zuiderkwast approved these changes Nov 21, 2024

View reviewed changes

valkey.conf Outdated Show resolved Hide resolved

zuiderkwast added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Nov 21, 2024

enjoy-binbin approved these changes Nov 21, 2024

View reviewed changes

README.md Show resolved Hide resolved

src/config.c Outdated Show resolved Hide resolved

src/config.c Outdated Show resolved Hide resolved

src/config.c Outdated Show resolved Hide resolved

pizhenwei mentioned this pull request Nov 21, 2024

Add performance tuning section for RDMA valkey-io/valkey-doc#190

Merged

zuiderkwast added release-notes This issue should get a line item in the release notes breaking-change Indicates a possible backwards incompatible change labels Nov 21, 2024

madolson reviewed Nov 21, 2024

View reviewed changes

valkey.conf Show resolved Hide resolved

valkey.conf Outdated Show resolved Hide resolved

madolson approved these changes Nov 22, 2024

View reviewed changes

zuiderkwast removed the breaking-change Indicates a possible backwards incompatible change label Nov 22, 2024

zuiderkwast added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Nov 28, 2024

pizhenwei force-pushed the rdma-build-yes branch from 7987845 to ee34a17 Compare November 29, 2024 09:28

zuiderkwast merged commit 4695d11 into valkey-io:unstable Nov 29, 2024
47 of 48 checks passed

pizhenwei deleted the rdma-build-yes branch December 2, 2024 06:44

pizhenwei mentioned this pull request Dec 2, 2024

Run RDMA builtin in CI workflow #1380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RDMA builtin support #1209

RDMA builtin support #1209

pizhenwei commented Oct 22, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

zuiderkwast commented Nov 1, 2024

pizhenwei commented Nov 8, 2024

PingXie commented Nov 9, 2024

zuiderkwast commented Nov 9, 2024

zuiderkwast left a comment

pizhenwei commented Nov 21, 2024

zuiderkwast left a comment

pizhenwei commented Nov 21, 2024

zuiderkwast commented Nov 21, 2024 •

edited

Loading

enjoy-binbin left a comment

madolson left a comment

pizhenwei commented Nov 25, 2024 •

edited

Loading

zuiderkwast commented Nov 27, 2024

hwware commented Nov 28, 2024

pizhenwei commented Nov 29, 2024

RDMA builtin support #1209

RDMA builtin support #1209

Conversation

pizhenwei commented Oct 22, 2024 • edited by zuiderkwast Loading

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

zuiderkwast commented Nov 1, 2024

pizhenwei commented Nov 8, 2024

PingXie commented Nov 9, 2024

zuiderkwast commented Nov 9, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

pizhenwei commented Nov 21, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

pizhenwei commented Nov 21, 2024

zuiderkwast commented Nov 21, 2024 • edited Loading

enjoy-binbin left a comment

Choose a reason for hiding this comment

madolson left a comment

Choose a reason for hiding this comment

pizhenwei commented Nov 25, 2024 • edited Loading

zuiderkwast commented Nov 27, 2024

hwware commented Nov 28, 2024

pizhenwei commented Nov 29, 2024

pizhenwei commented Oct 22, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

zuiderkwast commented Nov 21, 2024 •

edited

Loading

pizhenwei commented Nov 25, 2024 •

edited

Loading