pmem / rpma

Remote Persistent Memory Access Library
Other
101 stars 56 forks source link

common: use newer rdma-core on Ubuntu and Fedora CIs #2111

Closed ldorau closed 1 year ago

ldorau commented 1 year ago

Install rdma-core v44.0 (with support for both native atomic write and native flush) on Ubuntu and Fedora CIs.

During the CI build check if libibverbs installed from an OS package supports both native atomic write and native flush. If libibverbs installed from the OS package does not support native atomic write or native flush and another, built from sources, version of rdma-core exists (which supports both native atomic write and native flush), run one build using the rdma-core installed from the OS package and then switch to using only the built-from-sources version of rdma-core for the rest of the builds.

Requires:


This change is Reviewable

ldorau commented 1 year ago

This is a continuation of https://github.com/pmem/rpma/pull/2108

codecov[bot] commented 1 year ago

Codecov Report

Merging #2111 (dc57c91) into main (3edd789) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #2111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1590      1611   +21     
=========================================
+ Hits          1590      1611   +21     
yangx-jy commented 1 year ago

I think it actually skipped the "old" path of tests.

Is it better to avoid installing both rdma-core-devel package and built rdma-core source code in a docker image? In this case, we don’t need to check the support of native atomic write and flush additionally by adding verify-libibverbs-version.c and change the old logic.

ldorau commented 1 year ago

@grom72

It is a significant step forward, but does it mean we skip the "old" path of tests? Shall we now extend the librpma build system to force disabling NATIVE_ATOMIC_WRITE_SUPPORTED?

The old path of tests is still tested in: 1) Rocky Linux 8 on-each-pull-request build, 2) Rocky Linux 9 on-each-pull-request build, 3) In the following check: https://github.com/ldorau/rpma/blob/common-use-newer-rdma-core-on-Ubuntu-and-Fedora-CIs-2/utils/docker/run-build.sh#L160-L177 of all on-each-pull-request Fedora latest and Ubuntu latest builds. 4) all Nightly builds except of Fedora latest and Ubuntu latest

ldorau commented 1 year ago

I think it actually skipped the "old" path of tests.

It did not - see my previous answer above.

Is it better to avoid installing both rdma-core-devel package and built rdma-core source code in a docker image? In this case, we don’t need to check the support of native atomic write and flush additionally by adding verify-libibverbs-version.c and change the old logic.

Thanks to that we can test both paths in all Fedora latest and Ubuntu latest builds.

yangx-jy commented 1 year ago

I meant that the old path of tests has been changed in Fedora-latest and Ubuntu-latest.

The previous building tests: 1) with rdma-core package: a) Verify build with ASAN and UBSAN ($CC, DEBUG) b) Verify build and install (in dir: ${PREFIX}) ($CC, DEBUG) c) Verify build and install (in dir: ${PREFIX}) ($CC, RELEASE) The current building tests: 1) with rdma-core package: a) Verify build on the OS-dependent version of rdma-core ($CC, DEBUG) 2) with rdma-core source code: a) Verify build with ASAN and UBSAN ($CC, DEBUG) b) Verify build and install (in dir: ${PREFIX}) ($CC, DEBUG) c) Verify build and install (in dir: ${PREFIX}) ($CC, RELEASE)

If you care about this logic change, is it better to use rdma-core source code by adding two new dockerfiles (e.g. Dockerfile.fedora-latest-rdma-core-custom)? If you don't care about this logic change, is it clearer to only use rdma-core source code in Dockerfile.fedora-latest and Dockerfile.ubuntu-latest?

By the way, the draft of dockerfile is like:

diff --git a/utils/docker/images/Dockerfile.fedora-latest b/utils/docker/images/Dockerfile.fedora-latest
index 5815c278..e0563e4d 100644
--- a/utils/docker/images/Dockerfile.fedora-latest
+++ b/utils/docker/images/Dockerfile.fedora-latest
@@ -34,7 +34,6 @@ ENV RPMA_DEPS "\
        groff \
        graphviz \
        pandoc \
-       rdma-core-devel"

 # examples deps ('protobuf-c-devel' is required only for examples 9 and 9s)
 ENV EXAMPLES_DEPS "\
@@ -54,6 +53,10 @@ RUN dnf install -y \
        $DOC_UPDATE_DEPS \
 && dnf clean all

+# Install rdma-core
+COPY install-rdma-core.sh install-rdma-core.sh
+RUN ./install-rdma-core.sh
+
 # Install cmocka
 COPY install-cmocka.sh install-cmocka.sh
 RUN ./install-cmocka.sh
@@ -75,3 +78,6 @@ ENV OS fedora
 ENV OS_VER latest
 ENV PACKAGE_MANAGER rpm
 ENV NOTTY 1
+ENV PKG_CONFIG_PATH /rdma-core/build/lib/pkgconfig
+ENV LD_LIBRARY_PATH /rdma-core/build/lib
+ENV CPATH /rdma-core/build/include

Either of them can avoid checking the support of native atomic write and flush by adding verify-libibverbs-version.c and modifying run-build.sh.

yangx-jy commented 1 year ago

Yes, that's what I wanted. for example:

"N=7 OS=ubuntu OS_VER=latest CC=gcc  BUILD_RDMA_CORE=1",
"N=8 OS=fedora  OS_VER=latest CC=gcc  BUILD_RDMA_CORE=1",
ldorau commented 1 year ago

Either of them can avoid checking the support of native atomic write and flush by adding verify-libibverbs-version.c and modifying run-build.sh.

@yangx-jy You are right. That's the best solution. I will submit a new PR. Thanks!

ldorau commented 1 year ago

I will submit a new PR with a new different version (with separate Dockerfiles).

ldorau commented 1 year ago

Replaced by https://github.com/pmem/rpma/pull/2119