-
Notifications
You must be signed in to change notification settings - Fork 247
restrict rest workers based on open files limit #4108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
58ceadb
d55e2b3
5821c8a
56c9b8d
d1f8e70
c452b51
502a853
9d534d8
eab2fea
f3af316
d83678c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
| // limitations under the License. | ||
| //***************************************************************************** | ||
| #include "config.hpp" | ||
|
|
||
| #include <algorithm> | ||
| #include <filesystem> | ||
| #include <limits> | ||
| #include <regex> | ||
|
|
@@ -37,18 +37,46 @@ | |
| #include "stringutils.hpp" | ||
| #include "systeminfo.hpp" | ||
|
|
||
| #ifdef __linux__ | ||
| #include <sys/resource.h> | ||
| #endif | ||
|
|
||
| namespace ovms { | ||
|
|
||
| const uint32_t AVAILABLE_CORES = getCoreCount(); | ||
| const uint32_t WIN_MAX_GRPC_WORKERS = 1; | ||
| const uint32_t MAX_PORT_NUMBER = std::numeric_limits<uint16_t>::max(); | ||
|
|
||
| // For drogon, we need to minimize the number of default workers since this value is set for both: unary and streaming (making it always double) | ||
| const uint64_t DEFAULT_REST_WORKERS = AVAILABLE_CORES; | ||
| const uint32_t DEFAULT_GRPC_MAX_THREADS = AVAILABLE_CORES * 8.0; | ||
| const size_t DEFAULT_GRPC_MEMORY_QUOTA = (size_t)2 * 1024 * 1024 * 1024; // 2GB | ||
| const uint64_t MAX_REST_WORKERS = 10'000; | ||
|
|
||
| // We need to minimize the number of default drogon workers since this value is set for both: unary and streaming (making it always double) | ||
| // on linux, restrict also based on the max allowed number of open files | ||
| #ifdef __linux__ | ||
|
|
||
| static uint64_t getMaxOpenFilesLimit() { | ||
| struct rlimit limit; | ||
| if (getrlimit(RLIMIT_NOFILE, &limit) == 0) { | ||
| return limit.rlim_cur; | ||
| } | ||
| return std::numeric_limits<uint64_t>::max(); | ||
| } | ||
|
|
||
| const uint64_t RESERVED_OPEN_FILES = 15; // we need to reserve some file descriptors for other operations, so we don't want to use all of them for drogon workers | ||
| uint64_t getDefaultRestWorkers() { | ||
| const uint64_t maxOpenFiles = getMaxOpenFilesLimit(); | ||
| if (maxOpenFiles <= RESERVED_OPEN_FILES) { | ||
| return static_cast<uint64_t>(2); // minimum functional number | ||
| } | ||
| return std::min(static_cast<uint64_t>(AVAILABLE_CORES), (maxOpenFiles - RESERVED_OPEN_FILES) / 7); // 5x rest_workers to initialize ovms and 2x rest_workers for new connections | ||
| } | ||
| #else | ||
| uint64_t getDefaultRestWorkers() { | ||
| return AVAILABLE_CORES; | ||
| } | ||
| #endif | ||
|
|
||
| Config& Config::parse(int argc, char** argv) { | ||
| ovms::CLIParser parser; | ||
| ovms::ServerSettingsImpl serverSettings; | ||
|
|
@@ -306,6 +334,12 @@ bool Config::validate() { | |
| std::cerr << "rest_workers is set but rest_port is not set. rest_port is required to start rest servers" << std::endl; | ||
| return false; | ||
| } | ||
| #ifdef __linux__ | ||
| if (restWorkers() > (getMaxOpenFilesLimit() - RESERVED_OPEN_FILES) / 6) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is it 6 here, but 7 in getDefaultRestWorkers()?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is default rest workers also max possible rest workers? |
||
| std::cerr << "rest_workers count cannot be larger than " << (getMaxOpenFilesLimit() - RESERVED_OPEN_FILES) / 6 << " due to open files limit. Current open files limit: " << getMaxOpenFilesLimit() << std::endl; | ||
| return false; | ||
| } | ||
| #endif | ||
|
|
||
| #ifdef _WIN32 | ||
| if (grpcWorkers() > WIN_MAX_GRPC_WORKERS) { | ||
|
|
@@ -368,7 +402,7 @@ const std::string Config::restBindAddress() const { return this->serverSettings. | |
| uint32_t Config::grpcWorkers() const { return this->serverSettings.grpcWorkers; } | ||
| uint32_t Config::grpcMaxThreads() const { return this->serverSettings.grpcMaxThreads.value_or(DEFAULT_GRPC_MAX_THREADS); } | ||
| size_t Config::grpcMemoryQuota() const { return this->serverSettings.grpcMemoryQuota.value_or(DEFAULT_GRPC_MEMORY_QUOTA); } | ||
| uint32_t Config::restWorkers() const { return this->serverSettings.restWorkers.value_or(DEFAULT_REST_WORKERS); } | ||
| uint32_t Config::restWorkers() const { return this->serverSettings.restWorkers.value_or(getDefaultRestWorkers()); } | ||
| const std::string& Config::modelName() const { return this->modelsSettings.modelName; } | ||
| const std::string& Config::modelPath() const { return this->modelsSettings.modelPath; } | ||
| const std::string& Config::batchSize() const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,10 @@ | |
| #include "../systeminfo.hpp" | ||
| #include "test_utils.hpp" | ||
|
|
||
| #ifdef __linux__ | ||
| #include <sys/resource.h> | ||
| #endif | ||
|
|
||
| using testing::_; | ||
| using testing::ContainerEq; | ||
| using testing::Return; | ||
|
|
@@ -202,6 +206,38 @@ TEST_F(OvmsConfigDeathTest, restWorkersTooLarge) { | |
| EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "rest_workers count should be from 2 to "); | ||
| } | ||
|
|
||
| #ifdef __linux__ | ||
| TEST_F(OvmsConfigDeathTest, restWorkersDefaultReducedForOpenFilesLimit) { | ||
| // limit allowed number of open files to value that enforce default rest_workers to be determined based on open files limit instead of number of cpu cores alone. This is to test that default rest_workers count is reduced when open files limit is low. | ||
| int cpu_cores = ovms::getCoreCount(); | ||
| struct rlimit limit; | ||
| ASSERT_EQ(getrlimit(RLIMIT_NOFILE, &limit), 0); | ||
| struct rlimit newLimit = {std::min(static_cast<rlim_t>(cpu_cores * 5), limit.rlim_max), limit.rlim_max}; | ||
| std::cout << "Setting open files limit to " << newLimit.rlim_cur << " to test that default rest_workers count is reduced based on open files limit" << std::endl; | ||
|
Comment on lines
+210
to
+216
|
||
| ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &newLimit), 0); | ||
|
|
||
|
Comment on lines
+212
to
+218
|
||
| char* n_argv[] = {"ovms", "--config_path", "/path1", "--rest_port", "8080", "--port", "8081"}; | ||
| int arg_count = 7; | ||
| ovms::Config::instance().parse(arg_count, n_argv); | ||
| EXPECT_TRUE(ovms::Config::instance().validate()); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test doesnt check if number of workers has been changed |
||
|
|
||
| ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &limit), 0); // revert ulimit to original value | ||
| } | ||
|
|
||
| TEST_F(OvmsConfigDeathTest, restWorkersTooLargeForOpenFilesLimit) { | ||
| // limit allowed number of open files to 1024 to make sure that rest_workers count is too large. | ||
| struct rlimit limit; | ||
| ASSERT_EQ(getrlimit(RLIMIT_NOFILE, &limit), 0); | ||
| struct rlimit newLimit = {1024, limit.rlim_max}; | ||
| std::cout << "Setting open files limit to " << newLimit.rlim_cur << " to test that rest_workers count is too large for the limit based on number of cpu cores alone" << std::endl; | ||
| ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &newLimit), 0); | ||
| char* n_argv[] = {"ovms", "--config_path", "/path1", "--rest_port", "8080", "--port", "8081", "--rest_workers", "1000"}; | ||
| int arg_count = 9; | ||
| EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), "rest_workers count cannot be larger than 168 due to open files limit. Current open files limit: 1024"); | ||
| ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &limit), 0); | ||
| } | ||
| #endif | ||
|
|
||
| TEST_F(OvmsConfigDeathTest, restWorkersDefinedRestPortUndefined) { | ||
| char* n_argv[] = {"ovms", "--config_path", "/path1", "--port", "8080", "--rest_workers", "60"}; | ||
| int arg_count = 7; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add log that number of workers has been decreased due to limit of open files?