Copilot commented on code in PR #12892:
URL: https://github.com/apache/trafficserver/pull/12892#discussion_r2908304832
##########
src/proxy/ReverseProxy.cc:
##########
@@ -155,43 +153,25 @@ reloadUrlRewrite()
// Release the old one
oldTable->release();
- Dbg(dbg_ctl_url_rewrite, msg_format, ts::filename::REMAP);
- Note(msg_format, ts::filename::REMAP);
+ Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str());
+ Note("%s", msg_buffer.c_str());
+ ctx.complete(msg_buffer);
return true;
} else {
- static const char *msg_format = "%s failed to load";
+ swoc::bwprint(msg_buffer, "{} failed to load", ts::filename::REMAP);
delete newTable;
- Dbg(dbg_ctl_url_rewrite, msg_format, ts::filename::REMAP);
- Error(msg_format, ts::filename::REMAP);
+ Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str());
+ Error("%s", msg_buffer.c_str());
+ ctx.fail(msg_buffer);
return false;
}
}
int
-url_rewrite_CB(const char * /* name ATS_UNUSED */, RecDataT /* data_type
ATS_UNUSED */, RecData data, void *cookie)
+url_rewrite_CB(const char * /* name ATS_UNUSED */, RecDataT /* data_type
ATS_UNUSED */, RecData data,
+ void * /* cookie ATS_UNUSED */)
{
- int my_token = static_cast<int>((long)cookie);
-
- switch (my_token) {
- case REVERSE_CHANGED:
- rewrite_table->SetReverseFlag(data.rec_int);
- break;
-
- case TSNAME_CHANGED:
- case FILE_CHANGED:
- case HTTP_DEFAULT_REDIRECT_CHANGED:
- eventProcessor.schedule_imm(new UR_UpdateContinuation(reconfig_mutex),
ET_TASK);
- break;
-
- case URL_REMAP_MODE_CHANGED:
- // You need to restart TS.
- break;
-
- default:
- ink_assert(0);
- break;
- }
-
+ rewrite_table->SetReverseFlag(data.rec_int);
return 0;
Review Comment:
The `url_rewrite_CB` function now only handles `REVERSE_CHANGED` (calling
`SetReverseFlag`), since `TSNAME_CHANGED`, `FILE_CHANGED`, and
`HTTP_DEFAULT_REDIRECT_CHANGED` are now handled by `ConfigRegistry`. However,
the function still takes `cookie` parameter (previously used to determine which
case to handle) but now ignores it. The callback registration at line 91 still
passes `(void *)REVERSE_CHANGED` as cookie. This works, but the function
signature with unused `cookie` and the remaining `RecRegisterConfigUpdateCb`
for `proxy.config.reverse_proxy.enabled` should be documented or simplified to
clarify that this is now only for reverse proxy enable/disable toggling.
##########
tests/gold_tests/remap/remap_reload.test.py:
##########
@@ -114,3 +116,7 @@ def update_remap_config(path: str, lines: list) -> None:
tr = Test.AddTestRun("post update golf")
tr.AddVerifierClientProcess("client_4", replay_file_4,
http_ports=[tm.Variables.port])
+
+tr = Test.AddTestRun("remap_config reload, test")
+tr.Processes.Default.Env = tm.Env
+tr.Processes.Default.Command = 'sleep 2; traffic_ctl rpc invoke
get_reload_config_status -f json | jq'
Review Comment:
The `remap_reload.test.py` appends a test run that invokes
`get_reload_config_status` via `traffic_ctl rpc invoke` and pipes to `jq`, but
doesn't set a return code expectation or add any validation. This appears to be
a debug/exploratory command left in the test. It should either validate
something meaningful or be removed before merging.
##########
src/traffic_server/traffic_server.cc:
##########
@@ -1864,8 +1913,8 @@ main(int /* argc ATS_UNUSED */, const char **argv)
// Records init
initialize_records();
- // Initialize file manager for TS.
- initialize_file_manager();
+ // Register non reloadable config files and records.yaml.
Review Comment:
There's an extra space in the comment: "Register non reloadable" — should
be "Register non-reloadable".
##########
src/proxy/http/HttpConfig.cc:
##########
@@ -191,7 +191,7 @@ int
HttpConfigCont::handle_event(int /* event ATS_UNUSED */, void * /* edata
ATS_UNUSED */)
{
if (ink_atomic_increment(&http_config_changes, -1) == 1) {
- HttpConfig::reconfigure();
+ HttpConfig::reconfigure(); // TODO: who's calling?
Review Comment:
The TODO comment "who's calling?" was added in this PR. This should be
addressed or documented rather than left as an open question in the code.
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -122,9 +123,60 @@ main([[maybe_unused]] int argc, const char **argv)
.add_example_usage("traffic_ctl config match [OPTIONS] REGEX [REGEX ...]")
.add_option("--records", "", "Emit output in YAML format")
.add_option("--default", "", "Include the default value");
- config_command.add_command("reload", "Request a configuration reload",
Command_Execute)
- .add_example_usage("traffic_ctl config reload");
- config_command.add_command("status", "Check the configuration status",
Command_Execute)
+
+ //
+ // Start a new reload. If used without any extra options, it will start a
new reload
+ // or show the details of the current reload if one is in progress.
+ // A new token will be assigned by the server if no token is provided.
+ config_command.add_command("reload", "Request a configuration reload", [&]()
{ command->execute(); })
+ .add_example_usage("traffic_ctl config reload")
+ //
+ // Start a new reload with a specific token. If no token is provided, the
server will assign one.
+ // If a reload is already in progress, it will try to show the details of
the current reload.
+ // If token already exists, you must use another token, or let the server
assign one.
+ .add_option("--token", "-t", "Configuration token to reload.", "", 1, "")
+ //
+ // Start a new reload and monitor its progress until completion.
+ // Polls the server at regular intervals (see --refresh-int).
+ // If a reload is already in progress, monitors that one instead.
+ .add_option("--monitor", "-m", "Monitor reload progress until completion")
+ //
+ // Start a new reload. if one in progress it will show de details of the
current reload.
+ // if no reload in progress, it will start a new one and it will show the
details of it.
+ // This cannot be used with --monitor, if both are set, --show-details
will be ignored.
+ .add_option("--show-details", "-s", "Show detailed information of the
reload.")
+ .add_option("--include-logs", "-l", "include logs in the details. only
work together with --show-details")
+
+ //
+ // Refresh interval in seconds used with --monitor.
+ // Controls how often to poll the server for reload status.
+ .add_option("--refresh-int", "-r", "Refresh interval in seconds (used with
--monitor). Accepts fractional values (e.g. 0.5)",
+ "", 1, "0.5")
+ //
+ // The server will not let you start two reload at the same time. This
option will force a new reload
+ // even if there is one in progress. Use with caution as this may have
unexpected results.
+ // This is mostly for debugging and testing purposes. note: Should we keep
it here?
+ .add_option("--force", "-F", "Force reload even if there are unsaved
changes")
Review Comment:
The `--force` option's description says "Force reload even if there are
unsaved changes" but according to the PR description and the RPC handler,
`--force` forces a new reload even if one is **in progress**. The help text is
misleading — "unsaved changes" implies something different. The description
should be something like "Force a new reload even if one is in progress".
##########
tests/gold_tests/parent_config/parent_config_reload.test.py:
##########
@@ -0,0 +1,86 @@
+'''
+Test parent.config reload via ConfigRegistry.
+
+Verifies that:
+1. parent.config reload works after file touch
+2. Record value change (retry_time) triggers parent reload
+'''
+# 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.
+
+import os
+
+Test.Summary = '''
+Test parent.config reload via ConfigRegistry.
+'''
+
+Test.ContinueOnFail = True
+
+ts = Test.MakeATSProcess("ts")
+ts.Disk.records_config.update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'parent_select|config',
+})
+
+# Initial parent.config with a simple rule
+ts.Disk.parent_config.AddLine('dest_domain=example.com
parent="origin.example.com:80"')
+
+config_dir = ts.Variables.CONFIGDIR
+
+# ================================================================
+# Test 1: Touch parent.config → reload → handler fires
+# ================================================================
+
+tr = Test.AddTestRun("Touch parent.config")
+tr.Processes.Default.StartBefore(ts)
+tr.Processes.Default.Command = f"sleep 3 && touch {os.path.join(config_dir,
'parent.config')} && sleep 1"
+tr.Processes.Default.ReturnCode = 0
+tr.StillRunningAfter = ts
+
+tr = Test.AddTestRun("Reload after parent.config touch")
+p = tr.Processes.Process("reload-1")
+p.Command = 'traffic_ctl config reload; sleep 30'
+p.Env = ts.Env
+p.ReturnCode = Any(0, -2)
+# Wait for the 2nd "finished loading" (1st is startup)
+p.Ready = When.FileContains(ts.Disk.diags_log.Name, "parent.config finished
loading", 2)
+p.Timeout = 20
+tr.Processes.Default.StartBefore(p)
+tr.Processes.Default.Command = 'echo "waiting for parent.config reload after
file touch"'
+tr.TimeOut = 25
+tr.StillRunningAfter = ts
+
+# ================================================================
+# Test 2: Change retry_time record value → triggers parent reload
+# No file touch, no explicit config reload — the
+# RecRegisterConfigUpdateCb fires automatically.
+# ================================================================
+
+tr = Test.AddTestRun("Change parent retry_time record value")
+p = tr.Processes.Process("reload-2")
+p.Command = ("traffic_ctl config set proxy.config.http.parent_proxy.retry_time
60; "
+ "sleep 30")
+p.Env = ts.Env
+p.ReturnCode = Any(0, -2)
+# Wait for the 3rd "finished loading"
+p.Ready = When.FileContains(ts.Disk.diags_log.Name, "parent.config finished
loading", 3)
+p.Timeout = 20
+tr.Processes.Default.StartBefore(p)
+## TODO: we should have an extension like When.ReloadCompleted(token, success)
to validate this inetasd of parsing
Review Comment:
Typo in comment: "inetasd" should be "instead".
##########
include/proxy/logging/LogConfig.h:
##########
@@ -103,11 +104,14 @@ class LogConfig : public ConfigInfo
void display(FILE *fd = stdout);
void setup_log_objects();
- static int reconfigure(const char *name, RecDataT data_type, RecData data,
void *cookie);
+ // static int reconfigure(const char *name, RecDataT data_type, RecData
data, void *cookie);
Review Comment:
The commented-out old declaration `// static int reconfigure(const char
*name, RecDataT data_type, RecData data, void *cookie);` should be removed. The
new declaration is directly below it.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_config_reload.test.py:
##########
@@ -0,0 +1,192 @@
+# 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.
+import sys
+import os
+
+# To include util classes
+sys.path.insert(0, f'{Test.TestDirectory}')
+
+from traffic_ctl_test_utils import Make_traffic_ctl
+# import ruamel.yaml Uncomment only when GoldFilePathFor is used.
+
+
+def touch(fname, times=None):
+ with open(fname, 'a'):
+ os.utime(fname, times)
+
+
+Test.Summary = '''
+Test traffic_ctl config reload.
+'''
+
+Test.ContinueOnFail = True
+
+records_config = '''
+ udp:
+ threads: 1
+ diags:
+ debug:
+ enabled: 1
+ tags: rpc|config
+ throttling_interval_msec: 0
+ '''
+
+traffic_ctl = Make_traffic_ctl(Test, records_config, Any(0, 2))
+# todo: we need to get the status just in json format
+
+#### CONFIG STATUS
+
+# Config status with no token, no reloads exist, should return error.
+traffic_ctl.config().status().validate_with_text("""
+``
+Message: No reload tasks found, Code: 6005
+``
+""")
+
+traffic_ctl.config().status().token("test1").validate_with_text("""
+``
+Message: Token 'test1' not found, Code: 6001
+``
+""")
+
+traffic_ctl.config().status().count("all").validate_with_text("""
+``
+Message: No reload tasks found, Code: 6005
+``
+""")
+
+traffic_ctl.config().status().token("test1").count("all").validate_with_text(
+ """
+``
+You can't use both --token and --count options together. Ignoring --count
+``
+Message: Token 'test1' not found, Code: 6001
+``
+""")
+##### CONFIG RELOAD
+
+# basic reload, no params. no existing reload in progress, we expect this to
start a new reload.
+traffic_ctl.config().reload().validate_with_text(
+ "\u2714 Reload scheduled [``]\n\n Monitor : traffic_ctl config reload -t
`` -m\n Details : traffic_ctl config reload -t `` -s -l"
+)
+
+# basic reload, but traffic_ctl should create and wait for the details,
showing the newly created
+# reload and some details.
+traffic_ctl.config().reload().show_details().validate_contains_all("Reload
scheduled", "Waiting for details", "Reload [success]")
+
+# Now we try with a token, this should start a new reload with the given token.
+token = "testtoken_1234"
+traffic_ctl.config().reload().token(token).validate_with_text(
+ f"\u2714 Reload scheduled [{token}]\n\n Monitor : traffic_ctl config
reload -t {token} -m\n Details : traffic_ctl config reload -t {token} -s -l"
+)
+
+# traffic_ctl config status should show the last reload, same as the above.
+traffic_ctl.config().status().token(token).validate_contains_all("success",
"testtoken_1234")
+
+# Now we try again, with same token, this should fail as the token already
exists.
+traffic_ctl.config().reload().token(token).validate_with_text(
+ f"\u2717 Token '{token}' already in use\n\n Status : traffic_ctl config
status -t {token}\n Retry : traffic_ctl config reload"
+)
+
+# Modify ip_allow.yaml and validate the reload status.
+
+tr = Test.AddTestRun("rouch file to trigger ip_allow reload")
Review Comment:
Typo: "rouch" should be "touch" in the test run name.
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -122,9 +123,60 @@ main([[maybe_unused]] int argc, const char **argv)
.add_example_usage("traffic_ctl config match [OPTIONS] REGEX [REGEX ...]")
.add_option("--records", "", "Emit output in YAML format")
.add_option("--default", "", "Include the default value");
- config_command.add_command("reload", "Request a configuration reload",
Command_Execute)
- .add_example_usage("traffic_ctl config reload");
- config_command.add_command("status", "Check the configuration status",
Command_Execute)
+
+ //
+ // Start a new reload. If used without any extra options, it will start a
new reload
+ // or show the details of the current reload if one is in progress.
+ // A new token will be assigned by the server if no token is provided.
+ config_command.add_command("reload", "Request a configuration reload", [&]()
{ command->execute(); })
+ .add_example_usage("traffic_ctl config reload")
+ //
+ // Start a new reload with a specific token. If no token is provided, the
server will assign one.
+ // If a reload is already in progress, it will try to show the details of
the current reload.
+ // If token already exists, you must use another token, or let the server
assign one.
+ .add_option("--token", "-t", "Configuration token to reload.", "", 1, "")
+ //
+ // Start a new reload and monitor its progress until completion.
+ // Polls the server at regular intervals (see --refresh-int).
+ // If a reload is already in progress, monitors that one instead.
+ .add_option("--monitor", "-m", "Monitor reload progress until completion")
+ //
+ // Start a new reload. if one in progress it will show de details of the
current reload.
+ // if no reload in progress, it will start a new one and it will show the
details of it.
+ // This cannot be used with --monitor, if both are set, --show-details
will be ignored.
+ .add_option("--show-details", "-s", "Show detailed information of the
reload.")
+ .add_option("--include-logs", "-l", "include logs in the details. only
work together with --show-details")
+
+ //
+ // Refresh interval in seconds used with --monitor.
+ // Controls how often to poll the server for reload status.
+ .add_option("--refresh-int", "-r", "Refresh interval in seconds (used with
--monitor). Accepts fractional values (e.g. 0.5)",
+ "", 1, "0.5")
+ //
+ // The server will not let you start two reload at the same time. This
option will force a new reload
+ // even if there is one in progress. Use with caution as this may have
unexpected results.
+ // This is mostly for debugging and testing purposes. note: Should we keep
it here?
+ .add_option("--force", "-F", "Force reload even if there are unsaved
changes")
+ //
+ // Pass inline config data for reload. Like curl's -d flag:
+ // -d @file.yaml - read config from file
+ // -d @file1.yaml @file2.yaml - read multiple files
+ // -d @- - read config from stdin
+ // -d "yaml: content" - inline yaml string
+ .add_option("--data", "-d", "Inline config data (@file, @- for stdin, or
yaml string)", "", MORE_THAN_ZERO_ARG_N, "")
+ .add_option(
+ "--initial-wait", "-w",
+ "Initial wait before first poll, giving the server time to schedule all
handlers (seconds). Accepts fractional values", "", 1,
+ "2")
+ .add_option("--timeout", "-T",
+ "Maximum time to wait for reload completion (used with
--monitor). "
+ "Accepts duration units: 30s, 1m, 500ms, etc. 0 means no
timeout",
+ "", 1, "0")
+ .with_required("--monitor");
Review Comment:
The `--timeout` option uses `.with_required("--monitor")` (line 175), but
the `add_option` for `--timeout` is chained with `--initial-wait`, making
`.with_required("--monitor")` apply only to `--timeout`. Is this correct? It
seems like `--initial-wait` should also require `--monitor` based on the PR
description ("Use with monitor() or show_details()"). If `--initial-wait` is
also dependent on `--monitor`, it needs its own `.with_required()` call.
##########
src/traffic_ctl/jsonrpc/CtrlRPCRequests.h:
##########
@@ -39,15 +41,67 @@ struct GetAllRecordsRequest :
shared::rpc::RecordLookupRequest {
};
//------------------------------------------------------------------------------------------------------------------------------------
///
-/// @brief Models the config reload request. No params are needed.
+/// @brief Models the config reload request. Supports both file-based and
rpc-supplied modes.
+/// rpc-supplied mode is triggered when configs is present.
///
struct ConfigReloadRequest : shared::rpc::ClientRequest {
+ struct Params {
+ std::string token;
+ bool force{false};
+ YAML::Node configs; // Optional: if present, triggers inline mode
+ };
+ ConfigReloadRequest(Params p) { super::params = std::move(p); }
std::string
get_method() const override
{
return "admin_config_reload";
}
};
+
+// Full list of reload tasks, could be nested.
+struct ConfigReloadResponse {
+ // Existing reload task info, could be nested.
+ struct ReloadInfo {
+ std::string config_token;
+ std::string status;
+ std::string description;
+ std::string filename;
+ std::vector<std::string> logs;
+ std::vector<ReloadInfo> sub_tasks;
+ struct Meta { // internal info.
+ int64_t created_time_ms{0};
+ int64_t last_updated_time_ms{0};
+ bool is_main_task{false};
+ } meta;
+ };
+
+ struct Error {
+ int code;
+ std::string message;
+ };
+ std::vector<Error> error; ///< Error list, if any.
+
+ // when requestiong existing tasks.
Review Comment:
Typo in comment: "requestiong" should be "requesting".
##########
src/mgmt/config/ConfigContext.cc:
##########
@@ -0,0 +1,159 @@
+/** @file
+
+ ConfigContext implementation
+
+ @section license License
+
+ 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.
+ */
+
+#include "mgmt/config/ConfigContext.h"
+#include "mgmt/config/ConfigReloadTrace.h"
+#include "mgmt/config/ReloadCoordinator.h"
+
+#include <yaml-cpp/yaml.h>
+
+// Defined here (not = default in header) so that YAML::Node ctor/dtor/copy
+// symbols are only emitted in this TU (part of librecords, which links
yaml-cpp).
+// Otherwise every consumer of RecCore.h would need yaml-cpp at link time.
+ConfigContext::ConfigContext() = default;
+ConfigContext::~ConfigContext() = default;
+ConfigContext::ConfigContext(ConfigContext const &) = default;
+ConfigContext &ConfigContext::operator=(ConfigContext const &) = default;
+
+ConfigContext::ConfigContext(std::shared_ptr<ConfigReloadTask> t,
std::string_view description, std::string_view filename)
+ : _task(t)
+{
+ if (auto p = _task.lock()) {
+ if (!description.empty()) {
+ p->set_description(description);
+ }
+ if (!filename.empty()) {
+ p->set_filename(filename);
+ }
+ }
+}
+
+bool
+ConfigContext::is_terminal() const
+{
+ if (auto p = _task.lock()) {
+ return ConfigReloadTask::is_terminal(p->get_state());
+ }
+ return true; // expired task is supposed to be terminal
+}
+
+void
+ConfigContext::in_progress(std::string_view text)
+{
+ if (auto p = _task.lock()) {
+ p->set_in_progress();
+ if (!text.empty()) {
+ p->log(std::string{text});
+ }
+ }
+}
+
+void
+ConfigContext::log(std::string_view text)
+{
+ if (auto p = _task.lock()) {
+ p->log(std::string{text});
+ }
+}
+
+void
+ConfigContext::complete(std::string_view text)
+{
+ if (auto p = _task.lock()) {
+ p->set_completed();
+ if (!text.empty()) {
+ p->log(std::string{text});
+ }
+ }
+}
+
+void
+ConfigContext::fail(std::string_view reason)
+{
+ if (auto p = _task.lock()) {
+ p->set_failed();
+ if (!reason.empty()) {
+ p->log(std::string{reason});
+ }
+ }
+}
+
+void
+ConfigContext::fail(swoc::Errata const &errata, std::string_view summary)
+{
+ if (auto p = _task.lock()) {
+ p->set_failed();
+ // Log the summary first
+ if (!summary.empty()) {
+ p->log(std::string{summary});
+ }
+ // Log each error from the errata
+ for (auto const &err : errata) {
+ p->log(std::string{err.text()});
+ }
+ }
+}
+
+std::string
+ConfigContext::get_description() const
+{
+ if (auto p = _task.lock()) {
+ return p->get_description();
+ }
+ return {};
+}
+
+ConfigContext
+ConfigContext::add_dependent_ctx(std::string_view description)
+{
+ if (auto p = _task.lock()) {
+ auto child = p->add_child(description);
+ // child task will get the full content of the parent task
+ // TODO: eventyually we can have a "key" passed so child module
Review Comment:
Typo in comment: "eventyually" should be "eventually".
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -796,17 +798,26 @@ SSLTicketParams::LoadTicketData(char *ticket_data, int
ticket_data_len)
void
SSLTicketKeyConfig::startup()
{
- sslTicketKey.reset(new ConfigUpdateHandler<SSLTicketKeyConfig>());
+
config::ConfigRegistry::Get_Instance().register_record_config("ssl_ticket_key",
// key
+
[](ConfigContext ctx) { // handler callback
+ //
eventually ctx should passed throuough to the reconfigure fn to
Review Comment:
Typo in comment: "throuough" should be "through".
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -90,7 +90,8 @@ main([[maybe_unused]] int argc, const char **argv)
.add_option("--run-root", "", "using TS_RUNROOT as sandbox", "TS_RUNROOT",
1)
.add_option("--format", "-f", "Use a specific output format {json|rpc}",
"", 1, "", "format")
.add_option("--read-timeout-ms", "", "Read timeout for RPC (in
milliseconds)", "", 1, "10000", "read-timeout")
- .add_option("--read-attempts", "", "Read attempts for RPC", "", 1, "100",
"read-attempts");
+ .add_option("--read-attempts", "", "Read attempts for RPC", "", 1, "100",
"read-attempts")
+ .add_option("--watch", "-w", "Execute a program periodically. Watch
interval(in seconds) can be passed.", "", 1, "-1", "watch");
Review Comment:
The `--watch` global option uses `-w` as its short flag, but `config reload`
also uses `-w` for `--initial-wait`. These are on different scopes (global vs
subcommand), but depending on how ArgParser resolves short flags across scopes,
this could cause ambiguity. Please verify that the ArgParser correctly scopes
short options to their respective command levels and that `-w` on the `config
reload` subcommand does not conflict with the global `-w`.
##########
src/proxy/IPAllow.cc:
##########
@@ -108,34 +121,36 @@ IpAllow::startup()
}
void
-IpAllow::reconfigure()
+IpAllow::reconfigure(ConfigContext ctx)
{
- self_type *new_table;
-
+ self_type *new_table;
+ std::string text;
Note("%s loading ...", ts::filename::IP_ALLOW);
+ ctx.in_progress();
new_table = new self_type("proxy.config.cache.ip_allow.filename",
"proxy.config.cache.ip_categories.filename");
// IP rules need categories, so load them first (if they exist).
if (auto errata = new_table->BuildCategories(); !errata.is_ok()) {
- std::string text;
- swoc::bwprint(text, "{} failed to load\n{}",
new_table->ip_categories_config_file, errata);
- Error("%s", text.c_str());
+ swoc::bwprint(text, "{} failed to load",
new_table->ip_categories_config_file);
+ Error("%s\n%s", text.c_str(), swoc::bwprint(text, "{}", errata).c_str());
+ ctx.fail(errata, "{} failed to load",
new_table->ip_categories_config_file);
delete new_table;
return;
}
if (auto errata = new_table->BuildTable(); !errata.is_ok()) {
- std::string text;
- swoc::bwprint(text, "{} failed to load\n{}", ts::filename::IP_ALLOW,
errata);
+ swoc::bwprint(text, "{} failed to load", ts::filename::IP_ALLOW);
if (errata.severity() <= ERRATA_ERROR) {
- Error("%s", text.c_str());
+ Error("%s\n%s", text.c_str(), swoc::bwprint(text, "{}", errata).c_str());
} else {
- Fatal("%s", text.c_str());
+ Fatal("%s\n%s", text.c_str(), swoc::bwprint(text, "{}", errata).c_str());
Review Comment:
Same buffer-reuse bug as the `BuildCategories` block above:
`swoc::bwprint(text, ...)` is called on line 141 and then `swoc::bwprint(text,
"{}", errata).c_str()` on line 143/145 overwrites `text` before the first `%s`
argument is consumed by `Error`/`Fatal`. Use a separate buffer for the errata
string.
##########
include/records/RecCore.h:
##########
@@ -244,9 +245,17 @@ RecErrT RecGetRecordPersistenceType(const char *name,
RecPersistT *persist_type,
RecErrT RecGetRecordSource(const char *name, RecSourceT *source, bool lock =
true);
/// Generate a warning if any configuration name/value is not registered.
-void RecConfigWarnIfUnregistered();
+// void RecConfigWarnIfUnregistered();
Review Comment:
The commented-out old declaration `// void RecConfigWarnIfUnregistered();`
should be removed. It serves no purpose and could be confusing alongside the
new declaration immediately below it.
##########
tests/gold_tests/jsonrpc/config_reload_tracking.test.py:
##########
@@ -0,0 +1,304 @@
+'''
+Test config reload tracking functionality.
+
+Tests the following features:
+1. Basic reload with token generation
+2. Querying reload status while in progress
+3. Reload history tracking
+4. Force reload while one is in progress
+5. Custom token names
+6. Duplicate token prevention
+'''
+# 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 Request, Response
+import time
+
+Test.Summary = 'Test config reload tracking with tokens and status'
+Test.ContinueOnFail = True
+
+ts = Test.MakeATSProcess('ts', dump_runroot=True)
+
+Test.testName = 'config_reload_tracking'
+
+# Initial configuration
+ts.Disk.records_config.update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'rpc|config',
+})
+
+# Store tokens for later tests
+stored_tokens = []
+
+# ============================================================================
+# Test 1: Basic reload - verify token is returned
+# ============================================================================
+tr = Test.AddTestRun("Basic reload with auto-generated token")
+tr.Processes.Default.StartBefore(ts)
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload())
+
+
+def validate_basic_reload(resp: Response):
+ '''Verify reload returns a token'''
+ if resp.is_error():
+ return (False, f"Error: {resp.error_as_str()}")
+
+ result = resp.result
+ token = result.get('token', '')
+ created_time = result.get('created_time', '')
+ messages = result.get('message', [])
+
+ if not token:
+ return (False, "No token returned")
+
+ if not token.startswith('rldtk-'):
+ return (False, f"Token should start with 'rldtk-', got: {token}")
+
+ # Store for later tests
+ stored_tokens.append(token)
+
+ return (True, f"Reload started: token={token}, created={created_time},
messages={messages}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_basic_reload)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 2: Query status of completed reload
+# ============================================================================
+tr = Test.AddTestRun("Query status of completed reload")
+tr.DelayStart = 2 # Give time for reload to complete
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload_status())
+
+
+def validate_status_query(resp: Response):
+ '''Check reload status after completion'''
+ if resp.is_error():
+ # If method doesn't exist, that's OK - we're testing the main reload
+ return (True, f"Status query: {resp.error_as_str()}")
+
+ result = resp.result
+ return (True, f"Reload status: {result}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_status_query)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 3: Reload with custom token
+# ============================================================================
+tr = Test.AddTestRun("Reload with custom token")
+tr.DelayStart = 1
+custom_token = f"my-custom-token-{int(time.time())}"
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(token=custom_token))
+
+
+def validate_custom_token(resp: Response):
+ '''Verify custom token is accepted'''
+ if resp.is_error():
+ # Check if it's a "reload in progress" error
+ error_str = resp.error_as_str()
+ if 'in progress' in error_str.lower():
+ return (True, f"Reload in progress (expected): {error_str}")
+ return (False, f"Error: {error_str}")
+
+ result = resp.result
+ token = result.get('token', '')
+
+ if token != custom_token:
+ return (False, f"Expected custom token '{custom_token}', got
'{token}'")
+
+ stored_tokens.append(token)
+ return (True, f"Custom token accepted: {token}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_custom_token)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 4: Force reload while previous might still be processing
+# ============================================================================
+tr = Test.AddTestRun("Force reload")
+tr.DelayStart = 1
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(force=True))
+
+
+def validate_force_reload(resp: Response):
+ '''Verify force reload works'''
+ if resp.is_error():
+ return (False, f"Force reload failed: {resp.error_as_str()}")
+
+ result = resp.result
+ token = result.get('token', '')
+ if token:
+ stored_tokens.append(token)
+
+ return (True, f"Force reload succeeded: token={token}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_force_reload)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 5: Try duplicate token (should fail)
+# ============================================================================
+tr = Test.AddTestRun("Duplicate token rejection")
+tr.DelayStart = 2 # Wait for previous reload to complete
+
+
+def make_duplicate_test():
+ # Use the first token we stored
+ if stored_tokens:
+ return Request.admin_config_reload(token=stored_tokens[0])
+ return Request.admin_config_reload(token="rldtk-duplicate-test")
+
+
+tr.AddJsonRPCClientRequest(ts, make_duplicate_test())
+
+
+def validate_duplicate_rejection(resp: Response):
+ '''Verify duplicate tokens are rejected'''
+ if resp.is_error():
+ return (True, f"Duplicate rejected (expected): {resp.error_as_str()}")
+
+ result = resp.result
+ errors = result.get('error', [])
+ if errors:
+ return (True, f"Duplicate token rejected: {errors}")
+
+ # If no error, check if token was actually reused
+ token = result.get('token', '')
+ return (True, f"Reload result: token={token}, errors={errors}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_duplicate_rejection)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 6: Rapid succession reloads
+# ============================================================================
+tr = Test.AddTestRun("Rapid succession reloads - first")
+tr.DelayStart = 1
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload())
+
+
+def validate_rapid_first(resp: Response):
+ '''First rapid reload'''
+ if resp.is_error():
+ return (True, f"First rapid: {resp.error_as_str()}")
+
+ result = resp.result
+ token = result.get('token', '')
+ return (True, f"First rapid reload: {token}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_rapid_first)
+tr.StillRunningAfter = ts
+
+# Second rapid reload (should see in-progress or succeed)
+tr = Test.AddTestRun("Rapid succession reloads - second")
+tr.DelayStart = 0 # No delay - immediately after first
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload())
+
+
+def validate_rapid_second(resp: Response):
+ '''Second rapid reload - may see in-progress'''
+ if resp.is_error():
+ return (True, f"Second rapid (may be in progress):
{resp.error_as_str()}")
+
+ result = resp.result
+ token = result.get('token', '')
+ error = result.get('error', [])
+
+ if error:
+ # In-progress is expected
+ return (True, f"Second rapid - in progress or error: {error}")
+
+ return (True, f"Second rapid reload: {token}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_rapid_second)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 7: Get reload history
+# ============================================================================
+tr = Test.AddTestRun("Get reload history")
+tr.DelayStart = 2
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload_history())
+
+
+def validate_history(resp: Response):
+ '''Check reload history'''
+ if resp.is_error():
+ # Method may not exist
+ return (True, f"History query: {resp.error_as_str()}")
+
+ result = resp.result
+ history = result.get('history', [])
+ return (True, f"Reload history: {len(history)} entries")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_history)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 8: Trigger reload and verify new config is loaded
+# ============================================================================
+tr = Test.AddTestRun("Reload after config change")
+tr.DelayStart = 1
+tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(force=True))
+
+
+def validate_reload_after_change(resp: Response):
+ '''Verify reload after config change'''
+ if resp.is_error():
+ return (False, f"Reload after change failed: {resp.error_as_str()}")
+
+ result = resp.result
+ token = result.get('token', '')
+ return (True, f"Reload after config change: token={token}")
+
+
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(validate_reload_after_change)
+tr.StillRunningAfter = ts
+
+# ============================================================================
+# Test 10: Final status check
+# ============================================================================
Review Comment:
The test numbering jumps from "Test 8" to "Test 10" — "Test 9" is missing
from the comments. This may be intentional (a removed test), but the comment
still says "Test 10" which is inconsistent.
##########
src/proxy/IPAllow.cc:
##########
@@ -108,34 +121,36 @@ IpAllow::startup()
}
void
-IpAllow::reconfigure()
+IpAllow::reconfigure(ConfigContext ctx)
{
- self_type *new_table;
-
+ self_type *new_table;
+ std::string text;
Note("%s loading ...", ts::filename::IP_ALLOW);
+ ctx.in_progress();
new_table = new self_type("proxy.config.cache.ip_allow.filename",
"proxy.config.cache.ip_categories.filename");
// IP rules need categories, so load them first (if they exist).
if (auto errata = new_table->BuildCategories(); !errata.is_ok()) {
- std::string text;
- swoc::bwprint(text, "{} failed to load\n{}",
new_table->ip_categories_config_file, errata);
- Error("%s", text.c_str());
+ swoc::bwprint(text, "{} failed to load",
new_table->ip_categories_config_file);
+ Error("%s\n%s", text.c_str(), swoc::bwprint(text, "{}", errata).c_str());
Review Comment:
The `IpAllow::reconfigure` reuses the same `text` string variable for both
the `bwprint` formatting call and the `Error`/`Fatal` macro. On line 135,
`swoc::bwprint(text, ...)` writes into `text`, then the `Error` macro passes
`text.c_str()` as the first `%s` argument, and `swoc::bwprint(text, "{}",
errata).c_str()` as the second — but this second call overwrites `text`
in-place, so `text.c_str()` in the first argument now points to the errata
content, not the original error message. The same issue exists on line 143/145.
Each `bwprint` call should use a separate buffer for the errata string.
##########
src/proxy/http/remap/unit-tests/CMakeLists.txt:
##########
@@ -201,10 +201,20 @@ target_compile_definitions(
target_include_directories(test_NextHopStrategyFactory PRIVATE
${PROJECT_SOURCE_DIR}/tests/include)
target_link_libraries(
- test_NextHopStrategyFactory PRIVATE Catch2::Catch2WithMain ts::hdrs
ts::inkutils tscore libswoc::libswoc
- yaml-cpp::yaml-cpp
+ test_NextHopStrategyFactory
+ PRIVATE Catch2::Catch2WithMain
+ ts::hdrs
+ ts::inkutils
+ tscore
+ libswoc::libswoc
+ yaml-cpp::yaml-cpp
+ configmanager
)
+if(NOT APPLE)
+ target_link_options(test_NextHopStrategyFactory PRIVATE
-Wl,--allow-multiple-definition)
+endif()
Review Comment:
Using `--allow-multiple-definition` linker flag is a workaround that
suppresses legitimate duplicate symbol warnings. This can mask real issues
where the same symbol is defined in multiple compilation units. Consider using
proper library factoring or weak symbols instead. If this is truly needed as a
short-term fix, at minimum add a comment explaining which symbols conflict and
why.
##########
src/traffic_ctl/traffic_ctl.cc:
##########
@@ -122,9 +123,60 @@ main([[maybe_unused]] int argc, const char **argv)
.add_example_usage("traffic_ctl config match [OPTIONS] REGEX [REGEX ...]")
.add_option("--records", "", "Emit output in YAML format")
.add_option("--default", "", "Include the default value");
- config_command.add_command("reload", "Request a configuration reload",
Command_Execute)
- .add_example_usage("traffic_ctl config reload");
- config_command.add_command("status", "Check the configuration status",
Command_Execute)
+
+ //
+ // Start a new reload. If used without any extra options, it will start a
new reload
+ // or show the details of the current reload if one is in progress.
+ // A new token will be assigned by the server if no token is provided.
+ config_command.add_command("reload", "Request a configuration reload", [&]()
{ command->execute(); })
+ .add_example_usage("traffic_ctl config reload")
+ //
+ // Start a new reload with a specific token. If no token is provided, the
server will assign one.
+ // If a reload is already in progress, it will try to show the details of
the current reload.
+ // If token already exists, you must use another token, or let the server
assign one.
+ .add_option("--token", "-t", "Configuration token to reload.", "", 1, "")
+ //
+ // Start a new reload and monitor its progress until completion.
+ // Polls the server at regular intervals (see --refresh-int).
+ // If a reload is already in progress, monitors that one instead.
+ .add_option("--monitor", "-m", "Monitor reload progress until completion")
+ //
+ // Start a new reload. if one in progress it will show de details of the
current reload.
+ // if no reload in progress, it will start a new one and it will show the
details of it.
+ // This cannot be used with --monitor, if both are set, --show-details
will be ignored.
+ .add_option("--show-details", "-s", "Show detailed information of the
reload.")
+ .add_option("--include-logs", "-l", "include logs in the details. only
work together with --show-details")
+
+ //
+ // Refresh interval in seconds used with --monitor.
+ // Controls how often to poll the server for reload status.
+ .add_option("--refresh-int", "-r", "Refresh interval in seconds (used with
--monitor). Accepts fractional values (e.g. 0.5)",
+ "", 1, "0.5")
+ //
+ // The server will not let you start two reload at the same time. This
option will force a new reload
+ // even if there is one in progress. Use with caution as this may have
unexpected results.
+ // This is mostly for debugging and testing purposes. note: Should we keep
it here?
Review Comment:
The comment says "note: Should we keep it here?" — this internal discussion
question should be resolved before merging. Either remove the option or remove
the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
