You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2026/10/15 11:57:23 UTC
[PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
brbzull0 opened a new pull request, #13842:
URL: https://github.com/apache/trafficserver/pull/13842
Follow-up to #13032. Fixes #13832.
`traffic_ctl --error-level` (#13032) grades a JSONRPC handler failure by the severity of its annotations. A `config reload` outcome is not a JSONRPC error: it comes back inside a successful response, as a task state plus task logs, so the level did not apply to it. On master, `config reload --monitor` exits 2 when the top-level task ends `fail` or `timeout` and 0 on `success`, whatever its log entries say, and `config status` exits 0 for every reload it shows.
This PR grades a finished reload by its task states and log entries, in `config reload --monitor` and in `config status`. Nothing changes on the server or the wire.
### What it does
- **The grade** (`appExitCodeFromReload` in `TrafficCtlStatus.h`): the most severe log entry of the task and of its sub-tasks, at any depth, is compared with `--error-level`.
- The state of each task bounds its entries. A task that ended `fail` or `timeout` counts as at least `error`, also when it logged nothing. The entries of any other task count as at most `warn`, since its handler did not fail it. With the default level the exit status therefore follows the task states: 2 when any task, at any depth, ended `fail` or `timeout`.
- A task that ended `timeout` has no outcome: the progress checker marked it after `proxy.config.admin.reload.timeout`, and its handler can still finish afterwards. So a reload with such a task exits 75 (EX_TEMPFAIL) below the level, not 0.
- An entry without a level is not graded: the one-argument `ctx.log(text)` entries, the reason traffic_server logs when it marks a reload `timeout`, and the plain-string logs of a traffic_server built before #13090. An entry whose level is not a known level (an integer out of range, or not an integer) counts as `emergency`. It is printed as before.
- A reload that is still running, has no state, or is in a state `traffic_ctl` does not know, is not graded.
- **`config reload --monitor`** grades the first finished state it sees. The `Details :` hint is printed when the reload did not end `success`, or when its grade is not 0, which includes 75 for a sub-task that ended `timeout`. `--monitor` prints text whatever `-f` says, as on master.
- **`config status`** grades the newest reload it shows. With `--count N`, the older reloads are history and do not change the exit status. `--min-level` still only filters what is printed: the grade covers every entry with a level, shown or hidden. `-f json` is graded the same as text.
- **Inline reloads (`--data`)**: the exit status now depends on `--error-level`, so the "not persisted to disk" note follows how the reload ended instead.
- Without `--monitor`, it is printed once the reload is scheduled.
- With `--monitor`, it is printed when the reload ended `success`, whatever its grade, or was still running when the monitor stopped, and not when it ended `fail` or `timeout`. When the monitor cannot read the state (a status poll failed, or the reply had no task), it is printed unless the exit status is 2, as before.
- It is not printed when the server refused the request because another reload was in progress.
How the open points of #13832 were decided:
| Open point | Decision |
|---|---|
| A `fail` task with no entry at or above the level, or no entries | Counts as at least `error`: exits 2 at the default level. `--error-level fatal` lets it exit 0 unless a task that failed logged `fatal` or above. |
| `timeout` | Same as `fail`, and 75 instead of 0 below the level. |
| Sub-task entries; `DL_Undefined` | Sub-task entries count, at any depth. Entries without a level are not graded. |
| `config status --count N` | The newest reload shown decides. |
| `--min-level` and `--error-level` | `--min-level` filters the display only. |
| (not in the issue) A `success` task with `error` or worse entries | Counts as at most `warn`: exits 0 at the default level, 2 at `--error-level warn`. Its handler chose to complete, as `cache.config` does when it drops a bad line. |
### Behaviour changes at the default level
- `config status` exits 2 when the newest reload it shows ended `fail` or `timeout`. It used to exit 0, so a script that runs it to print the report must now accept 2. Eight runs in four AuTests under `tests/gold_tests/jsonrpc/` asserted 0 for that case and now assert 2.
- `config status` and `config reload --monitor` also exit 2, and `--monitor` prints the `Details :` hint, for a reload that reports `success` while one of its sub-tasks ended `fail` or `timeout`. traffic_server does not report such a reload: its parent task takes the worst state of its sub-tasks. Another JSONRPC node could.
- The inline-reload note is no longer printed when the server refused the request because another reload was in progress.
For the reloads traffic_server reports, `config reload --monitor`, and so the `AddConfigReload` test helper, keep their exit status: 0 for `success`, also with `[Err]` entries, and 2 for `fail` or `timeout`. There, 75 comes from the monitor's own `--timeout`, or from a request it depends on that failed below the level (#13032), as before. The first two changes are listed in `doc/release-notes/upgrading.en.rst`.
### What this PR does not do
- `config reload` without `--monitor`, and with `--show-details`, is not graded. It returns once the reload is scheduled, or after one snapshot taken after `--initial-wait`, before there is an outcome. `--monitor` is the way to wait for one.
- A reload that is still running, has no state, or is in an unknown state is not graded. `config status` exits 0 for it, not 75. `--monitor` keeps polling it until `--timeout`, as on master.
- No line explains why a graded reload failed. `config status` prints the entries as before, minus those `--min-level` hides, and `--monitor` prints the `Details :` hint.
- Some traffic_server warnings carry no level in the task log, so they are not graded. The `records.yaml` handler logs its errata with `ctx.log("{}", zret)`, which stores no level, so `--error-level warn` does not fail a `records.yaml` reload that only logged warnings. Logging them with their level is a server change, left for a follow-up.
- After `config reload -D` without `--data`, the "not persisted to disk" note is still printed, as on master: `-D` writes into the same `configs` node as `--data` (#13110), and the note is keyed on that node. `virtualhost.yaml` (#13108) is the first core handler that reads a directive, so `-D` now has a core use. The fix needs a flag of its own for `--data` content, separate from the grade; left for a follow-up.
- `config reload --monitor` refused because another reload is in progress follows that other reload and exits with its grade, as on master it exited with its state. With `-d` it can exit 0 although the content was refused; only the note now says nothing was applied.
### Tests
- `src/traffic_ctl/unit_tests/test_traffic_ctl_status.cc`: 12 new `[reload]` cases: the grade, the bounds of each state, sub-tasks at any depth (including "A sub-task that failed counts as at least an error, whatever its parent says"), timeouts, unfinished and unknown states, malformed levels, and decoding the level.
- `tests/gold_tests/traffic_ctl/traffic_ctl_reload_error_level.test.py` (new, 71 runs):
- against traffic_server: a `cache.config` reload that rejects a line (`success` with an `[Err]` entry), a clean reload, and a directory in place of `cache.config` (`fail`);
- against a second traffic_server with the `cfg_plugin_deferred_test` plugin and `proxy.config.admin.reload.timeout` at 1 s, so the reload ends `timeout`;
- against the stand-in JSONRPC node from #13032 (`jsonrpc_error_stub.py`), for task trees traffic_server does not produce on demand: levels out of range or not integers, unknown states, sub-tasks that failed or timed out under a parent that succeeded, `--count`, poll errors, and the inline-reload note.
- The four reload tests under `tests/gold_tests/jsonrpc/`: `config status` on a failed reload now expects 2.
- Red/green: with the `src/traffic_ctl` sources reverted to master and the tests kept, the new test and the four updated ones fail. 47 runs fail, 8 in the four updated tests and 39 in the new one: 46 on the exit code, and one on the inline note. With the change, all five pass.
- Mutation: two passes over earlier versions of the change, 39 and 46 mutants. Each mutant that survived got a run in the new AuTest that killed it at the time. The last fix round came after both passes and reworked the inline-note condition and the clamping of out-of-range levels; no mutation pass has run on those lines.
### Docs
- `traffic_ctl.en.rst`: how a reload is graded, under `--error-level`; the `config reload` exit codes; a new exit-code list for `config status`; `--min-level`; the `--data` note.
- `upgrading.en.rst`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4349349185
##########
src/traffic_ctl/TrafficCtlStatus.h:
##########
@@ -27,6 +27,7 @@ limitations under the License.
#include <string>
#include <string_view>
+#include "jsonrpc/CtrlRPCRequests.h"
#include "mgmt/rpc/jsonrpc/error/RPCError.h"
#include "shared/rpc/RPCRequests.h"
#include "shared/rpc/yaml_codecs.h"
Review Comment:
`appExitCodeFromReload()` uses `std::min`/`std::max` but this header doesn’t include `<algorithm>`. This can fail to compile depending on transitive includes. Add `#include <algorithm>` to make the header self-sufficient.
##########
tests/uranium_tests/jsonrpc/config_reload_helpers.py:
##########
@@ -71,15 +73,18 @@ def wait_for_status(
:param excludes: Text fragments forbidden from the final output.
:param extra_arguments: Additional ``config status`` arguments.
:param timeout: Maximum number of seconds to wait.
+ :param return_code: Exact expected ``config status`` exit status: 2 for a reload that failed.
"""
deadline = time.monotonic() + timeout
output = ""
+ status = None
while time.monotonic() < deadline:
result = ats.traffic_ctl("config", "status", "-t", token, *extra_arguments)
output = result.output
- if result.returncode == 0 and "in_progress" not in output and all(fragment in output for fragment in contains) and all(
+ status = result.returncode
Review Comment:
`status` is initialized to `None` but later interpolated into an error message as an exit code. Initializing it to an integer sentinel (e.g., `-1`) keeps the type consistent and makes the failure message more uniform (especially if `timeout` is ever configured to 0 or very small values).
##########
tests/uranium_tests/traffic_ctl/test_traffic_ctl_reload_error_level.py:
##########
@@ -0,0 +1,459 @@
+# 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 json
+import re
+import shlex
+import sys
+import time
+from collections.abc import Sequence
+from pathlib import Path
+from tools.uranium.services import ATS, ATSFactory, ServiceFactory, ProcessService, CommandResult, wait_for_file_lines
+
+DL_NOTE, DL_WARNING, DL_ERROR, DL_FATAL, DL_UNDEFINED = 3, 4, 5, 6, 9
+NOTE = 'Inline configuration is NOT persisted'
+
+
+def run(ats: ATS, command: str, return_code: int) -> CommandResult:
+ """Run one shell command with the environment of @a ats.
+
+ The exit status must come from the grade: traffic_ctl also exits 2 when it cannot decode a reply, so a report must
+ have been printed and no error found.
+
+ :param ats: Traffic Server whose environment and run root the command uses.
+ :param command: Bash command line.
+ :param return_code: Exact expected command status.
+ """
+ # Longer than the sleep plus the 30 s monitor timeout of MONITOR_TS, so a traffic_server reload that never ends shows the
+ # monitor's own report.
+ result = ats.run_shell(command, timeout=60)
+ assert result.returncode == return_code, result.output
+ assert 'Error found' not in result.stderr, result.output
+ if ' config status' in command:
+ report = r'"tasks"' if ' -f json ' in command else r'Reload \['
+ assert has(result, report), result.output
+ return result
+
+
+def has(result: CommandResult, pattern: str) -> bool:
+ """Return whether a line of the command's stdout matches.
+
+ :param result: Completed command.
+ :param pattern: Regular expression searched in stdout, line anchors per line.
+ """
+ return re.search(pattern, result.stdout, re.MULTILINE) is not None
+
+
+def task(status: str, levels: Sequence[int | None] = (), sub_tasks: Sequence[dict] = (), token: str = 'r1') -> dict:
+ """Build a reload task as get_reload_config_status reports it.
+
+ :param status: Task state.
+ :param levels: One log entry per level; a level of None is an entry sent without one.
+ :param sub_tasks: Child tasks.
+ :param token: Reload token.
+ """
+ logs = [{'text': f'entry {i}'} if level is None else {'level': level, 'text': f'entry {i}'} for i, level in enumerate(levels)]
+ return {'config_token': token, 'status': status, 'description': 'stub task', 'logs': logs, 'sub_tasks': list(sub_tasks)}
+
+
+def status_reply(*tasks: dict) -> dict:
+ """Build a get_reload_config_status reply.
+
+ :param tasks: Reload tasks, oldest first.
+ """
+ return {'result': {'tasks': list(tasks)}}
+
+
+def test_reload_error_level(ats_factory: ATSFactory) -> None:
+ """Grade reloads of a real traffic_server by --error-level.
+
+ config reload --monitor and config status grade a finished reload by the most severe log entry of the task and its
+ sub-tasks. The state of each task bounds its entries: fail and timeout count as at least an error, and the entries of
+ a task that did not fail count as at most a warning, so at the default level the exit status follows the state.
+
+ :param ats_factory: Proxy factory.
+ """
+ ats = ats_factory.create('ts')
+ ats.cache_config.add_line('dest_domain=example.com ttl-in-cache=30d')
+ ats.start()
+ MONITOR_TS = '-m --initial-wait 0.5 --refresh-int 0.5 --timeout 30s'
+ cache_config = shlex.quote(str(ats.config_directory / 'cache.config'))
+
+ # A cache.config line with a malformed IP is discarded: the cache.config task logs an error and still ends success.
+ tr = run(
+ ats, f"echo 'dest_ip=999.1.1.1 action=never-cache' >> {cache_config} && sleep 2 && "
+ f"traffic_ctl config reload -t bad_line {MONITOR_TS}", 0)
+ assert has(tr, r'\d+/\d+ +success'), tr.output
+ assert not has(tr, 'Details :'), tr.output
+ tr = run(ats, 'traffic_ctl config status -t bad_line', 0)
+ assert has(tr, r'\[Err\] +\[CacheControl\] not a valid IP address range'), tr.output
+ # The error entry of a success counts as a warning.
+ run(ats, 'traffic_ctl --error-level warn config status -t bad_line', 2)
+ run(ats, 'traffic_ctl -f json --error-level warn config status -t bad_line', 2)
+
+ # Put the line right: the next reload of cache.config is clean, apart from its notes.
+ tr = run(
+ ats, f"echo 'dest_domain=example.com ttl-in-cache=30d' > {cache_config} && sleep 2 && "
+ f"traffic_ctl config reload -t clean {MONITOR_TS}", 0)
+ assert not has(tr, 'Details :'), tr.output
+ run(ats, 'traffic_ctl --error-level warn config status -t clean', 0)
+ run(ats, 'traffic_ctl --error-level note config status -t clean', 2)
+ # Entries hidden by --min-level are still graded.
+ run(ats, 'traffic_ctl --error-level note config status -t clean --min-level error', 2)
+
+ # A directory in place of cache.config cannot be read: the cache.config task fails.
+ tr = run(
+ ats, f"rm {cache_config} && mkdir {cache_config} && sleep 2 && "
+ f"traffic_ctl config reload -t unreadable {MONITOR_TS}", 2)
+ assert has(tr, r'\d+/\d+ +fail'), tr.output
+ assert has(tr, 'Details : traffic_ctl config status -t unreadable'), tr.output
+ run(ats, 'traffic_ctl config status -t unreadable', 2)
+ run(ats, 'traffic_ctl --error-level fatal config status -t unreadable', 0)
+ # Put cache.config back; the default level, given explicitly, still fails the reload.
+ run(
+ ats, f"rmdir {cache_config} && echo 'dest_domain=example.com ttl-in-cache=30d' > {cache_config} && "
+ "traffic_ctl --error-level error config status -t unreadable", 2)
+
+ # traffic_server reports the rejected line and the unreadable file in diags.log too.
+ diagnostics = wait_for_file_lines(ats.diags_log, 'cache.config failed to load', 1)
+ assert 'not a valid IP address range' in diagnostics, diagnostics
+
+
+def test_reload_error_level_stub(ats_factory: ATSFactory, services: ServiceFactory) -> None:
+ """Grade reload task trees that no handler produces on demand, served by a stand-in JSON-RPC node.
+
+ Entries without a severity are not graded, and a reload that is still running, or in a state traffic_ctl does not
+ know, is not graded. A timeout below the level exits 75. config status grades the newest reload it shows.
+
+ :param ats_factory: Proxy factory; traffic_ctl runs with the environment of its Traffic Server.
+ :param services: Stand-in RPC server factory.
+ """
+ ats = ats_factory.create('ts')
+ ats.start()
+ nodes: dict[str, ProcessService] = {}
+
+ def stub(name: str, replies: dict) -> str:
+ """Start a stand-in RPC node and return the traffic_ctl command that talks to it.
+
+ :param name: Unique process name, also the directory that holds its UNIX socket. Keep it short: macOS caps socket
+ paths at 104 bytes.
+ :param replies: Method names mapped to a JSON-RPC reply, or to a list of replies served in turn.
+ """
+ directory = ats.run_directory / name
+ process = services.process(
+ name,
+ [sys.executable, Path(__file__).with_name('jsonrpc_error_stub.py'), directory,
+ json.dumps(replies)])
+ process.start()
+ deadline = time.monotonic() + 10
+ while not (directory / 'ready').exists():
+ assert process.is_running, process.output
+ assert time.monotonic() < deadline, process.output
+ time.sleep(0.01)
+ ctl = f"traffic_ctl --run-root={shlex.quote(str(directory / 'runroot.yaml'))}"
Review Comment:
Potential test flakiness: `stub()` waits for `(directory / 'ready')` but doesn’t ensure the directory is clean. If a previous failed run left the directory (or `ready` file) behind, the wait can short-circuit and race the server startup. Consider removing any existing `ready` file (or using a uniquely-created temporary directory per stub) before starting the process.
##########
src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h:
##########
@@ -95,6 +95,31 @@ template <> struct convert<FetchConfigReloadStatusRequest::Params> {
};
template <> struct convert<ConfigReloadResponse> {
+ /// A missing level leaves the entry DL_Undefined. A level that is not one of the DiagsLevel values marks it @c unknown_level,
+ /// which the exit status grades as the highest severity, within the bounds the state of its task sets (see
+ /// appExitCodeFromReload()). Such a level still prints as it always did: an integer below or above the known range as the
+ /// lowest or the highest level, anything else without a tag. The server sends the level quoted, as it does every scalar.
+ static void
+ decode_level(Node const &level, ConfigReloadResponse::LogEntry &entry)
+ {
+ if (!level) {
+ return;
+ }
+ if (level.IsScalar()) {
+ try {
+ auto const value = level.as<int>();
+ if (value >= DL_Diag && value <= DL_Undefined) {
+ entry.level = static_cast<DiagsLevel>(value);
+ return;
+ }
+ entry.level = value < DL_Diag ? DL_Diag : DL_Emergency;
Review Comment:
The comment says “not one of the DiagsLevel values” is marked `unknown_level`, but the implementation currently treats *any* in-range integer (`DL_Diag..DL_Undefined`) as known. If `DiagsLevel` ever has gaps (or reserved values) this behavior will diverge from the doc. Either (mandatory) tighten validation to the exact set of supported `DiagsLevel` values, or (optional) update the comment to match the range-based behavior.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4292692541
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -351,6 +366,8 @@ ConfigCommand::config_status()
for (const auto &task : resp.tasks) {
_printer->as<ConfigReloadPrinter>()->print_reload_report(task, true);
}
+ // The server lists the newest reload last. Older ones shown with --count are history and are not graded.
+ grade_reload(resp.tasks.back());
Review Comment:
`resp.tasks.back()` is only reached inside `if (resp.tasks.size() > 0)` ([CtrlCommands.cc#L365-L371](https://github.com/apache/trafficserver/blob/0894eeb5a40efa1ad0b9ed3b462433f3642dbf81/src/traffic_ctl/CtrlCommands.cc#L365-L371)), the same block that prints the reports on master. A reply with no tasks prints nothing and exits 0, as on master. When there is no reload to show, traffic_server answers with a "No reload tasks found" error instead, which takes the `resp.error` branch above it ([#L359-L363](https://github.com/apache/trafficserver/blob/0894eeb5a40efa1ad0b9ed3b462433f3642dbf81/src/traffic_ctl/CtrlCommands.cc#L359-L363)) and exits 2.
##########
src/traffic_ctl/TrafficCtlStatus.h:
##########
@@ -27,6 +27,7 @@ limitations under the License.
#include <string>
#include <string_view>
+#include "jsonrpc/ctrl_yaml_codecs.h"
Review Comment:
Agreed. `appExitCodeFromReload()` needs only the reload types, so `TrafficCtlStatus.h` now includes `jsonrpc/CtrlRPCRequests.h`. The unit test decodes replies, so it includes `jsonrpc/ctrl_yaml_codecs.h` itself. Done in dc200bba82759573002b363d75f6faa0145d8c74.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4282754158
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -351,6 +366,8 @@ ConfigCommand::config_status()
for (const auto &task : resp.tasks) {
_printer->as<ConfigReloadPrinter>()->print_reload_report(task, true);
}
+ // The server lists the newest reload last. Older ones shown with --count are history and are not graded.
+ grade_reload(resp.tasks.back());
Review Comment:
`get_reload_config_status` returns history in most-recent-first order (`ReloadCoordinator::get_all()` and its public comment use that order), so `resp.tasks.back()` is the oldest reload, not the newest. With `--count` this grades the wrong task and can report success for a failed newest reload (or vice versa). Grade `resp.tasks.front()` instead, and update the test fixtures that currently model the reverse order.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4283195003
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -351,6 +366,8 @@ ConfigCommand::config_status()
for (const auto &task : resp.tasks) {
_printer->as<ConfigReloadPrinter>()->print_reload_report(task, true);
}
+ // The server lists the newest reload last. Older ones shown with --count are history and are not graded.
+ grade_reload(resp.tasks.back());
Review Comment:
The server lists the oldest reload first, so `resp.tasks.back()` is the newest. `ReloadCoordinator` appends each new reload to `_history` with `push_back` ([ReloadCoordinator.cc#L88](https://github.com/apache/trafficserver/blob/c26c091e8baaade7ee6f16bd0b562b0828eb203b/src/mgmt/config/ReloadCoordinator.cc#L88)), `get_all()` copies the last N in that order ([ReloadCoordinator.cc#L224-L230](https://github.com/apache/trafficserver/blob/c26c091e8baaade7ee6f16bd0b562b0828eb203b/src/mgmt/config/ReloadCoordinator.cc#L224-L230)), and the status handler sends them as they come ([Configuration.cc#L411-L416](https://github.com/apache/trafficserver/blob/c26c091e8baaade7ee6f16bd0b562b0828eb203b/src/mgmt/rpc/handlers/config/Configuration.cc#L411-L416)). The "Most recent first" comment on `get_all()` ([ReloadCoordinator.h#L44](https://github.com/apache/trafficserver/blob/c26c091e8baaade7ee6f16bd0b562b0828eb203b/include/mgmt/config/ReloadCoordinator.h#L44)) does not match the code.
Checked against traffic_server: after a failed reload `older` and then a successful one, `newer`, `traffic_ctl config status -c 2` prints `older` first and `newer` last, and exits 0. Grading `front()` would grade `older` and exit 2.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4292554130
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -351,6 +366,8 @@ ConfigCommand::config_status()
for (const auto &task : resp.tasks) {
_printer->as<ConfigReloadPrinter>()->print_reload_report(task, true);
}
+ // The server lists the newest reload last. Older ones shown with --count are history and are not graded.
+ grade_reload(resp.tasks.back());
Review Comment:
`resp.tasks.back()` is undefined behavior if `resp.tasks` is empty. `track_config_reload_progress()` already treats an empty task list as an error case, so `config_status()` should similarly guard this path (e.g., only grade when `!resp.tasks.empty()`, otherwise set the appropriate exit code / show the existing error output for 'no reload found').
##########
src/traffic_ctl/TrafficCtlStatus.h:
##########
@@ -27,6 +27,7 @@ limitations under the License.
#include <string>
#include <string_view>
+#include "jsonrpc/ctrl_yaml_codecs.h"
Review Comment:
Including `jsonrpc/ctrl_yaml_codecs.h` from `TrafficCtlStatus.h` tightly couples this widely-included header to YAML-cpp and decode logic, which can increase build times and propagate heavier dependencies across the binary. `appExitCodeFromReload()` only needs the reload types/levels (e.g., `ConfigReloadResponse` / `DiagsLevel`), so prefer including the narrow request/response types header and leaving YAML codec headers in the `.cc` files that actually perform decoding.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#issuecomment-6201173247
Examples of how a finished reload is graded, run as an AuTest against a real traffic_server. "master" is the same command with `traffic_ctl` built from master (c26c091e), and "this PR" is 0894eeb5. In these runs the printed report was the same in both; the exit status is what differs.
**The rule:** the most severe log entry of the task and its sub-tasks is compared with `--error-level` (default `error`). A task that ended `fail` or `timeout` counts as at least `error`, and the entries of any other task count as at most `warn`. Below the level, a reload with a task that ended `timeout` exits 75 instead of 0.
| # | Reload | Command | master | this PR |
|---|---|---|---|---|
| 1 | `success` with an `[Err]` entry: `cache.config` dropped a bad line | `config reload -m` | 0 | 0 |
| 2 | same | `config status` | 0 | 0 |
| 3 | same | `--error-level warn config status` | 0 | **2** |
| 4 | clean `success` | `config reload -m` | 0 | 0 |
| 5 | same; reload tasks log `[Note]` entries | `--error-level note config status` | 0 | **2** |
| 6 | `fail`: `cache.config` is a directory | `config reload -m` | 2 | 2 |
| 7 | same | `config status` | 0 | **2** |
| 8 | same | `--error-level fatal config status` | 0 | 0 |
| 9 | #6, then a reload that ended `success` | `config status -c 2` | 0 | 0 |
| 10 | `timeout`: a plugin reload outlives `proxy.config.admin.reload.timeout` (1s) | `config reload -m` | 2 | 2 |
| 11 | same | `--error-level fatal config reload -m` | 2 | **75** |
Of the rows at the default level, #7 is the one that changes: `config status` exited 0 for a failed reload. In #9 the newest reload shown decides.
<details>
<summary>Output of #3, #7, #8 and #11</summary>
```
$ traffic_ctl --error-level warn config status -t bad_line
✔ Reload [success] — bad_line
✔ 2 success ◌ 0 in-progress ✗ 0 failed (2 total)
Tasks:
✔ cache_control (…/cache.config) 1ms
[Note] cache.config loading ...
[Err] [CacheControl] not a valid IP address range at …/cache.config line 2
[Note] cache.config finished loading
✔ ssl_ticket_key ······························· 1ms
[Note] SSL ticket key loading ...
[Note] SSL ticket key reloaded
exit=2
```
```
$ traffic_ctl config status -t broken
✗ Reload [fail] — broken
✔ 1 success ◌ 0 in-progress ✗ 1 failed (3 total)
Tasks:
✗ cache_control (…/cache.config) 0ms ✗ FAIL
[Note] cache.config loading ...
[Err] ControlMatcher - …/cache.config failed to load: Is a directory
[Note] cache.config finished loading
...
exit=2
$ traffic_ctl --error-level fatal config status -t broken
(same report)
exit=0
```
```
$ traffic_ctl --error-level fatal config reload -t slow2 -m -w 0 -r 0.2 -d 'cfg_plugin_deferred_test: {mode: ok}'
✔ Reload scheduled [slow2]
⟳ [slow2] ░░░░░░░░░░░░░░░░░░░░ 0/1 timeout (1.0s)
Details : traffic_ctl config status -t slow2
exit=75
```
</details>
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4348736759
##########
src/traffic_ctl/TrafficCtlStatus.h:
##########
@@ -126,3 +127,49 @@ appExitCodeFromResponse(shared::rpc::JSONRPCResponse const &response)
return exit_severity_of(*worst) >= App_Exit_Level_Error ? CTRL_EX_ERROR : CTRL_EX_OK;
}
+
+/// Exit status for a config reload task, graded against @c App_Exit_Level_Error like a handler failure. The most severe log
+/// entry of the task and its sub-tasks decides. An entry without a severity (DL_Undefined) is not graded, and one whose level
+/// the server sent malformed counts as the highest severity. The state of each task bounds its entries: a task that ended
+/// `fail` or `timeout` counts as at least an error, the way an annotation without a severity does, and the entries of any
+/// other task count as at most a warning, since its handler did not fail it. A reload with a task that ended `timeout` exits
+/// CTRL_EX_TEMPFAIL below the level, since it never finished. A reload that is still running, has no state, or is in a state
+/// this client does not know, is not graded.
+inline int
+appExitCodeFromReload(ConfigReloadResponse::ReloadInfo const &task)
+{
+ auto failed = [](ConfigReloadResponse::ReloadInfo const &info) { return info.status == "fail" || info.status == "timeout"; };
+
+ if (task.status != "success" && !failed(task)) {
+ return CTRL_EX_OK;
+ }
+
+ bool timed_out = false;
+ auto worst = [&failed, &timed_out](auto &&self, ConfigReloadResponse::ReloadInfo const &info) -> int {
+ int level = -1;
+ for (auto const &entry : info.logs) {
+ if (entry.unknown_level) {
+ level = std::max(level, static_cast<int>(DL_Emergency));
+ } else if (entry.level != DL_Undefined) {
+ level = std::max(level, static_cast<int>(entry.level));
+ }
+ }
+ if (failed(info)) {
+ level = std::max(level, static_cast<int>(DL_Error));
+ timed_out = timed_out || info.status == "timeout";
+ } else {
+ level = std::min(level, static_cast<int>(DL_Warning));
+ }
+ for (auto const &sub : info.sub_tasks) {
+ level = std::max(level, self(self, sub));
+ }
+ return level;
+ };
+
+ int const level = worst(worst, task);
+
+ if (level >= 0 && swoc::Errata::severity_type(level) >= App_Exit_Level_Error) {
+ return CTRL_EX_ERROR;
+ }
+ return timed_out ? CTRL_EX_TEMPFAIL : CTRL_EX_OK;
Review Comment:
`DiagsLevel` and the errata severities are one scale by definition: `include/tsutil/ts_errata.h` builds each `ERRATA_*` constant from its `DL_*` value ([ts_errata.h#L32-L40](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/include/tsutil/ts_errata.h#L32-L40)). The annotation grade from #13032 converts the same way ([TrafficCtlStatus.h#L102](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/TrafficCtlStatus.h#L102)), so the reload grade keeps the direct conversion ([#L171](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/TrafficCtlStatus.h#L171)). A separate mapping would restate that header.
##########
src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h:
##########
@@ -95,6 +95,31 @@ template <> struct convert<FetchConfigReloadStatusRequest::Params> {
};
template <> struct convert<ConfigReloadResponse> {
+ /// A missing level leaves the entry DL_Undefined. A level that is not one of the DiagsLevel values marks it @c unknown_level,
+ /// which the exit status grades as the highest severity. Such a level still prints as it always did: an integer below or above
+ /// the known range as the lowest or the highest level, anything else without a tag. The server sends the level quoted, as it
+ /// does every scalar.
+ static void
+ decode_level(Node const &level, ConfigReloadResponse::LogEntry &entry)
+ {
+ if (!level) {
+ return;
+ }
+ if (level.IsScalar()) {
+ try {
+ auto const value = level.as<int>();
+ if (value >= DL_Diag && value <= DL_Undefined) {
+ entry.level = static_cast<DiagsLevel>(value);
+ return;
+ }
+ entry.level = value < DL_Diag ? DL_Diag : DL_Emergency;
+ } catch (YAML::Exception const &) {
+ // Not an integer.
+ }
+ }
+ entry.unknown_level = true;
+ }
Review Comment:
Agreed. Inside a task that did not fail, an unknown level counts as at most a warning, like every other entry there. The comment now says the state of the task bounds it ([ctrl_yaml_codecs.h#L98-L101](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h#L98-L101)), in 58c923178d1ec3cecde0baae1ef5fef2324a549d.
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -102,6 +102,21 @@ mark_not_done()
}
}
+/// Grade a reload into the exit status and return the grade, see appExitCodeFromReload(). A failure overrides an earlier status,
+/// and a reload that never finished only replaces a success, the same as the grade of any other response.
+int
+grade_reload(ConfigReloadResponse::ReloadInfo const &task)
+{
+ auto const code = appExitCodeFromReload(task);
+
+ if (code == CTRL_EX_ERROR) {
+ App_Exit_Status_Code = CTRL_EX_ERROR;
+ } else if (code == CTRL_EX_TEMPFAIL && App_Exit_Status_Code == CTRL_EX_OK) {
+ App_Exit_Status_Code = CTRL_EX_TEMPFAIL;
+ }
+ return code;
+}
Review Comment:
`grade_reload` already has internal linkage: it is inside the unnamed namespace that opens at [CtrlCommands.cc#L43](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/CtrlCommands.cc#L43) and closes at [#L146](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/CtrlCommands.cc#L146), with the other file-local helpers.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4348604806
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -102,6 +102,21 @@ mark_not_done()
}
}
+/// Grade a reload into the exit status and return the grade, see appExitCodeFromReload(). A failure overrides an earlier status,
+/// and a reload that never finished only replaces a success, the same as the grade of any other response.
+int
+grade_reload(ConfigReloadResponse::ReloadInfo const &task)
+{
+ auto const code = appExitCodeFromReload(task);
+
+ if (code == CTRL_EX_ERROR) {
+ App_Exit_Status_Code = CTRL_EX_ERROR;
+ } else if (code == CTRL_EX_TEMPFAIL && App_Exit_Status_Code == CTRL_EX_OK) {
+ App_Exit_Status_Code = CTRL_EX_TEMPFAIL;
+ }
+ return code;
+}
Review Comment:
`grade_reload` is a free function in a `.cc` file but has external linkage as written. To keep the symbol local (and avoid unintentional cross-TU visibility / collisions), put it in an anonymous namespace or mark it `static`.
##########
src/traffic_ctl/TrafficCtlStatus.h:
##########
@@ -126,3 +127,49 @@ appExitCodeFromResponse(shared::rpc::JSONRPCResponse const &response)
return exit_severity_of(*worst) >= App_Exit_Level_Error ? CTRL_EX_ERROR : CTRL_EX_OK;
}
+
+/// Exit status for a config reload task, graded against @c App_Exit_Level_Error like a handler failure. The most severe log
+/// entry of the task and its sub-tasks decides. An entry without a severity (DL_Undefined) is not graded, and one whose level
+/// the server sent malformed counts as the highest severity. The state of each task bounds its entries: a task that ended
+/// `fail` or `timeout` counts as at least an error, the way an annotation without a severity does, and the entries of any
+/// other task count as at most a warning, since its handler did not fail it. A reload with a task that ended `timeout` exits
+/// CTRL_EX_TEMPFAIL below the level, since it never finished. A reload that is still running, has no state, or is in a state
+/// this client does not know, is not graded.
+inline int
+appExitCodeFromReload(ConfigReloadResponse::ReloadInfo const &task)
+{
+ auto failed = [](ConfigReloadResponse::ReloadInfo const &info) { return info.status == "fail" || info.status == "timeout"; };
+
+ if (task.status != "success" && !failed(task)) {
+ return CTRL_EX_OK;
+ }
+
+ bool timed_out = false;
+ auto worst = [&failed, &timed_out](auto &&self, ConfigReloadResponse::ReloadInfo const &info) -> int {
+ int level = -1;
+ for (auto const &entry : info.logs) {
+ if (entry.unknown_level) {
+ level = std::max(level, static_cast<int>(DL_Emergency));
+ } else if (entry.level != DL_Undefined) {
+ level = std::max(level, static_cast<int>(entry.level));
+ }
+ }
+ if (failed(info)) {
+ level = std::max(level, static_cast<int>(DL_Error));
+ timed_out = timed_out || info.status == "timeout";
+ } else {
+ level = std::min(level, static_cast<int>(DL_Warning));
+ }
+ for (auto const &sub : info.sub_tasks) {
+ level = std::max(level, self(self, sub));
+ }
+ return level;
+ };
+
+ int const level = worst(worst, task);
+
+ if (level >= 0 && swoc::Errata::severity_type(level) >= App_Exit_Level_Error) {
+ return CTRL_EX_ERROR;
+ }
+ return timed_out ? CTRL_EX_TEMPFAIL : CTRL_EX_OK;
Review Comment:
This relies on `DiagsLevel` numeric values matching `swoc::Errata::Severity` ordinals (so that `swoc::Errata::severity_type(level)` is meaningful). That coupling is fragile and easy to break if either enum changes or diverges. Prefer an explicit mapping/conversion function from `DiagsLevel` (and “unknown/malformed”) to `swoc::Errata::severity_type` before comparing against `App_Exit_Level_Error`.
##########
src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h:
##########
@@ -95,6 +95,31 @@ template <> struct convert<FetchConfigReloadStatusRequest::Params> {
};
template <> struct convert<ConfigReloadResponse> {
+ /// A missing level leaves the entry DL_Undefined. A level that is not one of the DiagsLevel values marks it @c unknown_level,
+ /// which the exit status grades as the highest severity. Such a level still prints as it always did: an integer below or above
+ /// the known range as the lowest or the highest level, anything else without a tag. The server sends the level quoted, as it
+ /// does every scalar.
+ static void
+ decode_level(Node const &level, ConfigReloadResponse::LogEntry &entry)
+ {
+ if (!level) {
+ return;
+ }
+ if (level.IsScalar()) {
+ try {
+ auto const value = level.as<int>();
+ if (value >= DL_Diag && value <= DL_Undefined) {
+ entry.level = static_cast<DiagsLevel>(value);
+ return;
+ }
+ entry.level = value < DL_Diag ? DL_Diag : DL_Emergency;
+ } catch (YAML::Exception const &) {
+ // Not an integer.
+ }
+ }
+ entry.unknown_level = true;
+ }
Review Comment:
The doc comment says an `unknown_level` “grades as the highest severity”, but the grading logic later clamps *all* entries in non-failed tasks to at most `warn` (including unknown/malformed levels). To avoid misleading readers, clarify that malformed levels are treated as “emergency” for grading *subject to per-task state bounds* (e.g., capped to `warn` when the task did not fail).
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] traffic_ctl: grade config reload results by --error-level [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13842:
URL: https://github.com/apache/trafficserver/pull/13842#issuecomment-6341628479
Rebased onto master (48f1edc05b), after #13545 moved the tests from AuTest to Uranium. Range-diff against the previous head, dc200bba82:
| Previous | Now | Change |
|---|---|---|
| ec2497890d traffic_ctl: grade config reload results by level | 86ce559e22 | Same `src/` and `doc/` changes. The AuTest changes are now Uranium tests: the new `test_traffic_ctl_reload_error_level.py`, the four jsonrpc reload tests, `test_virtualhost_remap.py`, and `return_code` in `config_reload_helpers.py`. |
| 1ccae2c0d0 Format the reload error-level AuTest | dropped | The AuTest is gone. |
| efbd0fff10 Make the JSONRPC test stub exit on SIGINT | dropped | Uranium stops helper processes with SIGTERM, so the stub needs no handler. |
| 0894eeb5a4 Fix the order get_all() documents | 85e6bdef72 | Unchanged. |
| dc200bba82 Narrow the TrafficCtlStatus.h include | 36bd707580 | Unchanged. |
| | 58c923178d | New: the `decode_level` comment, from Copilot's review. |
The description is updated for the Uranium tests. The examples in the earlier comment were run on 0894eeb5. Since then `src/traffic_ctl` changed only in an include and a comment, so its exit codes still hold.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org