Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,28 @@ 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__
#include <sys/resource.h>
const uint64_t MAX_OPEN_FILES = []() {
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <sys/resource.h> is placed inside namespace ovms (and mid-file). Including system headers inside a namespace can unintentionally put all their declarations/types into that namespace and create subtle lookup/type issues. Move this include up with the other includes at file scope (outside the namespace) and keep only the constants inside ovms.

Copilot uses AI. Check for mistakes.
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 = 10; // we need to reserve some file descriptors for other operations, so we don't want to use all of them for drogon workers
const uint64_t DEFAULT_REST_WORKERS = (MAX_OPEN_FILES <= RESERVED_OPEN_FILES) ? AVAILABLE_CORES
: std::min(static_cast<uint64_t>(AVAILABLE_CORES), (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5);
Comment thread
dtrawins marked this conversation as resolved.
Outdated
#else
const uint64_t DEFAULT_REST_WORKERS = AVAILABLE_CORES;
#endif

Config& Config::parse(int argc, char** argv) {
ovms::CLIParser parser;
ovms::ServerSettingsImpl serverSettings;
Expand Down Expand Up @@ -306,6 +322,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() > (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5) {
std::cerr << "rest_workers count cannot be larger than " << (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5 << " due to open files limit. Current open files limit: " << MAX_OPEN_FILES << std::endl;
return false;
}
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Linux, this check subtracts RESERVED_OPEN_FILES from MAX_OPEN_FILES without guarding MAX_OPEN_FILES <= RESERVED_OPEN_FILES. With unsigned types this can underflow and effectively disable the limit check. Add a guard (or use a saturating subtraction) before computing (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5, and consider emitting a clear error when the open-files limit is too low to run REST workers at all.

Suggested change
if (restWorkers() > (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5) {
std::cerr << "rest_workers count cannot be larger than " << (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5 << " due to open files limit. Current open files limit: " << MAX_OPEN_FILES << std::endl;
return false;
}
if (MAX_OPEN_FILES <= RESERVED_OPEN_FILES) {
std::cerr << "Open files limit (" << MAX_OPEN_FILES
<< ") is too low to run REST workers. Increase the RLIMIT_NOFILE soft limit "
<< "or adjust the server configuration." << std::endl;
return false;
}
const uint64_t maxRestWorkersByFiles = (MAX_OPEN_FILES - RESERVED_OPEN_FILES) / 5;
if (restWorkers() > maxRestWorkersByFiles) {
std::cerr << "rest_workers count cannot be larger than " << maxRestWorkersByFiles
<< " due to open files limit. Current open files limit: " << MAX_OPEN_FILES << std::endl;
return false;
}

Copilot uses AI. Check for mistakes.
#endif

#ifdef _WIN32
if (grpcWorkers() > WIN_MAX_GRPC_WORKERS) {
Expand Down
31 changes: 31 additions & 0 deletions src/test/ovmsconfig_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <filesystem>
#include <fstream>
#include <regex>
#include <sys/resource.h>
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<sys/resource.h> (and the new getrlimit/setrlimit-based tests) are Linux-specific, but the include is unconditional. This will break builds on non-Linux platforms. Wrap the include and these two new tests in #ifdef __linux__ (or equivalent platform guard).

Suggested change
#include <regex>
#include <sys/resource.h>
#include <regex>
#ifdef __linux__
#include <sys/resource.h>
#endif

Copilot uses AI. Check for mistakes.

#include <gmock/gmock.h>
#include <gtest/gtest.h>
Expand Down Expand Up @@ -202,6 +203,36 @@ 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 ");
}

TEST_F(OvmsConfigDeathTest, restWorkersDefaultReducedForOpenFilesLimit) {
// limit allowed number of open files to 1024 to make sure that rest_workers count is too large for the limit based on number of cpu cores alone
int cpu_cores = ovms::getCoreCount();
struct rlimit limit;
ASSERT_EQ(getrlimit(RLIMIT_NOFILE, &limit), 0);
struct rlimit newLimit = {static_cast<rlim_t>(cpu_cores * 5), 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
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test comment says the open-files limit is set to 1024, but the code actually sets it to cpu_cores * 5. Please update the comment to match the behavior (or adjust the limit if 1024 is intended).

Copilot uses AI. Check for mistakes.
ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &newLimit), 0);

Comment on lines +212 to +218
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setrlimit(RLIMIT_NOFILE, ...) can fail if newLimit.rlim_cur exceeds limit.rlim_max (or if the process lacks permission to change limits). As written, cpu_cores * 5 might be > rlim_max on some systems/containers, making this test flaky. Consider clamping rlim_cur to limit.rlim_max and/or skipping the test with a clear message when the limit cannot be adjusted.

Copilot uses AI. Check for mistakes.
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());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test doesnt check if number of workers has been changed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never mind, it would require calculation how many cores the machine has


ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &limit), 0);
}

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 202 due to open files limit. Current open files limit: 1024");
ASSERT_EQ(setrlimit(RLIMIT_NOFILE, &limit), 0);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If test exits earlier we won't restore original rlimit value. Need either unique_ptr with custom deleter to handle that or helper struct. The same is true for previous test.

}

TEST_F(OvmsConfigDeathTest, restWorkersDefinedRestPortUndefined) {
char* n_argv[] = {"ovms", "--config_path", "/path1", "--port", "8080", "--rest_workers", "60"};
int arg_count = 7;
Expand Down
Loading