We also need to keep in mind jemalloc and ASAN:

https://pagure.io/389-ds-base/issue/50425

I was working on a Makefile change to drop in different conf files depending is ASAN was enabled or not, but if using custom.conf (and/or xasn.conf) is wrong we need another solution for these conflicting builds.

@mreynolds There is nothing wrong with custom.conf and drop in's, we just aren't allowed to ship a drop-in by default in the RPM - so perhaps for asan enabled builds we provide it, but for production we don't. IE --enable-asan makes an asan.conf drop in? But just the production build can't ship it.

@firstyear I haven't found anything like that in the Fedora Packaging Guidelines. Could you please provide a link to the SUSE's guidelines that say so and/or a link to the place where the flag has been raised? Thanks!

The review was in person. @darix please comment here ...

so my main points were:

1. global drop in files are good. so having a 389-ds-asan package which
   provides the /usr/lib/systemd/system/dirsrv@.service.d/asan.conf is
   a valid use case. I use it myself to inject the apparmor scope for
   services after installing the profile.

2. the point of the bug was to remove the global include file. you are
   just replacing one central include file with another which kind of
   defeats the purpose.

3. just move the content of the custom.conf to the normal service file
   or have it as documentation file for people to have a starting point
   if they use "systemctl edit dirsrv@service"

--
openSUSE - SUSE Linux is my linux
openSUSE is good for you
www.opensuse.org

The other issue is jemalloc. We need to preload it by default, unless we are doing a ASAN build. Otherwise the ASAN server will fail to start. I have not tried this, but can xasan.conf unset LD_PRELOAD from dirsrv@.service?

@mreynolds You can do something like Environment=LD_PRELOAD= which should, I think, effectively unset the variable (but haven't tested). One point to note is that the files from .../dirsrv@.service.d/ are read in alphabetical order (think initv scripts prefixed with 00-, 01-, ...) and are applied after the main dirsrv@.service file.

@darix My thinking when creating the drop-in file was to separate opinionated settings (those user should customize) from unopinionated (those user should have no reason to change and will likely break the thing). The timeouts in custom.conf are opinionated and sometimes would make sense to change, yet we need to have them by default overridden.

Anyway, in any of the points you provided I see no factual reason as to why this is incorrect, apart from ... kind of defeats the purpose. What is the purpose then?

On Thu, 13 Jun 2019 14:38:10 +0000 (UTC)
Mark Reynolds pagure@pagure.io wrote:

mreynolds added a new comment to an issue you are following:
The other issue is jemalloc. We need to preload it by default, unless we are doing a ASAN build. Otherwise the ASAN server will fail to start. I have not tried this, but can xasan.conf unset LD_PRELOAD from dirsrv@.service?

But yes you can unset values ... the problem is probably that you are
confined to that Environment= thing ... which might make it tricky.

Why not link jemalloc in the non asan case instead of doing the
preloading?

On Thu, 13 Jun 2019 15:03:45 +0000 (UTC)
Matus Honek pagure@pagure.io wrote:

mhonek added a new comment to an issue you are following:
` @mreynolds You can do something likeEnvironment=LD_PRELOAD=which should, I think, effectively unset the variable (but haven't tested). One point to note is that the files from.../dirsrv@.service.d/are read in alphabetical order (think initv scripts prefixed with 00-, 01-, ...) and are applied after the maindirsrv@.service` file.

@darix My thinking when creating the drop-in file was to separate
opinionated settings (those user should customize) from unopinionated
(those user should have no reason to change and will likely break the
thing). The timeouts in custom.conf are opinionated and sometimes
would make sense to change, yet we need to have them by default
overridden.

your opinionated settings are still there. even harder to find for the
user. as most user will not expect global drop in files.

Anyway, in any of the points you provided I see no factual reason as
to why this is incorrect, apart from ... kind of defeats the
purpose. What is the purpose then? ``

I only use that systemd feature when I actually want to override values
from the package. not for default values.

@mreynolds As mentioned, I think for development we can have a "drop in" file for jemalloc/asan, that's no issue because we don't need to follow guidelines, and WE know what's going on. But for released production builds, we should have our settings all in the dirsrv@.service file, and then have the "custom.conf" as an example of how to make a drop in. I do agree with @darix that it's a bit surprising that a global drop in exists, when we could just set defaults into the .service file instead, but I also agree with @mhonek that we should document and demonstrate HOW a drop in should work to help admins, so I think providing custom.conf as documentation is a good middle ground.

So to make a more concrete proposal here:

• We provide the custom.conf in /usr/share/dirsrv/examples/
• We reference /usr/share/dirsrv/examples/dropin.conf from in the dirsrv@.service file so that people can find it (use autoconf to insert this path).
• We COULD create /etc/systemd/systemd/dirsrv@<name>.service.conf.d/ as part of dscreate to make it easier for someone to find the needed location
• For ASAN builds which are DEBUG/DEVEL only (internal) we install and ship a global drop in unit file to support the QE and debug uses cases, or to ship to customers if required. The file must NOT be marked as %config in to ensure it is removed on upgrade/uninstall to prevent tattooing

SUSE wants to use the upstream systemd files as much as possible, so we'd really like to make sure these conform to our packaging standards, and that way we can support each other as much as possible :)

• We COULD create /etc/systemd/systemd/dirsrv@<name>.service.conf.d/
  as part of dscreate to make it easier for someone to find the needed
  location

JFYI:

valid options are:

systemctl edit dirsrv@instance
systemctl edit dirsrv@instance.service

which results in

/etc/systemd/systemd/dirsrv@<name>.service.d/override.conf

as well as

systemctl edit dirsrv@.service

/etc/systemd/systemd/dirsrv@.service.d/override.conf

This will affect all instances.

--
openSUSE - SUSE Linux is my linux
openSUSE is good for you
www.opensuse.org

Perhaps then we could have the comment in the dirsrv@.service to be "you can find an example at <location>. You can then edit an instance with 'systemctl edit dirsrv@instance', or affect all instances by 'systemctl edit dirsrv@.service'"

That would be cleaner than the creation of the .conf.d for each instance.

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/3500

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.