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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
