brbzull0 commented on code in PR #13842: URL: https://github.com/apache/trafficserver/pull/13842#discussion_r4387649239
########## 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: `TrafficCtlStatus.h` already includes `<algorithm>` ([TrafficCtlStatus.h#L23](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/TrafficCtlStatus.h#L23)). Master has it for `std::max_element`, and this PR keeps it; the only include the PR adds there is `jsonrpc/CtrlRPCRequests.h`. ########## 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: `DiagsLevel` has no gaps: it runs from `DL_Diag = 0` to `DL_Undefined`, each value one more than the one before, and `DL_Undefined` is marked "must be last, used for size!" ([ts_diag_levels.h#L27-L38](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/include/tsutil/ts_diag_levels.h#L27-L38)). So the range check ([ctrl_yaml_codecs.h#L111](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h#L111)) accepts exactly the `DiagsLevel` values, as the comment says. ########## 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: A `ready` file from an earlier run cannot be there. Uranium empties each test's sandbox before the test starts ([context.py#L69-L70](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/tests/tools/uranium/services/context.py#L69-L70), [runtime.py#L209-L211](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/tests/tools/uranium/runtime.py#L209-L211)), and the stub directories live in it ([test_traffic_ctl_reload_error_level.py#L155](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/tests/uranium_tests/traffic_ctl/test_traffic_ctl_reload_error_level.py#L155)). Within one test, `services.process` refuses a name used twice ([service_factory.py#L61-L62](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/tests/tools/uranium/services/service_factory.py#L61-L62)). ########## 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: `None` is deliberate. It is what the message shows when no `config status` ran at all, because the deadline passed before the first poll, as with a `timeout` of 0 ([config_reload_helpers.py#L81-L90](https://github.com/apache/trafficserver/blob/58c923178d1ec3cecde0baae1ef5fef2324a549d/tests/uranium_tests/jsonrpc/config_reload_helpers.py#L81-L90)). An integer such as -1 would read like a real exit status: `subprocess` reports a process killed by a signal with a negative one. -- 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]
