Skip to content
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

cmake: enable ndctl when building PMDK for WITH_BLUESTORE_PMEM #46260

Merged
merged 2 commits into from Jul 14, 2022

Conversation

CongMinYin
Copy link
Contributor

@CongMinYin CongMinYin commented May 13, 2022

In order to support the character device of pmem usage in bluestore via
libpmem built by Ceph itself, we need to enable daxctl and ndctl
dependency. And this PR is used to add ndctl and daxctl dependency
when WITH_BLUSE_STORE_PMEM is ON.

add the installation of ndctl and daxctl, then find them.
ndctl library requires >63. daxctl library requires >63.

Because after find_ndctl and find_daxctl are added, the dependency packages
required by WITH_BLUESTORE_PMEM and WITH_RBD_RWL become more different.
The DWITH_RBD_RWL option is turned on by default in some cases (see #39049),
while WITH_BLUESTORE_PMEM is usually turned off by default. These two should not be put together.
So separated these two. libpmem has no version required. libpmemobj
required version >=1.8.

the installation of ndctl-devel and daxctl-devel in ceph.spec.in has not been verified.

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
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

@CongMinYin CongMinYin changed the title [cmake] add findndctl.cmake [WIP] cmake: add findndctl.cmake May 13, 2022
@CongMinYin
Copy link
Contributor Author

CongMinYin commented May 13, 2022

Hi @tchaikov , this pr passed ./do_cmake.sh -DWITH_BLUESTORE=ON -DWITH_BLUESTORE_PMEM=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo -DWITH_CCACHE=ON, but failed after: cd build && ninja -j 96, report:
ninja: error: 'ndctl::ndctl-NOTFOUND', needed by 'bin/ceph-osd', missing and no known rule to make it.
Do you have any ideas?

part of output during do_cmake:
-- Found ndctl: /usr/lib/x86_64-linux-gnu/libndctl.so (Required is at least version "63")

install-deps.sh Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/modules/Findndctl.cmake Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor

my guess is that WITH_PMEM=true never gets set in https://github.com/ceph/ceph-build/blob/master/ceph-pull-requests/build/build

@CongMinYin
Copy link
Contributor Author

Hi @tchaikov , this pr passed ./do_cmake.sh -DWITH_BLUESTORE=ON -DWITH_BLUESTORE_PMEM=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo -DWITH_CCACHE=ON, but failed after: cd build && ninja -j 96, report: ninja: error: 'ndctl::ndctl-NOTFOUND', needed by 'bin/ceph-osd', missing and no known rule to make it. Do you have any ideas?

part of output during do_cmake: -- Found ndctl: /usr/lib/x86_64-linux-gnu/libndctl.so (Required is at least version "63")

the detail output: https://gist.github.com/CongMinYin/db3c96be013f63df139f9674452782f9

@CongMinYin
Copy link
Contributor Author

my guess is that WITH_PMEM=true never gets set in https://github.com/ceph/ceph-build/blob/master/ceph-pull-requests/build/build

take a look this pr: ceph/ceph-build#2011

@CongMinYin CongMinYin changed the title [WIP] cmake: add findndctl.cmake cmake: add findndctl.cmake May 23, 2022
@CongMinYin
Copy link
Contributor Author

Hi @tchaikov , thanks your guide. Now passed the local test. Please review.

@djgalloway djgalloway changed the base branch from master to main May 25, 2022 19:59
@CongMinYin
Copy link
Contributor Author

ping @tchaikov

install-deps.sh Outdated Show resolved Hide resolved
install-deps.sh Outdated Show resolved Hide resolved
@CongMinYin CongMinYin force-pushed the wip-enable-ndctl branch 2 times, most recently from 9dfe5f6 to a1693db Compare May 31, 2022 04:21
@github-actions
Copy link

github-actions bot commented Jun 2, 2022

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@CongMinYin
Copy link
Contributor Author

rebased, ping @tchaikov

@CongMinYin
Copy link
Contributor Author

CongMinYin commented Jun 5, 2022

I rebased to newest upstream/main, but lable needs-rebase still exists.

tchaikov
tchaikov previously approved these changes Jun 6, 2022
@tchaikov tchaikov dismissed their stale review June 6, 2022 08:22

dismissed for now.

@tchaikov
Copy link
Contributor

tchaikov commented Jun 6, 2022

-- mgr module disabled for 3.8.10: diskprediction_local
CMake Error at /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
  Could NOT find ndctl (missing: ndctl_LIBRARY ndctl_INCLUDE_DIR) (Required
  is at least version "63")

@CongMinYin
Copy link
Contributor Author

-- mgr module disabled for 3.8.10: diskprediction_local
CMake Error at /usr/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
  Could NOT find ndctl (missing: ndctl_LIBRARY ndctl_INCLUDE_DIR) (Required
  is at least version "63")

Because WITH_PMEM is not enabled in the ceph-build project. ndctl and daxctl is not installed in the test environment. so ndctl can't be found, then resulte in an error of make check. Please take a look at ceph/ceph-build#2011.

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@CongMinYin
Copy link
Contributor Author

Hi @idryomov , could you please also have a look at this PR? I want to separate the dependencies of BLUESTORE_PMEM and RWL. Another reason is that the DWITH_RBD_RWL option is turned on by default in some cases (see #39049), while WITH_BLUESTORE_PMEM is usually turned off by default. These two should not be put together.

@CongMinYin
Copy link
Contributor Author

ping @tchaikov

src/CMakeLists.txt Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please fold "cmake: add findndctl function" and "cmake: add finddaxctl function" into a single commit as they are really the same.

src/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/modules/Buildpmdk.cmake Outdated Show resolved Hide resolved
cmake/modules/Buildpmdk.cmake Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/modules/Findndctl.cmake Outdated Show resolved Hide resolved
cmake/modules/Finddaxctl.cmake Outdated Show resolved Hide resolved
cmake/modules/Finddaxctl.cmake Outdated Show resolved Hide resolved
@CongMinYin
Copy link
Contributor Author

jenkins test make check

@tchaikov
Copy link
Contributor

@tchaikov
Copy link
Contributor

tchaikov commented Jul 10, 2022

hi @idryomov , could you take another look? i think CongMin has addressed all of the comments.

Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack from the RBD perspective

ceph.spec.in Outdated Show resolved Hide resolved
In order to support the character device of pmem usage in bluestore via
libpmem built by Ceph itself, we need to enable daxctl and ndctl
dependency. add the installation of ndctl and find it. the version of
ndctl and daxctl library requires >63. "apt-get install" meet the version
under ubuntu focal.

the installation of ndctl-devel in ceph.spec.in has not been verified.

Signed-off-by: Yin Congmin <congmin.yin@intel.com>
In order to enable the pmem character device, add ndctl=y to the parameter
of compiling the pmdk library when WITH_BLUESTORE_PMEM is ON. Because
after find_ndctl and find_daxctl are added, the dependency packages
required by WITH_BLUESTORE_PMEM and WITH_RBD_RWL become more different.
So separated these two. libpmem has no version required. libpmemobj
required version >=1.8.

Signed-off-by: Yin Congmin <congmin.yin@intel.com>
@CongMinYin
Copy link
Contributor Author

Hi @tchaikov @idryomov , added the version requirement in ceph.spec.in, thanks. please check.

@idryomov
Copy link
Contributor

LGTM!

@idryomov
Copy link
Contributor

jenkins test windows

@idryomov
Copy link
Contributor

idryomov commented Jul 12, 2022

@tchaikov I'm going to run this through the part of the rbd suite that exercises WITH_RBD_RWL. If additional testing is desired, please re-add needs-qa label.

@tchaikov
Copy link
Contributor

@tchaikov I'm going to run this through the part of the rbd suite that exercises WITH_RBD_RWL. If additional testing is desired, please re-add needs-qa label.

hi Ilya, that should suffice. thank you!

@idryomov idryomov changed the title cmake: add findndctl.cmake cmake: enable ndctl when building PMDK for WITH_BLUESTORE_PMEM Jul 14, 2022
@idryomov idryomov merged commit 795daff into ceph:main Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants