Hi,
the attached patches are the first self-contained part of my performance work. Using them, I analyzed the performance of 'id' as the worst-case, then realized most of the issues are around processing and storing large groups. The results gathered using these scripts are in fact the base of the improvements documented at: https://fedorahosted.org/sssd/wiki/DesignDocs/OneFourteenPerformanceImprovem...
I tried to add descriptive commit messages to the patches so that hopefully all the info is in those commit messages. We will probably add more probes into different areas of code, but these are the ones that I already haven't modified for weeks, so I think it should be safe to merge them.
The probes are enabled in our RPMs and I would encourage downstreams to enable them as well. There is virtually no cost of having the probes compiled in by default and the benefit would be that admins can run them to see what are the bottlenecks in their environment. I was also wondering if it makes sense to add a generic back-end-request tracking probes and an associated script, but since the data provider is being re-factored at the moment, I think it would be better to land the DP changes first.
The reason I split the sysdb probes into several commits (unlike the LDAP probes) is that I think in future, we can use these commits as a reference for adding more probes to different areas of SSSD.
CI: http://sssd-ci.duckdns.org/logs/job/42/99/summary.html
(BTW, thanks to Lukas for helping me a lot with the build failures on #sssd last week)
On Mon, May 09, 2016 at 10:04:37AM +0200, Jakub Hrozek wrote:
Hi,
the attached patches are the first self-contained part of my performance work. Using them, I analyzed the performance of 'id' as the worst-case, then realized most of the issues are around processing and storing large groups. The results gathered using these scripts are in fact the base of the improvements documented at: https://fedorahosted.org/sssd/wiki/DesignDocs/OneFourteenPerformanceImprovem...
I tried to add descriptive commit messages to the patches so that hopefully all the info is in those commit messages. We will probably add more probes into different areas of code, but these are the ones that I already haven't modified for weeks, so I think it should be safe to merge them.
The probes are enabled in our RPMs and I would encourage downstreams to enable them as well. There is virtually no cost of having the probes compiled in by default and the benefit would be that admins can run them to see what are the bottlenecks in their environment. I was also wondering if it makes sense to add a generic back-end-request tracking probes and an associated script, but since the data provider is being re-factored at the moment, I think it would be better to land the DP changes first.
The reason I split the sysdb probes into several commits (unlike the LDAP probes) is that I think in future, we can use these commits as a reference for adding more probes to different areas of SSSD.
CI: http://sssd-ci.duckdns.org/logs/job/42/99/summary.html
(BTW, thanks to Lukas for helping me a lot with the build failures on #sssd last week)
Hi,
the attached patches just fix a Coverity issue in one of the patches.
On (03/06/16 12:44), Jakub Hrozek wrote:
On Mon, May 09, 2016 at 10:04:37AM +0200, Jakub Hrozek wrote:
Hi,
the attached patches are the first self-contained part of my performance work. Using them, I analyzed the performance of 'id' as the worst-case, then realized most of the issues are around processing and storing large groups. The results gathered using these scripts are in fact the base of the improvements documented at: https://fedorahosted.org/sssd/wiki/DesignDocs/OneFourteenPerformanceImprovem...
I tried to add descriptive commit messages to the patches so that hopefully all the info is in those commit messages. We will probably add more probes into different areas of code, but these are the ones that I already haven't modified for weeks, so I think it should be safe to merge them.
The probes are enabled in our RPMs and I would encourage downstreams to enable them as well. There is virtually no cost of having the probes compiled in by default and the benefit would be that admins can run them to see what are the bottlenecks in their environment. I was also wondering if it makes sense to add a generic back-end-request tracking probes and an associated script, but since the data provider is being re-factored at the moment, I think it would be better to land the DP changes first.
The reason I split the sysdb probes into several commits (unlike the LDAP probes) is that I think in future, we can use these commits as a reference for adding more probes to different areas of SSSD.
CI: http://sssd-ci.duckdns.org/logs/job/42/99/summary.html
(BTW, thanks to Lukas for helping me a lot with the build failures on #sssd last week)
Hi,
the attached patches just fix a Coverity issue in one of the patches.
From 2e6de9cf0278525e61af915220b8bdcf667abb13 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:05:59 +0100 Subject: [PATCH 02/10] BUILD: Add build infrastructure for systemtap scripts
Adds infrastructure that generatest the probes.h and probes.o from the dtrace probes.d file. The probes.d file is empty except for the provider name in this commit, its content will be added with later commits that actually add some content. The probes.d file is always distributed in the tarball so that distributions can optionally enable systemtap support.
The generation is done using the "dtrace" command because the probes.d file is compatible with the Solaris dtrace format. Please see "man 1 dtrace" for more information on the dtrace format and the command line tool.
In order to make libtool happy, a fake libtool object is generated. This hunk was taken from the libvirt code.
The AM_V_GEN macro is used to make the build compatible with the silent build configuration.
To enable systemtap probing, configure sssd with: --enable-systemtap In order to do so, the 'dtrace' command-line utility must be installed. On Fedora and RHEL, this package is installed as part of the "systemtap-sdt-devel" package.
You'll also want the 'systemtap' package installed as well as the matching versions of kernel-devel and kernel-debuginfo on your machine.
Makefile.am | 43 +++++++++++++++++++++++++++++++++++++++++-- configure.ac | 4 ++++ src/external/systemtap.m4 | 35 +++++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 2 ++ src/tests/cwrap/Makefile.am | 6 ++++++ src/util/probes.h | 2 ++ 6 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 src/external/systemtap.m4 create mode 100644 src/systemtap/sssd_probes.d
//snip
diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 new file mode 100644 index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba --- /dev/null +++ b/src/external/systemtap.m4 @@ -0,0 +1,35 @@ +dnl A macro to check the availability of systemtap user-space probes +AC_DEFUN([AM_CHECK_SYSTEMTAP], +[
- AC_ARG_ENABLE([systemtap],
[AS_HELP_STRING([--enable-systemtap],[Enable inclusion of systemtap trace support])],[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])- if test "x${ENABLE_SYSTEMTAP}" = xyes; then
AC_CHECK_PROGS(DTRACE, dtrace)if test -z "$DTRACE"; thenAC_MSG_ERROR([dtrace not found])fiAC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],
^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"
[SDT_H_FOUND='no';AC_MSG_ERROR([systemtap support needs sys/sdt.h header])])
e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled])HAVE_SYSTEMTAP=1
^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.
AC_ARG_WITH([tapset-install-dir],[AS_HELP_STRING([--with-tapset-install-dir],
^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"
[The absolute path where the tapset dir will be installed])],[if test "x${withval}" = x; thentapset_dir="\$(datadir)/systemtap/tapset"elsetapset_dir="${withval}"fi],
It can be simplified to 'tapset_dir="${withval}"' after updating help string.
[tapset_dir="\$(datadir)/systemtap/tapset"])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)
AC_SUBST(tapset_dir)- fi
- AM_CONDITIONAL([BUILD_SYSTEMTAP], [test x$HAVE_SYSTEMTAP = x1])
+]) diff --git a/src/systemtap/sssd_probes.d b/src/systemtap/sssd_probes.d new file mode 100644 index 0000000000000000000000000000000000000000..7579577c8661e862d83b9fc7c9fe205d25be30ed --- /dev/null +++ b/src/systemtap/sssd_probes.d @@ -0,0 +1,2 @@ +provider sssd { +} diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am index a5afb8c020ee00b680aac0680bcf6f6495dd8222..abf50a56f3a9b5e24759e819b24ba01d65192f37 100644 --- a/src/tests/cwrap/Makefile.am +++ b/src/tests/cwrap/Makefile.am @@ -88,6 +88,9 @@ server_tests_LDADD = \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \ $(NULL) +if BUILD_SYSTEMTAP +server_tests_LDADD += $(abs_top_builddir)/stap_generated_probes.lo +endif
usertools_tests_SOURCES = \ test_usertools.c \ @@ -103,6 +106,9 @@ usertools_tests_LDADD = \ $(abs_top_builddir)/libsss_debug.la \ $(abs_top_builddir)/libsss_test_common.la \ $(NULL) +if BUILD_SYSTEMTAP +usertools_tests_LDADD += $(abs_top_builddir)/stap_generated_probes.lo +endif
responder_common_tests_SOURCES =\ test_responder_common.c \ diff --git a/src/util/probes.h b/src/util/probes.h index 349f29086e86a51b4551f09f542d0f45bf6f1aae..effce492d0a6cbcfe5b4c347b8931e102b853db2 100644 --- a/src/util/probes.h +++ b/src/util/probes.h @@ -20,6 +20,8 @@
#ifdef HAVE_SYSTEMTAP
+#include "stap_generated_probes.h"
/* Probe expansion inspired by libvirt */ #define PROBE_EXPAND(NAME, ...) NAME(__VA_ARGS__)
-- 2.4.11
From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:20:28 +0100 Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions
Actually adds marks for sysdb transactions that receive the transaction nesting level as an argument. The nesting is passed on from probes to marks along with a human-friendly description.
The transaction commit is decorated with two probes, before and after. This would allow the caller to distinguish between the time we spend in the transaction (which might be important, because if a transaction is active on an ldb context, even the readers are blocked before the transaction completes) and the time we spend commiting the transaction (which is important because that's when the disk writes occur)
The probes would be installed into /usr/share/systemtap/tapset on RHEL and Fedora. This is in line with systemtap's paths which are described in detail in "man 7 stappaths".
Makefile.am | 4 +++- configure.ac | 1 + src/db/sysdb.c | 8 ++++++++ src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 4 ++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 src/systemtap/sssd.stp.in
diff --git a/Makefile.am b/Makefile.am index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ sudolibdir = @sudolibpath@ polkitdir = @polkitdir@ pamconfdir = $(sysconfdir)/pam.d +systemtap_tapdir = @tapset_dir@
UNICODE_LIBS=@UNICODE_LIBS@
@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL)
+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
Do we need to install this file if sssd is not build with systemtap support?
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
int sysdb_transaction_commit(struct sysdb_ctx *sysdb) { int ret; +#ifdef HAVE_SYSTEMTAP
- int commit_nesting = sysdb->transaction_nesting-1;
^^ missing spaces :-)
+#endif
From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Fri, 6 May 2016 15:51:12 +0200 Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI
So far, systemtap is only enabled for Red Hat distributions.
contrib/ci/configure.sh | 6 ++++++ contrib/sssd.spec.in | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
+fi
Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
LS
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 new file mode 100644 index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba --- /dev/null +++ b/src/external/systemtap.m4 @@ -0,0 +1,35 @@ +dnl A macro to check the availability of systemtap user-space probes +AC_DEFUN([AM_CHECK_SYSTEMTAP], +[
- AC_ARG_ENABLE([systemtap],
[AS_HELP_STRING([--enable-systemtap],[Enable inclusion of systemtap trace support])],[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])- if test "x${ENABLE_SYSTEMTAP}" = xyes; then
AC_CHECK_PROGS(DTRACE, dtrace)if test -z "$DTRACE"; thenAC_MSG_ERROR([dtrace not found])fiAC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"
Fixed
[SDT_H_FOUND='no';AC_MSG_ERROR([systemtap support needs sys/sdt.h header])])e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled])HAVE_SYSTEMTAP=1^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.
Didn't seem so, I thought AC_DEFINE really only sets the config.h variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
AC_ARG_WITH([tapset-install-dir],[AS_HELP_STRING([--with-tapset-install-dir],^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"
Fixed
[The absolute path where the tapset dir will be installed])],[if test "x${withval}" = x; thentapset_dir="\$(datadir)/systemtap/tapset"elsetapset_dir="${withval}"fi],It can be simplified to 'tapset_dir="${withval}"' after updating help string.
Fixed
[tapset_dir="\$(datadir)/systemtap/tapset"])^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)
Fixed
From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:20:28 +0100 Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions
Actually adds marks for sysdb transactions that receive the transaction nesting level as an argument. The nesting is passed on from probes to marks along with a human-friendly description.
The transaction commit is decorated with two probes, before and after. This would allow the caller to distinguish between the time we spend in the transaction (which might be important, because if a transaction is active on an ldb context, even the readers are blocked before the transaction completes) and the time we spend commiting the transaction (which is important because that's when the disk writes occur)
The probes would be installed into /usr/share/systemtap/tapset on RHEL and Fedora. This is in line with systemtap's paths which are described in detail in "man 7 stappaths".
Makefile.am | 4 +++- configure.ac | 1 + src/db/sysdb.c | 8 ++++++++ src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 4 ++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 src/systemtap/sssd.stp.in
diff --git a/Makefile.am b/Makefile.am index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ sudolibdir = @sudolibpath@ polkitdir = @polkitdir@ pamconfdir = $(sysconfdir)/pam.d +systemtap_tapdir = @tapset_dir@
UNICODE_LIBS=@UNICODE_LIBS@
@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL)
+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
Do we need to install this file if sssd is not build with systemtap support?
I think we need to distribute it in the tarball, but not install without systemtap. So I guess we can use dist_noist_DATA if systemtap is not configured?
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Sorry, can't reproduce it here. Did you run make srpms from Makefile?
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
int sysdb_transaction_commit(struct sysdb_ctx *sysdb) { int ret; +#ifdef HAVE_SYSTEMTAP
- int commit_nesting = sysdb->transaction_nesting-1;
^^ missing spaces :-)+#endif
From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Fri, 6 May 2016 15:51:12 +0200 Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI
So far, systemtap is only enabled for Red Hat distributions.
contrib/ci/configure.sh | 6 ++++++ contrib/sssd.spec.in | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
+fi
Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
Me neither so far, I just sent a CI build to see what happens..
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
OK, so far I added some comments and pushed along with the first fixes to github. I will wait for the next round of patches until we figure out the distcheck failures.
On (07/06/16 23:27), Jakub Hrozek wrote:
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 new file mode 100644 index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba --- /dev/null +++ b/src/external/systemtap.m4 @@ -0,0 +1,35 @@ +dnl A macro to check the availability of systemtap user-space probes +AC_DEFUN([AM_CHECK_SYSTEMTAP], +[
- AC_ARG_ENABLE([systemtap],
[AS_HELP_STRING([--enable-systemtap],[Enable inclusion of systemtap trace support])],[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])- if test "x${ENABLE_SYSTEMTAP}" = xyes; then
AC_CHECK_PROGS(DTRACE, dtrace)if test -z "$DTRACE"; thenAC_MSG_ERROR([dtrace not found])fiAC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"Fixed
[SDT_H_FOUND='no';AC_MSG_ERROR([systemtap support needs sys/sdt.h header])])e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled])HAVE_SYSTEMTAP=1^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.Didn't seem so, I thought AC_DEFINE really only sets the config.h variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
You are right. I didn't remember it properly. AC_SUBST sets variable and not AC_DEFINE But we do not need to use substitution in makefile.am http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html...
AC_ARG_WITH([tapset-install-dir],[AS_HELP_STRING([--with-tapset-install-dir],^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"Fixed
[The absolute path where the tapset dir will be installed])],[if test "x${withval}" = x; thentapset_dir="\$(datadir)/systemtap/tapset"elsetapset_dir="${withval}"fi],It can be simplified to 'tapset_dir="${withval}"' after updating help string.Fixed
[tapset_dir="\$(datadir)/systemtap/tapset"])^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)Fixed
From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:20:28 +0100 Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions
Actually adds marks for sysdb transactions that receive the transaction nesting level as an argument. The nesting is passed on from probes to marks along with a human-friendly description.
The transaction commit is decorated with two probes, before and after. This would allow the caller to distinguish between the time we spend in the transaction (which might be important, because if a transaction is active on an ldb context, even the readers are blocked before the transaction completes) and the time we spend commiting the transaction (which is important because that's when the disk writes occur)
The probes would be installed into /usr/share/systemtap/tapset on RHEL and Fedora. This is in line with systemtap's paths which are described in detail in "man 7 stappaths".
Makefile.am | 4 +++- configure.ac | 1 + src/db/sysdb.c | 8 ++++++++ src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 4 ++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 src/systemtap/sssd.stp.in
diff --git a/Makefile.am b/Makefile.am index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ sudolibdir = @sudolibpath@ polkitdir = @polkitdir@ pamconfdir = $(sysconfdir)/pam.d +systemtap_tapdir = @tapset_dir@
UNICODE_LIBS=@UNICODE_LIBS@
@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL)
+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
Do we need to install this file if sssd is not build with systemtap support?
I think we need to distribute it in the tarball, but not install without systemtap. So I guess we can use dist_noist_DATA if systemtap is not configured?
It will be part of tarball even if is part of conditional build. if BUILD_SYSTEMTAP ...
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Sorry, can't reproduce it here. Did you run make srpms from Makefile?
I forgot to mention that you need to test without spec file chages. But following diff fixed issue.
diff --git a/Makefile.am b/Makefile.am index 3a9858f..7bcac65 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1079,6 +1079,7 @@ endif # Systemtap tracing # #########################
+if BUILD_SYSTEMTAP SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL) @@ -1088,7 +1089,11 @@ dist_systemtap_tap_DATA = \ $(builddir)/src/systemtap/sssd_functions.stp \ $(NULL)
-if BUILD_SYSTEMTAP +dist_sssdtapscript_DATA = \ + contrib/systemtap/id_perf.stp \ + contrib/systemtap/nested_group_perf.stp \ + $(NULL) + stap_generated_probes.h: $(srcdir)/src/systemtap/sssd_probes.d $(AM_V_GEN)$(DTRACE) -C -h -s $< -o $@
@@ -1112,11 +1117,6 @@ CLEANFILES += stap_generated_probes.h \ $(NULL) endif
-dist_sssdtapscript_DATA = \ - contrib/systemtap/id_perf.stp \ - contrib/systemtap/nested_group_perf.stp \ - $(NULL) -
I moved dist_sssdtapscript_DATA closer to the dist_systemtap_tap_DATA so it is part of conditional build and IMHO it's nicer if they are together :-)
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
int sysdb_transaction_commit(struct sysdb_ctx *sysdb) { int ret; +#ifdef HAVE_SYSTEMTAP
- int commit_nesting = sysdb->transaction_nesting-1;
^^ missing spaces :-)+#endif
From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Fri, 6 May 2016 15:51:12 +0200 Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI
So far, systemtap is only enabled for Red Hat distributions.
contrib/ci/configure.sh | 6 ++++++ contrib/sssd.spec.in | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
+fi
Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
Me neither so far, I just sent a CI build to see what happens..
I could not see any issue on debian with following change diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index 823d327..8e779cf 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -28,6 +28,7 @@ declare -a CONFIGURE_ARG_LIST=( "--disable-static" "--enable-ldb-version-check" "--with-syslog=journald" + "--enable-systemtap" )
@@ -47,12 +48,6 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
-if [[ "$DISTRO_FAMILY" == "redhat" ]]; then - CONFIGURE_ARG_LIST+=( - "--enable-systemtap" - ) -fi - declare -r -a CONFIGURE_ARG_LIST
fi # _CONFIGURE_SH diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..4fc1d2d 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd + systemtap-sdt-dev ) DEPS_INTGCHECK_SATISFIED=true fi
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
OK, so far I added some comments and pushed along with the first fixes to github. I will wait for the next round of patches until we figure out the distcheck failures.
which distcheck failures do you mean?
LS
On (08/06/16 19:11), Lukas Slebodnik wrote:
On (07/06/16 23:27), Jakub Hrozek wrote:
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 new file mode 100644 index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba --- /dev/null +++ b/src/external/systemtap.m4 @@ -0,0 +1,35 @@ +dnl A macro to check the availability of systemtap user-space probes +AC_DEFUN([AM_CHECK_SYSTEMTAP], +[
- AC_ARG_ENABLE([systemtap],
[AS_HELP_STRING([--enable-systemtap],[Enable inclusion of systemtap trace support])],[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])- if test "x${ENABLE_SYSTEMTAP}" = xyes; then
AC_CHECK_PROGS(DTRACE, dtrace)if test -z "$DTRACE"; thenAC_MSG_ERROR([dtrace not found])fiAC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"Fixed
[SDT_H_FOUND='no';AC_MSG_ERROR([systemtap support needs sys/sdt.h header])])e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled])HAVE_SYSTEMTAP=1^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.Didn't seem so, I thought AC_DEFINE really only sets the config.h variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
You are right. I didn't remember it properly. AC_SUBST sets variable and not AC_DEFINE But we do not need to use substitution in makefile.am http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html...
AC_ARG_WITH([tapset-install-dir],[AS_HELP_STRING([--with-tapset-install-dir],^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"Fixed
[The absolute path where the tapset dir will be installed])],[if test "x${withval}" = x; thentapset_dir="\$(datadir)/systemtap/tapset"elsetapset_dir="${withval}"fi],It can be simplified to 'tapset_dir="${withval}"' after updating help string.Fixed
[tapset_dir="\$(datadir)/systemtap/tapset"])^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)Fixed
From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:20:28 +0100 Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions
Actually adds marks for sysdb transactions that receive the transaction nesting level as an argument. The nesting is passed on from probes to marks along with a human-friendly description.
The transaction commit is decorated with two probes, before and after. This would allow the caller to distinguish between the time we spend in the transaction (which might be important, because if a transaction is active on an ldb context, even the readers are blocked before the transaction completes) and the time we spend commiting the transaction (which is important because that's when the disk writes occur)
The probes would be installed into /usr/share/systemtap/tapset on RHEL and Fedora. This is in line with systemtap's paths which are described in detail in "man 7 stappaths".
Makefile.am | 4 +++- configure.ac | 1 + src/db/sysdb.c | 8 ++++++++ src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 4 ++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 src/systemtap/sssd.stp.in
diff --git a/Makefile.am b/Makefile.am index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ sudolibdir = @sudolibpath@ polkitdir = @polkitdir@ pamconfdir = $(sysconfdir)/pam.d +systemtap_tapdir = @tapset_dir@
UNICODE_LIBS=@UNICODE_LIBS@
@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL)
+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
Do we need to install this file if sssd is not build with systemtap support?
I think we need to distribute it in the tarball, but not install without systemtap. So I guess we can use dist_noist_DATA if systemtap is not configured?
It will be part of tarball even if is part of conditional build. if BUILD_SYSTEMTAP ...
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Sorry, can't reproduce it here. Did you run make srpms from Makefile?
I forgot to mention that you need to test without spec file chages. But following diff fixed issue.
diff --git a/Makefile.am b/Makefile.am index 3a9858f..7bcac65 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1079,6 +1079,7 @@ endif # Systemtap tracing # #########################
+if BUILD_SYSTEMTAP SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL) @@ -1088,7 +1089,11 @@ dist_systemtap_tap_DATA = \ $(builddir)/src/systemtap/sssd_functions.stp \ $(NULL)
-if BUILD_SYSTEMTAP +dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)stap_generated_probes.h: $(srcdir)/src/systemtap/sssd_probes.d $(AM_V_GEN)$(DTRACE) -C -h -s $< -o $@
@@ -1112,11 +1117,6 @@ CLEANFILES += stap_generated_probes.h \ $(NULL) endif
-dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)I moved dist_sssdtapscript_DATA closer to the dist_systemtap_tap_DATA so it is part of conditional build and IMHO it's nicer if they are together :-)
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
int sysdb_transaction_commit(struct sysdb_ctx *sysdb) { int ret; +#ifdef HAVE_SYSTEMTAP
- int commit_nesting = sysdb->transaction_nesting-1;
^^ missing spaces :-)+#endif
From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Fri, 6 May 2016 15:51:12 +0200 Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI
So far, systemtap is only enabled for Red Hat distributions.
contrib/ci/configure.sh | 6 ++++++ contrib/sssd.spec.in | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
+fi
Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
Me neither so far, I just sent a CI build to see what happens..
I could not see any issue on debian with following change diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index 823d327..8e779cf 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -28,6 +28,7 @@ declare -a CONFIGURE_ARG_LIST=( "--disable-static" "--enable-ldb-version-check" "--with-syslog=journald"
- "--enable-systemtap"
)
@@ -47,12 +48,6 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
-if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
-fi
declare -r -a CONFIGURE_ARG_LIST
fi # _CONFIGURE_SH diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..4fc1d2d 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd
systemtap-sdt-devfi
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
OK, so far I added some comments and pushed along with the first fixes to github. I will wait for the next round of patches until we figure out the distcheck failures.
which distcheck failures do you mean?
BTW I also noticed that tarball contains src/systemtap/sssd.stp and also template src/systemtap/sssd.stp.in
IMHO there should be only template
tar -tnvzf sssd-1.13.90.tar.gz | grep systemtap drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/src/systemtap/ -rw-rw-r-- user/user 7988 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd.stp.in -rw-rw-r-- user/user 2201 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_functions.stp -rw-rw-r-- user/user 8408 2016-06-09 11:30 sssd-1.13.90/src/systemtap/sssd.stp -rw-rw-r-- user/user 2682 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_probes.d -rw-rw-r-- user/user 1385 2016-06-09 11:29 sssd-1.13.90/src/external/systemtap.m4 drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/contrib/systemtap/ -rw-rw-r-- user/user 9614 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/nested_group_perf.stp -rw-rw-r-- user/user 4991 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/id_perf.stp
LS
On Thu, Jun 09, 2016 at 11:33:40AM +0200, Lukas Slebodnik wrote:
On (08/06/16 19:11), Lukas Slebodnik wrote:
On (07/06/16 23:27), Jakub Hrozek wrote:
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 new file mode 100644 index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba --- /dev/null +++ b/src/external/systemtap.m4 @@ -0,0 +1,35 @@ +dnl A macro to check the availability of systemtap user-space probes +AC_DEFUN([AM_CHECK_SYSTEMTAP], +[
- AC_ARG_ENABLE([systemtap],
[AS_HELP_STRING([--enable-systemtap],[Enable inclusion of systemtap trace support])],[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])- if test "x${ENABLE_SYSTEMTAP}" = xyes; then
AC_CHECK_PROGS(DTRACE, dtrace)if test -z "$DTRACE"; thenAC_MSG_ERROR([dtrace not found])fiAC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"Fixed
[SDT_H_FOUND='no';AC_MSG_ERROR([systemtap support needs sys/sdt.h header])])e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled])HAVE_SYSTEMTAP=1^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.Didn't seem so, I thought AC_DEFINE really only sets the config.h variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
You are right. I didn't remember it properly. AC_SUBST sets variable and not AC_DEFINE But we do not need to use substitution in makefile.am http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html...
AC_ARG_WITH([tapset-install-dir],[AS_HELP_STRING([--with-tapset-install-dir],^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"Fixed
[The absolute path where the tapset dir will be installed])],[if test "x${withval}" = x; thentapset_dir="\$(datadir)/systemtap/tapset"elsetapset_dir="${withval}"fi],It can be simplified to 'tapset_dir="${withval}"' after updating help string.Fixed
[tapset_dir="\$(datadir)/systemtap/tapset"])^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)Fixed
From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:20:28 +0100 Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions
Actually adds marks for sysdb transactions that receive the transaction nesting level as an argument. The nesting is passed on from probes to marks along with a human-friendly description.
The transaction commit is decorated with two probes, before and after. This would allow the caller to distinguish between the time we spend in the transaction (which might be important, because if a transaction is active on an ldb context, even the readers are blocked before the transaction completes) and the time we spend commiting the transaction (which is important because that's when the disk writes occur)
The probes would be installed into /usr/share/systemtap/tapset on RHEL and Fedora. This is in line with systemtap's paths which are described in detail in "man 7 stappaths".
Makefile.am | 4 +++- configure.ac | 1 + src/db/sysdb.c | 8 ++++++++ src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 4 ++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 src/systemtap/sssd.stp.in
diff --git a/Makefile.am b/Makefile.am index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ sudolibdir = @sudolibpath@ polkitdir = @polkitdir@ pamconfdir = $(sysconfdir)/pam.d +systemtap_tapdir = @tapset_dir@
UNICODE_LIBS=@UNICODE_LIBS@
@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL)
+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
Do we need to install this file if sssd is not build with systemtap support?
I think we need to distribute it in the tarball, but not install without systemtap. So I guess we can use dist_noist_DATA if systemtap is not configured?
It will be part of tarball even if is part of conditional build. if BUILD_SYSTEMTAP ...
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Sorry, can't reproduce it here. Did you run make srpms from Makefile?
I forgot to mention that you need to test without spec file chages. But following diff fixed issue.
diff --git a/Makefile.am b/Makefile.am index 3a9858f..7bcac65 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1079,6 +1079,7 @@ endif # Systemtap tracing # #########################
+if BUILD_SYSTEMTAP SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL) @@ -1088,7 +1089,11 @@ dist_systemtap_tap_DATA = \ $(builddir)/src/systemtap/sssd_functions.stp \ $(NULL)
-if BUILD_SYSTEMTAP +dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)stap_generated_probes.h: $(srcdir)/src/systemtap/sssd_probes.d $(AM_V_GEN)$(DTRACE) -C -h -s $< -o $@
@@ -1112,11 +1117,6 @@ CLEANFILES += stap_generated_probes.h \ $(NULL) endif
-dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)I moved dist_sssdtapscript_DATA closer to the dist_systemtap_tap_DATA so it is part of conditional build and IMHO it's nicer if they are together :-)
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
int sysdb_transaction_commit(struct sysdb_ctx *sysdb) { int ret; +#ifdef HAVE_SYSTEMTAP
- int commit_nesting = sysdb->transaction_nesting-1;
^^ missing spaces :-)+#endif
From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Fri, 6 May 2016 15:51:12 +0200 Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI
So far, systemtap is only enabled for Red Hat distributions.
contrib/ci/configure.sh | 6 ++++++ contrib/sssd.spec.in | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
+fi
Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
Me neither so far, I just sent a CI build to see what happens..
I could not see any issue on debian with following change diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index 823d327..8e779cf 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -28,6 +28,7 @@ declare -a CONFIGURE_ARG_LIST=( "--disable-static" "--enable-ldb-version-check" "--with-syslog=journald"
- "--enable-systemtap"
)
@@ -47,12 +48,6 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
-if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
-fi
declare -r -a CONFIGURE_ARG_LIST
fi # _CONFIGURE_SH diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..4fc1d2d 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd
systemtap-sdt-devfi
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
OK, so far I added some comments and pushed along with the first fixes to github. I will wait for the next round of patches until we figure out the distcheck failures.
which distcheck failures do you mean?
BTW I also noticed that tarball contains src/systemtap/sssd.stp and also template src/systemtap/sssd.stp.in
IMHO there should be only template
tar -tnvzf sssd-1.13.90.tar.gz | grep systemtap drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/src/systemtap/ -rw-rw-r-- user/user 7988 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd.stp.in -rw-rw-r-- user/user 2201 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_functions.stp -rw-rw-r-- user/user 8408 2016-06-09 11:30 sssd-1.13.90/src/systemtap/sssd.stp -rw-rw-r-- user/user 2682 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_probes.d -rw-rw-r-- user/user 1385 2016-06-09 11:29 sssd-1.13.90/src/external/systemtap.m4 drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/contrib/systemtap/ -rw-rw-r-- user/user 9614 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/nested_group_perf.stp -rw-rw-r-- user/user 4991 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/id_perf.stp
Thank you for the review, the attached patches should hopefully fix all the issues.
On (09/06/16 18:15), Jakub Hrozek wrote:
On Thu, Jun 09, 2016 at 11:33:40AM +0200, Lukas Slebodnik wrote:
On (08/06/16 19:11), Lukas Slebodnik wrote:
On (07/06/16 23:27), Jakub Hrozek wrote:
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 new file mode 100644 index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba --- /dev/null +++ b/src/external/systemtap.m4 @@ -0,0 +1,35 @@ +dnl A macro to check the availability of systemtap user-space probes +AC_DEFUN([AM_CHECK_SYSTEMTAP], +[
- AC_ARG_ENABLE([systemtap],
[AS_HELP_STRING([--enable-systemtap],[Enable inclusion of systemtap trace support])],[ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no'])- if test "x${ENABLE_SYSTEMTAP}" = xyes; then
AC_CHECK_PROGS(DTRACE, dtrace)if test -z "$DTRACE"; thenAC_MSG_ERROR([dtrace not found])fiAC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'],^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"Fixed
[SDT_H_FOUND='no';AC_MSG_ERROR([systemtap support needs sys/sdt.h header])])e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers]))
AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled])HAVE_SYSTEMTAP=1^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.Didn't seem so, I thought AC_DEFINE really only sets the config.h variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
You are right. I didn't remember it properly. AC_SUBST sets variable and not AC_DEFINE But we do not need to use substitution in makefile.am http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html...
AC_ARG_WITH([tapset-install-dir],[AS_HELP_STRING([--with-tapset-install-dir],^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"Fixed
[The absolute path where the tapset dir will be installed])],[if test "x${withval}" = x; thentapset_dir="\$(datadir)/systemtap/tapset"elsetapset_dir="${withval}"fi],It can be simplified to 'tapset_dir="${withval}"' after updating help string.Fixed
[tapset_dir="\$(datadir)/systemtap/tapset"])^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)Fixed
From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 29 Feb 2016 13:20:28 +0100 Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions
Actually adds marks for sysdb transactions that receive the transaction nesting level as an argument. The nesting is passed on from probes to marks along with a human-friendly description.
The transaction commit is decorated with two probes, before and after. This would allow the caller to distinguish between the time we spend in the transaction (which might be important, because if a transaction is active on an ldb context, even the readers are blocked before the transaction completes) and the time we spend commiting the transaction (which is important because that's when the disk writes occur)
The probes would be installed into /usr/share/systemtap/tapset on RHEL and Fedora. This is in line with systemtap's paths which are described in detail in "man 7 stappaths".
Makefile.am | 4 +++- configure.ac | 1 + src/db/sysdb.c | 8 ++++++++ src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ src/systemtap/sssd_probes.d | 4 ++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 src/systemtap/sssd.stp.in
diff --git a/Makefile.am b/Makefile.am index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ sudolibdir = @sudolibpath@ polkitdir = @polkitdir@ pamconfdir = $(sysconfdir)/pam.d +systemtap_tapdir = @tapset_dir@
UNICODE_LIBS=@UNICODE_LIBS@
@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL)
+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp
Do we need to install this file if sssd is not build with systemtap support?
I think we need to distribute it in the tarball, but not install without systemtap. So I guess we can use dist_noist_DATA if systemtap is not configured?
It will be part of tarball even if is part of conditional build. if BUILD_SYSTEMTAP ...
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Sorry, can't reproduce it here. Did you run make srpms from Makefile?
I forgot to mention that you need to test without spec file chages. But following diff fixed issue.
diff --git a/Makefile.am b/Makefile.am index 3a9858f..7bcac65 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1079,6 +1079,7 @@ endif # Systemtap tracing # #########################
+if BUILD_SYSTEMTAP SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL) @@ -1088,7 +1089,11 @@ dist_systemtap_tap_DATA = \ $(builddir)/src/systemtap/sssd_functions.stp \ $(NULL)
-if BUILD_SYSTEMTAP +dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)stap_generated_probes.h: $(srcdir)/src/systemtap/sssd_probes.d $(AM_V_GEN)$(DTRACE) -C -h -s $< -o $@
@@ -1112,11 +1117,6 @@ CLEANFILES += stap_generated_probes.h \ $(NULL) endif
-dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)I moved dist_sssdtapscript_DATA closer to the dist_systemtap_tap_DATA so it is part of conditional build and IMHO it's nicer if they are together :-)
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
int sysdb_transaction_commit(struct sysdb_ctx *sysdb) { int ret; +#ifdef HAVE_SYSTEMTAP
- int commit_nesting = sysdb->transaction_nesting-1;
^^ missing spaces :-)+#endif
From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Fri, 6 May 2016 15:51:12 +0200 Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI
So far, systemtap is only enabled for Red Hat distributions.
contrib/ci/configure.sh | 6 ++++++ contrib/sssd.spec.in | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+)
diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
+fi
Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
Me neither so far, I just sent a CI build to see what happens..
I could not see any issue on debian with following change diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index 823d327..8e779cf 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -28,6 +28,7 @@ declare -a CONFIGURE_ARG_LIST=( "--disable-static" "--enable-ldb-version-check" "--with-syslog=journald"
- "--enable-systemtap"
)
@@ -47,12 +48,6 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
-if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
-fi
declare -r -a CONFIGURE_ARG_LIST
fi # _CONFIGURE_SH diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..4fc1d2d 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd
systemtap-sdt-devfi
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
OK, so far I added some comments and pushed along with the first fixes to github. I will wait for the next round of patches until we figure out the distcheck failures.
which distcheck failures do you mean?
BTW I also noticed that tarball contains src/systemtap/sssd.stp and also template src/systemtap/sssd.stp.in
IMHO there should be only template
tar -tnvzf sssd-1.13.90.tar.gz | grep systemtap drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/src/systemtap/ -rw-rw-r-- user/user 7988 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd.stp.in -rw-rw-r-- user/user 2201 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_functions.stp -rw-rw-r-- user/user 8408 2016-06-09 11:30 sssd-1.13.90/src/systemtap/sssd.stp -rw-rw-r-- user/user 2682 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_probes.d -rw-rw-r-- user/user 1385 2016-06-09 11:29 sssd-1.13.90/src/external/systemtap.m4 drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/contrib/systemtap/ -rw-rw-r-- user/user 9614 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/nested_group_perf.stp -rw-rw-r-- user/user 4991 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/id_perf.stp
Thank you for the review, the attached patches should hopefully fix all the issues.
http://sssd-ci.duckdns.org/logs/job/45/06/summary.html
ACK
LS
On (10/06/16 18:14), Lukas Slebodnik wrote:
On (09/06/16 18:15), Jakub Hrozek wrote:
On Thu, Jun 09, 2016 at 11:33:40AM +0200, Lukas Slebodnik wrote:
On (08/06/16 19:11), Lukas Slebodnik wrote:
On (07/06/16 23:27), Jakub Hrozek wrote:
On Mon, Jun 06, 2016 at 05:50:03PM +0200, Lukas Slebodnik wrote:
>diff --git a/src/external/systemtap.m4 b/src/external/systemtap.m4 >new file mode 100644 >index 0000000000000000000000000000000000000000..d1caa2017f0730394339f0a439046df6b56cb2ba >--- /dev/null >+++ b/src/external/systemtap.m4 >@@ -0,0 +1,35 @@ >+dnl A macro to check the availability of systemtap user-space probes >+AC_DEFUN([AM_CHECK_SYSTEMTAP], >+[ >+ AC_ARG_ENABLE([systemtap], >+ [AS_HELP_STRING([--enable-systemtap], >+ [Enable inclusion of systemtap trace support])], >+ [ENABLE_SYSTEMTAP="${enableval}"], [ENABLE_SYSTEMTAP='no']) >+ >+ if test "x${ENABLE_SYSTEMTAP}" = xyes; then >+ AC_CHECK_PROGS(DTRACE, dtrace) >+ if test -z "$DTRACE"; then >+ AC_MSG_ERROR([dtrace not found]) >+ fi >+ >+ AC_CHECK_HEADER([sys/sdt.h], [SDT_H_FOUND='yes'], ^^^^^^^^^^^ this variable is not used anywhere. it would be better to use default action "[]"
Fixed
>+ [SDT_H_FOUND='no'; >+ AC_MSG_ERROR([systemtap support needs sys/sdt.h header])]) e.g. AC_CHECK_HEADERS([check.h],,AC_MSG_ERROR([Could not find CHECK headers])) >+ >+ AC_DEFINE([HAVE_SYSTEMTAP], [1], [Define to 1 if systemtap is enabled]) >+ HAVE_SYSTEMTAP=1 ^^^^^^^^^^^^^^ it should already be set to 1 by AC_DEFINE.
Didn't seem so, I thought AC_DEFINE really only sets the config.h variable. When I removed this line, the AC_CONDITIONAL wasn't evaluated.
You are right. I didn't remember it properly. AC_SUBST sets variable and not AC_DEFINE But we do not need to use substitution in makefile.am http://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html...
>+ >+ AC_ARG_WITH([tapset-install-dir], >+ [AS_HELP_STRING([--with-tapset-install-dir], ^ It would be good to append here string "=DIR" So it will be clear that there is expected argument We already have it for "--with-systemdunitdir"
Fixed
>+ [The absolute path where the tapset dir will be installed])], >+ [if test "x${withval}" = x; then >+ tapset_dir="$(datadir)/systemtap/tapset" >+ else >+ tapset_dir="${withval}" >+ fi], It can be simplified to 'tapset_dir="${withval}"' after updating help string.
Fixed
>+ [tapset_dir="$(datadir)/systemtap/tapset"]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this default value could be mentioned in help string as well. @see output of ./configure --help (with-pubconf-path, with-pipe-path ...)
Fixed
>From c1a4fba84679bb4ce10badcc9458088e4a94d281 Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek jhrozek@redhat.com >Date: Mon, 29 Feb 2016 13:20:28 +0100 >Subject: [PATCH 04/10] SYSDB: Add systemtap probes to track sysdb transactions > >Actually adds marks for sysdb transactions that receive the transaction >nesting level as an argument. The nesting is passed on from probes to >marks along with a human-friendly description. > >The transaction commit is decorated with two probes, before and after. >This would allow the caller to distinguish between the time we spend in >the transaction (which might be important, because if a transaction is >active on an ldb context, even the readers are blocked before the >transaction completes) and the time we spend commiting the transaction >(which is important because that's when the disk writes occur) > >The probes would be installed into /usr/share/systemtap/tapset on RHEL >and Fedora. This is in line with systemtap's paths which are described >in detail in "man 7 stappaths". >--- > Makefile.am | 4 +++- > configure.ac | 1 + > src/db/sysdb.c | 8 ++++++++ > src/systemtap/sssd.stp.in | 32 ++++++++++++++++++++++++++++++++ > src/systemtap/sssd_probes.d | 4 ++++ > 5 files changed, 48 insertions(+), 1 deletion(-) > create mode 100644 src/systemtap/sssd.stp.in > >diff --git a/Makefile.am b/Makefile.am >index 33930759ab82b65643a9a0a071fd92b025dab145..1b4ba3cc651b29c55f7b43c90bc479f584a911e2 100644 >--- a/Makefile.am >+++ b/Makefile.am >@@ -81,6 +81,7 @@ krb5rcachedir = @krb5rcachedir@ > sudolibdir = @sudolibpath@ > polkitdir = @polkitdir@ > pamconfdir = $(sysconfdir)/pam.d >+systemtap_tapdir = @tapset_dir@ > > UNICODE_LIBS=@UNICODE_LIBS@ > >@@ -1081,6 +1082,8 @@ SYSTEMTAP_PROBES = \ > $(srcdir)/src/systemtap/sssd_probes.d \ > $(NULL) > >+dist_systemtap_tap_DATA = $(builddir)/src/systemtap/sssd.stp >+ Do we need to install this file if sssd is not build with systemtap support?
I think we need to distribute it in the tarball, but not install without systemtap. So I guess we can use dist_noist_DATA if systemtap is not configured?
It will be part of tarball even if is part of conditional build. if BUILD_SYSTEMTAP ...
The same applies to other patches and automake variables dist_systemtap_tap_DATA, dist_sssdtapscript_DATA
It caused mock failures RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_tools.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ldap.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_krb5.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ipa.lang Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_ad.lang Installed (but unpackaged) file(s) found: /usr/share/sssd/systemtap/id_perf.stp
Sorry, can't reproduce it here. Did you run make srpms from Makefile?
I forgot to mention that you need to test without spec file chages. But following diff fixed issue.
diff --git a/Makefile.am b/Makefile.am index 3a9858f..7bcac65 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1079,6 +1079,7 @@ endif # Systemtap tracing # #########################
+if BUILD_SYSTEMTAP SYSTEMTAP_PROBES = \ $(srcdir)/src/systemtap/sssd_probes.d \ $(NULL) @@ -1088,7 +1089,11 @@ dist_systemtap_tap_DATA = \ $(builddir)/src/systemtap/sssd_functions.stp \ $(NULL)
-if BUILD_SYSTEMTAP +dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)stap_generated_probes.h: $(srcdir)/src/systemtap/sssd_probes.d $(AM_V_GEN)$(DTRACE) -C -h -s $< -o $@
@@ -1112,11 +1117,6 @@ CLEANFILES += stap_generated_probes.h \ $(NULL) endif
-dist_sssdtapscript_DATA = \
contrib/systemtap/id_perf.stp \contrib/systemtap/nested_group_perf.stp \$(NULL)I moved dist_sssdtapscript_DATA closer to the dist_systemtap_tap_DATA so it is part of conditional build and IMHO it's nicer if they are together :-)
Lang files might be unrelated but there are *.stp files in output. http://sssd-ci.duckdns.org/logs/job/44/50/summary.html It works after applying the last patch from patchset.
> int sysdb_transaction_commit(struct sysdb_ctx *sysdb) > { > int ret; >+#ifdef HAVE_SYSTEMTAP >+ int commit_nesting = sysdb->transaction_nesting-1; ^^ missing spaces :-) >+#endif
>From def7601820c687c55db87136f77ce1cd06affa29 Mon Sep 17 00:00:00 2001 >From: Jakub Hrozek jhrozek@redhat.com >Date: Fri, 6 May 2016 15:51:12 +0200 >Subject: [PATCH 10/10] BUILD: Enable systemtap during RPM build and CI > >So far, systemtap is only enabled for Red Hat distributions. >--- > contrib/ci/configure.sh | 6 ++++++ > contrib/sssd.spec.in | 19 +++++++++++++++++++ > 2 files changed, 25 insertions(+) > >diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh >index c850eb9ce9a4228c1a89b8b2b49311e1c748b7de..823d3277c0467d1abf0e746c910d2a0e16783693 100644 >--- a/contrib/ci/configure.sh >+++ b/contrib/ci/configure.sh >@@ -47,6 +47,12 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || > ) > fi > >+if [[ "$DISTRO_FAMILY" == "redhat" ]]; then >+ CONFIGURE_ARG_LIST+=( >+ "--enable-systemtap" >+ ) >+fi >+ Maybe we can enable a build also on debian with installed package systemtap-sdt-dev (I didn't try)
Me neither so far, I just sent a CI build to see what happens..
I could not see any issue on debian with following change diff --git a/contrib/ci/configure.sh b/contrib/ci/configure.sh index 823d327..8e779cf 100644 --- a/contrib/ci/configure.sh +++ b/contrib/ci/configure.sh @@ -28,6 +28,7 @@ declare -a CONFIGURE_ARG_LIST=( "--disable-static" "--enable-ldb-version-check" "--with-syslog=journald"
- "--enable-systemtap"
)
@@ -47,12 +48,6 @@ if [[ "$DISTRO_BRANCH" == -redhat-redhatenterprise*-7.*- || ) fi
-if [[ "$DISTRO_FAMILY" == "redhat" ]]; then
- CONFIGURE_ARG_LIST+=(
"--enable-systemtap"- )
-fi
declare -r -a CONFIGURE_ARG_LIST
fi # _CONFIGURE_SH diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh index c9a8a63..4fc1d2d 100644 --- a/contrib/ci/deps.sh +++ b/contrib/ci/deps.sh @@ -114,6 +114,7 @@ if [[ "$DISTRO_BRANCH" == -debian-* ]]; then python-ldap ldap-utils slapd
systemtap-sdt-devfi
I briefly look at system tap script and they looks good to me. some lines are longer than 80 characters :-) and there could be more comments but it's not a blocker.
OK, so far I added some comments and pushed along with the first fixes to github. I will wait for the next round of patches until we figure out the distcheck failures.
which distcheck failures do you mean?
BTW I also noticed that tarball contains src/systemtap/sssd.stp and also template src/systemtap/sssd.stp.in
IMHO there should be only template
tar -tnvzf sssd-1.13.90.tar.gz | grep systemtap drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/src/systemtap/ -rw-rw-r-- user/user 7988 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd.stp.in -rw-rw-r-- user/user 2201 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_functions.stp -rw-rw-r-- user/user 8408 2016-06-09 11:30 sssd-1.13.90/src/systemtap/sssd.stp -rw-rw-r-- user/user 2682 2016-06-09 11:29 sssd-1.13.90/src/systemtap/sssd_probes.d -rw-rw-r-- user/user 1385 2016-06-09 11:29 sssd-1.13.90/src/external/systemtap.m4 drwxrwxr-x user/user 0 2016-06-09 11:30 sssd-1.13.90/contrib/systemtap/ -rw-rw-r-- user/user 9614 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/nested_group_perf.stp -rw-rw-r-- user/user 4991 2016-06-09 11:29 sssd-1.13.90/contrib/systemtap/id_perf.stp
Thank you for the review, the attached patches should hopefully fix all the issues.
http://sssd-ci.duckdns.org/logs/job/45/06/summary.html
ACK
master: * acf7cee13f07b368b0ccae69776309f7f69cbca1 * 94b860e8fc51c659c6ac09d69481b838555fff76 * c23ea7772113a163139a7b7669303e9e80dc1d09 * 41291f19dbc5bf14f20729959b852fa605fcc02d * 630f3ff08c1d17c7900b9bde814922f775ca2703 * 8c829226ce0cf98c35ffce39a66f9645cff65767 * 6dcbfe52d5e64205c0d922f3e89add066b42c496 * bd93ef2db6d24946ebf98a23fa18d34d45f6b072 * 29c5542feb4c45865ea61be97e0e84a1d1f04918 * 53f1b03f4e61ebe21df0c2fd05e09e0504fd8881
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik