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