DAOS-16645 cart: Bump file descriptor limit #15224

jolivier23 · 2024-09-30T19:49:40Z

With tcp provider, using many sockets can cause significant file descriptor usage. Bump the soft limit, if possible and warn if it appears insufficient.

Required-githooks: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

With tcp provider, using many sockets can cause significant file descriptor usage. Bump the soft limit, if possible and warn if it appears insufficient. Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

github-actions · 2024-09-30T19:49:59Z

Ticket title is 'Automatically bump soft file descriptor limit for tcp provider'
Status is 'In Review'
Labels: 'google-cloud-daos'
https://daosio.atlassian.net/browse/DAOS-16645

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

frostedcmos · 2024-09-30T20:42:31Z

src/cart/crt_init.c

+
+		if (rlim.rlim_cur < MIN_TCP_FD) {
+			if (rlim.rlim_max < MIN_TCP_FD) {
+				D_ERROR("File descriptor soft limit should be at least %d\n",


since you are checking rlim_max, shoudlnt error read 'hard limit' instead of soft here?

frostedcmos · 2024-09-30T20:43:40Z

src/cart/crt_init.c

+				DS_ERROR(errno,
+					 "setrlimit() failed. Unable to bump file descriptor"
+					 " limit to suitable value (>= %d)",
+					 MIN_TCP_FD);


why not print rlim_max instead to know what value exactly it failed on?

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

frostedcmos · 2024-09-30T21:23:40Z

src/cart/crt_init.c

+				DS_ERROR(errno,
+					 "setrlimit() failed. Unable to bump file descriptor"
+					 " limit to value >= %d, limit is %lu",
+					 MIN_TCP_FD, rlim.rlim_max);


goto next; missing here? ele on error you will print d_info below:)

done, good catch. I added the print later.

mchaarawi · 2024-09-30T21:26:56Z

I thought we had some documentation that recommends setting the nopen files to 1048576 somewhere.

jolivier23 · 2024-09-30T21:42:16Z

I thought we had some documentation that recommends setting the nopen files to 1048576 somewhere.

you might be right and I can change it to a minimum of 1048576 if we want that. I just think we should bump it automatically if we can regardless.

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 · 2024-09-30T21:47:40Z

I thought we had some documentation that recommends setting the nopen files to 1048576 somewhere.

you might be right and I can change it to a minimum of 1048576 if we want that. I just think we should bump it automatically if we can regardless.

Looks like we don't or at least I can't find it anywhere.

mchaarawi · 2024-09-30T21:54:52Z

I thought we had some documentation that recommends setting the nopen files to 1048576 somewhere.

you might be right and I can change it to a minimum of 1048576 if we want that. I just think we should bump it automatically if we can regardless.

Looks like we don't or at least I can't find it anywhere.

yea i couldn't find it either but i know it was a requirement for using tcp on boro for example

jolivier23 · 2024-09-30T23:19:32Z

I thought we had some documentation that recommends setting the nopen files to 1048576 somewhere.

you might be right and I can change it to a minimum of 1048576 if we want that. I just think we should bump it automatically if we can regardless.

Looks like we don't or at least I can't find it anywhere.

yea i couldn't find it either but i know it was a requirement for using tcp on boro for example

We've been using 131072 in our documentation for parallelstore. Should be sufficient for most cases as with our largest instance and dfuse with 8 eq would only use around 3500 fds for sockets. Anyway, I think it's a reasonable minimum and this patch will set it higher if allowed.

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

mchaarawi · 2024-10-01T14:33:40Z

I thought we had some documentation that recommends setting the nopen files to 1048576 somewhere.

you might be right and I can change it to a minimum of 1048576 if we want that. I just think we should bump it automatically if we can regardless.

Looks like we don't or at least I can't find it anywhere.

yea i couldn't find it either but i know it was a requirement for using tcp on boro for example

We've been using 131072 in our documentation for parallelstore. Should be sufficient for most cases as with our largest instance and dfuse with 8 eq would only use around 3500 fds for sockets. Anyway, I think it's a reasonable minimum and this patch will set it higher if allowed.

fine with me. ill defer my +1 to @soumagne

soumagne

minus the comments that I had, the PR looks good to me overall.

soumagne · 2024-10-01T14:37:23Z

src/cart/crt_init.c

@@ -492,10 +492,14 @@ check_grpid(crt_group_id_t grpid)
 	return rc;
 }

+#define MIN_TCP_FD 131072


please prefix internal macros with CRT_

src/cart/crt_init.c

soumagne · 2024-10-01T14:51:23Z

src/cart/crt_init.c

+		/* Bump file descriptor limit if low and if possible */
+		rc = getrlimit(RLIMIT_NOFILE, &rlim);
+		if (rc != 0) {
+			DS_WARN(errno, "getrlimit() failed. Unable to check file descriptor limit");


why is this a warn ? to me it sounds like it should be an error with appropriate errno checking ?

done. Looking at the man page, it doesn't look like it should ever fail in our case.

soumagne · 2024-10-01T14:54:09Z

src/cart/crt_init.c

+		if (rlim.rlim_cur < MIN_TCP_FD) {
+			if (rlim.rlim_max < MIN_TCP_FD) {
+				D_ERROR("File descriptor hard limit should be at least %d\n",
+					MIN_TCP_FD);


I would consider that as a warning, not as an error, if let's say someone wanted for any reason to have a lower limit...

I agree, this is a warning - in effect it's asking the admin to reconfigure the node but there's no error here.

src/cart/crt_init.c

ashleypittman · 2024-10-01T15:01:07Z

src/cart/crt_init.c

+		if (rlim.rlim_cur < MIN_TCP_FD) {
+			if (rlim.rlim_max < MIN_TCP_FD) {
+				D_ERROR("File descriptor hard limit should be at least %d\n",
+					MIN_TCP_FD);


I agree, this is a warning - in effect it's asking the admin to reconfigure the node but there's no error here.

src/cart/crt_init.c

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

ashleypittman · 2024-10-17T14:46:17Z

utils/node_local_test.py

+        # valgrind reduces the hard limit unless we bump the soft limit first
+        (soft, hard) = resource.getrlimit(resource.RLIMIT_NOFILE)
+        if soft < hard:
+            resource.setrlimit(resource.RLIMIT_NOFILE, (hard, hard))
+


This would be better set after passing the command line at https://github.com/daos-stack/daos/blob/master/utils/node_local_test.py#L6530

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

Allow-unstable-test: true Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

With tcp provider, using many sockets can cause significant file descriptor usage. Bump the soft limit, if possible and warn if it appears insufficient. Valgrind sets hard limit to soft limit, so work around that in NLT. Signed-off-by: Jeff Olivier <[email protected]>

DAOS-16645 cart: Bump file descriptor limit

6f723ee

With tcp provider, using many sockets can cause significant file descriptor usage. Bump the soft limit, if possible and warn if it appears insufficient. Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested review from a team as code owners September 30, 2024 19:49

jolivier23 added 2 commits September 30, 2024 13:59

clang-format

cce4e6d

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

One more checkpatch issue

c7f7403

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested review from ashleypittman, mchaarawi and frostedcmos September 30, 2024 20:42

frostedcmos reviewed Sep 30, 2024

View reviewed changes

Address review comments

6c49a8e

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested a review from frostedcmos September 30, 2024 21:10

frostedcmos reviewed Sep 30, 2024

View reviewed changes

Address review comment

28b70ad

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested a review from frostedcmos September 30, 2024 21:44

frostedcmos previously approved these changes Sep 30, 2024

View reviewed changes

Allow-unstable-test: true

5df6142

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

mchaarawi requested a review from soumagne October 1, 2024 13:57

soumagne requested changes Oct 1, 2024

View reviewed changes

ashleypittman reviewed Oct 1, 2024

View reviewed changes

Address review comments

772d239

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 dismissed frostedcmos’s stale review via 772d239 October 1, 2024 21:03

jolivier23 requested review from frostedcmos and soumagne October 1, 2024 21:03

frostedcmos previously approved these changes Oct 17, 2024

View reviewed changes

ashleypittman reviewed Oct 17, 2024

View reviewed changes

jolivier23 added 2 commits October 17, 2024 09:00

Merge branch 'master' into jvolivie/setrlimit

ed1d48a

Move rlimit setting to main

bf5aee8

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 dismissed frostedcmos’s stale review via bf5aee8 October 17, 2024 15:19

jolivier23 added 6 commits October 17, 2024 12:36

retrigger

2465445

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

try again

89f6a11

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

Test again

99dbd33

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

Merge branch 'master' into jvolivie/setrlimit

808030f

test again

560f030

Allow-unstable-test: true Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

try again

782c700

Required-githooks: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested review from ashleypittman and frostedcmos October 18, 2024 22:43

frostedcmos approved these changes Oct 18, 2024

View reviewed changes

pavelgritsai approved these changes Oct 21, 2024

View reviewed changes

jolivier23 merged commit e117a6c into master Oct 21, 2024
75 checks passed

jolivier23 deleted the jvolivie/setrlimit branch October 21, 2024 17:15

jolivier23 added a commit that referenced this pull request Nov 8, 2024

DAOS-16645 cart: Bump file descriptor limit (#15224) (#15473)

f4c4ac2

This was referenced Nov 9, 2024

mjmac/DAOS 7203 google 2.6 #15483

Closed

mjmac/DAOS 16787 google 2.6 #15498

Closed

DAOS-16787 utils: Suppress NLT valgrind false positives (#15478) #15499

Merged

mjmac mentioned this pull request Nov 18, 2024

DAOS-16791 control: Add include_fabric_ifaces to agent config (#15470) #15513

Merged

This was referenced Nov 25, 2024

mjmac/DAOS 16355 google 2.6 #15535

Closed

DAOS-16355 client: pydaos.torch module (#15475) #15536

Merged

mjmac mentioned this pull request Dec 2, 2024

DAOS-16822 client: Add mkdir_p variant to dfs_sys API (#15526) #15550

Merged

juszhan1 mentioned this pull request Dec 23, 2024

Merge release/2.6 into google/2.6 #15663

Closed

mjmac mentioned this pull request Jan 13, 2025

Merge release/2.6 into google/2.6 #15725

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-16645 cart: Bump file descriptor limit #15224

DAOS-16645 cart: Bump file descriptor limit #15224

jolivier23 commented Sep 30, 2024

github-actions bot commented Sep 30, 2024 •

edited

Loading

frostedcmos Sep 30, 2024

jolivier23 Sep 30, 2024

frostedcmos Sep 30, 2024

jolivier23 Sep 30, 2024

frostedcmos Sep 30, 2024

jolivier23 Sep 30, 2024

mchaarawi commented Sep 30, 2024

jolivier23 commented Sep 30, 2024

jolivier23 commented Sep 30, 2024

mchaarawi commented Sep 30, 2024

jolivier23 commented Sep 30, 2024

mchaarawi commented Oct 1, 2024

soumagne left a comment

soumagne Oct 1, 2024

jolivier23 Oct 1, 2024

soumagne Oct 1, 2024

jolivier23 Oct 1, 2024

soumagne Oct 1, 2024

ashleypittman Oct 1, 2024

jolivier23 Oct 1, 2024

ashleypittman Oct 1, 2024

ashleypittman Oct 17, 2024

DAOS-16645 cart: Bump file descriptor limit #15224

DAOS-16645 cart: Bump file descriptor limit #15224

Conversation

jolivier23 commented Sep 30, 2024

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchaarawi commented Sep 30, 2024

jolivier23 commented Sep 30, 2024

jolivier23 commented Sep 30, 2024

mchaarawi commented Sep 30, 2024

jolivier23 commented Sep 30, 2024

mchaarawi commented Oct 1, 2024

soumagne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024 •

edited

Loading