Set higher open files limit in init script

[nineinchnick]

#5066 added mention of ulimit to Trino's requirements. These recommended values should be used by default when installing Trino using the RPM package.

The reason to set these in the init script, not in a /etc/security/limits.d/99-trino.conf file is that it's a simpler change and at some point in time Trino might migrate from a SysV script to a SystemD unit file, which would ignore limits from /etc/security since those are only used by PAM (the pam_limits.so module).

Defining these limits before sourcing the env.sh file allows users to adjust them further if required.

[hashhar]

Good change % verification.

[hashhar]

I expect these to fail (i.e. not take effect) unless one of /etc/security/limits.conf or /etc/systemd/user.conf or /etc/systemd/system.conf has a value higher than this allowed for the Trino user.

I'll try and verify in a VM.

EDIT: Correction. It's /etc/security/limits.conf and /etc/systemd/user.conf. They need to have higher values to allow ulimit to succeed otherwise it fails with an error. To check values you can use sysctl -a | grep 'files.nr_open'.

[nineinchnick]

I think root can execute them regardless of what's in /etc/security/limits.conf.

[nineinchnick]

BTW I checked CentOS packages and found only one that defines limits in /etc/security/limits.d, which is libvma. But it's also the single package that has a SysV init script, not a SystemD unit file.

[hashhar]

This only works because CentOS (since 7) uses SystemD and systemd sets the default hard limit to a high enough value that your command doesn't fail. This is not an assumption that is safe to make.

See https://www.freedesktop.org/software/systemd/man/systemd-system.conf.html#DefaultLimitCPU=.

And since we are setting the values we should also be setting nproc too either way.

My suggestion is to test whether we get a meaningful error message at startup.
We don't want the default state to be a situation where out of the box it's not visible why Trino fails to startup.

[hashhar]

Try this:

# CentOS 7
[root@centos hashhar]# sysctl -a | grep 'fs.nr_open'
fs.nr_open = 1048576
[root@centos hashhar]# ulimit -Hn 1048577
bash: ulimit: open files: cannot modify limit: Operation not permitted
[root@centos hashhar]# sysctl -w fs.nr_open=1048579
[root@centos hashhar]# ulimit -Hn 1048579
[root@centos hashhar]# ulimit -Hn
1048579

Also the soft limit needs to be increased too to actually allow any running process to make use of the increased hard limit. (EDIT: I see you set both).

I found https://unix.stackexchange.com/questions/447583/ulimit-vs-file-max very helpful.

[nineinchnick]

nproc at some point was dropped from the docs PR I linked. I'm not sure why, none of the comments there refer to that part. So we need an agreement for the right value and also put it in docs. Sounds like a separate issue.

I'm not sure what's the ask here. To set these to the least of 131072 and current fs.nr_open?

[nineinchnick]

I've made changes here to only raise the limit, never lower it, and never raise it above fs.nr_open.

[hashhar]

I'm starting to think the better solution is to only verify the ulimits are not insanely low (e.g. maybe values from

trino/testing/trino-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/Environment.java

Lines 404 to 411 in 55230fb

	private static List<Ulimit> standardUlimits()
	{
	return ImmutableList.of(
	// Number of open file descriptors
	new Ulimit("nofile", 65535L, 65535L),
	// Number of processes
	new Ulimit("nproc", 8096L, 8096L));
	}

) and throw an error with more information if not.

The correct place to do this seems to be the launcher since that's not limited to just the RPM and will work regardless of how people are running Trino (Docker, tarball or RPM etc.).

cc: @trinodb/maintainers Please chime in with opinions.

For context the issue is that most OSs default ulimits are very low (1024/4096) which can cause the ServiceLoader to fail but silently swallow too many open files and instead fail with a generic looking undebuggable error.

2021-08-05T09:42:06.396Z        ERROR   main    io.trino.server.Server  No service providers of type io.trino.spi.Plugin
java.lang.IllegalStateException: No service providers of type io.trino.spi.Plugin
        at com.google.common.base.Preconditions.checkState(Preconditions.java:591)
        at io.trino.server.PluginManager.loadPlugin(PluginManager.java:139)
        at io.trino.server.PluginManager.loadPlugin(PluginManager.java:129)
        at io.trino.server.ServerPluginsProvider.loadPlugins(ServerPluginsProvider.java:48)
        at io.trino.server.PluginManager.loadPlugins(PluginManager.java:110)
        at io.trino.server.Server.doStart(Server.java:122)
        at io.trino.server.Server.lambda$start$0(Server.java:77)
        at io.trino.$gen.Trino_360_e_0____20210805_094147_1.run(Unknown Source)
        at io.trino.server.Server.start(Server.java:77)

[mosabua]

I think we should not modify the system, but instead in the launcher verify everything is sufficient and if not - report error and fail start up. We dont automatically install / upgrade / downgrade Python or Java either. And that verification should be compatible so that it works with tarball, rpm and on containers and is independent of the Linux distro.

This change downgrades the ulimit for systems that might have higher values for some good reason ..

[nineinchnick]

This change downgrades the ulimit for systems that might have higher values for some good reason ..

@mosabua I've made changes here to only raise the limit, never lower it, and never raise it above fs.nr_open.

[mosabua]

Looks good now.. ultimately I still think this should go into the validation in the launcher script. Could be a follow up task and then we remove this here again.

[findepi]

cc @losipiuk

[findepi]

nit: When the current_limit is gt what we want, we should not invoke ulimit at all.
Avoid calling ulimit unnecessarily, as it could, in theory, fail.

[nineinchnick]

We'd have to either:

1. conditionally call ulimit -Hn and always call ulimit -Sn
2. also read the current value of ulimit -S and conditionally call both ulimit -Hn and ulimit -Sn
3. always call ulimit -n - I just checked the manual, and when neither -H nor -S is specified, it sets both limits.

To keep things simple, I'd assume ulimit is declarative and always call it. It's a bash built-in, so we don't even start another process here. I changed this to be a single call.

[hashhar]

@findepi i dont agree about doing this in RPM. This problem is general and affects every deployment mechanism like tarball, RPM and Docker.

I think the better solution is for the launcher to verify the limits are at-least some sane value.

[hashhar]

Turns out we already have validation for this (thanks to Piotr for pointing to it) in place at

trino/core/trino-main/src/main/java/io/trino/server/TrinoSystemRequirements.java

Lines 126 to 131 in 66f7d23

	if (maxFileDescriptorCount.getAsLong() < MIN_FILE_DESCRIPTORS) {
	failRequirement("Trino requires at least %s file descriptors (found %s)", MIN_FILE_DESCRIPTORS, maxFileDescriptorCount.getAsLong());
	}
	if (maxFileDescriptorCount.getAsLong() < RECOMMENDED_FILE_DESCRIPTORS) {
	warnRequirement("Current OS file descriptor limit is %s. Trino recommends at least %s", maxFileDescriptorCount.getAsLong(), RECOMMENDED_FILE_DESCRIPTORS);
	}

which runs as the first thing during Server startup and gives useful error messages.

I think we can close this PR now.

[findepi]

I think we can close this PR now.

The added value of the change is that RPM 'just works' instead of having user to update things. I don't feel strongly about this though.

[nineinchnick]

@hashhar @findepi this is good to go.

[hashhar]

Looks good % comment + a justification in commit msg.

[hashhar]

Thanks