Validate RLIMIT_NOFILE against limits.http_max_conns_per_client #7434

pierresouchay · 2020-03-11T15:41:41Z

I spent some time today on my local Mac to figure out why Consul 1.6.3+ was not accepting limits.http_max_conns_per_client.

This adds an explicit check on the number of file descriptors to be sure it might work (this is no guarantee as if many clients are reaching the agent, it might consume even more file descriptors)

Anyway, many users are fighting with RLIMIT_NOFILE, having a clear message would allow them to figure out what to fix.

Example of message (reload or start):

2020-03-11T16:38:37.062+0100 [ERROR] agent: Error starting agent: error="system allows a max of 512 file descriptors, but limits.http_max_conns_per_client: 8192 needs at least 8212"

dnephin

Thank you for the PR!

Would it be possible to do this validation earlier in the flow? It seems like this check is validating a configuration option. Could it be validated as part of the config validation?

The lint timeout in CI should be fixed by #7496. It seems that the default timeout is 1 minute and sometimes it takes a bit longer.

pierresouchay · 2020-03-25T17:03:17Z

@dnephin DONE, now, it fails with consul validate

pierresouchay · 2020-03-25T21:10:17Z

@dnephin DONE, with fix in unit test (since validation now fails), try to fix another unstable unit test in last commit

Message:

consul validate test.json
Config validation failed: system allows a max of 256 file descriptors, but limits.http_max_conns_per_client: 65535 needs at least 65555

hanshasselberg

LGTM, except this one little comment about a change that doesn't belong in here.

api/agent_test.go

pierresouchay · 2020-04-01T11:01:29Z

Unstable test for go-tests:
TestForwardSignals/signal-terminated: util_test.go:285: expected to read line "signal: terminated" but got "signal: urgent I/O condition"

Probably due to a VM issue, restarting test

hanshasselberg

LGTM!

dnephin

Thank you for making those changes! I think I did not explain myself well in the previous comment. I was hoping we could do this validation in a single place as part of config validation.

I've left some comments below that I think will allow us to isolate this validation to a single package. Please let me know if I've missed something, or if that won't work for some reason.

agent/agent.go

lib/agent_limits.go

pierresouchay · 2020-04-01T15:45:02Z

@dnephin Fixed your concerns in the last commit

dnephin

Thank you again for making those changes! This change is looking good!

I think my last comment about unexporting the helpers may have been missed. Added more explanation inline, and one more suggestion about the test.

agent/agent_test.go

dnephin · 2020-04-01T16:22:42Z

agent/agent_test.go

+			# This value is more than max on Windows as well
+			http_max_conns_per_client = 16777217
+		}`
+	_, _, validationError := TestConfigWithErr(testutil.Logger(t), config.Source{Name: t.Name(), Format: "hcl", Data: hcl})


It can be difficult to understand what is being tested when test helpers hide the function-under-test. TestConfig as a helper for testing other code makes a lot of sense, but in this case since we are testing validate I think we want to be calling Builder.Validate or Builder.BuildAndValidate directly, not through a helper.

dnephin · 2020-04-01T16:23:26Z

agent/config/agent_limits.go

+
+// CheckLimitsFromMaxConnsPerClient check that value provided might be OK
+// return an error if values are not compatible
+func CheckLimitsFromMaxConnsPerClient(maxConnsPerClient int) error {


This can be unexported (checkLimitsFromMaxConnsPerClient) now that it is called from inside the same package.

dnephin · 2020-04-01T16:25:56Z

agent/testagent.go

+
+// TestConfig returns a unique default configuration for testing an
+// agent.
+func TestConfig(logger hclog.Logger, sources ...config.Source) *config.RuntimeConfig {


I think with the change suggested above (to call Builder.BuildAndValidate directly from the test), this new function won't be necessary .

lib/limits.go

I spent some time today on my local Mac to figure out why Consul 1.6.3+ was not accepting limits.http_max_conns_per_client. This adds an explicit check on number of file descriptors to be sure it might work (this is no guarantee as if many clients are reaching the agent, it might consume even more file descriptors) Anyway, many users are fighting with RLIMIT_NOFILE, having a clear message would allow them to figure out what to fix. Example of message (reload or start): ``` 2020-03-11T16:38:37.062+0100 [ERROR] agent: Error starting agent: error="system allows a max of 512 file descriptors, but limits.http_max_conns_per_client: 8192 needs at least 8212" ```

…ration

pierresouchay · 2020-04-01T17:33:08Z

@dnephin DONE

hanshasselberg · 2020-04-01T20:05:36Z

Hi Pierre, after a little internal discussion, what do you think about putting all the code into one place without exporting any of it? It could go all in config, without touching lib at all.

pierresouchay · 2020-04-01T20:19:29Z

@i0rek @dnephin DONE, moved limits*.go to config package + un-exported getrlimit()

hanshasselberg

Found another tiny thing...

agent/config/agent_limits_test.go

dnephin

Looks great! Thank you!

Just one question I wasn't sure about

dnephin · 2020-04-01T20:54:26Z

agent/config/agent_limits_test.go

@@ -0,0 +1,45 @@
+// +build !consulent


Was this by mistake? Why do we want to exclude this test on consulent ?

pierresouchay · 2020-04-01T21:09:29Z

@i0rek @dnephin Oops, yes, I took it from another test, wrong copy/paste. Fixed in last commit

hanshasselberg

👍

pierresouchay force-pushed the check_for_max_file_descriptors branch from cee0944 to 3adfcb5 Compare March 11, 2020 15:59

pierresouchay force-pushed the check_for_max_file_descriptors branch from b1d6422 to d1c9c84 Compare March 25, 2020 14:53

dnephin reviewed Mar 25, 2020

View reviewed changes

pierresouchay force-pushed the check_for_max_file_descriptors branch 2 times, most recently from a4e3f6e to 27b4f78 Compare March 25, 2020 17:02

pierresouchay force-pushed the check_for_max_file_descriptors branch from 27b4f78 to 7373bcd Compare March 25, 2020 18:27

pierresouchay force-pushed the check_for_max_file_descriptors branch from 28692bd to 9a9553a Compare March 26, 2020 20:42

hanshasselberg requested changes Apr 1, 2020

View reviewed changes

api/agent_test.go Outdated Show resolved Hide resolved

hanshasselberg self-assigned this Apr 1, 2020

hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Apr 1, 2020

pierresouchay force-pushed the check_for_max_file_descriptors branch 2 times, most recently from db25c61 to e66bfbf Compare April 1, 2020 10:50

ghost removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Apr 1, 2020

pierresouchay force-pushed the check_for_max_file_descriptors branch from e66bfbf to 9da0843 Compare April 1, 2020 11:03

hanshasselberg approved these changes Apr 1, 2020

View reviewed changes

dnephin reviewed Apr 1, 2020

View reviewed changes

agent/agent.go Outdated Show resolved Hide resolved

agent/agent.go Outdated Show resolved Hide resolved

lib/agent_limits.go Outdated Show resolved Hide resolved

dnephin reviewed Apr 1, 2020

View reviewed changes

pierresouchay added 6 commits April 1, 2020 18:53

Added unit test for GetRLimits()

d1b33a5

Now enforce at validation time to allow consul validate to fail

8593ca7

Updated unit test to take into account validation failing early

1902584

Since validation fails now, fix unit test TestAgent_GetRLimits().

995f7b9

Removed duplicate test + only validate RLimit() while parsing configu…

d609c06

…ration

pierresouchay force-pushed the check_for_max_file_descriptors branch from b196a28 to 5869ee6 Compare April 1, 2020 17:31

Refactored and moved everything into config package

f6b83a8

pierresouchay force-pushed the check_for_max_file_descriptors branch from 5869ee6 to f6b83a8 Compare April 1, 2020 17:32

Moved everything in package config

765fd8a

hanshasselberg requested changes Apr 1, 2020

View reviewed changes

agent/config/agent_limits_test.go Outdated Show resolved Hide resolved

dnephin approved these changes Apr 1, 2020

View reviewed changes

Removed wrong build directive in test.

1bd0553

pierresouchay force-pushed the check_for_max_file_descriptors branch from 4bf6d49 to 1bd0553 Compare April 1, 2020 21:53

hanshasselberg approved these changes Apr 2, 2020

View reviewed changes

hanshasselberg merged commit be1c5c4 into hashicorp:master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate RLIMIT_NOFILE against limits.http_max_conns_per_client #7434

Validate RLIMIT_NOFILE against limits.http_max_conns_per_client #7434

pierresouchay commented Mar 11, 2020

dnephin left a comment

pierresouchay commented Mar 25, 2020

pierresouchay commented Mar 25, 2020

hanshasselberg left a comment

pierresouchay commented Apr 1, 2020

hanshasselberg left a comment

dnephin left a comment

pierresouchay commented Apr 1, 2020

dnephin left a comment

dnephin Apr 1, 2020 •

edited

Loading

dnephin Apr 1, 2020

dnephin Apr 1, 2020 •

edited

Loading

pierresouchay commented Apr 1, 2020

hanshasselberg commented Apr 1, 2020

pierresouchay commented Apr 1, 2020

hanshasselberg left a comment

dnephin left a comment

dnephin Apr 1, 2020

pierresouchay commented Apr 1, 2020

hanshasselberg left a comment

Validate RLIMIT_NOFILE against limits.http_max_conns_per_client #7434

Validate RLIMIT_NOFILE against limits.http_max_conns_per_client #7434

Conversation

pierresouchay commented Mar 11, 2020

dnephin left a comment

Choose a reason for hiding this comment

pierresouchay commented Mar 25, 2020

pierresouchay commented Mar 25, 2020

hanshasselberg left a comment

Choose a reason for hiding this comment

pierresouchay commented Apr 1, 2020

hanshasselberg left a comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

pierresouchay commented Apr 1, 2020

dnephin left a comment

Choose a reason for hiding this comment

dnephin Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

dnephin Apr 1, 2020

Choose a reason for hiding this comment

dnephin Apr 1, 2020 • edited Loading

Choose a reason for hiding this comment

pierresouchay commented Apr 1, 2020

hanshasselberg commented Apr 1, 2020

pierresouchay commented Apr 1, 2020

hanshasselberg left a comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dnephin Apr 1, 2020

Choose a reason for hiding this comment

pierresouchay commented Apr 1, 2020

hanshasselberg left a comment

Choose a reason for hiding this comment

dnephin Apr 1, 2020 •

edited

Loading

dnephin Apr 1, 2020 •

edited

Loading