Arvados

I suspect so, my fix was also to add `ulimit -n 1048576` to the runit script that starts keepstore, and the problem hasn't come back so far.

I reported it as a bug because (a) this is not documented in the install docs and (b) it seems like there is a problem with the way keepstore handles open files if it can get into a state where it fails to accept every single incoming connection for many days. If it just occasionally had a "too many open files" issue when under heavy load, but then later it started working again (presumably because it closed some sockets) then (b) would not be an issue. but it would still be good to have a doc fix for (a). As it is, it seems like something must be wrong with the way keepstore handles closing files in order for it to be possible to get into a state where it is perpetually out of file descriptors?

We suspect there is a bug here in an error path. We'll investigate.

In any case, perhaps we should increase the default value in the systemd config file at /lib/systemd/system/keepstore.service.

Perhaps 8192 is a better default:

# Copyright (C) The Arvados Authors. All rights reserved.
#
# SPDX-License-Identifier: AGPL-3.0

[Unit]
Description=Arvados Keep Storage Daemon
Documentation=https://doc.arvados.org/
After=network.target
AssertPathExists=/etc/arvados/keepstore/keepstore.yml
# systemd<230
StartLimitInterval=0
# systemd>=230
StartLimitIntervalSec=0

[Service]
Type=notify
LimitNOFILE=8192
ExecStart=/usr/bin/keepstore
Restart=always
RestartSec=1

[Install]
WantedBy=multi-user.target

Overriding per system is also possible, one can put a file at /etc/systemd/system/keepstore.service.d/override.conf with these contents, for instance:

[Service]
LimitNOFILE=8192

12859-keepstore-fd-leak @ 9ff58642c2c1eaae8d6dd7c01e8fb85a030c2dfc -- developer-run-tests: #2762

Tom Clegg wrote:

12859-keepstore-fd-leak @ 9ff58642c2c1eaae8d6dd7c01e8fb85a030c2dfc -- developer-run-tests: #2762

Only one question, on line 295 in `services/keepstore/unix_volume.go`, is there a reason to keep that explicit call to tmpfile.Close() ? We now have a new `defer tmpfile.Close()` higher up, so...

LGTM, otherwise!

Ward Vandewege wrote:

is there a reason to keep that explicit call to tmpfile.Close() ? We now have a new `defer tmpfile.Close()` higher up, so...

Only to detect errors (unflushed write?). The deferred Close just covers the return-early-because-we-hit-some-other-error-first case, where we don't care whether Close fails.

As noted in the commit message, 9ff58642c fixes a file descriptor leak when a filesystem-backed volume has Serialize: true and the client hangs up while waiting for another operation to finish on the same volume. When clients are piling up faster than keepstore can serve requests, many of the clients would time out at that stage, so this seems like a plausible explanation for the reported problem.