Created attachment 225247 [details] rc.subr support for services with multiple processes For ports which install services managed by the rc subsystem, such as haproxy and nginx, which result in multiple processes being started, attempting to set e.g. either "haproxy_oomprotect=yes" or "haproxy_oomprotect=all" results in the following error: root@hostname:~# service haproxy start Starting haproxy. usage: protect [-i] command protect [-cdi] -g pgrp | -p pid root@hostname:~# As well as none of the haproxy processes obtaining protection: root@hostname:~# ps -ax -o flags,flags2,pid,command | grep haproxy 10000180 00000000 26037 /usr/local/sbin/haproxy -q -f /usr/local/etc/haproxy.conf -p /var/run/haproxy.pid 10000000 00000000 41907 /usr/local/sbin/haproxy -q -f /usr/local/etc/haproxy.conf -p /var/run/haproxy.pid Due to the fact that /etc/rc.subr passes the output of "check_process $command" ( a list of one or more space-separated PIDs) straight to "protect", e.g. "protect -i -p 123 456", whereas protect's "-p" argument can only handle one PID The attached patch instead uses the result of "check_process $command" in a for loop to iterate through potentially multiple PIDs that are passed to "protect". Example correct output using attached patch: root@hostname:~# ps -ax -o flags,flags2,pid,command | grep haproxy 10100180 00000001 75344 /usr/local/sbin/haproxy -q -f /usr/local/etc/haproxy.conf -p /var/run/haproxy.pid 10100000 00000001 87137 /usr/local/sbin/haproxy -q -f /usr/local/etc/haproxy.conf -p /var/run/haproxy.pid "OOM Protect" status accurately reflected in third digit of first column (flags) as well as "inherit OOM Protect" status reflected in final digit of second column (flags2) I've also tested this patch against services which only return one PID from "check_process $command" and it works as expected.
This bit me recently too. I see it's been fixed a different way in CURRENT, but not MFC'ed The patch applies successfully to 13 and 12, and works as intended, so could it please be MFCed? https://cgit.freebsd.org/src/commit/libexec/rc/rc.subr?id=6ba108e52d175b6833437c8627ae5d0546a4e102 Cheers, Jamie
(In reply to Jamie Landeg-Jones from comment #1) Hi Jamie, Thanks for the update! I filed this a year ago and was literally in the process of adding a comment asking for an update at the exact minute you just posted this. Wild. Though I've verified that the `eval $_pidcmd` way of doing it still only runs protect against the process identified by the pidfile, and not any other processes it has *already* forked. Can we tweak the way CURRENT is using it take advantage of protect's -d flag? That fixes the issue for me. I've attached a revised diff.
Created attachment 233999 [details] Revised rc.subr patch to use $_pidcmd as well as protect -d
(In reply to Mike Walker from comment #2) That's weird! Get out of my head! Or is it me in your head? :-) Good catch with the '-d' addition. Obviously that will need to be added to head too Just a shame we are the only 2 here!
(In reply to Mike Walker from comment #3) Hi Mike. There was some activity on this on the freebsd-rc and freebsd-stable lists. I replied, pointing out your latest update on this bug. (I Bcc'd you on the mailing list because you may not want your email address appearing publicly, so you won't get any replies to that thread) Cheers, Jamie
I MFC'd the following commit to 12 and 13: https://cgit.freebsd.org/src/commit/libexec/rc/rc.subr?id=6ba108e52d175b6833437c8627ae5d0546a4e102 Adding -d is one solution. Perhaps the whole block should be rewritten and protect(1) should be chained in into the _doit variable similarly to limits(1).
It probably makes sense to keep the protect block where it is because this way *_oomprotect supports both the services using the "command" variable and those using the "start_cmd" variable.
I've added a commit message to the patch and adjusted it to the current src tree: https://reviews.freebsd.org/D35747
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=68e035c0172b441db772de41ad0f8977679bfedc commit 68e035c0172b441db772de41ad0f8977679bfedc Author: Mike Walker <mike.walker@napkindrawing.com> AuthorDate: 2022-07-07 20:28:37 +0000 Commit: Mateusz Piotrowski <0mp@FreeBSD.org> CommitDate: 2022-07-08 06:19:50 +0000 rc.subr: Make sure oomprotect protects existing children The rc(8) framework support protecting services from OOM killer. The current implementation applies the protection after the service has already started. This works fine if only the main process is to be protected (*_oomprotect=yes). However, the current implementation fails to protect existing children when children are also to be protected (*_oomprotect=all). This patch fixes that. Note: it is not easy to apply the protectoin earlier because we want to support both the services which use the "command" variable and those that use the "start_cmd" variable. PR: 256148 Approved by: adrian, osogbo Tested by: Jamie Landeg-Jones <jamie@catflap.org> Fixes: 3bead71e959d - Add a global option where we can protect MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D35747 libexec/rc/rc.subr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
I'll MFC the change in a week or so. Thanks for the patch!
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=42422c5a4a5035296b2c39c584b8cfc3aa6f4012 commit 42422c5a4a5035296b2c39c584b8cfc3aa6f4012 Author: Mike Walker <mike.walker@napkindrawing.com> AuthorDate: 2022-07-07 20:28:37 +0000 Commit: Mateusz Piotrowski <0mp@FreeBSD.org> CommitDate: 2022-07-22 00:09:10 +0000 rc.subr: Make sure oomprotect protects existing children The rc(8) framework support protecting services from OOM killer. The current implementation applies the protection after the service has already started. This works fine if only the main process is to be protected (*_oomprotect=yes). However, the current implementation fails to protect existing children when children are also to be protected (*_oomprotect=all). This patch fixes that. Note: it is not easy to apply the protectoin earlier because we want to support both the services which use the "command" variable and those that use the "start_cmd" variable. PR: 256148 Approved by: adrian, osogbo Tested by: Jamie Landeg-Jones <jamie@catflap.org> Fixes: 3bead71e959d - Add a global option where we can protect MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D35747 (cherry picked from commit 68e035c0172b441db772de41ad0f8977679bfedc) libexec/rc/rc.subr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=62cd401b90ea2597e446e17b9d76045cfec59770 commit 62cd401b90ea2597e446e17b9d76045cfec59770 Author: Mike Walker <mike.walker@napkindrawing.com> AuthorDate: 2022-07-07 20:28:37 +0000 Commit: Mateusz Piotrowski <0mp@FreeBSD.org> CommitDate: 2022-07-22 00:09:30 +0000 rc.subr: Make sure oomprotect protects existing children The rc(8) framework support protecting services from OOM killer. The current implementation applies the protection after the service has already started. This works fine if only the main process is to be protected (*_oomprotect=yes). However, the current implementation fails to protect existing children when children are also to be protected (*_oomprotect=all). This patch fixes that. Note: it is not easy to apply the protectoin earlier because we want to support both the services which use the "command" variable and those that use the "start_cmd" variable. PR: 256148 Approved by: adrian, osogbo Tested by: Jamie Landeg-Jones <jamie@catflap.org> Fixes: 3bead71e959d - Add a global option where we can protect MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D35747 (cherry picked from commit 68e035c0172b441db772de41ad0f8977679bfedc) libexec/rc/rc.subr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
(In reply to Mateusz Piotrowski from comment #10) Mateusz, thanks for doing this!
(In reply to Jamie Landeg-Jones from comment #13) Cheers!
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e7437ae907c89bf85a99c5cbb7ddd194a1ff1354 commit e7437ae907c89bf85a99c5cbb7ddd194a1ff1354 Author: Mateusz Piotrowski <0mp@FreeBSD.org> AuthorDate: 2022-07-07 18:24:27 +0000 Commit: Mateusz Piotrowski <0mp@FreeBSD.org> CommitDate: 2022-07-26 13:39:39 +0000 rc: Start testing the rc(8) framework (beginning with *_oomprotect) This change adds 2 tests to make sure that the *_oomprotect variable sets the protection against OOM killer properly within rc(8) scripts. This is also adding the first tests for the rc(8) framework. More tests will be added as we go. PR: 256148 Approved by: des MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D35745 etc/mtree/BSD.tests.dist | 2 + libexec/rc/Makefile | 3 + libexec/rc/tests/Makefile (new) | 3 + libexec/rc/tests/rc_subr_test.sh (new) | 103 +++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=220af70281c5c04610f6bf4ce904e269d1532992 commit 220af70281c5c04610f6bf4ce904e269d1532992 Author: Mateusz Piotrowski <0mp@FreeBSD.org> AuthorDate: 2022-07-07 18:24:27 +0000 Commit: Mateusz Piotrowski <0mp@FreeBSD.org> CommitDate: 2023-10-04 11:11:18 +0000 rc: Start testing the rc(8) framework (beginning with *_oomprotect) This change adds 2 tests to make sure that the *_oomprotect variable sets the protection against OOM killer properly within rc(8) scripts. This is also adding the first tests for the rc(8) framework. More tests will be added as we go. PR: 256148 Approved by: des MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D35745 (cherry picked from commit e7437ae907c89bf85a99c5cbb7ddd194a1ff1354) etc/mtree/BSD.tests.dist | 2 + libexec/rc/Makefile | 3 + libexec/rc/tests/Makefile (new) | 3 + libexec/rc/tests/rc_subr_test.sh (new) | 103 +++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+)
I just tried to upgrade to the most recent version of 13.2-STABLE into a Virtualbox guest instance (FreeBSD host), and got a build error > uname -a FreeBSD fbsd13_2 13.2-STABLE FreeBSD 13.2-STABLE stable/13-n256726-7c25a53a2cb9 FBSD13_2 amd64 it's a custom kernel - I haven't tried GENERIC build yet Here's the error: ===> libexec/rc (install) installing DIRS CONFETCDEFAULTSDIR install -d -m 0755 -o root -g wheel /etc/defaults installing DIRS CONFETCDIR CONFETCEXECDIR install -d -m 0755 -o root -g wheel /etc ===> libexec/rc/rc.d (install) installing DIRS ACCTDIR ACPIDIR APMDIR AUDITDIR BLUETOOTHDIR BSNMPDIR CONFSDIR DEVDDIR DEVMATCHDIR DHCLIENTDIR GSSDDIR HASTDIR JAILDIR RESOLVCONFDIR SMRCDDIR SSHDIR UNBOUNDDIR VIDIR ZFSDIR install -d -m 0755 -o root -g wheel /etc/rc.d installing DIRS VAR_HEMIDAL install -d -m 700 -o root -g wheel /var/heimdal ===> libexec/rc/tests (install) install -o root -g wheel -m 555 rc_subr_test /usr/tests/libexec/rc/rc_subr_test install: /usr/tests/libexec/rc/rc_subr_test: No such file or directory *** Error code 71 I don't quite get why/how this happened, & there seems to be no other folks mentioning this issue so far - what might i try next to fix it?
(In reply to Bruce Becker from comment #17) Here's the libexec/rc part of 'make buildworld' - [...] --- all_subdir_libexec --- echo '#! /usr/libexec/atf-sh' > rc_subr_test.tmp cat /usr/src/libexec/rc/tests/rc_subr_test.sh >>rc_subr_test.tmp [...] --- all_subdir_libexec --- chmod +x rc_subr_test.tmp [...] --- all_subdir_libexec --- mv rc_subr_test.tmp rc_subr_test ... which ended up in '/usr/obj/usr/src/amd64.amd64/libexec/rc/tests'
(In reply to Bruce Becker from comment #17) Here's the libexec/rc part of 'make buildworld' - [...] --- all_subdir_libexec --- echo '#! /usr/libexec/atf-sh' > rc_subr_test.tmp cat /usr/src/libexec/rc/tests/rc_subr_test.sh >>rc_subr_test.tmp [...] --- all_subdir_libexec --- chmod +x rc_subr_test.tmp [...] --- all_subdir_libexec --- mv rc_subr_test.tmp rc_subr_test ... which ended up in '/usr/obj/usr/src/amd64.amd64/libexec/rc/tests' drwxrwxr-x 2 root wheel 4 Nov 26 05:07 ./ drwxrwxr-x 4 root wheel 4 Nov 25 17:48 ../ -rw-r--r-- 1 root wheel 118 Nov 26 05:07 Kyuafile -rwxr-xr-x 1 root wheel 3589 Nov 26 05:07 rc_subr_test*
(In reply to Bruce Becker from comment #19) I moved /usr/obj to /usr/obj.sav & repeated build process - now it works. Is it possible that residual /usr/obj things from a previous build caused this kind of issue?