389-ds-base

#51185 Issue 51039 - systemd-tmpfiles warnings
Closed by spichugi. Opened by vashirov.
vashirov/389-ds-base ds51039  into  master

Download 51185.patch

Bug Description:
systemd-tmpfiles warns about legacy paths in our tmpfiles configs.
Using /var/run also introduces a race condition, see the following
issue https://pagure.io/389-ds-base/issue/47429

Fix Description:
Instead of using @localstatedir@/run use /run directly,
this is also encouraged by FHS v3.0:
https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.html#runRuntimeVariableData

Relates: https://pagure.io/389-ds-base/issue/47429
Fixes: https://pagure.io/389-ds-base/issue/51039

Reviewed by: ???

LGTM

Shouldn't this be prefixed? iirc localstate dir is a prefix/var, so this should actually just be "prefix" now I think?

That's exactly the point - /run shouldn't be prefixed.
localstatedir is $(prefix)/var, but /var/run is deprecated in favor of /run.
This change should affect only users of systemd (for tmpfiles), so in a prefix install or in a container it shouldn't matter.
Do you have any concerns?

At the moment running tests with prefix install creates /var/run/dirsrv/slapd-master1.pid, the patch will create /run/dirsrv/slapd-master1.pid. Correct ?

It will create /run/dirsrv/slapd-master1.pid, without prefix.

IMO this theen to create in /run rather than /run directly. Is that okay?

rebased onto ce50433de1902dbc38e06fcb63980fac227e8f15

After digging a bit, I found that we already have @localrundir@ for this exact purpose implemented in #47513 by @mreynolds :)
By default it's /run. If used with --with-fhs-opt, it becomes /var/opt/run. I've also added a configure parameter --with-localrundir if you want to override it.

Also, there is runstatedir variable in autoconf 2.70, but that version is unreleased and not available in Fedora/RHEL.

After digging a bit, I found that we already have @localrundir@ for this exact purpose implemented in #47513 by @mreynolds :)

Wow forgot all about this one :-) Getting old haha!

Again LGTM, but lets make sure @firstyear and @tbordaz are happy with it.

So to be sure - this still defaults to /run, and if we want to override it, we have to set a ./configure flag to add the prefix? Or is the prefix added in ./configure?

It defaults to /run.
With --prefix explicitly set it is still /run.
With --with-fhs-opt it is /var/opt/run. See https://pagure.io/389-ds-base/blob/master/f/configure.ac#_433
And finally --with-localrundir overrides these defaults from above.

Hmmm I think this is an improvement, but I still think if --prefix exists, we should respect it even for the run dir.

Autoconf doesn't prefix /run, there is a special variable just for that. So why should we prefix it? What does it solve?

/run is a part of FHS, please check: https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.html#runRuntimeVariableData and https://lists.fedoraproject.org/pipermail/devel/2011-March/150031.html. I think we should respect FHS.

When I develop 389, I put it in /opt/dirsrv to isolate it. If I said there is a prefix, everything goes in the prefix. That's why it matters :)

If I said there is a prefix, everything goes in the prefix.

I dare to say you're using prefix wrong :)
/run was/is a separate entity when it was used with the prefix and the perl installer in 1.3.x. But in 1.4.x with the python installer it was broken, and now we have a regression.

I've added --with-localrundir to override the default value, so maybe you can use that?

Yeah, but containers ... means no systemd ...

Anyway, I can probably change to do this, but wondering how this might impact @tbordaz too.

Yeah, but containers ... means no systemd ...

Not all containers are the same, some of them can have systemd inside, for various reasons:
https://developers.redhat.com/blog/2019/04/24/how-to-run-systemd-in-a-container/

I personally run 389-ds in a container with systemd for testing purposes.
But I assume you mean a scenario when the entry point is dscontainer. Let's see how this change affects it:

  • pid_file: dscontainer comment says: 
        # This container version doesn't actually use or need the pidfile to track
        # the process.
  • ldapi and lock_dir: dscontainer comment says
- the ldapi socket is placed into /data so you can access it from the
container host.

And on line 254: 

        s2b.set('run_dir', '/data/run')
        s2b.set('lock_dir', '/data/run/lock')
        s2b.set('ldapi', '/data/run/slapd.socket')

Looking at how it's done in the Dockerfile:
https://pagure.io/389-ds-base/blob/master/f/docker/389-ds-suse/Dockerfile.release#_61

    ln -s /data/run /var/run/dirsrv

Let's look in the container itself:

$ podman run -ti opensuse/leap:15.1 /bin/bash
38510bc4b0c1:/ # ls -la /var/ | grep run 
lrwxrwxrwx.  1 root root    9 Jun 22 18:36 lock -> /run/lock
lrwxrwxrwx.  1 root root    4 Jun 22 18:36 run -> /run

Anything that's placed in /run will appear in /var/run and your /data will see that too.
So you've been already using /run just not knowing it :)

Indeed the current patch is breaking the use of prefix install (ldapi path). The patch below , on top of the current patch to make it work with prefix installation. 

From ec475079bf40b31fa4e7fde305c6094058507498 Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbordaz@redhat.com>
Date: Thu, 2 Jul 2020 14:43:20 +0200
Subject: [PATCH] ext 51059 prefix install
---
 src/lib389/lib389/instance/options.py | 5 +++++
 src/lib389/lib389/instance/setup.py   | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/lib389/lib389/instance/options.py b/src/lib389/lib389/instance/options.py
index fd60d433c..699f836e2 100644
--- a/src/lib389/lib389/instance/options.py
+++ b/src/lib389/lib389/instance/options.py
@@ -25,6 +25,7 @@ format_keys = [
     'sysconf_dir',
     'data_dir',
     'local_state_dir',
+    'ldapi',
     'lib_dir',
     'cert_dir',
     'config_dir',
@@ -216,6 +217,10 @@ class Slapd2Base(Options2):
         self._type['local_state_dir'] = str
         self._helptext['local_state_dir'] = "Sets the location of Directory Server variable data. Only set this parameter in a development environment."
+        self._options['ldapi'] = ds_paths.ldapi
+        self._type['ldapi'] = str
+        self._helptext['ldapi'] = "Sets the location of socket interface of the Directory Server."
+
         self._options['lib_dir'] = ds_paths.lib_dir
         self._type['lib_dir'] = str
         self._helptext['lib_dir'] = "Sets the location of Directory Server shared libraries. Only set this parameter in a development environment."
diff --git a/src/lib389/lib389/instance/setup.py b/src/lib389/lib389/instance/setup.py
index 418b71b57..18563d44a 100644
--- a/src/lib389/lib389/instance/setup.py
+++ b/src/lib389/lib389/instance/setup.py
@@ -966,7 +966,10 @@ class SetupDs(object):
                     raise ValueError("Suffix RDN '{}' in '{}' is not supported.  Supported RDN's are: 'c', 'cn', 'dc', 'o', and 'ou'".format(suffix_rdn_attr, backend['nsslapd-suffix']))
         # Initialise ldapi socket information. IPA expects this ....
-        ldapi_path = os.path.join(slapd['local_state_dir'], "run/slapd-%s.socket" % slapd['instance_name'])
+        if os.path.exists(os.path.dirname(slapd['ldapi'])):
+            ldapi_path = slapd['ldapi']
+        else:
+            ldapi_path = os.path.join(slapd['run_dir'], "/slapd-%s.socket" % slapd['instance_name'])
         ds_instance.config.set('nsslapd-ldapifilepath', ldapi_path)
         ds_instance.config.set('nsslapd-ldapilisten', 'on')
         ds_instance.config.set('nsslapd-ldapiautobind', 'on')

@vashirov My "development containers" are the problem here - but that's also my problem too resolve too. In those, I prefet /run in /opt/dirsrv so I can nuke a bad build easily.

The other issue could actually be with prefixed builds on say a system with both RPM installed and prefix installed DS. Because they both share /run, but split over /etc and /opt/dirsrv/etc, if the instances had the same name, they could stomp on each other.

Which is a pretty edge edge case that would only trip up a developer, but even those little details matter to me - any new developer shouldn't have to worry about footguns in our project because they held it wrong.

Honestly, setting /run into a prefix if there is one seems like a pretty minor change, it doesn't interfer with the systemd side of this? So I'd like it prefixed please. :)

Just for information, to run successfully the basic tests (especially ldapagent) in prefix install I had to modify the proposed patch above.

From d835160e133afe52a6caf6a0e429374c6ea6fdba Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbordaz@redhat.com>
Date: Thu, 2 Jul 2020 14:43:20 +0200
Subject: [PATCH] ext 51059 prefix install
---
 Makefile.am                                  | 3 ++-
 dirsrvtests/tests/suites/basic/basic_test.py | 4 ++--
 ldap/admin/src/defaults.inf.in               | 4 ++--
 ldap/servers/snmp/main.c                     | 8 ++++----
 src/lib389/lib389/__init__.py                | 3 +++
 src/lib389/lib389/instance/options.py        | 5 +++++
 src/lib389/lib389/instance/setup.py          | 5 ++++-
 7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 3f2164599..0e3c9315d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -141,7 +141,8 @@ PATH_DEFINES = -DLOCALSTATEDIR="\"$(localstatedir)\"" -DSYSCONFDIR="\"$(sysconfd
    -DLIBDIR="\"$(libdir)\"" -DBINDIR="\"$(bindir)\"" \
    -DDATADIR="\"$(datadir)\"" -DDOCDIR="\"$(docdir)\"" \
    -DSBINDIR="\"$(sbindir)\"" -DPLUGINDIR="\"$(serverplugindir)\"" \
-   -DTEMPLATEDIR="\"$(sampledatadir)\"" -DSYSTEMSCHEMADIR="\"$(systemschemadir)\""
+   -DTEMPLATEDIR="\"$(sampledatadir)\"" -DSYSTEMSCHEMADIR="\"$(systemschemadir)\"" \
+   -DLOCALRUNDIR="\"$(localrundir)\""
 # Now that we have all our defines in place, setup the CPPFLAGS
diff --git a/dirsrvtests/tests/suites/basic/basic_test.py b/dirsrvtests/tests/suites/basic/basic_test.py
index c85dc4b43..c9a005a3e 100644
--- a/dirsrvtests/tests/suites/basic/basic_test.py
+++ b/dirsrvtests/tests/suites/basic/basic_test.py
@@ -835,7 +835,6 @@ def test_basic_ldapagent(topology_st, import_example_ldif):
     """
     log.info('Running test_basic_ldapagent...')
-
     var_dir = topology_st.standalone.get_local_state_dir()
     config_file = os.path.join(topology_st.standalone.get_sysconf_dir(), 'dirsrv/config/agent.conf')
@@ -849,7 +848,8 @@ def test_basic_ldapagent(topology_st, import_example_ldif):
     # Remember, this is *forking*
     check_output([os.path.join(topology_st.standalone.get_sbin_dir(), 'ldap-agent'), config_file])
     # First kill any previous agents ....
-    pidpath = os.path.join(var_dir, 'run/ldap-agent.pid')
+    run_dir = topology_st.standalone.get_run_dir()
+    pidpath = os.path.join(run_dir, 'ldap-agent.pid')
     pid = None
     with open(pidpath, 'r') as pf:
         pid = pf.readlines()[0].strip()
diff --git a/ldap/admin/src/defaults.inf.in b/ldap/admin/src/defaults.inf.in
index ccc31ac0b..c1bb97b36 100644
--- a/ldap/admin/src/defaults.inf.in
+++ b/ldap/admin/src/defaults.inf.in
@@ -35,10 +35,10 @@ sysconf_dir = @sysconfdir@
 initconfig_dir = @initconfigdir@
 config_dir = @instconfigdir@/slapd-{instance_name}
 local_state_dir = @localstatedir@
-run_dir = @localrundir@/dirsrv
+run_dir = @localrundir@
 # This is the expected location of ldapi.
 ldapi = @localrundir@/slapd-{instance_name}.socket
-pid_file = @localrundir@/dirsrv/slapd-{instance_name}.pid
+pid_file = @localrundir@/slapd-{instance_name}.pid
 inst_dir = @serverdir@/slapd-{instance_name}
 plugin_dir = @serverplugindir@
 system_schema_dir = @systemschemadir@
diff --git a/ldap/servers/snmp/main.c b/ldap/servers/snmp/main.c
index 4b672ead0..e5beb3cc2 100644
--- a/ldap/servers/snmp/main.c
+++ b/ldap/servers/snmp/main.c
@@ -286,14 +286,14 @@ load_config(char *conf_path)
     }
     /* set pidfile path */
-    if ((pidfile = malloc(strlen(LOCALSTATEDIR) + strlen("/run/") +
+    if ((pidfile = malloc(strlen(LOCALRUNDIR) + strlen("/") +
                           strlen(LDAP_AGENT_PIDFILE) + 1)) != NULL) {
-        strncpy(pidfile, LOCALSTATEDIR, strlen(LOCALSTATEDIR) + 1);
+        strncpy(pidfile, LOCALRUNDIR, strlen(LOCALRUNDIR) + 1);
         /* The above will likely not be NULL terminated, but we need to
          * be sure that we're properly NULL terminated for the below
          * strcat() to work properly. */
-        pidfile[strlen(LOCALSTATEDIR)] = (char)0;
-        strcat(pidfile, "/run/");
+        pidfile[strlen(LOCALRUNDIR)] = (char)0;
+        strcat(pidfile, "/");
         strcat(pidfile, LDAP_AGENT_PIDFILE);
     } else {
         printf("ldap-agent: malloc error processing config file\n");
diff --git a/src/lib389/lib389/__init__.py b/src/lib389/lib389/__init__.py
index 8c3e7c338..3c0333143 100644
--- a/src/lib389/lib389/__init__.py
+++ b/src/lib389/lib389/__init__.py
@@ -1699,6 +1699,9 @@ class DirSrv(SimpleLDAPObject, object):
     def get_bin_dir(self):
         return self.ds_paths.bin_dir
+    def get_run_dir(self):
+        return self.ds_paths.run_dir
+
     def get_plugin_dir(self):
         return self.ds_paths.plugin_dir
diff --git a/src/lib389/lib389/instance/options.py b/src/lib389/lib389/instance/options.py
index fd60d433c..699f836e2 100644
--- a/src/lib389/lib389/instance/options.py
+++ b/src/lib389/lib389/instance/options.py
@@ -25,6 +25,7 @@ format_keys = [
     'sysconf_dir',
     'data_dir',
     'local_state_dir',
+    'ldapi',
     'lib_dir',
     'cert_dir',
     'config_dir',
@@ -216,6 +217,10 @@ class Slapd2Base(Options2):
         self._type['local_state_dir'] = str
         self._helptext['local_state_dir'] = "Sets the location of Directory Server variable data. Only set this parameter in a development environment."
+        self._options['ldapi'] = ds_paths.ldapi
+        self._type['ldapi'] = str
+        self._helptext['ldapi'] = "Sets the location of socket interface of the Directory Server."
+
         self._options['lib_dir'] = ds_paths.lib_dir
         self._type['lib_dir'] = str
         self._helptext['lib_dir'] = "Sets the location of Directory Server shared libraries. Only set this parameter in a development environment."
diff --git a/src/lib389/lib389/instance/setup.py b/src/lib389/lib389/instance/setup.py
index 418b71b57..bf97bf1a5 100644
--- a/src/lib389/lib389/instance/setup.py
+++ b/src/lib389/lib389/instance/setup.py
@@ -966,7 +966,10 @@ class SetupDs(object):
                     raise ValueError("Suffix RDN '{}' in '{}' is not supported.  Supported RDN's are: 'c', 'cn', 'dc', 'o', and 'ou'".format(suffix_rdn_attr, backend['nsslapd-suffix']))
         # Initialise ldapi socket information. IPA expects this ....
-        ldapi_path = os.path.join(slapd['local_state_dir'], "run/slapd-%s.socket" % slapd['instance_name'])
+        if os.path.exists(os.path.dirname(slapd['ldapi'])):
+            ldapi_path = slapd['ldapi']
+        else:
+            ldapi_path = os.path.join(slapd['run_dir'], "slapd-%s.socket" % slapd['instance_name'])
         ds_instance.config.set('nsslapd-ldapifilepath', ldapi_path)
         ds_instance.config.set('nsslapd-ldapilisten', 'on')
         ds_instance.config.set('nsslapd-ldapiautobind', 'on')

@firstyear, can you confirm if ThIerry's amendment works for you?

@vashirov - does Thierry's change still work as expected for non-prefix builds?
Edited by mreynolds

@mreynolds, sorry, I haven't tested it yet as other things got in the way. I'll do that today.

rebased onto e7fb32d1badc44cda4336f2d00a08662f1e93409

@mreynolds, I've updated the PR with the patch from @tbordaz, it works for non-prefix builds as expected.

LGTM!

rebased onto 543202555995864e1b02d690daa26067e81c2fab

Second attempt to add Thierry's changes :)

Second attempt to add Thierry's changes :)

Still looks good :-)

Previously pidfile was under /run/dirsrv/ but now it's under /run. @tbordaz, is this a copy-paste issue or it is required to work in prefix builds?

@vashirov thanks for the patch and sorry for the late tests.
The patch allows to run successfully tests (basic) in prefix installation.
Initially I forgot to define '--with-localrundir=$DIR_INSTALL/dirsrv' for the build but after building with this option it ran like a charm.

Note that I still have to apply (very) old pending #50297 to make prefix install run. I will reactivate this PR.

In conclusion your patch looks good to me

@tbordaz, thank you for testing. But I still have a question for you :)
In your patch, that you provided, you used 

pid_file = @localrundir@/slapd-{instance_name}.pid

But previously pid_file was 

pid_file = @localstatedir@/run/dirsrv/slapd-{instance_name}.pid

(note the dirsrv part)

Is this required for prefix builds or this can be changed to

pid_file = @localrundir@/dirsrv/slapd-{instance_name}.pid

?

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4238

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.