Copilot commented on code in PR #13090:
URL: https://github.com/apache/trafficserver/pull/13090#discussion_r3114074153
##########
src/mgmt/config/ConfigReloadTrace.cc:
##########
@@ -287,6 +296,65 @@ ConfigReloadTask::aggregate_status()
}
}
+void
+ConfigReloadTask::log_reload_summary(State final_state)
+{
+ if (!_info.main_task || !is_terminal(final_state) || _summary_logged) {
+ return;
+ }
+ _summary_logged = true;
+
Review Comment:
ConfigReloadTask::log_reload_summary reads/writes `_info` (e.g.,
`_info.main_task`, `_info.sub_tasks`, `_info.token`) and `_summary_logged`
without holding `_mutex`. Other methods in this class treat `_info` as
protected by `_mutex`, and readers (e.g., status queries) can run concurrently,
so this risks data races. Consider taking a lock (or copying via `get_info()`
under lock) and making `_summary_logged` atomic or also guarded by the same
lock.
##########
doc/developer-guide/config-reload-framework.en.rst:
##########
@@ -672,6 +672,228 @@ See the :ref:`get_reload_config_status` response examples
for how log messages a
task tree output.
+.. _config-reload-unified-macros:
+
+Unified Diagnostic Macros (``CfgLoad*``)
+=========================================
+
+Config handlers often need the same message in two places: the ATS diagnostic
log
+(``diags.log`` / ``error.log``) **and** the reload task log (visible via
+:option:`traffic_ctl config status`). The ``CfgLoad*`` macros in
+``mgmt/config/ConfigContextDiags.h`` format the message once and dispatch to
both destinations.
+
+Include the header in any source file that uses these macros:
+
+.. code-block:: cpp
+
+ #include "mgmt/config/ConfigContextDiags.h"
+
+Quick Reference
+---------------
+
+.. list-table::
+ :header-rows: 1
+ :widths: 15 15 40
+
+ * - Want in diags?
+ - Want in task log?
+ - Use
+ * - Note
+ - yes + in_progress
+ - ``CfgLoadInProgress(ctx, ...)`` (subtasks)
+ * - Note
+ - yes + complete
+ - ``CfgLoadComplete(ctx, ...)``
+ * - Error
+ - yes + fail
+ - ``CfgLoadFail(ctx, ...)``
+ * - Error + Errata
+ - yes + fail
+ - ``CfgLoadFailWithErrata(ctx, errata, ...)``
+ * - Note / Warning
+ - yes (no state change)
+ - ``CfgLoadLog(ctx, DL_Note|DL_Warning, ...)``
+ * - Dbg (conditional on tag)
+ - yes
+ - ``CfgLoadDbg(ctx, ctl, ...)``
+ * - no
+ - yes
+ - ``ctx.log(...)``
+ * - no
+ - yes + state
+ - ``ctx.complete()`` / ``ctx.fail()``
+ * - yes
+ - no
+ - ``Note()`` / ``Warning()`` / ``Error()`` / ``Dbg()`` directly
+
+Macro Details
+-------------
+
+``CfgLoadInProgress(ctx, fmt, ...)``
+ Emits a ``Note()`` to ``diags.log`` and calls ``ctx.in_progress(msg)``. The
framework
+ sets ``IN_PROGRESS`` on handler tasks automatically, so this macro is
primarily useful
+ for subtasks created via ``add_dependent_ctx()``:
+
+ .. code-block:: cpp
+
+ CfgLoadInProgress(ctx, "%s loading ...", filename);
+
+``CfgLoadComplete(ctx, fmt, ...)``
+ Emits a ``Note()`` to ``diags.log`` and calls ``ctx.complete(msg)``. Use
when a config
+ operation finishes successfully:
+
+ .. code-block:: cpp
+
+ CfgLoadComplete(ctx, "%s finished loading", filename);
+
+``CfgLoadFail(ctx, fmt, ...)``
+ Emits an ``Error()`` to ``diags.log`` and the task log, then marks the task
as FAIL.
+ Fail always implies ``DL_Error`` — if the condition is merely degraded (not
fatal to
+ the load), use ``CfgLoadLog(ctx, DL_Warning, ...)`` + ``CfgLoadComplete()``
instead:
+
+ .. code-block:: cpp
+
+ CfgLoadFail(ctx, "%s failed to load", filename);
+
+``CfgLoadFailWithErrata(ctx, errata, fmt, ...)``
+ Like ``CfgLoadFail`` but also appends ``swoc::Errata`` annotations to the
task log.
+ Combines ``CfgLoadFail`` + ``ctx.fail(errata)`` in one call — see
+ :ref:`config-reload-errata-handling` below.
+
+``CfgLoadLog(ctx, level, fmt, ...)``
+ Emits at the given ``DiagsLevel`` and calls ``ctx.log(level, msg)``
**without changing
+ task state**. Use for intermediate informational messages:
+
+ .. code-block:: cpp
+
+ CfgLoadLog(ctx, DL_Warning, "ControlMatcher - Cannot open config file:
%s", path);
+ CfgLoadLog(ctx, DL_Note, "loaded %d categories from %s", count,
filename);
+
+``CfgLoadDbg(ctx, dbg_ctl, fmt, ...)``
+ Emits via ``Dbg()`` (conditional on the tag being enabled) and always adds
to the task log
+ at ``DL_Debug``. Use for debug-level messages that should also appear in
reload status:
+
+ .. code-block:: cpp
+
+ CfgLoadDbg(ctx, dbg_ctl_ssl, "Reload SNI file");
+
+.. _config-reload-errata-handling:
+
+Errata Handling
+---------------
+
+For failures with ``swoc::Errata`` detail, use ``CfgLoadFailWithErrata`` to
combine
+the diags summary, errata detail, and state change in a single call:
+
+.. code-block:: cpp
+
+ CfgLoadFailWithErrata(ctx, errata, "%s failed to load", filename);
+
+This logs the formatted message to ``diags.log`` at ``DL_Error``, appends it to
+the task log, then calls ``ctx.fail(errata)`` which stores each errata
annotation
+(with its own severity) in the task log and marks the task as FAIL.
+
+For errors that should not change state, pair ``CfgLoadLog`` with
``ctx.log(errata)``:
+
+.. code-block:: cpp
+
+ CfgLoadLog(ctx, DL_Error, "Cannot open %s", path);
+ ctx.log(errata); // errata detail -> task log only
+
+When NOT to Use Macros
+-----------------------
+
+- **Task-log-only messages** — use ``ctx.log()`` directly when the message is
only useful in
+ ``traffic_ctl`` output and should not appear in ``diags.log``.
+- **State-only transitions** — use ``ctx.in_progress()`` / ``ctx.complete()``
/ ``ctx.fail()``
+ directly when there is no message to emit to ``diags.log``.
+- **Fatal errors** — ``Fatal()`` terminates the process; reload status is
irrelevant.
+ Call ``Fatal()`` directly.
+
+
+Severity-Aware Task Logs
+=========================
+
+Each task log entry carries a ``DiagsLevel`` severity. State-transition
methods carry implicit
+severity: ``in_progress(text)`` and ``complete(text)`` store ``DL_Note``,
``fail(text)`` stores
+``DL_Error``. The ``CfgLoad*`` macros and ``ctx.log(level, text)`` store the
caller-specified
+level. Only the one-argument ``ctx.log(text)`` (no level) stores
``DL_Undefined`` — these
+entries are always shown regardless of ``--min-level`` filtering.
+
+In :option:`traffic_ctl config status` output, entries with a defined severity
are prefixed
+with a tag:
+
+.. code-block:: text
+
+ ✗ ssl_client_coordinator ······················· 2ms ✗ FAIL
+ │ [Note] SSL configs reloaded
+ ├─ ✔ SSLConfig ································· 1ms
+ │ [Note] SSLConfig loading ...
+ │ [Note] SSLConfig reloaded
+ ├─ ✗ SNIConfig ································· 1ms ✗ FAIL
+ │ [Note] sni.yaml loading ...
+ │ [Err] sni.yaml failed to load
+ └─ ✔ SSLCertificateConfig ······················ 0ms
+ [Note] (ssl) ssl_multicert.yaml loading ...
+ [Warn] Cannot open SSL certificate configuration
"ssl_multicert.yaml" - No such file or directory
+ [Note] (ssl) ssl_multicert.yaml finished loading
+
+The ``--min-level`` option on :option:`traffic_ctl config status` filters log
entries
+by severity — see :option:`traffic_ctl config status` for details.
+
+The severity is also available in JSON output (``--format json``) as an
integer ``level``
+field on each log entry, where the value maps to the ``DiagsLevel`` enum (e.g.
``1`` = Debug,
+``3`` = Note, ``4`` = Warning, ``5`` = Error).
+
+
+.. _config-reload-diags-log:
+
+Reload Summary in ``diags.log``
+================================
+
+After a reload reaches a terminal state (confirmed after a 5-second grace
period), a summary
+line is logged to ``diags.log``:
+
+**Success:**
+
+.. code-block:: text
+
+ NOTE: Config reload [my-token] completed successfully: 3 tasks succeeded (3
total)
Review Comment:
The documented success summary line here ("completed successfully: 3 tasks
succeeded (3 total)") doesn’t match the current implementation in
`ConfigReloadTask::log_reload_summary()` (which logs "completed: %d/%d tasks
succeeded"). Please align the documentation with the actual output, or adjust
the code to emit the documented message format.
```suggestion
NOTE: Config reload [my-token] completed: 3/3 tasks succeeded
```
##########
src/mgmt/config/ConfigReloadTrace.cc:
##########
@@ -287,6 +296,65 @@ ConfigReloadTask::aggregate_status()
}
}
+void
+ConfigReloadTask::log_reload_summary(State final_state)
+{
+ if (!_info.main_task || !is_terminal(final_state) || _summary_logged) {
+ return;
+ }
+ _summary_logged = true;
+
+ int success_count{0}, fail_count{0}, total{0};
+ for (const auto &sub : _info.sub_tasks) {
+ State st = sub->get_state();
+ if (st == State::SUCCESS) {
+ success_count++;
+ } else if (st == State::FAIL || st == State::TIMEOUT) {
+ fail_count++;
+ }
+ total++;
+ }
+
+ if (final_state == State::SUCCESS) {
+ Note("Config reload [%s] completed: %d/%d tasks succeeded",
_info.token.c_str(), success_count, total);
Review Comment:
The success summary log text here ("completed: %d/%d tasks succeeded") does
not match the documented example in
`doc/developer-guide/config-reload-framework.en.rst` ("completed successfully:
3 tasks succeeded (3 total)"). Either update the log format to match the
docs/examples or adjust the documentation so operators can grep/recognize the
exact message.
```suggestion
Note("Config reload [%s] completed successfully: %d tasks succeeded (%d
total)", _info.token.c_str(), success_count,
total);
```
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -572,6 +573,7 @@ SSLConfigParams::initialize()
Emergency("Can't initialize the SSL client, HTTPS in remap rules will not
function");
} else {
SSLError("Can't initialize the SSL client, HTTPS in remap rules will not
function");
+ CfgLoadLog(ctx, DL_Warning, "Can't initialize the SSL client, HTTPS in
remap rules will not function");
Review Comment:
This logs the same message twice to diags: once via `SSLError(...)` (which
already emits to the diagnostics system) and again via `CfgLoadLog(...)` (which
also emits to diags via `DiagsError`). If the goal is to get the message into
the reload task log, consider using `ctx.log(DL_Warning, ...)` (task-log-only)
instead of `CfgLoadLog`, or otherwise avoid duplicating the diags entry.
```suggestion
ctx.log(DL_Warning, "Can't initialize the SSL client, HTTPS in remap
rules will not function");
```
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -267,6 +268,22 @@ ConfigCommand::config_status()
count = ""; // server will ignore this if token is set anyways.
}
+ if (!min_level.empty()) {
+ static const std::unordered_map<std::string_view, DiagsLevel> level_map = {
+ {"debug", DL_Debug },
+ {"note", DL_Note },
+ {"warning", DL_Warning},
+ {"error", DL_Error },
+ };
+ if (auto it = level_map.find(min_level); it != level_map.end()) {
+ _printer->as<ConfigReloadPrinter>()->set_min_level(it->second);
+ } else {
+ _printer->write_output("Invalid --min-level value. Use: debug, note,
warning, error");
Review Comment:
`--min-level` parsing is currently case-sensitive
(`level_map.find(min_level)`), but the CLI docs describe the values as
case-insensitive. Consider normalizing the input (e.g., lowercase) before
lookup (and possibly trimming) so `--min-level Warning` works as documented.
##########
include/mgmt/config/ConfigReloadTrace.h:
##########
@@ -171,6 +171,15 @@ class ConfigReloadTask : public
std::enable_shared_from_this<ConfigReloadTask>
return
std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
}
+ /// A single log entry with optional severity.
+ /// Entries from log() carry a DiagsLevel; entries from state-change methods
+ /// (in_progress, complete, fail) use DL_Undefined (the task state itself
+ /// conveys severity).
+ struct LogEntry {
+ DiagsLevel level{DL_Undefined}; ///< DL_Undefined for state-change
messages
Review Comment:
This comment says state-change methods (in_progress/complete/fail) store log
entries with `DL_Undefined`, but `ConfigContext` now records `DL_Note` for
in_progress/complete messages and `DL_Error` for fail(reason) messages (see
`src/mgmt/config/ConfigContext.cc`). Please update the comment to reflect the
new severity behavior so it stays consistent with the implementation.
```suggestion
/// Entries from log() carry a DiagsLevel; state-change methods use
/// DL_Note for in_progress/complete messages and DL_Error for fail()
/// messages.
struct LogEntry {
DiagsLevel level{DL_Undefined}; ///< log() supplies the level; state
changes use DL_Note or DL_Error
```
--
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]
