Open 389-ds-bot opened 4 years ago
389-ds-bot
commented
4 years ago Comment from mreynolds (@mreynolds389) at 2019-06-13 14:59:30
We also need to keep in mind jemalloc and ASAN:
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.
389-ds-bot
commented
4 years ago Comment from mreynolds (@mreynolds389) at 2019-06-13 14:59:30
Metadata Update from @mreynolds389:
389-ds-bot
commented
4 years ago Comment from firstyear (@Firstyear) at 2019-06-13 15:07:55
@mreynolds389 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.
389-ds-bot
commented
4 years ago Comment from mhonek (@kenoh) at 2019-06-13 15:34:56
@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!
389-ds-bot
commented
4 years ago Comment from firstyear (@Firstyear) at 2019-06-13 15:36:13
The review was in person. @darix please comment here ...
389-ds-bot
commented
4 years ago Comment from darix at 2019-06-13 16:21:15
so my main points were:
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.
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.
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
389-ds-bot
commented
4 years ago Comment from mreynolds (@mreynolds389) at 2019-06-13 16:38:10
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?
389-ds-bot
commented
4 years ago Comment from mhonek (@kenoh) at 2019-06-13 17:03:44
@mreynolds389 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?
389-ds-bot
commented
4 years ago Comment from mreynolds (@mreynolds389) at 2019-06-13 17:13:54
Metadata Update from @mreynolds389:
389-ds-bot
commented
4 years ago Comment from darix at 2019-06-13 20:18:45
On Thu, 13 Jun 2019 14:38:10 +0000 (UTC) Mark Reynolds pagure@pagure.io wrote:
mreynolds389 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?
389-ds-bot
commented
4 years ago Comment from darix at 2019-06-13 20:21:40
On Thu, 13 Jun 2019 15:03:45 +0000 (UTC) Matus Honek pagure@pagure.io wrote:
kenoh added a new comment to an issue you are following:
` @mreynolds389 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.
389-ds-bot
commented
4 years ago Comment from firstyear (@Firstyear) at 2019-06-14 09:41:58
@mreynolds389 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 @kenoh 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.
389-ds-bot
commented
4 years ago Comment from firstyear (@Firstyear) at 2019-06-18 15:15:26
So to make a more concrete proposal here:
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 :)
389-ds-bot
commented
4 years ago Comment from darix at 2019-06-18 15:19:41
- We COULD create /etc/systemd/systemd/dirsrv@
.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@
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
389-ds-bot
commented
4 years ago Comment from firstyear (@Firstyear) at 2019-06-18 15:39:31
Perhaps then we could have the comment in the dirsrv@.service to be "you can find an example at
That would be cleaner than the creation of the .conf.d for each instance.
389-ds-bot
commented
4 years ago Comment from mreynolds (@mreynolds389) at 2020-02-26 16:21:11
Metadata Update from @mreynolds389:
Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/50442
Issue Description
/usr/lib/systemd/system/dirsrv@.service.d/custom.confhas been flagged as an incorrect method of applying settings to all instances during SUSE package review. The content of the file (the timeouts) should be in the main dirsrv@.service file, and the custom.conf should be an example in /usr/share/dirsrv. Then the main dirsrv@.service file should reference this as documentation instead.We can't provide a global-drop in .conf for systemd in other words as it's now allowed downstream. RH/Fedora may provide or encounter similar feedback.