This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit f480e43aca3b5ac4daf75f9e697fe46247175a8b Author: Damian Meden <[email protected]> AuthorDate: Tue Oct 22 09:30:10 2024 +0200 records - Honour the minimum value for `--maxRecords` (#11816) * records - Honour the minimum value for --maxRecords. Although the default and minimum value is 2048 (as advertised) ATS will accept any value less than that, which will end up in ATS crashing. This change sets the advertised default value if the passed value is invalid or less than the default. This also makes the parameter function. (cherry picked from commit e54e8c8da41f9a9364bb8b15353fe41bba8fd750) --- src/records/P_RecCore.cc | 4 ++ src/records/P_RecDefs.h | 2 +- src/records/RecCore.cc | 2 +- src/records/RecUtils.cc | 4 +- src/records/RecordsConfigUtils.cc | 14 +++-- src/traffic_server/traffic_server.cc | 66 ++++++++++++++-------- .../records/ts_max_records_param.test.py | 56 ++++++++++++++++++ 7 files changed, 115 insertions(+), 33 deletions(-) diff --git a/src/records/P_RecCore.cc b/src/records/P_RecCore.cc index 810ceb14cd..c84df8a0b2 100644 --- a/src/records/P_RecCore.cc +++ b/src/records/P_RecCore.cc @@ -188,6 +188,10 @@ RecSetRecord(RecT rec_type, const char *name, RecDataT data_type, RecData *data, goto Ldone; } r1 = RecAlloc(rec_type, name, data_type); + if (r1 == nullptr) { + err = REC_ERR_FAIL; + goto Ldone; + } RecDataSet(data_type, &(r1->data), data); if (REC_TYPE_IS_STAT(r1->rec_type) && (data_raw != nullptr)) { r1->stat_meta.data_raw = *data_raw; diff --git a/src/records/P_RecDefs.h b/src/records/P_RecDefs.h index 65e65fd4d2..a712d2dc8b 100644 --- a/src/records/P_RecDefs.h +++ b/src/records/P_RecDefs.h @@ -30,7 +30,7 @@ // We need at least this many internal record entries for our configurations and metrics. // This may need adjustments if we make significant additions to librecords. Note that // plugins are using their own metrics systems. -#define REC_MAX_RECORDS 2048 +#define REC_DEFAULT_ELEMENTS_SIZE 2048 #define REC_CONFIG_UPDATE_INTERVAL_MS 3000 #define REC_REMOTE_SYNC_INTERVAL_MS 5000 diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index ec83d64cbd..57108943fc 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -43,7 +43,7 @@ using ts::Metrics; // This is needed to manage the size of the librecords record. It can't be static, because it needs to be modified // and used (read) from several binaries / modules. -int max_records_entries = REC_MAX_RECORDS; +int max_records_entries = REC_DEFAULT_ELEMENTS_SIZE; static bool g_initialized = false; RecRecord *g_records = nullptr; std::unordered_map<std::string, RecRecord *> g_records_ht; diff --git a/src/records/RecUtils.cc b/src/records/RecUtils.cc index 7adcc17757..e4751a8265 100644 --- a/src/records/RecUtils.cc +++ b/src/records/RecUtils.cc @@ -58,8 +58,8 @@ RecRecordFree(RecRecord *r) RecRecord * RecAlloc(RecT rec_type, const char *name, RecDataT data_type) { - if (g_num_records >= REC_MAX_RECORDS) { - Warning("too many stats/configs, please report a bug to [email protected]"); + if (g_num_records >= max_records_entries) { + Fatal("Too many config records already registered (%d). Hint: increase --maxRecords param.", max_records_entries); return nullptr; } diff --git a/src/records/RecordsConfigUtils.cc b/src/records/RecordsConfigUtils.cc index 122de5c6ea..58f7b0fd90 100644 --- a/src/records/RecordsConfigUtils.cc +++ b/src/records/RecordsConfigUtils.cc @@ -58,22 +58,22 @@ initialize_record(const RecordElement *record, void *) } RecDataSetFromString(record->value_type, &data, value); - + RecErrT reg_status{REC_ERR_FAIL}; switch (record->value_type) { case RECD_INT: - RecRegisterConfigInt(type, record->name, data.rec_int, update, check, record->regex, source, access); + reg_status = RecRegisterConfigInt(type, record->name, data.rec_int, update, check, record->regex, source, access); break; case RECD_FLOAT: - RecRegisterConfigFloat(type, record->name, data.rec_float, update, check, record->regex, source, access); + reg_status = RecRegisterConfigFloat(type, record->name, data.rec_float, update, check, record->regex, source, access); break; case RECD_STRING: - RecRegisterConfigString(type, record->name, data.rec_string, update, check, record->regex, source, access); + reg_status = RecRegisterConfigString(type, record->name, data.rec_string, update, check, record->regex, source, access); break; case RECD_COUNTER: - RecRegisterConfigCounter(type, record->name, data.rec_counter, update, check, record->regex, source, access); + reg_status = RecRegisterConfigCounter(type, record->name, data.rec_counter, update, check, record->regex, source, access); break; default: @@ -81,6 +81,10 @@ initialize_record(const RecordElement *record, void *) break; } // switch + if (reg_status != REC_ERR_OKAY) { + ink_fatal("%s cannot be registered. Please check the logs.", record->name); + } + RecDataZero(record->value_type, &data); } else { // Everything else, except PROCESS, are stats. TODO: Should modularize this too like PROCESS was done. ink_assert(REC_TYPE_IS_STAT(type)); diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index bf3886a10c..bdbc6692c2 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -150,8 +150,9 @@ const long MAX_LOGIN = ink_login_name_max(); void init_ssl_ctx_callback(void *ctx, bool server); -void load_ssl_file_callback(const char *ssl_file); -void task_threads_started_callback(); +void load_ssl_file_callback(const char *ssl_file); +void task_threads_started_callback(); +static void check_max_records_argument(const ArgumentDescription *arg, unsigned int nargs, const char *val); int num_of_net_threads = ink_number_of_processors(); int num_accept_threads = 0; @@ -208,39 +209,39 @@ int cmd_block = 0; int delay_listen_for_cache = 0; ArgumentDescription argument_descriptions[] = { - {"net_threads", 'n', "Number of Net Threads", "I", &num_of_net_threads, "PROXY_NET_THREADS", nullptr}, - {"udp_threads", 'U', "Number of UDP Threads", "I", &num_of_udp_threads, "PROXY_UDP_THREADS", nullptr}, - {"accept_thread", 'a', "Use an Accept Thread", "T", &num_accept_threads, "PROXY_ACCEPT_THREAD", nullptr}, - {"httpport", 'p', "Port descriptor for HTTP Accept", "S*", &http_accept_port_descriptor, "PROXY_HTTP_ACCEPT_PORT", nullptr}, - {"disable_freelist", 'f', "Disable the freelist memory allocator", "T", &cmd_disable_freelist, "PROXY_DPRINTF_LEVEL", nullptr}, + {"net_threads", 'n', "Number of Net Threads", "I", &num_of_net_threads, "PROXY_NET_THREADS", nullptr }, + {"udp_threads", 'U', "Number of UDP Threads", "I", &num_of_udp_threads, "PROXY_UDP_THREADS", nullptr }, + {"accept_thread", 'a', "Use an Accept Thread", "T", &num_accept_threads, "PROXY_ACCEPT_THREAD", nullptr }, + {"httpport", 'p', "Port descriptor for HTTP Accept", "S*", &http_accept_port_descriptor, "PROXY_HTTP_ACCEPT_PORT", nullptr }, + {"disable_freelist", 'f', "Disable the freelist memory allocator", "T", &cmd_disable_freelist, "PROXY_DPRINTF_LEVEL", nullptr }, {"disable_pfreelist", 'F', "Disable the freelist memory allocator in ProxyAllocator", "T", &cmd_disable_pfreelist, - "PROXY_DPRINTF_LEVEL", nullptr}, + "PROXY_DPRINTF_LEVEL", nullptr }, {"maxRecords", 'm', "Max number of librecords metrics and configurations (default & minimum: 2048)", "I", &max_records_entries, - "PROXY_MAX_RECORDS", nullptr}, + "PROXY_MAX_RECORDS", &check_max_records_argument}, #if TS_HAS_TESTS - {"regression", 'R', "Regression Level (quick:1..long:3)", "I", ®ression_level, "PROXY_REGRESSION", nullptr}, - {"regression_test", 'r', "Run Specific Regression Test", "S512", regression_test, "PROXY_REGRESSION_TEST", nullptr}, - {"regression_list", 'l', "List Regression Tests", "T", ®ression_list, "PROXY_REGRESSION_LIST", nullptr}, + {"regression", 'R', "Regression Level (quick:1..long:3)", "I", ®ression_level, "PROXY_REGRESSION", nullptr }, + {"regression_test", 'r', "Run Specific Regression Test", "S512", regression_test, "PROXY_REGRESSION_TEST", nullptr }, + {"regression_list", 'l', "List Regression Tests", "T", ®ression_list, "PROXY_REGRESSION_LIST", nullptr }, #endif // TS_HAS_TESTS #if TS_USE_DIAGS - {"debug_tags", 'T', "Vertical-bar-separated Debug Tags", "S1023", error_tags, "PROXY_DEBUG_TAGS", nullptr}, - {"action_tags", 'B', "Vertical-bar-separated Behavior Tags", "S1023", action_tags, "PROXY_BEHAVIOR_TAGS", nullptr}, + {"debug_tags", 'T', "Vertical-bar-separated Debug Tags", "S1023", error_tags, "PROXY_DEBUG_TAGS", nullptr }, + {"action_tags", 'B', "Vertical-bar-separated Behavior Tags", "S1023", action_tags, "PROXY_BEHAVIOR_TAGS", nullptr }, #endif - {"interval", 'i', "Statistics Interval", "I", &show_statistics, "PROXY_STATS_INTERVAL", nullptr}, + {"interval", 'i', "Statistics Interval", "I", &show_statistics, "PROXY_STATS_INTERVAL", nullptr }, {"command", 'C', "Maintenance Command to Execute\n" - " Commands: list, check, clear, clear_cache, clear_hostdb, verify_config, verify_global_plugin, verify_remap_plugin, help", "S511", &command_string, "PROXY_COMMAND_STRING", nullptr}, - {"conf_dir", 'D', "config dir to verify", "S511", &conf_dir, "PROXY_CONFIG_CONFIG_DIR", nullptr}, - {"clear_hostdb", 'k', "Clear HostDB on Startup", "F", &auto_clear_hostdb_flag, "PROXY_CLEAR_HOSTDB", nullptr}, - {"clear_cache", 'K', "Clear Cache on Startup", "F", &cacheProcessor.auto_clear_flag, "PROXY_CLEAR_CACHE", nullptr}, - {"bind_stdout", '-', "Regular file to bind stdout to", "S512", &bind_stdout, "PROXY_BIND_STDOUT", nullptr}, - {"bind_stderr", '-', "Regular file to bind stderr to", "S512", &bind_stderr, "PROXY_BIND_STDERR", nullptr}, - {"accept_mss", '-', "MSS for client connections", "I", &accept_mss, nullptr, nullptr}, - {"poll_timeout", 't', "poll timeout in milliseconds", "I", &poll_timeout, nullptr, nullptr}, - {"block", '-', "block for debug attach", "T", &cmd_block, nullptr, nullptr}, + " Commands: list, check, clear, clear_cache, clear_hostdb, verify_config, verify_global_plugin, verify_remap_plugin, help", "S511", &command_string, "PROXY_COMMAND_STRING", nullptr }, + {"conf_dir", 'D', "config dir to verify", "S511", &conf_dir, "PROXY_CONFIG_CONFIG_DIR", nullptr }, + {"clear_hostdb", 'k', "Clear HostDB on Startup", "F", &auto_clear_hostdb_flag, "PROXY_CLEAR_HOSTDB", nullptr }, + {"clear_cache", 'K', "Clear Cache on Startup", "F", &cacheProcessor.auto_clear_flag, "PROXY_CLEAR_CACHE", nullptr }, + {"bind_stdout", '-', "Regular file to bind stdout to", "S512", &bind_stdout, "PROXY_BIND_STDOUT", nullptr }, + {"bind_stderr", '-', "Regular file to bind stderr to", "S512", &bind_stderr, "PROXY_BIND_STDERR", nullptr }, + {"accept_mss", '-', "MSS for client connections", "I", &accept_mss, nullptr, nullptr }, + {"poll_timeout", 't', "poll timeout in milliseconds", "I", &poll_timeout, nullptr, nullptr }, + {"block", '-', "block for debug attach", "T", &cmd_block, nullptr, nullptr }, HELP_ARGUMENT_DESCRIPTION(), VERSION_ARGUMENT_DESCRIPTION(), RUNROOT_ARGUMENT_DESCRIPTION(), @@ -2381,5 +2382,22 @@ task_threads_started_callback() hook = hook->next(); } } +static void +check_max_records_argument(const ArgumentDescription * /* ATS_UNUSED arg*/, unsigned int /* nargs ATS_UNUSED */, const char *val) +{ + int32_t cmd_arg{0}; + try { + cmd_arg = std::stoi(val); + if (cmd_arg < REC_DEFAULT_ELEMENTS_SIZE) { + fprintf(stderr, "[WARNING] Passed maxRecords value=%d is lower than the default value %d. Default will be used.\n", cmd_arg, + REC_DEFAULT_ELEMENTS_SIZE); + max_records_entries = REC_DEFAULT_ELEMENTS_SIZE; + } + // max_records_entries keeps the passed value. + } catch (std::exception const &ex) { + fprintf(stderr, "[ERROR] Invalid %d value for maxRecords. Default %d will be used.\n", cmd_arg, REC_DEFAULT_ELEMENTS_SIZE); + max_records_entries = REC_DEFAULT_ELEMENTS_SIZE; + } +} } // end anonymous namespace diff --git a/tests/gold_tests/records/ts_max_records_param.test.py b/tests/gold_tests/records/ts_max_records_param.test.py new file mode 100644 index 0000000000..573b5e3283 --- /dev/null +++ b/tests/gold_tests/records/ts_max_records_param.test.py @@ -0,0 +1,56 @@ +''' +''' +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from jsonrpc import Notification, Request, Response + +Test.Summary = 'Basic test for traffic_server --maxRecords behavior when setting different values.' + +maxRecords = 1000 + +# 0 - maxRecords below the default value(2048). Traffic server should warn about this and use the default value. +ts = Test.MakeATSProcess(f"ts{maxRecords}", command=f"traffic_server --maxRecords {maxRecords}") +tr = Test.AddTestRun(f"--maxRecords {maxRecords}") +tr.Processes.Default.Command = 'echo 1' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore(ts) +tr.StillRunningAfter = ts +ts.Streams.All = Testers.ContainsExpression( + f"Passed maxRecords value={maxRecords} is lower than the default value 2048. Default will be used.", + "It should use the default value") + +# 1 - maxRecords with just invalid number. Traffic server should warn about this and use the default value. +maxRecords = "abc" +ts = Test.MakeATSProcess(f"ts{maxRecords}", command=f"traffic_server --maxRecords {maxRecords}") +tr = Test.AddTestRun(f"--maxRecords {maxRecords}") +tr.Processes.Default.Command = 'echo 1' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore(ts) +tr.StillRunningAfter = ts +ts.Streams.All = Testers.ContainsExpression( + f"Invalid 0 value for maxRecords. Default 2048 will be used.", "It should use the default value") + +# 2 - maxRecords over the default value +maxRecords = 5000 +ts = Test.MakeATSProcess(f"ts{maxRecords}", command=f"traffic_server --maxRecords {maxRecords}") +tr = Test.AddTestRun(f"--maxRecords {maxRecords}") +tr.Processes.Default.Command = 'echo 1' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore(ts) +tr.StillRunningAfter = ts +# At least it should not crash. +ts.Disk.traffic_out.Content = Testers.ContainsExpression(f"NOTE: records parsing completed", "should all be good")
