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.

Sign up for GitHub

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

Jump to bottom

cmake/ninja: support ninja for jaegertracing #38783

Merged
merged 16 commits into from Jul 22, 2021
Merged

cmake/ninja: support ninja for jaegertracing #38783

merged 16 commits into from Jul 22, 2021

Conversation

ideepika
Copy link
Member

@ideepika ideepika commented Jan 6, 2021 •
edited by tchaikov

Fixes: https://tracker.ceph.com/issues/51029

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@ideepika ideepika changed the title [WIP]: performacing testing for jaegertracing [WIP]: performace testing for jaegertracing Jan 6, 2021
@ideepika ideepika force-pushed the wip-osd-tracing branch 3 times, most recently from 520b898 to f5cceda Compare January 6, 2021 13:44
@ideepika ideepika mentioned this pull request Jan 7, 2021
Closed
@github-actions
Copy link

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

@ideepika
Copy link
Member Author

jenkins test make check

@ideepika ideepika force-pushed the wip-osd-tracing branch 3 times, most recently from 241fc6c to 2ec57d3 Compare February 17, 2021 09:27
@ideepika ideepika force-pushed the wip-osd-tracing branch 4 times, most recently from 532f214 to db4a5ea Compare March 17, 2021 11:42
@ideepika ideepika force-pushed the wip-osd-tracing branch 2 times, most recently from c9c8fb6 to e479183 Compare June 1, 2021 10:03
@ideepika ideepika changed the title [WIP]: performace testing for jaegertracing src/CMake/modules: support ninja for jaegertracing Jun 4, 2021
@ideepika ideepika changed the title src/CMake/modules: support ninja for jaegertracing cmake/ninja: support ninja for jaegertracing Jun 4, 2021
@t-msn
Copy link
Contributor

t-msn commented Jun 10, 2021 •
edited

Hi, I tried this and it works, thanks.
I'm not sure if the first commit is still needed since I think bionic will no longer be supported.
Anyway it would be good this will be merged soon as build on master is broken.

@ideepika
Copy link
Member Author

Hi, I tried this and it works, thanks.
I'm not sure if the first commit is still needed since I think bionic will no longer be supported.
Anyway it would be good this will be merged soon as build on master is broken.

yep, just testing in shaman: https://shaman.ceph.com/builds/ceph/wip-osd-tracing/ needs a fix, will investigate

@github-actions
Copy link

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

ideepika
ideepika commented Jul 21, 2021
View reviewed changes
Copy link
Member Author

@ideepika ideepika left a comment

Choose a reason for hiding this comment

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

@tchaikov tried to group logically all PR commits , hope it helps with review, thanks for your comments! :)

ceph.spec.in Show resolved Hide resolved
debian/control Show resolved Hide resolved
debian/control Outdated Show resolved Hide resolved
debian/libjaeger.install Show resolved Hide resolved
@ideepika ideepika removed the request for review from a team July 21, 2021 17:03
@yanghonggang yanghonggang mentioned this pull request Jul 22, 2021
Closed
3 tasks
tchaikov
tchaikov requested changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment •
edited

Choose a reason for hiding this comment

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

apart from the indent nit in vstart.sh, see #38783 (comment). LGTM.

src/vstart.sh Outdated Show resolved Hide resolved
Deepika Upadhyay added 12 commits July 22, 2021 10:56
vstart.sh: use quay to pull jaeger instead of docker
9581902 
In servers like the ones available in sepia labs, users might hit rate
limiting for docker pull, it is better to use quay image to avoid this
issue.

https://blog.container-solutions.com/dealing-with-docker-hub-rate-limiting

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake, ceph.spec, debian: use yaml-cpp >= 0.6
00f4478 
* since focal and centos both have yaml-cpp 0.6 available, which dropped
having boost as it's dependency, moving to 0.6 seems a good upgrade.

* cmake: delete Buildyaml, since distro suppilies v0.6 this is not needed

This fixes the build failure, as jaegertracing requires yaml-cpp v0.6+
```
Could NOT find yaml-cpp: Found unsuitable version "", but required is at
  least "0.5.1" (found yaml-cpp_LIBRARY-NOTFOUND)

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
ceph.spec: enable WITH_JAEGER=ON if --with-jaeger passed
e92b474 
Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
ceph.spec, cmake, debian: use thrift 0.13+ from distro pkg
80e8268 
the change to build and ship libthift was added when we didn't have 0.13.0
version shipped via distro pkgs, now that centos 8 and F34 supports req.
version, we do not need to build and ship it with jaeger library.

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: add find_package(thrift) support for thrift 0.13
84ad544 
adds Findthrift.cmake which is used to find thrift 0.13, as a dependency
for building jaegertracing

* bump up submodule version for jaeger-client-cpp for thrift compiler
removal

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
debian, install-deps.sh: use Build-Profiles to optionally build with …
be43f61 
…jaeger

we will now be using BuildProfileSpec based optional pkg building,
removing comment from .install file is no longer needed If
pkg.ceph.jaeger is enabled debian/control, it shall work to install
jaeger and it's dependencies.

  https://wiki.debian.org/BuildProfileSpec.

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: move ExternalProjectHelper & IncludeJaeger to BuildJaeger
f51480b 
set_library_properties_for_external_project assists with setting right target
properties for all jaeger dependencies.
IncludeJaeger would take care of linking and creating these targets
having them spread out when they are highly coupled seems not optimal.

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: target name Jaeger >> jaegertracing
2eea508 
since jaegertracing is the original target that jaeger submodule uses in
it's cmake, cmake build complained if named otherwise

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: minor reorder of boost path in jaeger build external projet
1e2278e 
* Boost is a dependency for jaeger, to use the right version, we pass
ceph build boost path to cmake jaeger build step

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: adds BUILD_BYPRODUCT for external build jaegertracing libs
ffc3764 
* adds BUILD_BYPRODUCT which tells ninja which library will be generated
after the build(needed for dependent build libs)

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: change debian DESTDIR for installing jaeger deps
a92a7aa 
debian uses debian/tmp as destination dir for installing build files,
but since we are using common path($build_dir/external) available both
for rpm and debian based dependency installation, it becomes far more
complicated to maintain include/link path for these external projects.

elaborating on it:
path we are configuring for both rpm and debian installing including,
and linking of external librarires:
/build/ceph-17.0.0-5779-g928f9e55/obj-x86_64-linux-gnu/external/
debian appends DESTDIR to this path, and hence our predefined target
artificats cannot find correct path for external libs, I tried adding
ENV${DESTDIR} so that it could include correct external lib install
path, but it still cannot find them:

failed to link in case of:
-  install(DIRECTORY $ENV{DESTDIR}${CMAKE_BINARY_DIR}/external/include/jaegertracing
-                $ENV{DESTDIR}${CMAKE_BINARY_DIR}/external/include/opentracing
-          DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
-  include_directories(SYSTEM ${CMAKE_INSTALL_INCLUDEDIR}/jaegertracing)
-  include_directories(SYSTEM ${CMAKE_INSTALL_INCLUDEDIR}/opentracing)

-- Installing: /build/ceph-17.0.0-5790-g6bc03cbd/debian/tmp/build/ceph-17.0.0-5790-g6bc03cbd/obj-x86_64-linux-gnu/external/include/jaegertracing/Tracer.h

cd /build/ceph-17.0.0-5790-g6bc03cbd/obj-x86_64-linux-gnu/src && /usr/bin/c++  -DBOOST_ASIO_DISABLE_THREAD_KEYWORD_EXTENSION -DBOOST_ASIO_USE_TS_EXECUTOR_AS_DEFAULT -DHAVE_CONFIG_H -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_REENTRANT -D_THREAD_SAFE -D__CEPH__ -D__STDC_FORMAT_MACROS -D__linux__ -I/build/ceph-17.0.0-5790-g6bc03cbd/obj-x86_64-linux-gnu/src/include -I/build/ceph-17.0.0-5790-g6bc03cbd/src -isystem /build/ceph-17.0.0-5790-g6bc03cbd/obj-x86_64-linux-gnu/boost/include -isystem /build/ceph-17.0.0-5790-g6bc03cbd/obj-x86_64-linux-gnu/include -isystem /build/ceph-17.0.0-5790-g6bc03cbd/src/xxHash -isystem /build/ceph-17.0.0-5790-g6bc03cbd/src/rapidjson/include -isystem /build/ceph-17.0.0-5790-g6bc03cbd/src/include/jaegertracing -isystem /build/ceph-17.0.0-5790-g6bc03cbd/src/include/opentracing  -g -O2 -fdebug-prefix-map=/build/ceph-17.0.0-5790-g6bc03cbd=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC   -U_FORTIFY_SOURCE -Wall -fno-strict-aliasing -fsigned-char -Wtype-limits -Wignored-qualifiers -Wpointer-arith -Werror=format-security -Winit-self -Wno-unknown-pragmas -Wnon-virtual-dtor -Wno-ignored-qualifiers -ftemplate-depth-1024 -Wpessimizing-move -Wredundant-move -Wstrict-null-sentinel -Woverloaded-virtual -fno-new-ttp-matching -fstack-protector-strong -fdiagnostics-color=auto -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -std=c++17 -o CMakeFiles/common-objs.dir/mds/mdstypes.cc.o -c /build/ceph-17.0.0-5790-g6bc03cbd/src/mds/mdstypes.cc
In file included from /build/ceph-17.0.0-5790-g6bc03cbd/src/osd/OpRequest.h:21,
                 from /build/ceph-17.0.0-5790-g6bc03cbd/src/osd/OpRequest.cc:3:
/build/ceph-17.0.0-5790-g6bc03cbd/src/common/tracer.h:10:10: fatal error: jaegertracing/Tracer.h: No such file or directory

Since the install path is in our build environment for these librarires,
skipping DESTDIR looks to me hacky fix, but does the job.
with empty destdir:

-- Installing: /build/ceph-17.0.0-5791-gb97b9640/obj-x86_64-linux-gnu/external/include/jaegertracing/Tracer.h

cd /build/ceph-17.0.0-5791-gb97b9640/obj-x86_64-linux-gnu/src/mon && /usr/bin/c++  -DBOOST_ASIO_DISABLE_THREAD_KEYWORD_EXTENSION -DBOOST_ASIO_USE_TS_EXECUTOR_AS_DEFAULT -DHAVE_CONFIG_H -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_REENTRANT -D_THREAD_SAFE -D__CEPH__ -D__STDC_FORMAT_MACROS -D__linux__ -I/build/ceph-17.0.0-5791-gb97b9640/obj-x86_64-linux-gnu/src/include -I/build/ceph-17.0.0-5791-gb97b9640/src -isystem /build/ceph-17.0.0-5791-gb97b9640/obj-x86_64-linux-gnu/boost/include -isystem /build/ceph-17.0.0-5791-gb97b9640/obj-x86_64-linux-gnu/include -isystem /build/ceph-17.0.0-5791-gb97b9640/src/xxHash -isystem /build/ceph-17.0.0-5791-gb97b9640/src/rapidjson/include -isystem /build/ceph-17.0.0-5791-gb97b9640/obj-x86_64-linux-gnu/external/include -isystem /build/ceph-17.0.0-5791-gb97b9640/src/rocksdb/include  -g -O2 -fdebug-prefix-map=/build/ceph-17.0.0-5791-gb97b9640=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC   -U_FORTIFY_SOURCE -Wall -fno-strict-aliasing -fsigned-char -Wtype-limits -Wignored-qualifiers -Wpointer-arith -Werror=format-security -Winit-self -Wno-unknown-pragmas -Wnon-virtual-dtor -Wno-ignored-qualifiers -ftemplate-depth-1024 -Wpessimizing-move -Wredundant-move -Wstrict-null-sentinel -Woverloaded-virtual -fno-new-ttp-matching -fstack-protector-strong -fdiagnostics-color=auto -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -std=c++17 -o CMakeFiles/mon.dir/MgrMonitor.cc.o -c /build/ceph-17.0.0-5791-gb97b9640/src/mon/MgrMonitor.cc

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
cmake: fix jaeger linking to ceph libraries
5f5de4c 
updates:
* custom target jaeger_base
* target jaeger-base which encapsulates all jaeger libs
* include external libraries path needed linking libraries and including
  opentracing and jaegertracing headers files

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
@tchaikov
Copy link
Contributor

https://shaman.ceph.com/builds/ceph/wip-kefu-testing-2021-07-22-1355/62349ba4ed828e2c66621b5adb1c6b435719d322/

@tchaikov tchaikov merged commit 7048172 into ceph:master Jul 22, 2021
@yanghonggang yanghonggang mentioned this pull request Jul 23, 2021
Closed
3 tasks
@yuvalif yuvalif mentioned this pull request Jul 26, 2021
Merged
cypherean pushed a commit to cypherean/ceph that referenced this pull request Aug 20, 2021
@cypherean
debian, install-deps.sh: use Build-Profiles to optionally build debia…
c616f3d 
…n libjaeger

library

* use pkg.ceph.jaeger for debian optional pkgs
remove mangling needing install-deps, instead use, buildProfileSpec feature
introduced for debian.  https://wiki.debian.org/BuildProfileSpec * check and

* set extraopts in debian/rules using pkg.ceph.jaeger
see: ceph#38783 (comment)

* cleanup libjaeger.install mangling from CMakeLists

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
Waadkh7 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Aug 25, 2021
debian, install-deps.sh: use Build-Profiles to optionally build debia…
cbe5ab3 
…n libjaeger

library

* use pkg.ceph.jaeger for debian optional pkgs
remove mangling needing install-deps, instead use, buildProfileSpec feature
introduced for debian.  https://wiki.debian.org/BuildProfileSpec * check and

* set extraopts in debian/rules using pkg.ceph.jaeger
see: ceph#38783 (comment)

* cleanup libjaeger.install mangling from CMakeLists

Signed-off-by: Deepika Upadhyay <dupadhya@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

Assignees
No one assigned
Labels
build/ops core needs-qa orchestrator pybind wip-kefu-testing
Projects
None yet
Milestone
No milestone
3 participants
@ideepika @t-msn @tchaikov