Add ${name}_oomprotect to rc.conf

[araujo]

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"

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

syslogd_oomprotect="ALL"

[araujo]

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

[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.

[araujo]

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?

[araujo]

usr.bin/protect/protect.1

57 ↗

(On Diff #12970)

I will consider your suggestion,

[araujo]

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.

[araujo]

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

[araujo]

• Update rc.subr(8) manpage.
• Enable syslogd_oomprotect by default on: /etc/defaults/rc.conf

[araujo]

Fix misspelling.

[rpokala]

etc/defaults/rc.conf

256 ↗

(On Diff #12979)

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

[araujo]

Address @rpokala report: Fix another misspelling.

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

[allanjude]

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

[rpokala]

Looks okay to me.

[jhb]

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]

Address @jhb suggestions.

[rpokala]

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]

Fix more mispelling pointed out by @rpokala.

[araujo]

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]

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

[rpokala]

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.

[araujo]

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]

• Address @rpokala suggestions.
• Use the ${PROTECT} variable to check if protect exist.

[rpokala]

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.

[jhb]

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

[araujo]

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

[jhb]

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.

[araujo]

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.

2. 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?

[jhb]

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]

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

[allanjude]

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

[araujo]

In D5176#162566, @allanjude wrote:

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,