Looks like jemalloc is source3 and nunc-stans is now source4?
{{{
7 7 NUNC_STANS_URL ?= $(shell rpmspec -P -D 'use_nunc_stans 1' $(PWD)/rpm/389-ds-base.spec.in | awk '/^Source3:/ {print $$2}')
8 8 NUNC_STANS_TARBALL ?= $(shell basename "$(NUNC_STANS_URL)")
9 JEMALLOC_URL ?= $(shell rpmspec -P $(PWD)/rpm/389-ds-base.spec.in | awk '/^Source4:/ {print $$2}')
10 JEMALLOC_TARBALL ?= $(shell basename "$(JEMALLOC_URL)")
}}}

but

{{{
128 Source3: http://www.port389.org/binaries/jemalloc-%{jemalloc_ver}.tar.bz2
125 129 %if %{use_nunc_stans}
126 Source3: https://git.fedorahosted.org/cgit/nunc-stans.git/snapshot/nunc-stans-%{nunc_stans_ver}.tar.xz
130 Source4: https://git.fedorahosted.org/cgit/nunc-stans.git/snapshot/nunc-stans-%{nunc_stans_ver}.tar.xz
}}}

I got conflict with Rich's review. :) In addition to the Source[34] issue, I have one request...

1. Could it be a good idea to define use_jemalloc as being done for nunc_stans and put the jemalloc related addition in "%if %{use_jemalloc}"? (It may bring in the same issue we ran into for NUNC_STANS_ON, though... ;)

Replying to [comment:2 rmeggins]:

Looks like jemalloc is source3 and nunc-stans is now source4?
{{{
7 7 NUNC_STANS_URL ?= $(shell rpmspec -P -D 'use_nunc_stans 1' $(PWD)/rpm/389-ds-base.spec.in | awk '/^Source3:/ {print $$2}')
8 8 NUNC_STANS_TARBALL ?= $(shell basename "$(NUNC_STANS_URL)")
9 JEMALLOC_URL ?= $(shell rpmspec -P $(PWD)/rpm/389-ds-base.spec.in | awk '/^Source4:/ {print $$2}')
10 JEMALLOC_TARBALL ?= $(shell basename "$(JEMALLOC_URL)")
}}}

but

{{{
128 Source3: http://www.port389.org/binaries/jemalloc-%{jemalloc_ver}.tar.bz2
125 129 %if %{use_nunc_stans}
126 Source3: https://git.fedorahosted.org/cgit/nunc-stans.git/snapshot/nunc-stans-%{nunc_stans_ver}.tar.xz
130 Source4: https://git.fedorahosted.org/cgit/nunc-stans.git/snapshot/nunc-stans-%{nunc_stans_ver}.tar.xz
}}}

You're right, I'm surprised that didn't cause any problems.

Replying to [comment:3 nhosoi]:

I got conflict with Rich's review. :) In addition to the Source[34] issue, I have one request...

1. Could it be a good idea to define use_jemalloc as being done for nunc_stans and put the jemalloc related addition in "%if %{use_jemalloc}"? (It may bring in the same issue we ran into for NUNC_STANS_ON, though... ;)

Originally I did do it this way, but it causes no harm to always include it(its not modifying slapd) and its not preloaded by default. And... the problems with NUNC_STANS_ON also played a factor in this decision. I don't really see a good reason to have this turned on or off at the build level.

If you feel strongly that it should be configurable at the build level I'll gladly add it in.

Replying to [comment:5 mreynolds]:

Originally I did do it this way, but it causes no harm to always include it(its not modifying slapd) and its not preloaded by default. And... the problems with NUNC_STANS_ON also played a factor in this decision. I don't really see a good reason to have this turned on or off at the build level.

If you feel strongly that it should be configurable at the build level I'll gladly add it in.

No, I do not. If there is no harm, I'm happy with it. Thanks for the explanation.

Replying to [comment:6 nhosoi]:

Replying to [comment:5 mreynolds]:

Originally I did do it this way, but it causes no harm to always include it(its not modifying slapd) and its not preloaded by default. And... the problems with NUNC_STANS_ON also played a factor in this decision. I don't really see a good reason to have this turned on or off at the build level.

If you feel strongly that it should be configurable at the build level I'll gladly add it in.

No, I do not. If there is no harm, I'm happy with it. Thanks for the explanation.

There's also no harm to add the on/off option, so I'm going to add it back. I'll fix the NUNC_STANS_ON issue as well.

Looks good to me. Thanks a lot, Mark!

I think we can build/package 389-ds-base with both use_nunc_stans and bundle_jemalloc are true as well as just bundle_jemalloc is true. But not just use_nunc_stans is true? (please correct me if I'm wrong...) I think that's fine. We are packaging 389-ds-base with both true...

Replying to [comment:8 nhosoi]:

Looks good to me. Thanks a lot, Mark!

I think we can build/package 389-ds-base with both use_nunc_stans and bundle_jemalloc are true as well as just bundle_jemalloc is true. But not just use_nunc_stans is true? (please correct me if I'm wrong...) I think that's fine. We are packaging 389-ds-base with both true...

Actually things break if you disable nunc-stans or jemalloc when using rpm.mk. I fixed this in the patch I just attached.

revision #3
0001-Ticket-48377-Bundle-jemalloc-with-Directory-Server.patch

You have my ack. And setting the target milestone to 1.3.5.0.
Thanks!

5a54717..f132cf4 master -> master
commit f132cf4
Author: Mark Reynolds mreynolds@redhat.com
Date: Wed Dec 16 15:19:24 2015 -0500

What's the reason behind bundling jemalloc? It's already packaged in Fedora and EPEL: http://koji.fedoraproject.org/koji/packageinfo?packageID=11238

Also bundling is not recommended by Fedora packaging guidelines: https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

Replying to [comment:12 vashirov]:

What's the reason behind bundling jemalloc? It's already packaged in Fedora and EPEL: http://koji.fedoraproject.org/koji/packageinfo?packageID=11238

Also bundling is not recommended by Fedora packaging guidelines: https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

First, we aren't adding the package, just including the shared library in the DS lib dir.

Second, this was discussed in our QE meeting last Thursday (you were a part of that discussion). jemalloc was not available in RHEL (which has no current plans to include it). So we need to provide it for platforms that currently do not have it available.

If Nathan is fine with using EPEL so am I, but I think he wanted something available outside of epel. We'll see what he says.

Replying to [comment:13 mreynolds]:

First, we aren't adding the package, just including the shared library in the DS lib dir.
I understand, but I think that bundling is not needed at least in Fedora, since the jemalloc is already packaged there. Also there is a [https://bugzilla.redhat.com/show_bug.cgi?id=788500 precedent], when jemalloc was unbundled from the package.
Second, this was discussed in our QE meeting last Thursday (you were a part of that discussion). jemalloc was not available in RHEL (which has no current plans to include it). So we need to provide it for platforms that currently do not have it available.
I'm sorry for misunderstanding, but I don't remember that we agreed on a final decision.
If Nathan is fine with using EPEL so am I, but I think he wanted something available outside of epel. We'll see what he says.
I don't think that using EPEL is also a viable solution, since EPEL is not supported. Ideally, we need to move package from EPEL to RHEL. I don't know how hard it would be, but considering that jemalloc is already packaged, might be a little bit easier.

I just found that Nathan already filed a bugzilla to include jemalloc: https://bugzilla.redhat.com/show_bug.cgi?id=1292560

Replying to [comment:12 vashirov]:

What's the reason behind bundling jemalloc? It's already packaged in Fedora and EPEL: http://koji.fedoraproject.org/koji/packageinfo?packageID=11238

Also bundling is not recommended by Fedora packaging guidelines: https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication_of_system_libraries

Disabled jemalloc by default in rpm.mk (note - upstream spec files were never updated to bundle jemalloc)

c7c3d59..c44a3e9 master -> master
commit c44a3e9
Author: Mark Reynolds mreynolds@redhat.com
Date: Mon Dec 21 08:56:45 2015 -0500

Well we need to bundle jemalloc afterall since tcmalloc is going away in Fedora 28 & RHEL 8. Reopening...

Looks good to me. Just one question: are we opting for LD_PRELOAD instead of direct linking to be able to select malloc() implementation on the fly?

Looks good to me. Just one question: are we opting for LD_PRELOAD instead of direct linking to be able to select malloc() implementation on the fly?

Correct, it's less intrusive to change it, or remove it in the future. If glibc wants to investigate the latest 389 server, they just need to comment out the LD_PRELOAD. Otherwise jemalloc is built-in, and testing glibc's malloc is impossible.

Got it, thanks!

Sorry, I have an issue with LD_PRELOAD:

Apr 23 11:50:28 server.example.com systemd[1]: Starting 389 Directory Server IPA-TEST....
Apr 23 11:50:28 server.example.com ds_systemd_ask_password_acl[18545]: ERROR: ld.so: object ';' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
Apr 23 11:50:28 server.example.com ds_systemd_ask_password_acl[18545]: ERROR: ld.so: object 'export' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
Apr 23 11:50:28 server.example.com ds_systemd_ask_password_acl[18545]: ERROR: ld.so: object 'LD_PRELOAD' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.

Hmm guess it doesn't like the "; export LD_PRELOAD", what platform did you test on?

I'm testing this on Fedora Rawhide.

Can you try this patch? So for me it works when I disable selinux. Then when I do a "lsof -p PID" I see libjemalloc, but when I disable selinux I do not see libjemalloc. There are no selinux errors or warnings. It just silently fails on F26. SO I'm wondering if you could double check the behavior on rawhide.

Indeed, this is a SELinux policy issue. Denials were not logged until I disabled dontaudit rules:

semodule -DB

And after that I see:

# ausearch -m AVC                                                                                                                       
----                                   
time->Tue Apr 24 04:15:41 2018 
type=AVC msg=audit(1524557741.999:497): avc:  denied  { siginh } for  pid=16828 comm="ds_systemd_ask_" scontext=system_u:system_r:init_t:s0 tcontext=system_u:system_r:unconfined_service_t:s0 tclass=process permissive=0
----
time->Tue Apr 24 04:15:42 2018
type=AVC msg=audit(1524557742.038:498): avc:  denied  { noatsecure } for  pid=16833 comm="(ns-slapd)" scontext=system_u:system_r:init_t:s0 tcontext=system_u:system_r:dirsrv_t:s0 tclass=process permissive=0

With your second patch it's the same. I think we need to file a bug for selinux-policy.

Filed bug in Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=1571328

selinux-policy bug is fixed now, I've tested with selinux-policy-3.14.1-30.fc28.noarch and I can see that jemalloc is loaded:

# pmap $(pgrep ns-slapd) | grep jem
00007ff362627000    424K r-x-- libjemalloc.so.2
00007ff362691000   2044K ----- libjemalloc.so.2
00007ff362890000     20K r---- libjemalloc.so.2
00007ff362895000      4K rw--- libjemalloc.so.2

https://pagure.io/389-ds-base/pull-request/49747

commit d5e1164

Reopening, we need to move the license to under /usr/share/licenses (via %license)

4f5f6bb..9110d2b master -> master

Reopening to fix LD_PRELOAD errors:

Aug 08 06:46:12 172.16.36.19 ds_systemd_ask_password_acl[7146]: ERROR: ld.so: object '/usr/lib64/dirsrv/lib/libjemalloc.so' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.

https://pagure.io/389-ds-base/pull-request/49892

commit ad640e9

Only ship libjemalloc.so.2

https://pagure.io/389-ds-base/pull-request/49906

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/1708

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.