Page MenuHomeFreeBSD

Add ${name}_oomprotect to rc.conf
ClosedPublic

Authored by araujo on Feb 3 2016, 9:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 23, 11:17 AM
Unknown Object (File)
Sun, Jul 21, 2:56 PM
Unknown Object (File)
Sun, Jul 7, 5:06 PM
Unknown Object (File)
Wed, Jul 3, 8:21 AM
Unknown Object (File)
Tue, Jul 2, 8:05 PM
Unknown Object (File)
Wed, Jun 26, 6:28 PM
Unknown Object (File)
Jun 21 2024, 5:47 PM
Unknown Object (File)
Jun 14 2024, 4:01 PM
Subscribers

Details

Summary

Add a global option where we can protect processes against OOM killer.

Basically we need to add on rc.conf an another option like:

  1. If we want to protect only the main processes.

syslogd_oomprotect="YES"

  1. If we want to protect all future children of the specified processes.

syslogd_oomprotect="ALL"

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
araujo retitled this revision from Add ${name}_oomprotection to rc.conf to Add ${name}_oomprotect to rc.conf.Feb 3 2016, 10:07 AM
araujo updated this object.
araujo added reviewers: jhb, rodrigc, bapt.
araujo updated this object.

I sent the wrong rc.subr script. Now fixed.

rpokala requested changes to this revision.Feb 3 2016, 3:19 PM
rpokala added a reviewer: rpokala.
rpokala added a subscriber: rpokala.

See my comments below; I'm not sure that protect.c or protect.1 need to be touched at all.

usr.bin/protect/protect.1
57 ↗(On Diff #12970)

Explicitly state that no children will be affected:

Apply the operation to only the specified processes; no children are protected.

Or something similar.

usr.bin/protect/protect.c
94 ↗(On Diff #12970)

Wait, you're recognizing the '-s' option, but you're not doing anything with that knowledge?

Hmm. Upon closer look at the existing version of the manpage, I see that it says:

Note that this protected state is not inherited by child processes by default.

So then why add this new flag at all? Simply run without any flags.

This revision now requires changes to proceed.Feb 3 2016, 3:19 PM
usr.bin/protect/protect.c
94 ↗(On Diff #12970)

Yes, I'm not doing anything because you probably have missed the line 79: flags = PPROT_SET;

About the -s flag, I guess my comment was clear:
pgrep syslogd | xargs protect -p

Here as you can see, I need to launch and get the PID and then run protect. This turns out a bit ugly to be write on rc.subr.

So, why not take the same approach and be able to launch protect -s <command> instead of do it in 2 steps and using xargs?

usr.bin/protect/protect.1
57 ↗(On Diff #12970)

I will consider your suggestion,

usr.bin/protect/protect.c
94 ↗(On Diff #12970)

Yes, I'm not doing anything because the line 79 is setting the flag already.

But you are right, it is not necessary a new option on protect.

Remove the protect(1) changes, it is really not needed.

araujo updated this object.
  • Update rc.subr(8) manpage.
  • Enable syslogd_oomprotect by default on: /etc/defaults/rc.conf
araujo edited edge metadata.

Fix misspelling.

rpokala requested changes to this revision.Feb 3 2016, 5:29 PM
rpokala edited edge metadata.
rpokala added inline comments.
etc/defaults/rc.conf
256 ↗(On Diff #12979)

Small but significant typo: "protec" -> "protect" (twice).

This revision now requires changes to proceed.Feb 3 2016, 5:29 PM
araujo edited edge metadata.

Address @rpokala report: Fix another misspelling.

NOTE: I shall go sleep and don't make more misspelling :)

I like what you did with 'YES' vs 'ALL'

rpokala edited edge metadata.

Looks okay to me.

This revision is now accepted and ready to land.Feb 3 2016, 5:51 PM

Functionality looks fine, just some suggestions on documentation.

etc/defaults/rc.conf
259 ↗(On Diff #12980)

This should probably not describe ALL vs YES. Instead, the manpage should cover that.
I would also reword this to be shorter and more direct:

"Don't kill syslogd when swap space is exhausted."

I think terser (ideally one-line) descriptions are more appropriate for this file and longer descriptions should be in the man page.

etc/rc.subr
758 ↗(On Diff #12980)

I would maybe use "killer" instead of "Killer". The phrase "OOM killer" is still a bit odd. The description for protect(1) is "Protest processes from being killed when swap space is exhausted", a variant of that that you could use here would be "Protect from being killed when swap space is exhausted."

share/man/man8/rc.subr.8
612 ↗(On Diff #12980)

This should describe ALL vs YES.

I would drop the sentence about rc.conf. This is an entry in a section that lists various variables that can be set in /etc/rc.conf or /etc/rc.conf.d/foo. None of the other variable descriptions mention this explicitly. Instead, that is implicitly assumed for all of these variables. Earlier in the manpage the description of load_rc_config describes what config files can be used to override variables used by run_rc_command.

araujo edited edge metadata.

Address @jhb suggestions.

This revision now requires review to proceed.Feb 4 2016, 1:26 AM
etc/defaults/rc.conf
256 ↗(On Diff #12987)

"swap"

share/man/man8/rc.subr.8
613 ↗(On Diff #12987)

"no child processes"

614 ↗(On Diff #12987)

If
.EM ALL

615 ↗(On Diff #12987)

"all child processes"

araujo edited edge metadata.

Fix more mispelling pointed out by @rpokala.

araujo edited edge metadata.

Rework the patch as at the first version the option YES was not
working.

To test it, just choose YES or ALL and check with:
ps -axwwo pid,flags,flags2,command | grep <command>

araujo edited edge metadata.

Don't call check_process() if ${name}_oomprotect is not set.

etc/defaults/rc.conf
256 ↗(On Diff #13048)

s/swape/swap/

etc/rc.subr
759 ↗(On Diff #13048)

"Don't kill ${command} when swap space is exhausted."

1194 ↗(On Diff #13048)

You put the full path in ${PROTECT}, so you should probably use it here like you use it below.

1203 ↗(On Diff #13048)

So wait - the idea is no longer to run the command directly via ${PROTECT}, but instead add PPROT_SET to the already-running process (and it's progeny, as needed)? Why was that change made? Doesn't that open up a window when the command is not protected?

share/man/man8/rc.subr.8
616 ↗(On Diff #13048)

If
.EM ALL
is used, protect all child processes.

etc/rc.subr
1203 ↗(On Diff #13048)

This change was made because protect(1) doesn't work in the way as: protect command.
At the man page it seems clear:
pgrep Xorg | xargs protect -p

If you make a test with these two version of this patch and use:
ps -axwwo pid,flags,flags2,command | grep syslogd

You will see that the patch using protect command, doesn't works. So, apply the protection over the pid is the right thing to do.

That doesn't open any window for the unprotected command.

araujo edited edge metadata.
  • Address @rpokala suggestions.
  • Use the ${PROTECT} variable to check if protect exist.
etc/rc.subr
1203 ↗(On Diff #13082)

"This change was made because protect(1) doesn't work in the way as: protect command."
"At the man page it seems clear:"

I agree that the manpage is clear; I disagree what it is clear about. :-)

  • protect [-i] command
  • command Execute command as a protected process.

That says to me that `protect ${command}' runs the process with PPROT_SET. If that doesn't work, then either that's a bug which needs to be fixed, or the manpage needs to be corrected.

"That doesn't open any window for the unprotected command."

Sure there is:

  1. Run command
  2. Get command's PID
  3. PID is killed
  4. ${PROTECT} -p ${pid}
share/man/man8/rc.subr.8
616 ↗(On Diff #13082)

You still need an "is used, " here.

etc/rc.subr
1203 ↗(On Diff #13082)

'proctect foo' definitely works. It's how I used this in production back when I first wrote it.

> sudo protect jot 100000000 >/dev/null
# in another window
> ps -o flags -p `pgrep jot`
       F
10104002
etc/rc.subr
1203 ↗(On Diff #13082)

With syslogd(8) it doesn't work! Maybe because syslogd fork itself?:

[root@Gandigate /usr/src]# protect syslogd >/dev/null
[root@Gandigate /usr/src]# ps -o flags -p `pgrep syslogd`
       F
10000000

protect over the PID:

[root@Gandigate /usr/src]# pgrep syslogd | xargs protect -i -p
[root@Gandigate /usr/src]#  ps -o flags -p `pgrep syslogd`
       F
10100000
etc/rc.subr
1203 ↗(On Diff #13082)

Correct, 'protect syslogd' will only protect the parent process which immediately exits if it is a daemon. You would either need to use 'protect -i' or you would want to do something like 'daemon protect syslogd -F' to move the fork earlier.

Neither solution is really great. If I were working on something like launchd I would want 'syslogd -F' anyway so I could know if the process died.

There definitely is a window where the process isn't protected, but it is relatively "small". Using the pid is perhaps the least evil, but I think this makes me want to replace rc.d with a real daemon manager even more.

etc/rc.subr
1203 ↗(On Diff #13082)

In such case like syslogd(8), in my point of view is:

  1. Forget this "global" patch and only apply the protection direct inside syslogd(8) code. I have a patch ready for this purpose.
  1. Commit this patch and apply protect(1) over the PID. Knowing there is a small "window" where the process isn't protected.

What do you think?

etc/rc.subr
1203 ↗(On Diff #13082)

I think 2) is ok (that is what I meant by "using the pid is perhaps the least evil"). It's not perfect, but I think it is the best option available. I think there is some value in having a general purpose protect knob.

rpokala edited edge metadata.

Sorry for the delay in reviewing this. Taking into the account the discussion you and @jhb had, this looks fine.

This revision is now accepted and ready to land.Feb 19 2016, 7:15 PM
bapt edited edge metadata.
This revision was automatically updated to reflect the committed changes.

This causes a warning for services inside jails, because you are not allowed to protect things inside a jail.

the daemon does start, so it is just a warning, but, it might be worth having the 'protect' step skipped if rc.subr detects it is running inside a jail

root@jail:/root#service syslogd restart
Stopping syslogd.
Waiting for PIDS: 8977.
Starting syslogd.
protect: procctl: Operation not permitted

This causes a warning for services inside jails, because you are not allowed to protect things inside a jail.

the daemon does start, so it is just a warning, but, it might be worth having the 'protect' step skipped if rc.subr detects it is running inside a jail

root@jail:/root#service syslogd restart
Stopping syslogd.
Waiting for PIDS: 8977.
Starting syslogd.
protect: procctl: Operation not permitted

Thanks Allan,

I will check it soon.

Best,