Full_Name: SATOH Fumiyasu Version: master OS: URL: Submission from: (NULL) (118.240.43.13) I've created patches for slapd to add systemd service notification (sd_notify(3)) support. Please see the following URLs: * Patch for master * https://github.com/osstech-jp/openldap/commit/4313f8685ac3365fe71639d37874080da6a54ebf.patch * Patch for 2.4.x (OPENLDAP_REL_ENG_2_4) * https://github.com/osstech-jp/openldap/commit/977d4483470d150839714cece64aeb700d0dbfef.patch
On Mon, Aug 07, 2017 at 01:30:44PM +0000, fumiyas@osstech.co.jp wrote: >I've created patches for slapd to add systemd service notification >(sd_notify(3)) support. Thanks for working on this! I'm also interested in having this feature. What do you think about including a slapd.service file? I know OpenLDAP has traditionally not included an init script, but systemd units are intended to be distro-agnostic as far as possible, and shipped by upstream projects in most cases, unlike init scripts. Ideally I'd like to include a template in the source and have the build system fill in the autoconf'ed path (i.e. to slapd) and have 'make install' install it to the right place. Autoconf bits look fine to me. >+ rc = sd_notifyf( 1, >+ "READY=1\n" >+ "STATUS=slapd: ready to serve connections...\n" >+ "MAINPID=%lu", >+ (unsigned long) getpid() ); unset_environment=1 seems reasonable, it's a little unfortunate that we can't call (for example) sd_notify("STOPPING=1") afterward, but it feels worthwhile compared to having to sanitize the environment when forking a child process. I'm not sure the STATUS= message adds value compared to just the basic readiness notification; can you comment on why you included it? I guess MAINPID= is actually needed, unless we run slapd with -d, regardless of whether we set Type=forking or Type=notify. Not exactly "needed", but better to have it than not. I see you've placed this call later than the parent's exit point. Any comments about the timing of this relative to the parent's exit, and to the listener startup and so on? Are the listeners more likely to be ready to serve connections at this point? I seem to recall that in the past there was opposition to moving the parent's exit later, but I can't remember why. (and I still wish we could do that, and dispense with the ldapsearch-loop hacks...) cheers, Ryan
On Mon, Aug 07, 2017 at 12:23:27PM -0700, Ryan Tandy wrote: >I guess MAINPID= is actually needed, unless we run slapd with -d, regardless of >whether we set Type=forking or Type=notify. Not exactly "needed", but better to >have it than not. I experimented a bit with a service file. It seems to work well with either Type=forking and NotifyAccess=all, or Type=notify and ExecStart=slapd -d none. The latter (disabling forking) is definitely what systemd upstream recommends. In either case, MAINPID= didn't actually seem to help anything. NotifyAccess=main has a chicken-and-egg problem, because systemd needs to know the main PID in order for us to send it the message containing the PID! :) I think the only reasonable way to leave forking enabled would be to also require a PIDFile= setting, which solves that problem. But I'd rather sidestep the entire thing, omit MAINPID= as well, and Looking at the systemctl output I still think STATUS= is redundant and could be omitted. So I guess my recommendation for the notify call boils down to: rc = sd_notify( 1, "READY=1" ); and a slapd.service along the lines of: [Unit] Description=OpenLDAP server [Service] Type=notify ExecStart=%LIBEXECDIR%/slapd -h 'ldap:/// ldapi:///' -d0 [Install] WantedBy=multi-user.target (basically identical to the example in systemd.service(5).) Side note: the version message from slapd appears in the journal twice, once with the timestamp and once without... not sure exactly why!
On Mon, Aug 07, 2017 at 01:12:02PM -0700, Ryan Tandy wrote: >Side note: the version message from slapd appears in the journal >twice, once with the timestamp and once without... not sure exactly >why! Sorry, meant to delete this paragraph before sending. The difference between "-dnone" and "-d0" is precisely an extra log to stderr :)
Hi, Sorry, I'm not good at English. :-) On Tue, 08 Aug 2017 05:12:02 +0900, Ryan Tandy wrote: > > I guess MAINPID= is actually needed, unless we run slapd with -d, regardless of > > whether we set Type=forking or Type=notify. Not exactly "needed", but better to > > have it than not. > > I experimented a bit with a service file. It seems to work well with either Type=forking and NotifyAccess=all, or Type=notify and ExecStart=slapd -d none. The latter (disabling forking) is definitely what systemd upstream recommends. I think so too. I don't like forking, thus I use "Type=notify" and "ExecStart=slapd -d0 ..." in my slapd.service file. > In either case, MAINPID= didn't actually seem to help anything. Yes. > NotifyAccess=main has a chicken-and-egg problem, because systemd needs to know the main PID in order for us to send it the message containing the PID! :) I think the only reasonable way to leave forking enabled would be to also require a PIDFile= setting, which solves that problem. But I'd rather sidestep the entire thing, omit MAINPID= as well, and > Looking at the systemctl output I still think STATUS= is redundant and could be omitted. > > So I guess my recommendation for the notify call boils down to: > > rc = sd_notify( 1, "READY=1" ); Agreed. I'll fix servers/slapd/daemon.c later. Thank you. > and a slapd.service along the lines of: > > [Unit] > Description=OpenLDAP server > > [Service] > Type=notify > ExecStart=%LIBEXECDIR%/slapd -h 'ldap:/// ldapi:///' -d0 > > [Install] > WantedBy=multi-user.target > > (basically identical to the example in systemd.service(5).) My slapd.service file: [Unit] Description=OpenLDAP Server After=syslog.target network-online.target Documentation=man:slapd Documentation=man:slapd.conf Documentation=man:slapd-config Documentation=man:slapd-bdb Documentation=man:slapd-hdb Documentation=man:slapd-mdb [Service] Type=notify ExecStart=%LIBEXECDIR%/slapd -d 0 -h "ldapi:/// ldap:///" [Install] WantedBy=multi-user.target Any comment? -- -- Name: SATOH Fumiyasu @ OSS Technology Corp. (fumiyas @ osstech co jp) -- Business Home: http://www.OSSTech.co.jp/ -- GitHub Home: https://GitHub.com/fumiyas/ -- PGP Fingerprint: BBE1 A1C9 525A 292E 6729 CDEC ADC2 9DCA 5E1C CBCA
On Tue, Aug 08, 2017 at 03:10:35PM +0900, SATOH Fumiyasu wrote: >My slapd.service file: > >[Unit] >Description=OpenLDAP Server >After=syslog.target network-online.target >Documentation=man:slapd >Documentation=man:slapd.conf >Documentation=man:slapd-config >Documentation=man:slapd-bdb >Documentation=man:slapd-hdb >Documentation=man:slapd-mdb > >[Service] >Type=notify >ExecStart=%LIBEXECDIR%/slapd -d 0 -h "ldapi:/// ldap:///" > >[Install] >WantedBy=multi-user.target > >Any comment? No comment, looks good to me. :)
On Tue, 08 Aug 2017 04:23:27 +0900, Ryan Tandy wrote: > > + rc = sd_notifyf( 1, > > + "READY=1\n" > > + "STATUS=slapd: ready to serve connections...\n" > > + "MAINPID=%lu", > > + (unsigned long) getpid() ); > I see you've placed this call later than the parent's exit point. Any comments about the timing of this relative to the parent's exit, and to the listener startup and so on? Are the listeners more likely to be ready to serve connections at this point? I seem to recall that in the past there was opposition to moving the parent's exit later, but I can't remember why. (and I still wish we could do that, and dispense with the ldapsearch-loop hacks...) I've revised the patch for master to check if the listener initialization suceeds or not. With this patch, `slapd -d0` can return non-zero exit code if the listener initialization fails, thus systemd can detect slapd.service startup failure. * https://github.com/osstech-jp/openldap/pull/4 * https://patch-diff.githubusercontent.com/raw/osstech-jp/openldap/pull/4.patch -- -- Name: SATOH Fumiyasu @ OSS Technology Corp. (fumiyas @ osstech co jp) -- Business Home: http://www.OSSTech.co.jp/ -- GitHub Home: https://GitHub.com/fumiyas/ -- PGP Fingerprint: BBE1 A1C9 525A 292E 6729 CDEC ADC2 9DCA 5E1C CBCA
On Wed, Aug 09, 2017 at 05:13:44PM +0900, SATOH Fumiyasu wrote: >I've revised the patch for master to check if the listener initialization >suceeds or not. With this patch, `slapd -d0` can return non-zero exit code >if the listener initialization fails, thus systemd can detect slapd.service >startup failure. Nice catch! That change looks correct to me, but I hope Howard will check it too. Actually it's probably relevant for any init system; possibly worth splitting out as its own commit? Just checking, do you plan to add the service file and its installation (conditional on enabled systemd support) in this branch as well, or should I work on that myself if I want it? Thanks!
On Thu, 10 Aug 2017 02:16:56 +0900, Ryan Tandy wrote: > > I've revised the patch for master to check if the listener initialization > > suceeds or not. With this patch, `slapd -d0` can return non-zero exit code > > if the listener initialization fails, thus systemd can detect slapd.service > > startup failure. > > Nice catch! That change looks correct to me, but I hope Howard will check it too. Actually it's probably relevant for any init system; possibly worth splitting out as its own commit? This patch does NOT affect SysV-init system because an init script for slapd normally does NOT use the -d option. > Just checking, do you plan to add the service file and its installation (conditional on enabled systemd support) in this branch as well, or should I work on that myself if I want it? Yes, I'll do that later. Thank you. -- -- Name: SATOH Fumiyasu @ OSS Technology Corp. (fumiyas @ osstech co jp) -- Business Home: http://www.OSSTech.co.jp/ -- GitHub Home: https://GitHub.com/fumiyas/ -- PGP Fingerprint: BBE1 A1C9 525A 292E 6729 CDEC ADC2 9DCA 5E1C CBCA
Hi, I've added slapd.service file. Please check. * https://github.com/osstech-jp/openldap/pull/4 * https://patch-diff.githubusercontent.com/raw/osstech-jp/openldap/pull/4.patch On Thu, 10 Aug 2017 19:26:48 +0900, fumiyas wrote: > > > I've revised the patch for master to check if the listener initialization > > > suceeds or not. With this patch, `slapd -d0` can return non-zero exit code > > > if the listener initialization fails, thus systemd can detect slapd.service > > > startup failure. > > > > Nice catch! That change looks correct to me, but I hope Howard will check it too. Actually it's probably relevant for any init system; possibly worth splitting out as its own commit? > > This patch does NOT affect SysV-init system because an init script > for slapd normally does NOT use the -d option. > > > Just checking, do you plan to add the service file and its installation (conditional on enabled systemd support) in this branch as well, or should I work on that myself if I want it? > > Yes, I'll do that later. -- -- Name: SATOH Fumiyasu @ OSS Technology Corp. (fumiyas @ osstech co jp) -- Business Home: http://www.OSSTech.co.jp/ -- GitHub Home: https://GitHub.com/fumiyas/ -- PGP Fingerprint: BBE1 A1C9 525A 292E 6729 CDEC ADC2 9DCA 5E1C CBCA
On Fri, Aug 11, 2017 at 08:10:09PM +0900, SATOH Fumiyasu wrote: >I've added slapd.service file. Please check. Looks good to me. Thanks again for working on this! :) Just minor comments: >- $(srcdir)/slapd.conf > slapd.conf.tmp ; \ >+ $(srcdir)/slapd.conf > slapd.conf.tmp || exit $$?; \ >- $(srcdir)/slapd.ldif > slapd.ldif.tmp ; \ >+ $(srcdir)/slapd.ldif > slapd.ldif.tmp || exit $$? Is there any difference between this and using && instead of ;? (Not asking for any change, I just want to make sure I understand.) >+ if test -n "$(systemdsystemunitdir)"; then \ This is echoed while the rest aren't; maybe it could be either appended to the above command, or change to @if.
On Sat, 12 Aug 2017 02:58:20 +0900, Ryan Tandy wrote: > > I've added slapd.service file. Please check. > > Looks good to me. Thanks again for working on this! :) > > Just minor comments: > > > - $(srcdir)/slapd.conf > slapd.conf.tmp ; \ > > + $(srcdir)/slapd.conf > slapd.conf.tmp || exit $$?; \ > > > - $(srcdir)/slapd.ldif > slapd.ldif.tmp ; \ > > + $(srcdir)/slapd.ldif > slapd.ldif.tmp || exit $$? > > Is there any difference between this and using && instead of ;? (Not asking for any change, I just want to make sure I understand.) In Makefile, /bin/sh is invoked without -e option. > > + if test -n "$(systemdsystemunitdir)"; then \ > > This is echoed while the rest aren't; maybe it could be either appended to the above command, or change to @if. Fixed. Thanks! -- -- Name: SATOH Fumiyasu @ OSS Technology Corp. (fumiyas @ osstech co jp) -- Business Home: http://www.OSSTech.co.jp/ -- GitHub Home: https://GitHub.com/fumiyas/ -- PGP Fingerprint: BBE1 A1C9 525A 292E 6729 CDEC ADC2 9DCA 5E1C CBCA
has patch
changed notes
--On Sunday, August 13, 2017 12:55 PM +0000 fumiyas@osstech.co.jp wrote: A couple of things: a) There is no IPR statement provided, so this cannot be examined. b) The OpenLDAP project has never provided init scripts of their equivalents. I'm not sure it would be correct to include the systemd unit file as a part of the project. c) The referenced pull request has unrelated changes around the wired tiger backend mixed in. A clean git-format-patch addressing the above issues would be appreciated. Thanks! --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
quanah@symas.com wrote: > b) The OpenLDAP project has never provided init scripts of > their equivalents. I'm not sure it would be correct to include > the systemd unit file as a part of the project. I also suspect that many Linux distributions are picky regarding their own systemd flavors. My suggestion would be to provide an example systemd unit file as documentation. Ciao, Michael.
On Tue, Sep 12, 2017 at 05:17:11PM +0000, quanah@symas.com wrote: >b) The OpenLDAP project has never provided init scripts of their >equivalents. I'm not sure it would be correct to include the systemd unit >file as a part of the project. An explicit design goal of systemd is that unit files should be uniform enough that it's reasonable for upstream projects to ship them, in contrast to init scripts where the conventions and helper scripts differ a lot from distro to distro. (This obviously does not preclude distro customization of the shipped unit file.) http://0pointer.de/blog/projects/on-etc-sysinit.html: "Part of this is that we want that unit files are supplied with upstream, and not just added by the packager -- how it has usually been done in the SysV world." Of course the unit file should only be installed if the configure switch for systemd is enabled, and I believe the latest patch does this.
--On Tuesday, September 12, 2017 11:30 AM -0700 Ryan Tandy <ryan@nardis.ca> wrote: > "Part of this is that we want that unit files are supplied with upstream, > and not just added by the packager -- how it has usually been done in the > SysV world." > > Of course the unit file should only be installed if the configure switch > for systemd is enabled, and I believe the latest patch does this. I'll leave it to hyc to weigh in. It's not been the policy of the project so far to include such items. --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
ryan@nardis.ca wrote: > An explicit design goal of systemd is that unit files should be > uniform enough that it's reasonable for upstream projects to > ship them, Yes, systemd has many admirable goals. On the other hands Linux distributions differ a lot especially regarding file system layout, not to speak of their systemd back-port patches. Ciao, Michael. P.S.: Right at this moment I'm trying to figure out the appropriate Requires and After lines in systemd unit file template in Æ-DIR's ansible role. And the ansible role has only support for three (and a half) different Linux distributions [1]. [1] https://www.ae-dir.com/install.html#prereq
michael@stroeder.com wrote: > ryan@nardis.ca wrote: >> An explicit design goal of systemd is that unit files should be >> uniform enough that it's reasonable for upstream projects to >> ship them, > > Yes, systemd has many admirable goals. And not enough competent developers to achieve any of them. > On the other hands Linux distributions differ a lot especially > regarding file system layout, not to speak of their systemd > back-port patches. My experience so far with Arch/Debian/Ubuntu/Centos mirrors this - they're all different in FS layouts (/var/run vs /run, etc etc etc) and the situation is no better than it was in SysV init. I see no reason for us to change our policy regarding system-specific dependencies. re: providing an example in OpenLDAP documentation - this is an only slightly less-bad suggestion. The onus is on systemd to provide adequate documentation on how to write unit files, and adequate examples of working files. Our only responsibility is to document the parameters for correctly running the code; it's up to distro packagers to encapsulate a correct invocation into whatever their launch mechanism is. If no one has any other reasons to offer, I'm inclined to reject and close this ITS. > > Ciao, Michael. > > P.S.: > Right at this moment I'm trying to figure out the appropriate > Requires and After lines in systemd unit file template in Æ-DIR's > ansible role. And the ansible role has only support for three (and > a half) different Linux distributions [1]. > > [1] https://www.ae-dir.com/install.html#prereq > > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
On Tue, Sep 12, 2017 at 07:04:48PM +0000, hyc@symas.com wrote: >My experience so far with Arch/Debian/Ubuntu/Centos mirrors this - >they're all different in FS layouts (/var/run vs /run, etc etc etc) and >the situation is no better than it was in SysV init In the case of the unit file proposed, the only system-specific path is the path to slapd itself, and the patch already handles substituting that at install time. (OT: /run as a directory and /var/run as a symlink to it for backwards compatibility is the cross-distro standard today as far as I'm aware, and has been for a few years now. RHEL/CentOS 5/6 are the outliers since they predate /run but are still supported.) >If no one has any other reasons to offer, I'm inclined to reject and >close this ITS. Even if you don't want to ship the unit file, please consider at least accepting the code change to enable the startup notification, which should be completely distro-independent, and no more system-specific than any of our other optional library dependencies.
Howard Chu wrote: > If no one has any other reasons to offer, I'm inclined to reject > and close this ITS. Note that the systemd unit file was only a little detail in this ITS. The most important part is the C code change. Ciao, Michael.
michael@stroeder.com wrote: > Howard Chu wrote: >> If no one has any other reasons to offer, I'm inclined to reject >> and close this ITS. > > Note that the systemd unit file was only a little detail in this > ITS. The most important part is the C code change. OK. Looking at the patch: +dnl +dnl Check for systemd +dnl +WITH_SYSTEMD=no +ol_link_systemd=no +if test $ol_with_systemd != no ; then + AC_CHECK_HEADERS(systemd/sd-daemon.h) + + if test $ac_cv_header_systemd_sd_daemon_h = yes; then + AC_CHECK_LIB(systemd, sd_notify, + [ol_link_systemd="-lsystemd"]) + fi + + if test $ol_link_systemd = no ; then + if test $ol_with_systemd != auto ; then + AC_MSG_ERROR([Could not locate systemd]) + else + AC_MSG_WARN([Could not locate systemd]) + AC_MSG_WARN([systemd service notification not supported!]) + fi + else + AC_DEFINE(HAVE_SYSTEMD,1,[define if you have systemd]) + SYSTEMD_LIBS="$ol_link_systemd" + WITH_SYSTEMD=yes + fi +fi + There should not be any warning if with-systemd is defaulted to auto and it's not found. It should be silently ignored. This isn't like SASL, which is an explicit part of the LDAP spec and therefore has a warning in its absence. +#ifdef HAVE_SYSTEMD + rc = sd_notifyf( 1, + "READY=1\n" + "STATUS=slapd: ready to serve connections...\n" + "MAINPID=%lu", + (unsigned long) getpid() ); + if ( rc < 0 ) + Debug( LDAP_DEBUG_ANY, + "systemd sd_notify failed (%d)\n", rc, 0, 0 ); +#endif /* HAVE_SYSTEMD */ + I don't have documentation for sd_notify() on my machine - what does it return if systemd isn't running at the moment? What does it return if the current program wasn't started by systemd (and thus, the notification is irrelevant)? It strikes me that this code should only be invoked if slapd was actually started by systemd. In particular, I don't want to see a lot of "sd_notify failed" messages in our logs, for manually started slapd processes. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
On Tue, Sep 12, 2017 at 08:24:15PM +0000, hyc@symas.com wrote: >I don't have documentation for sd_notify() on my machine https://www.freedesktop.org/software/systemd/man/sd_notify.html >- what does it return if systemd isn't running at the moment? What does >it return if the current program wasn't started by systemd (and thus, >the notification is irrelevant)? 0 in both of these cases (assuming there is not an unrelated NOTIFY_SOCKET env var). However, it also says: >In order to support both service managers that implement this scheme >and those which do not, it is generally recommended to ignore the >return value of this call." and the current patch doesn't follow that recommendation, since it does check the return value. I don't feel strongly either way about that. >It strikes me that this code should only be invoked if slapd was actually >started by systemd. As I understand it, sd_notify() should be a no-op (no error) if NOTIFY_SOCKET is not set. I don't think we should check NOTIFY_SOCKET ourselves; it's to some extent an implementation detail.
Ryan Tandy wrote: > On Tue, Sep 12, 2017 at 08:24:15PM +0000, hyc@symas.com wrote: >> I don't have documentation for sd_notify() on my machine > > https://www.freedesktop.org/software/systemd/man/sd_notify.html > >> - what does it return if systemd isn't running at the moment? What does it >> return if the current program wasn't started by systemd (and thus, the >> notification is irrelevant)? > > 0 in both of these cases (assuming there is not an unrelated NOTIFY_SOCKET env > var). However, it also says: > >> In order to support both service managers that implement this scheme and >> those which do not, it is generally recommended to ignore the return value >> of this call." > > and the current patch doesn't follow that recommendation, since it does check > the return value. I don't feel strongly either way about that. OK. You realize how egregiously gross this kind of API design is? "Oh, yeah, we return a value, but properly written programs should ignore it." Everything about this makes me nauseous. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Putting on 2.5.0 review list, still lacking any IPR notice.
(In reply to Quanah Gibson-Mount from comment #25) > Putting on 2.5.0 review list, still lacking any IPR notice. The attached file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by Open Source Solution Technology (OSSTech) in Japan. OSSTech has not assigned rights and/or interest in this work to any party. I, SATOH Fumiyasu am authorized by OSSTech, my employer, to release this work under the following terms. OSSTech hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice.
https://git.openldap.org/openldap/openldap/-/merge_requests/261 Rebased for 2.5 and added in lloadd.service as well.
With this patch, the test suite takes an extremely long amount of time. I suspect there are some significant issues with it, as it shouldn't increase the amount of time it takes slapd to execute.
This patch is broken, at the least it's missing a mutex lock prior to calling ldap_pvt_thread_cond_wait( &listener_init_cond, &listener_init_mutex );, see https://git.openldap.org/openldap/openldap/-/merge_requests/261#note_2412
This patch is seriously deficient and will be rejected unless there is follow up in the near future from the patch author.
Commits: • 629cafc9 by Ondřej Kuzník at 2021-04-20T22:54:19+00:00 ITS#8707 Add systemd service notification support • f3501534 by SATOH Fumiyasu at 2021-04-20T22:54:19+00:00 ITS#8707 - Add slapd.service and lloadd.service for systemd • 0786f6d5 by Quanah Gibson-Mount at 2021-04-20T22:54:19+00:00 ITS#8707 - Update CI/CD for systemd notification support • 72caa56a by Ondřej Kuzník at 2021-04-20T22:54:19+00:00 ITS#8707 systemd notifications from lloadd