Page MenuHomeFreeBSD

rc.conf(5): Add _limits, _loginclass, and _oomprotect
Needs ReviewPublic

Authored by debdrup on May 18 2021, 12:52 PM.

Details

Summary

Add a few very useful variables that might easily be overlooked, since
they're only documented in rc.subr(8) which might not be the first place
that people look.

At least _oomprotect has existed since 11.0-RELEASE, and doesn't appear
to be very well-known. While the others aren't as new, in my estimation,
a lot more people would use them if they knew about them.

While here, also add a reference to rc.subr(8).

Test Plan

Tested using mantra, igor, and mandoc.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 39276
Build 36165: arc lint + arc unit

Event Timeline

I found out the hard way, that an rc script overriding start_cmd ignores some mechanisms. This may be by design. Those ignored are at the least:

  • _oomprotect
  • _user
  • _limits
  • _fib
  • _chdir
  • _nice

Unfortunately, some ports like PostgreSQL redefine start_cmd which would make _oomprotect="ALL" silently not work for the database. I am not sure where it would be a good place, but I think it would be worthwhile to document that redefining ${name}_cmd has such a pitfall.

share/man/man5/rc.conf.5
226
230
ceri added inline comments.
share/man/man5/rc.conf.5
230

Add login.conf(5) to SEE ALSO ?

It would also be nice to have discoverability from protect(1). Something to the notion of: "Daemons can be automatically protected using _oomprotect in rc.conf(5)" and a SEE ALSO to rc.conf(5).

I found out the hard way, that an rc script overriding start_cmd ignores some mechanisms. This may be by design. Those ignored are at the least:

  • _oomprotect
  • _user
  • _limits
  • _fib
  • _chdir
  • _nice

Unfortunately, some ports like PostgreSQL redefine start_cmd which would make _oomprotect="ALL" silently not work for the database. I am not sure where it would be a good place, but I think it would be worthwhile to document that redefining ${name}_cmd has such a pitfall.

Yeah, documenting this is a good idea - but I feel like it's outside the scope of this review.
If you want to open something here on Phabricator and add a note about it to rc.conf(5) and rc.subr(8) (until it's addressed for the ports or in the base system), I'll be happy to have a look at it.

It would also be nice to have discoverability from protect(1). Something to the notion of: "Daemons can be automatically protected using _oomprotect in rc.conf(5)" and a SEE ALSO to rc.conf(5).

Absolutely, but it also belongs in its own review. ;)

Address feedback by ceri and Adam Wolk

lgtm, however please note that I am not a committer.

This revision is now accepted and ready to land.May 18 2021, 1:38 PM

One last nit, sorry - could you sort these alphabetically, with _limits and _login_class above _nice and _oomprotect after it?

In D30330#681159, @ceri wrote:

One last nit, sorry - could you sort these alphabetically, with _limits and _login_class above _nice and _oomprotect after it?

I was thinking about that - but I'll want to sort all of the (name)_ variables alphabetically in that case.

In D30330#681159, @ceri wrote:

One last nit, sorry - could you sort these alphabetically, with _limits and _login_class above _nice and _oomprotect after it?

I was thinking about that - but I'll want to sort all of the (name)_ variables alphabetically in that case.

Ah yes, that would work for me.

This revision now requires review to proceed.May 18 2021, 2:06 PM
0mp requested changes to this revision.May 18 2021, 2:54 PM
0mp added a subscriber: 0mp.
0mp added inline comments.
share/man/man5/rc.conf.5
202
223
This revision now requires changes to proceed.May 18 2021, 2:54 PM
debdrup marked an inline comment as done.

Address feedback by 0mp

Unfortunately, some ports like PostgreSQL redefine start_cmd which would make _oomprotect="ALL" silently not work for the database. I am not sure where it would be a good place, but I think it would be worthwhile to document that redefining ${name}_cmd has such a pitfall.

In that particular case, that may not actually be a bad thing, although for the wrong reason. Protecting processes with large or unpredictable memory use is likely to deadlock the system (or close to it) if it runs out of memory, while killing a database server is bad but is likely to be picked up properly by monitoring systems.

Unfortunately, some ports like PostgreSQL redefine start_cmd which would make _oomprotect="ALL" silently not work for the database. I am not sure where it would be a good place, but I think it would be worthwhile to document that redefining ${name}_cmd has such a pitfall.

In that particular case, that may not actually be a bad thing, although for the wrong reason. Protecting processes with large or unpredictable memory use is likely to deadlock the system (or close to it) if it runs out of memory, while killing a database server is bad but is likely to be picked up properly by monitoring systems.

Killing PostgreSQL processes leads to the database entering recovery mode (as it properly assumes that corruption may have happened). This is a lenghty process for large databases. If someone went as far as to add postgresql_oomprotect="ALL" to his rc.conf then that person explicitly wanted the database protected and likely weighted the potential deadlock. On our production systems the cases where I found PostgreSQL dead were almost always caused by some other process (not Postgres) going crazy. In our case, we have a hardware watchdog and can live with the machine rebooting itself in the unlikely case that it was actually PostgreSQL deadlocking the machine but reducing the amount of cases where the database has been restarted with no good reason is a much better tradeoff for us.

We are not the only people assuming that you can do postgresql_oomprotect="ALL" and expect it to behave. Someone pointed me at this article: https://fluca1978.github.io/2021/04/02/OOMKillerFreeBSD.html which tried the same solution and also learned that it is not working with the provided rc script.

gbe added a subscriber: gbe.

LGTM, since all previously comments are addressed.