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.71%. Comparing base (43b5026) to head (4fec63d).
Report is 3 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.73%   70.71%   -0.03%     
============================================
  Files           116      117       +1     
  Lines         63271    63341      +70     
============================================
+ Hits          44757    44792      +35     
- Misses        18514    18549      +35

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.43% <27.27%> (-0.21%)`	⬇️
src/config.c	`77.63% <50.84%> (-1.21%)`	⬇️

... and 12 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

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

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. Signed-off-by: zhenwei pi <[email protected]>

Socket family use close() syscall to close listener, however RDMA has another style. Use an abstract function handler instead of hard code syscall style. Signed-off-by: zhenwei pi <[email protected]>

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. Signed-off-by: zhenwei pi <[email protected]>

Support RDMA builtin and module together. To build a builtin version: $ make BUILD_RDMA=yes Or build it by cmake: $ cmake .. -DBUILD_RDMA=yes Signed-off-by: zhenwei pi <[email protected]>

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

Co-authored-by: Binbin <[email protected]> Signed-off-by: zhenwei pi <[email protected]>

Apply suggestion from Binbin. Co-authored-by: Binbin <[email protected]> Signed-off-by: zhenwei pi <[email protected]>

madolson

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

valkey.conf

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 added 2 commits November 21, 2024 09:57

Introduce closeListener for connection type

feeb589

Socket family use close() syscall to close listener, however RDMA has another style. Use an abstract function handler instead of hard code syscall style. Signed-off-by: zhenwei pi <[email protected]>

pizhenwei force-pushed the rdma-build-yes branch from 047c7ec to b8378df Compare November 21, 2024 03:13

pizhenwei added 2 commits November 21, 2024 13:20

RDMA: Use valkey.conf style instead of module parameters

ec730d0

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. Signed-off-by: zhenwei pi <[email protected]>

RDMA: Support builtin

3bdd658

Support RDMA builtin and module together. To build a builtin version: $ make BUILD_RDMA=yes Or build it by cmake: $ cmake .. -DBUILD_RDMA=yes Signed-off-by: zhenwei pi <[email protected]>

pizhenwei force-pushed the rdma-build-yes branch 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 and others added 3 commits November 21, 2024 17:01

Update src/config.c

78fd08b

Co-authored-by: Binbin <[email protected]> Signed-off-by: zhenwei pi <[email protected]>

Update src/config.c

24acb40

Apply suggestion from Binbin. Co-authored-by: Binbin <[email protected]> Signed-off-by: zhenwei pi <[email protected]>

Update src/config.c

ad5c1ea

Apply suggestion from Binbin. Co-authored-by: Binbin <[email protected]> Signed-off-by: zhenwei pi <[email protected]>

pizhenwei mentioned this pull request Nov 21, 2024

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

Open

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

pizhenwei added 2 commits November 22, 2024 11:20

Rename "rdma-comp-vector" to "rdma-completion-vector"

7c2a971

Signed-off-by: zhenwei pi <[email protected]>

Merge branch 'unstable' into rdma-build-yes

7dda567

Signed-off-by: zhenwei pi <[email protected]>

madolson approved these changes Nov 22, 2024

View reviewed changes

pizhenwei added 2 commits November 22, 2024 11:31

Rename "rdma-comp-vector" to "rdma-completion-vector" for test

2644d71

Signed-off-by: zhenwei pi <[email protected]>

Typo fix

4fec63d

Signed-off-by: zhenwei pi <[email protected]>

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

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

RDMA builtin support #1209

Are you sure you want to change the base?

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 Oct 22, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

zuiderkwast commented Nov 21, 2024 •

edited

Loading