You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/11/07 00:40:58 UTC
[kudu-CR](branch-1.10.x) KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14648
to review the following change.
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6 and Ubuntu 18.04 repositories all carried memkind 1.1.0. CentOS 7
has memkind 1.7.0. Persistent memory hardware itself also has a pretty
steep kernel version requirement, so it's unlikely to be found outside of
a new distro in the first place.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM and without
memkind to verify the crashing behavior.
I had to resolve conflicts in src/kudu/cfile/cfile-test.cc.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Reviewed-on: http://gerrit.cloudera.org:8080/14620
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
(cherry picked from commit ba908efa15774bb24b5bfa4ea88915161d1100d1)
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 195 insertions(+), 397 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/14648/1
--
To view, visit http://gerrit.cloudera.org:8080/14648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.10.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14648
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.10.x) KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14648 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.10.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14648
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 07 Nov 2019 01:18:52 +0000
Gerrit-HasComments: No
[kudu-CR](branch-1.10.x) KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14648 )
Change subject: KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
......................................................................
KUDU-2990: remove memkind/libnuma and dynamically link memkind at runtime
This patch removes memkind and libnuma from the thirdparty tree and changes
the NVM cache implementation to dynamically link memkind at runtime using
dlopen() and friends. KUDU-2990 explains why we need to do this in great
detail so I won't repeat that here. The alternatives I considered:
1. Patch memkind to dlopen() libnuma.
This is probably the most user-friendly approach because libnuma is found in
most systems (or at least in most package repositories), but a new enough
memkind is not, and having Kudu distribute memkind eases adoption of the NVM
cache. However, I was nervous about performing deep surgery in memkind as
it's a codebase with which I'm not familiar, and I wanted to minimize risk
because we'll be backporting this patch to several older branches.
2. Patch memkind to define weak libnuma symbols.
If done correctly, behavior is unchanged when libnuma is present on the host
system, but if it's not there, calls from memkind to libnuma will crash.
Again, I didn't feel comfortable hacking into memkind, plus I've found weak
symbols difficult to use in the past.
3. Remove the NVM cache from Kudu altogether.
This is obviously the safest and simplest option, but it punishes Kudu users
who actually use the NVM cache.
4. Gate the build of the NVM cache behind a CMake option.
This ought to satisfy the ASF's definition of an "optional" feature, but
does nothing for binary redistributors who wish to offer the NVM cache and
who build Kudu as a statically linked "fat" binary.
5. Build as we do today, but forbid static linkage of libnuma.
Binary redistributors will need to choose between including libnuma in their
distributions, or forcing Kudu to look for libnuma at runtime. The former
still violates ASF policy, and the latter means Kudu won't start on a system
lacking libnuma, regardless of whether the NVM cache is actually in use.
So what are the ramifications of the chosen approach?
- Kudu no longer distributes memkind or libnuma. To use the NVM cache, the
host system must provide both, and memkind must be version 1.6.0 or newer.
CentOS 6 and Ubuntu 18.04 repositories all carried memkind 1.1.0. CentOS 7
has memkind 1.7.0. Persistent memory hardware itself also has a pretty
steep kernel version requirement, so it's unlikely to be found outside of
a new distro in the first place.
- Tests that exercise the NVM cache will be skipped if they can't find a
conformant memkind (and libnuma).
- When starting Kudu, if you don't set --block_cache_type=NVM, you shoudn't
notice any change.
- If you do, Kudu will crash at startup if it can't find a conformant
memkind. This affects upgrades: if you were already an NVM cache user but
you didn't have memkind installed, your Kudu will crash post-upgrade.
Note: this doesn't preclude implementing alternative #1 (the one I think is
ideal) in the future; we'll just have to revert the bulk of this patch when
we do so.
To test, I ran cfile-test and cache-test as follows:
- Without memkind installed: DRAM tests passed, NVM tests were skipped
- With an old memkind installed: DRAM tests passed, NVM tests were skipped
- With LD_LIBRARY_PATH=/path/to/memkind-1.9.0: All tests passed
I also manually ran a Kudu master with --block_cache_type=NVM and without
memkind to verify the crashing behavior.
I had to resolve conflicts in src/kudu/cfile/cfile-test.cc.
Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Reviewed-on: http://gerrit.cloudera.org:8080/14620
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
(cherry picked from commit ba908efa15774bb24b5bfa4ea88915161d1100d1)
Reviewed-on: http://gerrit.cloudera.org:8080/14648
Reviewed-by: Grant Henke <gr...@apache.org>
---
M CMakeLists.txt
D cmake_modules/FindMemkind.cmake
D cmake_modules/FindNuma.cmake
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/nvm_cache.h
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
D thirdparty/patches/memkind-fix-build-with-old-autoconf.patch
D thirdparty/patches/memkind-fix-build-with-old-glibc.patch
D thirdparty/patches/memkind-fix-jemalloc-build-with-old-autoconf.patch
M thirdparty/vars.sh
17 files changed, 195 insertions(+), 397 deletions(-)
Approvals:
Grant Henke: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/14648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.10.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I4f474196aa98b5fa6e5966b9a3aea9a7e466805c
Gerrit-Change-Number: 14648
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)