From 61abfd733d2cf35053266d0ce1df72377fca9e06 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 14 Apr 2026 18:27:40 +0200 Subject: [PATCH 1/6] Add severity-aware logging to config reload framework Unify diagnostic logging (diags.log) with ConfigContext task logs (traffic_ctl config status) via CfgLoad* macros. Each task log entry now carries a DiagsLevel severity, enabling --min-level filtering in traffic_ctl and severity tags in output ([Note], [Err], etc). Reload summaries are logged to diags.log after a grace period, with detailed per-subtask dumps under the config.reload debug tag. Fixes: #12963 --- .../command-line/traffic_ctl.en.rst | 92 ++++++- .../config-reload-framework.en.rst | 228 +++++++++++++++++- include/iocore/net/SSLSNIConfig.h | 5 +- include/iocore/net/quic/QUICConfig.h | 3 +- include/mgmt/config/ConfigContext.h | 3 + include/mgmt/config/ConfigContextDiags.h | 180 ++++++++++++++ include/mgmt/config/ConfigReloadTrace.h | 19 +- include/proxy/ControlMatcher.h | 9 +- include/proxy/http/remap/RemapConfig.h | 5 +- include/proxy/http/remap/RemapYamlConfig.h | 5 +- include/proxy/http/remap/UrlRewrite.h | 6 +- include/records/YAMLConfigReloadTaskEncoder.h | 7 +- src/iocore/cache/Cache.cc | 5 +- src/iocore/cache/CacheHosting.cc | 37 +-- src/iocore/cache/P_CacheHosting.h | 7 +- src/iocore/dns/SplitDNS.cc | 15 +- src/iocore/net/P_SSLConfig.h | 5 +- src/iocore/net/QUICMultiCertConfigLoader.cc | 13 +- src/iocore/net/SSLConfig.cc | 46 ++-- src/iocore/net/SSLSNIConfig.cc | 32 +-- src/iocore/net/SSLUtils.cc | 3 +- src/iocore/net/quic/QUICConfig.cc | 11 +- src/mgmt/config/ConfigContext.cc | 28 ++- src/mgmt/config/ConfigReloadTrace.cc | 73 +++++- src/proxy/CacheControl.cc | 13 +- src/proxy/ControlMatcher.cc | 21 +- src/proxy/IPAllow.cc | 30 +-- src/proxy/ParentSelection.cc | 15 +- src/proxy/ReverseProxy.cc | 36 +-- src/proxy/http/PreWarmConfig.cc | 2 +- src/proxy/http/remap/RemapConfig.cc | 13 +- src/proxy/http/remap/RemapYamlConfig.cc | 20 +- src/proxy/http/remap/UrlRewrite.cc | 27 ++- src/proxy/logging/LogConfig.cc | 12 +- src/proxy/logging/YamlLogConfig.cc | 7 +- .../unit_tests/test_ConfigReloadTask.cc | 6 +- src/traffic_ctl/CtrlCommands.cc | 21 +- src/traffic_ctl/CtrlPrinters.cc | 21 +- src/traffic_ctl/CtrlPrinters.h | 13 + src/traffic_ctl/jsonrpc/CtrlRPCRequests.h | 18 +- src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h | 9 +- src/traffic_ctl/traffic_ctl.cc | 1 + src/traffic_server/traffic_server.cc | 6 +- 43 files changed, 881 insertions(+), 247 deletions(-) create mode 100644 include/mgmt/config/ConfigContextDiags.h diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index a60999b10bc..9bd269b5dc4 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -227,7 +227,7 @@ Display the current value of a configuration record. - **Monitor** a reload in real-time: ``traffic_ctl config reload -t -m`` - **Query** the final status: ``traffic_ctl config status -t `` - - **Get detailed logs**: ``traffic_ctl config status -t -l`` + - **Get detailed logs**: ``traffic_ctl config status -t `` The timestamp of the last reconfiguration event (in seconds since epoch) is published in the ``proxy.process.proxy.reconfigure_time`` metric. @@ -659,7 +659,8 @@ Display the current value of a configuration record. **Failed reload report:** When a reload has failed handlers, the output shows which handlers succeeded and which failed, - along with durations for each: + along with durations and per-handler log entries. Log entries carry severity tags when a + severity level was recorded: .. code-block:: bash @@ -673,10 +674,18 @@ Display the current value of a configuration record. Tasks: ✔ ip_allow.yaml ·························· 18ms - ✗ logging.yaml ·························· 120ms ✗ FAIL ✗ ssl_client_coordinator ················· 85ms ✗ FAIL - ├─ ✔ sni.yaml ··························· 20ms - └─ ✗ ssl_multicert.config ··············· 65ms ✗ FAIL + │ [Note] SSL configs reloaded + ├─ ✔ SSLConfig ·························· 10ms + │ [Note] SSLConfig loading ... + │ [Dbg] Reload SSLConfig + │ [Note] SSLConfig reloaded + ├─ ✗ SNIConfig ·························· 12ms ✗ FAIL + │ [Note] sni.yaml loading ... + │ [Err] sni.yaml failed to load: yaml-cpp error ... + └─ ✔ SSLCertificateConfig ·············· 13ms + [Note] (ssl) ssl_multicert.yaml loading ... + [Note] (ssl) ssl_multicert.yaml finished loading ... Supports the following options: @@ -704,6 +713,79 @@ Display the current value of a configuration record. # Show last 5 reloads $ traffic_ctl config status -c 5 + .. option:: --min-level + + Filter task log entries by minimum severity level. Only entries at or above the specified + level are displayed. State-transition messages carry implicit severity: + ``in_progress()`` and ``complete()`` produce ``[Note]`` entries, ``fail()`` produces + ``[Err]`` entries. Entries without a severity (``DL_Undefined``) — typically those logged + via the one-argument ``ctx.log(text)`` — are always shown regardless of this filter. + + Valid levels (case-insensitive): ``debug``, ``note``, ``warning``, ``error``. + + .. code-block:: bash + + # Show only warnings and errors + $ traffic_ctl config status -t my-token --min-level warning + + # Show only errors + $ traffic_ctl config status -t my-token --min-level error + + **Example — all logs (no filter):** + + .. code-block:: text + + ✗ ssl_client_coordinator ······················· 2ms ✗ FAIL + │ [Note] SSL configs reloaded + ├─ ✔ SSLConfig ································· 1ms + │ [Note] SSLConfig loading ... + │ [Dbg] Reload SSLConfig + │ [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 + + **Example — --min-level warning (note and debug entries filtered out):** + + .. code-block:: text + + ✗ ssl_client_coordinator ······················· 2ms ✗ FAIL + ├─ ✗ SNIConfig ································· 1ms ✗ FAIL + │ [Err] sni.yaml failed to load + └─ ✔ SSLCertificateConfig ······················ 0ms + [Warn] Cannot open SSL certificate configuration "ssl_multicert.yaml" - No such file or directory + + All entries from state transitions and ``CfgLoad*`` macros carry a severity tag + (e.g. ``[Dbg]``, ``[Note]``, ``[Warn]``, ``[Err]``). Entries without a tag are + "unleveled" (from the one-argument ``ctx.log(text)``) and always pass the filter. + + .. tip:: + + For deeper investigation beyond what ``traffic_ctl config status`` shows, enable the + ``config.reload`` debug tag. This writes a full dump of every subtask and its log entries + (with severity tags) to ``diags.log`` after each reload completes. + See :ref:`config-reload-diags-log` in the developer guide for details and examples. + + Enable at runtime without restarting: + + .. code-block:: bash + + $ traffic_ctl server debug enable --tags "config.reload" + + Or persistently in ``records.yaml``: + + .. code-block:: yaml + + records: + diags: + debug: + enabled: 1 + tags: config.reload + **JSON output:** All ``config status`` commands support the global ``--format json`` option to output the raw diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index 5b115078940..f38a115e437 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -663,7 +663,7 @@ Logging Best Practices ====================== - Use ``ctx.log()`` for operational messages that appear in - ``traffic_ctl config status -l`` and :ref:`get_reload_config_status` responses. + ``traffic_ctl config status`` and :ref:`get_reload_config_status` responses. - Use ``ctx.fail(errata, summary)`` when you have a ``swoc::Errata`` with detailed error context. - Use ``ctx.fail(reason)`` for simple error strings. - Keep log messages concise — they are stored in memory and included in JSONRPC responses. @@ -672,6 +672,227 @@ 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 + - ``CfgLoadInProgress`` / ``CfgLoadComplete`` + * - Error / Warning + - yes + - ``CfgLoadFail(ctx, DL_xxx, ...)`` + * - Error + Errata + - yes + fail + - ``CfgLoadFailWithErrata(ctx, ...)`` + * - Note / Warning + - yes (no state change) + - ``CfgLoadLog(ctx, DL_xxx, ...)`` + * - 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)``. Use at the start + of a config load/reload: + + .. 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, level, fmt, ...)`` + Emits at the given ``DiagsLevel`` (typically ``DL_Error`` or ``DL_Warning``) to both + ``diags.log`` and the task log, then marks the task as FAIL (state-only, no additional + log entry): + + .. code-block:: cpp + + CfgLoadFail(ctx, DL_Error, "%s failed to load", filename); + CfgLoadFail(ctx, DL_Warning, "No NAMEDs provided for %s", filename); + +``CfgLoadFailWithErrata(ctx, level, 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, DL_Error, errata, "%s failed to load", filename); + +This logs the formatted message to ``diags.log`` at the given severity, 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 ... + │ [Dbg] Reload SSLConfig + │ [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) + +**Failure:** + +.. code-block:: text + + WARNING: Config reload [my-token] finished with failures: 1 succeeded, 1 failed (3 total) — run: traffic_ctl config status -t my-token + +When the ``config.reload`` debug tag is enabled, a detailed dump of all subtasks and their +log entries is written to ``traffic.out`` / ``diags.log``: + +.. code-block:: text + + DIAG: (config.reload) [fail] ssl_client_coordinator + DIAG: (config.reload) [Note] SSL configs reloaded + DIAG: (config.reload) [success] SSLConfig + DIAG: (config.reload) [Note] SSLConfig loading ... + DIAG: (config.reload) [Dbg] Reload SSLConfig + DIAG: (config.reload) [Note] SSLConfig reloaded + DIAG: (config.reload) [fail] SNIConfig + DIAG: (config.reload) [Note] sni.yaml loading ... + DIAG: (config.reload) [Err] sni.yaml failed to load + DIAG: (config.reload) [success] ssl_ticket_key + DIAG: (config.reload) [Note] SSL ticket key loading ... + DIAG: (config.reload) [Note] SSL ticket key reloaded + +Enable this tag for troubleshooting: + +.. code-block:: yaml + + records: + diags: + debug: + enabled: 1 + tags: config.reload + + Testing ======== @@ -683,7 +904,10 @@ After registering a new handler: 3. Run :option:`traffic_ctl config status` to verify the handler appears in the task tree with the correct status. 4. Introduce a parse error in the config file and reload — verify the handler reports ``FAIL``. -5. Use :option:`traffic_ctl config status` ``--format json`` to inspect the raw +5. Check that severity tags (``[Dbg]``, ``[Err]``, etc.) appear correctly in + :option:`traffic_ctl config status` output and that ``--min-level`` filtering works. +6. Enable the ``config.reload`` debug tag and verify the detailed dump appears in ``diags.log``. +7. Use :option:`traffic_ctl config status` ``--format json`` to inspect the raw :ref:`get_reload_config_status` response for automation testing. **Autests** — the project includes autest helpers for config reload testing. Use diff --git a/include/iocore/net/SSLSNIConfig.h b/include/iocore/net/SSLSNIConfig.h index b71502b6e3c..64dd23a7c5d 100644 --- a/include/iocore/net/SSLSNIConfig.h +++ b/include/iocore/net/SSLSNIConfig.h @@ -43,6 +43,7 @@ #include "iocore/eventsystem/ConfigProcessor.h" #include "iocore/net/SNIActionItem.h" #include "iocore/net/YamlSNIConfig.h" +#include "mgmt/config/ConfigContext.h" #include @@ -90,8 +91,8 @@ class SNIConfigParams : public ConfigInfo ~SNIConfigParams() override; const NextHopProperty *get_property_config(const std::string &servername) const; - bool initialize(); - bool initialize(const std::string &sni_filename); + bool initialize(ConfigContext ctx = {}); + bool initialize(const std::string &sni_filename, ConfigContext ctx = {}); /** Walk sni.yaml config and populate sni_action_list @return 0 for success, 1 is failure */ diff --git a/include/iocore/net/quic/QUICConfig.h b/include/iocore/net/quic/QUICConfig.h index 3bc871091aa..1ebec4f6aa0 100644 --- a/include/iocore/net/quic/QUICConfig.h +++ b/include/iocore/net/quic/QUICConfig.h @@ -28,6 +28,7 @@ #include "iocore/eventsystem/ConfigProcessor.h" #include "iocore/net/SSLTypes.h" +#include "mgmt/config/ConfigContext.h" class QUICConfigParams : public ConfigInfo { @@ -35,7 +36,7 @@ class QUICConfigParams : public ConfigInfo QUICConfigParams(){}; ~QUICConfigParams(); - void initialize(); + void initialize(ConfigContext ctx = {}); uint32_t instance_id() const; uint32_t stateless_retry() const; diff --git a/include/mgmt/config/ConfigContext.h b/include/mgmt/config/ConfigContext.h index 0dbce853a1e..49807c02dd6 100644 --- a/include/mgmt/config/ConfigContext.h +++ b/include/mgmt/config/ConfigContext.h @@ -33,6 +33,7 @@ #include "swoc/Errata.h" #include "swoc/BufferWriter.h" +#include "tsutil/ts_diag_levels.h" #include "yaml-cpp/node/node.h" // Forward declarations @@ -118,6 +119,8 @@ class ConfigContext } void log(std::string_view text); + void log(DiagsLevel level, std::string_view text); + void log(swoc::Errata const &errata); template void log(swoc::TextView fmt, Args &&...args) diff --git a/include/mgmt/config/ConfigContextDiags.h b/include/mgmt/config/ConfigContextDiags.h new file mode 100644 index 00000000000..fbebef38827 --- /dev/null +++ b/include/mgmt/config/ConfigContextDiags.h @@ -0,0 +1,180 @@ +/** @file + + ConfigContextDiags.h — Convenience macros for config handler logging. + + These macros combine diags output (Note/Warning/Error) with ConfigContext + task tracking in a single call. They format the message once and send it + to both destinations: + 1. The ATS diagnostics system (diags.log / error.log) + 2. The ConfigContext reload task log (visible via traffic_ctl config status) + + This ensures operators see the same information whether they look at + diags.log or query reload status via traffic_ctl / JSONRPC. + + @section when When to use these macros + + Use a macro when you want the message in BOTH diags and the reload task log. + This is the common case for operational messages in config handlers: + + @code + CfgLoadInProgress(ctx, "%s loading ...", filename); + CfgLoadComplete(ctx, "%s finished loading", filename); + CfgLoadFail(ctx, DL_Error, "%s failed to load", filename); + @endcode + + Use ctx methods directly when you only want the reload task log (no diags): + + @code + ctx.log("parsed %d rules", count); // task log only + ctx.in_progress(); // state change only, no message + ctx.complete(); // state change only, no message + @endcode + + Use Dbg() directly when you only want debug output (no task log): + + @code + Dbg(dbg_ctl_ssl, "internal detail ..."); // diags only, not in reload status + @endcode + + Use CfgLoadDbg when you want BOTH debug output and the task log: + + @code + CfgLoadDbg(ctx, dbg_ctl_ssl, "Reload SNI file"); + @endcode + + @section summary Quick reference + + | Want diags? | Want task log? | Use | + |-------------|----------------|-----------------------------------| + | Note | yes | CfgLoadInProgress / Complete | + | Error/Warn | yes | CfgLoadFail(ctx, DL_xxx, ...) | + | Err + Errata| yes + fail | CfgLoadFailWithErrata(...) | + | Note/Warn | yes (no state) | CfgLoadLog(ctx, DL_xxx, ...) | + | Dbg(tag) | yes | CfgLoadDbg(ctx, ctl, ...) | + | no | yes | ctx.log(...) | + | no | yes + state | ctx.complete() / ctx.fail() | + | yes | no | Note/Warning/Error/Dbg directly | + + @section errata Errata handling + + For failures with swoc::Errata detail, use CfgLoadFailWithErrata to + combine the diags summary, errata detail, and state change in one call: + + @code + CfgLoadFailWithErrata(ctx, DL_Error, errata, "%s failed to load", filename); + @endcode + + This logs the formatted message to diags and the task log at the given + severity, appends each errata annotation (with its own severity) to the + task log, and marks the task as FAIL. + + @section fatal Fatal errors + + Fatal/Emergency terminate the process — reload status is irrelevant. + Call Fatal() directly; do not use these macros for it. + + @section license License + + 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. +*/ + +#pragma once + +#include "mgmt/config/ConfigContext.h" +#include "tscore/Diags.h" + +/// Log a Note and mark the context as IN_PROGRESS. +/// Use at the start of a config load/reload operation. +/// +/// CfgLoadInProgress(ctx, "%s loading ...", ts::filename::IP_ALLOW); +/// +#define CfgLoadInProgress(CTX, FMT, ...) \ + do { \ + char _cfgctx_buf[1024]; \ + snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ + Note("%s", _cfgctx_buf); \ + (CTX).in_progress(_cfgctx_buf); \ + } while (false) + +/// Log a Note and mark the context as SUCCESS. +/// Use when a config load/reload operation finishes successfully. +/// +/// CfgLoadComplete(ctx, "%s finished loading", ts::filename::IP_ALLOW); +/// +#define CfgLoadComplete(CTX, FMT, ...) \ + do { \ + char _cfgctx_buf[1024]; \ + snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ + Note("%s", _cfgctx_buf); \ + (CTX).complete(_cfgctx_buf); \ + } while (false) + +/// Log at the given DiagsLevel and mark the context as FAIL. +/// Use when a config load/reload operation fails. +/// +/// CfgLoadFail(ctx, DL_Error, "%s failed to load", ts::filename::IP_ALLOW); +/// CfgLoadFail(ctx, DL_Warning, "No NAMEDs provided for %s", ts::filename::SPLITDNS); +/// +#define CfgLoadFail(CTX, LEVEL, FMT, ...) \ + do { \ + char _cfgctx_buf[1024]; \ + snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ + DiagsError(LEVEL, "%s", _cfgctx_buf); \ + (CTX).log(LEVEL, _cfgctx_buf); \ + (CTX).fail(); \ + } while (false) + +/// Log at the given DiagsLevel, append errata detail to the task log, and mark +/// the context as FAIL. Combines CfgLoadFail + ctx.fail(errata) in one call. +/// +/// CfgLoadFailWithErrata(ctx, DL_Error, errata, "%s failed to load", filename); +/// +#define CfgLoadFailWithErrata(CTX, LEVEL, ERRATA, FMT, ...) \ + do { \ + char _cfgctx_buf[1024]; \ + snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ + DiagsError(LEVEL, "%s", _cfgctx_buf); \ + (CTX).log(LEVEL, _cfgctx_buf); \ + (CTX).fail(ERRATA); \ + } while (false) + +/// Log at the given DiagsLevel and add to the task log, without changing state. +/// Use for intermediate informational messages during load/reload. +/// +/// CfgLoadLog(ctx, DL_Note, "loaded %d categories from %s", count, filename); +/// +#define CfgLoadLog(CTX, LEVEL, FMT, ...) \ + do { \ + char _cfgctx_buf[1024]; \ + snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ + DiagsError(LEVEL, "%s", _cfgctx_buf); \ + (CTX).log(LEVEL, _cfgctx_buf); \ + } while (false) + +/// Log via a DbgCtl (debug-level, conditional on the tag) and add to the task +/// log. The debug output only appears when the tag is enabled; the task log +/// always receives the message. +/// +/// CfgLoadDbg(ctx, dbg_ctl_ssl, "Reload SNI file"); +/// +#define CfgLoadDbg(CTX, CTL, FMT, ...) \ + do { \ + char _cfgctx_buf[1024]; \ + snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ + Dbg((CTL), "%s", _cfgctx_buf); \ + (CTX).log(DL_Debug, _cfgctx_buf); \ + } while (false) diff --git a/include/mgmt/config/ConfigReloadTrace.h b/include/mgmt/config/ConfigReloadTrace.h index e52078e0b9e..396878e6085 100644 --- a/include/mgmt/config/ConfigReloadTrace.h +++ b/include/mgmt/config/ConfigReloadTrace.h @@ -171,6 +171,15 @@ class ConfigReloadTask : public std::enable_shared_from_this return std::chrono::duration_cast(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 + std::string text; + }; + struct Info { friend class ConfigReloadTask; /// Grant friendship to the specific YAML::convert specialization. @@ -184,7 +193,7 @@ class ConfigReloadTask : public std::enable_shared_from_this protected: int64_t created_time_ms{now_ms()}; ///< milliseconds since epoch int64_t last_updated_time_ms{now_ms()}; ///< last time this task was updated (ms) - std::vector logs; ///< log messages from handler + std::vector logs; ///< log messages from handler State state{State::CREATED}; std::string token; std::string description; @@ -214,6 +223,7 @@ class ConfigReloadTask : public std::enable_shared_from_this [[nodiscard]] ConfigContext add_child(std::string_view description = ""); self_type &log(std::string const &text); + self_type &log(DiagsLevel level, std::string const &text); void set_completed(); void set_failed(); void set_in_progress(); @@ -297,7 +307,7 @@ class ConfigReloadTask : public std::enable_shared_from_this /// Mark task as TIMEOUT with an optional reason logged void mark_as_bad_state(std::string_view reason = ""); - [[nodiscard]] std::vector + [[nodiscard]] std::vector get_logs() const { std::shared_lock lock(_mutex); @@ -356,8 +366,13 @@ class ConfigReloadTask : public std::enable_shared_from_this void notify_parent(); void set_state_and_notify(State state); + friend struct ConfigReloadProgress; + void log_reload_summary(State final_state); + static void dump_subtask_tree(const std::vector &tasks, int indent); + mutable std::shared_mutex _mutex; bool _reload_progress_checker_started{false}; + bool _summary_logged{false}; Info _info; ConfigReloadTaskPtr _parent; ///< parent task, if any diff --git a/include/proxy/ControlMatcher.h b/include/proxy/ControlMatcher.h index 96e9c36e966..505f09e343d 100644 --- a/include/proxy/ControlMatcher.h +++ b/include/proxy/ControlMatcher.h @@ -99,6 +99,8 @@ #include +#include "mgmt/config/ConfigContext.h" + #ifdef HAVE_CTYPE_H #include #endif @@ -302,11 +304,12 @@ template class ControlMatcher public: // Parameter name must not be deallocated before this object is ControlMatcher(const char *file_var, const char *name, const matcher_tags *tags, - int flags_in = (ALLOW_HOST_TABLE | ALLOW_IP_TABLE | ALLOW_REGEX_TABLE | ALLOW_HOST_REGEX_TABLE | ALLOW_URL_TABLE)); + int flags_in = (ALLOW_HOST_TABLE | ALLOW_IP_TABLE | ALLOW_REGEX_TABLE | ALLOW_HOST_REGEX_TABLE | ALLOW_URL_TABLE), + ConfigContext ctx = {}); ~ControlMatcher(); - int BuildTable(); - int BuildTableFromString(char *str); + int BuildTable(ConfigContext ctx = {}); + int BuildTableFromString(char *str, ConfigContext ctx = {}); void Match(RequestData *rdata, MatchResult *result) const; void Print() const; diff --git a/include/proxy/http/remap/RemapConfig.h b/include/proxy/http/remap/RemapConfig.h index 129d619f26f..d745a4d93b0 100644 --- a/include/proxy/http/remap/RemapConfig.h +++ b/include/proxy/http/remap/RemapConfig.h @@ -23,6 +23,7 @@ #pragma once +#include "mgmt/config/ConfigContext.h" #include "proxy/http/remap/AclFiltering.h" class UrlRewrite; @@ -80,7 +81,7 @@ struct BUILD_TABLE_INFO { }; const char *remap_parse_directive(BUILD_TABLE_INFO *bti, char *errbuf, size_t errbufsize); -bool remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti); +bool remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti, ConfigContext ctx = {}); const char *remap_validate_filter_args(acl_filter_rule **rule_pp, const char *const *argv, int argc, char *errStrBuf, size_t errStrBufSize, ACLBehaviorPolicy behavior_policy); @@ -88,7 +89,7 @@ const char *remap_validate_filter_args(acl_filter_rule **rule_pp, const char *co unsigned long remap_check_option(const char *const *argv, int argc, unsigned long findmode = 0, int *_ret_idx = nullptr, const char **argptr = nullptr); -bool remap_parse_config(const char *path, UrlRewrite *rewrite); +bool remap_parse_config(const char *path, UrlRewrite *rewrite, ConfigContext ctx = {}); using load_remap_file_func = void (*)(const char *, const char *); diff --git a/include/proxy/http/remap/RemapYamlConfig.h b/include/proxy/http/remap/RemapYamlConfig.h index a5ec4919c0a..a93ee9d8b10 100644 --- a/include/proxy/http/remap/RemapYamlConfig.h +++ b/include/proxy/http/remap/RemapYamlConfig.h @@ -30,6 +30,7 @@ #include #include "swoc/Errata.h" +#include "mgmt/config/ConfigContext.h" #include "proxy/http/remap/RemapConfig.h" #include "proxy/hdrs/URL.h" @@ -68,6 +69,6 @@ swoc::Errata parse_yaml_include_directive(const std::string &include_path, BUILD swoc::Errata parse_yaml_remap_rule(const YAML::Node &node, BUILD_TABLE_INFO *bti); // Parse remap YAML node -bool remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti); +bool remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti, ConfigContext ctx = {}); -bool remap_parse_yaml(const char *path, UrlRewrite *rewrite); +bool remap_parse_yaml(const char *path, UrlRewrite *rewrite, ConfigContext ctx = {}); diff --git a/include/proxy/http/remap/UrlRewrite.h b/include/proxy/http/remap/UrlRewrite.h index 2f4be91b479..2ef85b9ed70 100644 --- a/include/proxy/http/remap/UrlRewrite.h +++ b/include/proxy/http/remap/UrlRewrite.h @@ -25,6 +25,7 @@ #pragma once #include "iocore/eventsystem/Freer.h" +#include "mgmt/config/ConfigContext.h" #include "proxy/http/remap/UrlMapping.h" #include "proxy/http/remap/UrlMappingPathIndex.h" #include "proxy/http/HttpTransact.h" @@ -77,14 +78,15 @@ class UrlRewrite : public RefCountObjInHeap * * @return @c true if the instance state is valid, @c false if not. */ - bool load(); + bool load(ConfigContext ctx = {}); /** Build the internal url write tables. * * @param path Path to configuration file. + * @param ctx ConfigContext for reload status tracking. * @return 0 on success, non-zero error code on failure. */ - int BuildTable(const char *path); + int BuildTable(const char *path, ConfigContext ctx = {}); mapping_type Remap_redirect(HTTPHdr *request_header, URL *redirect_url); bool ReverseMap(HTTPHdr *response_header); diff --git a/include/records/YAMLConfigReloadTaskEncoder.h b/include/records/YAMLConfigReloadTaskEncoder.h index 5aee5a7bca4..a86b863ba9d 100644 --- a/include/records/YAMLConfigReloadTaskEncoder.h +++ b/include/records/YAMLConfigReloadTaskEncoder.h @@ -54,8 +54,11 @@ template <> struct convert { node["logs"] = YAML::Node(YAML::NodeType::Sequence); // if no logs, it will be empty sequence. - for (const auto &log : info.logs) { - node["logs"].push_back(log); + for (const auto &entry : info.logs) { + YAML::Node log_node; + log_node["level"] = static_cast(entry.level); + log_node["text"] = entry.text; + node["logs"].push_back(log_node); } node["sub_tasks"] = YAML::Node(YAML::NodeType::Sequence); diff --git a/src/iocore/cache/Cache.cc b/src/iocore/cache/Cache.cc index f7e302daa96..69692d890d9 100644 --- a/src/iocore/cache/Cache.cc +++ b/src/iocore/cache/Cache.cc @@ -31,6 +31,7 @@ #include "Stripe.h" #include "StripeSM.h" #include "iocore/cache/Cache.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include "tscore/Filenames.h" #include "tscore/InkErrno.h" @@ -244,8 +245,8 @@ Cache::open_done() type = ht->getType(); cache = ht->getCache(); } - ppt->reset(new CacheHostTable(cache, type)); - ctx.complete("Finished loading"); + ppt->reset(new CacheHostTable(cache, type, ctx)); + CfgLoadComplete(ctx, "%s finished loading", ts::filename::HOSTING); }, config::ConfigSource::FileOnly, // no RPC content source. Legacy for now. {"proxy.config.cache.hosting_filename"}); // trigger records diff --git a/src/iocore/cache/CacheHosting.cc b/src/iocore/cache/CacheHosting.cc index 41df08db992..1a36f34f482 100644 --- a/src/iocore/cache/CacheHosting.cc +++ b/src/iocore/cache/CacheHosting.cc @@ -24,6 +24,7 @@ #include "P_CacheHosting.h" #include "Stripe.h" #include "iocore/cache/CacheDefs.h" +#include "mgmt/config/ConfigContextDiags.h" #include "swoc/swoc_file.h" #include "tscore/HostLookup.h" @@ -188,7 +189,7 @@ CacheHostMatcher::NewEntry(matcher_line *line_info) * End class HostMatcher *************************************************************/ -CacheHostTable::CacheHostTable(Cache *c, CacheType typ) +CacheHostTable::CacheHostTable(Cache *c, CacheType typ, ConfigContext ctx) { ats_scoped_str config_path; @@ -199,7 +200,7 @@ CacheHostTable::CacheHostTable(Cache *c, CacheType typ) config_path = RecConfigReadConfigPath("proxy.config.cache.hosting_filename"); ink_release_assert(config_path); - m_numEntries = this->BuildTable(config_path); + m_numEntries = this->BuildTable(config_path, ctx); } CacheHostTable::~CacheHostTable() {} @@ -236,9 +237,9 @@ int fstat_wrapper(int fd, struct stat *s); // from it // int -CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_buf) +CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_buf, ConfigContext ctx) { - Note("%s loading ...", ts::filename::HOSTING); + CfgLoadInProgress(ctx, "%s loading ...", ts::filename::HOSTING); // Table build locals Tokenizer bufTok("\n"); @@ -260,7 +261,7 @@ CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_bu /* no hosting customers -- put all the volumes in the generic table */ if (gen_host_rec.Init(type)) { - Warning("Problems encountered while initializing the Generic Volume"); + CfgLoadLog(ctx, DL_Warning, "Problems encountered while initializing the Generic Volume"); } return 0; } @@ -280,7 +281,7 @@ CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_bu errPtr = parseConfigLine(const_cast(tmp), current, &config_tags); if (errPtr != nullptr) { - Warning("%s discarding %s entry at line %d : %s", matcher_name, config_file_path, line_num, errPtr); + CfgLoadLog(ctx, DL_Warning, "%s discarding %s entry at line %d : %s", matcher_name, config_file_path, line_num, errPtr); ats_free(current); } else { // Line parsed ok. Figure out what the destination @@ -317,9 +318,9 @@ CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_bu generic table */ if (gen_host_rec.Init(type)) { - Warning("Problems encountered while initializing the Generic Volume"); + CfgLoadLog(ctx, DL_Warning, "Problems encountered while initializing the Generic Volume"); } - Note("%s finished loading", ts::filename::HOSTING); + CfgLoadLog(ctx, DL_Note, "%s finished loading", ts::filename::HOSTING); return 0; } @@ -345,21 +346,22 @@ CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_bu if (current->dest_entry < MATCHER_MAX_TOKENS) { current->line[0][current->dest_entry] = nullptr; } else { - Warning("Problems encountered while initializing the Generic Volume"); + CfgLoadLog(ctx, DL_Warning, "Problems encountered while initializing the Generic Volume"); } current->num_el--; if (!gen_host_rec.Init(current, type)) { generic_rec_initd = 1; } else { - Warning("Problems encountered while initializing the Generic Volume"); + CfgLoadLog(ctx, DL_Warning, "Problems encountered while initializing the Generic Volume"); } } else { hostMatch->NewEntry(current); } } else { - Warning("%s discarding %s entry with unknown type at line %d", matcher_name, config_file_path, current->line_num); + CfgLoadLog(ctx, DL_Warning, "%s discarding %s entry with unknown type at line %d", matcher_name, config_file_path, + current->line_num); } // Deallocate the parsing structure @@ -368,11 +370,12 @@ CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_bu ats_free(last); } - Note("%s finished loading", ts::filename::HOSTING); + CfgLoadLog(ctx, DL_Note, "%s finished loading", ts::filename::HOSTING); if (!generic_rec_initd) { const char *cache_type = (type == CacheType::HTTP) ? "http" : "mixt"; - Warning("No Volumes specified for Generic Hostnames for %s documents: %s cache will be disabled", cache_type, cache_type); + CfgLoadLog(ctx, DL_Warning, "No Volumes specified for Generic Hostnames for %s documents: %s cache will be disabled", + cache_type, cache_type); } ink_assert(second_pass == numEntries); @@ -384,7 +387,7 @@ CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_bu } int -CacheHostTable::BuildTable(const char *config_file_path) +CacheHostTable::BuildTable(const char *config_file_path, ConfigContext ctx) { std::error_code ec; std::string content{swoc::file::load(swoc::file::path{config_file_path}, ec)}; @@ -392,16 +395,16 @@ CacheHostTable::BuildTable(const char *config_file_path) if (ec) { switch (ec.value()) { case ENOENT: - Warning("Cannot open the config file: %s - %s", config_file_path, strerror(ec.value())); + CfgLoadLog(ctx, DL_Warning, "Cannot open the config file: %s - %s", config_file_path, strerror(ec.value())); break; default: - Error("%s failed to load: %s", config_file_path, strerror(ec.value())); + CfgLoadFail(ctx, DL_Error, "%s failed to load: %s", config_file_path, strerror(ec.value())); gen_host_rec.Init(type); return 0; } } - return BuildTableFromString(config_file_path, content.data()); + return BuildTableFromString(config_file_path, content.data(), ctx); } int diff --git a/src/iocore/cache/P_CacheHosting.h b/src/iocore/cache/P_CacheHosting.h index 69b104a5f13..3605ff95b68 100644 --- a/src/iocore/cache/P_CacheHosting.h +++ b/src/iocore/cache/P_CacheHosting.h @@ -23,6 +23,7 @@ #pragma once #include "iocore/cache/CacheDefs.h" +#include "mgmt/config/ConfigContext.h" #include "records/RecCore.h" #include "tscore/MatcherUtils.h" #include "tscore/HostLookup.h" @@ -225,11 +226,11 @@ class CacheHostTable public: // Parameter name must not be deallocated before this // object is - CacheHostTable(Cache *c, CacheType typ); + CacheHostTable(Cache *c, CacheType typ, ConfigContext ctx = {}); ~CacheHostTable(); - int BuildTable(const char *config_file_path); - int BuildTableFromString(const char *config_file_path, char *str); + int BuildTable(const char *config_file_path, ConfigContext ctx = {}); + int BuildTableFromString(const char *config_file_path, char *str, ConfigContext ctx = {}); void Match(std::string_view rdata, CacheHostResult *result) const; void Print() const; diff --git a/src/iocore/dns/SplitDNS.cc b/src/iocore/dns/SplitDNS.cc index d136161989f..bb017269962 100644 --- a/src/iocore/dns/SplitDNS.cc +++ b/src/iocore/dns/SplitDNS.cc @@ -31,6 +31,7 @@ #include "P_SplitDNSProcessor.h" #include "tscore/Tokenizer.h" #include "tscore/Filenames.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include @@ -132,22 +133,23 @@ void SplitDNSConfig::reconfigure(ConfigContext ctx) { if (0 == gsplit_dns_enabled) { - ctx.complete("SplitDNS disabled, skipping reload"); + CfgLoadComplete(ctx, "SplitDNS disabled, skipping reload"); return; } - Note("%s loading ...", ts::filename::SPLITDNS); + CfgLoadInProgress(ctx, "%s loading ...", ts::filename::SPLITDNS); SplitDNS *params = new SplitDNS; params->m_SplitDNSlEnable = gsplit_dns_enabled; - params->m_DNSSrvrTable = std::make_unique("proxy.config.dns.splitdns.filename", modulePrefix, &sdns_dest_tags); + params->m_DNSSrvrTable = std::make_unique( + "proxy.config.dns.splitdns.filename", modulePrefix, &sdns_dest_tags, + ALLOW_HOST_TABLE | ALLOW_IP_TABLE | ALLOW_REGEX_TABLE | ALLOW_HOST_REGEX_TABLE | ALLOW_URL_TABLE, ctx); if (nullptr == params->m_DNSSrvrTable || (0 == params->m_DNSSrvrTable->getEntryCount())) { - Warning("Failed to load %s - No NAMEDs provided! Disabling SplitDNS", ts::filename::SPLITDNS); gsplit_dns_enabled = 0; delete params; - ctx.fail("No NAMEDs provided, disabling SplitDNS"); + CfgLoadFail(ctx, DL_Warning, "Failed to load %s - No NAMEDs provided! Disabling SplitDNS", ts::filename::SPLITDNS); return; } params->m_numEle = params->m_DNSSrvrTable->getEntryCount(); @@ -165,8 +167,7 @@ SplitDNSConfig::reconfigure(ConfigContext ctx) SplitDNSConfig::print(); } - Note("%s finished loading", ts::filename::SPLITDNS); - ctx.complete("Finished loading"); + CfgLoadComplete(ctx, "%s finished loading", ts::filename::SPLITDNS); } /* -------------------------------------------------------------- diff --git a/src/iocore/net/P_SSLConfig.h b/src/iocore/net/P_SSLConfig.h index 0d6ee6b14e9..01343ff191f 100644 --- a/src/iocore/net/P_SSLConfig.h +++ b/src/iocore/net/P_SSLConfig.h @@ -34,6 +34,7 @@ #include "SSLSessionCache.h" #include "iocore/eventsystem/ConfigProcessor.h" #include "iocore/net/YamlSNIConfig.h" +#include "mgmt/config/ConfigContext.h" #include #include @@ -152,7 +153,7 @@ struct SSLConfigParams : public ConfigInfo { void cleanupCTXTable(); - void initialize(); + void initialize(ConfigContext ctx = {}); void cleanup(); void reset(); void SSLConfigInit(swoc::IPRangeSet *global); @@ -209,7 +210,7 @@ struct SSLTicketParams : public ConfigInfo { ssl_ticket_key_block *default_global_keyblock = nullptr; time_t load_time = 0; char *ticket_key_filename = nullptr; - bool LoadTicket(bool &nochange); + bool LoadTicket(bool &nochange, ConfigContext ctx = {}); bool LoadTicketData(char *ticket_data, int ticket_data_len); void cleanup(); diff --git a/src/iocore/net/QUICMultiCertConfigLoader.cc b/src/iocore/net/QUICMultiCertConfigLoader.cc index 8e5bbe1dca7..04d56c47542 100644 --- a/src/iocore/net/QUICMultiCertConfigLoader.cc +++ b/src/iocore/net/QUICMultiCertConfigLoader.cc @@ -25,6 +25,7 @@ #include "P_SSLConfig.h" #include "iocore/net/QUICMultiCertConfigLoader.h" #include "iocore/net/quic/QUICConfig.h" +#include "mgmt/config/ConfigContextDiags.h" int QUICCertConfig::_config_id = 0; @@ -57,19 +58,13 @@ QUICCertConfig::reconfigure(ConfigContext ctx) } if (!errata.empty()) { - errata.assign_annotation_glue_text("\n "); - errata.assign_severity_glue_text(" -> \n "); - bwprint(ts::bw_dbg, "\n{}", errata); - } else { - ts::bw_dbg = ""; + ctx.log(errata); } if (retStatus) { - Note("(quic) %s finished loading%s", params->configFilePath, ts::bw_dbg.c_str()); - ctx.complete("QUICCertConfig loaded"); + CfgLoadComplete(ctx, "(quic) %s finished loading", params->configFilePath); } else { - Error("(quic) %s failed to load%s", params->configFilePath, ts::bw_dbg.c_str()); - ctx.fail("QUICCertConfig failed to load"); + CfgLoadFail(ctx, DL_Error, "(quic) %s failed to load", params->configFilePath); } } diff --git a/src/iocore/net/SSLConfig.cc b/src/iocore/net/SSLConfig.cc index 936821b41bf..4a0b6fdad96 100644 --- a/src/iocore/net/SSLConfig.cc +++ b/src/iocore/net/SSLConfig.cc @@ -43,6 +43,7 @@ #include "tscore/Layout.h" #include "records/RecHttp.h" #include "records/RecCore.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include @@ -251,7 +252,7 @@ SSLConfigParams::SetServerPolicy(const char *verify_server) } void -SSLConfigParams::initialize() +SSLConfigParams::initialize(ConfigContext ctx) { cleanup(); @@ -552,7 +553,7 @@ SSLConfigParams::initialize() ssl_ktls_enabled = RecGetRecordInt("proxy.config.ssl.ktls.enabled").value_or(0); #ifndef SSL_OP_ENABLE_KTLS if (ssl_ktls_enabled) { - Error("kTLS configured but not supported by OpenSSL library"); + CfgLoadLog(ctx, DL_Error, "kTLS configured but not supported by OpenSSL library"); } #endif @@ -571,7 +572,7 @@ SSLConfigParams::initialize() if (this->clientCertExitOnLoadError) { 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"); + CfgLoadFail(ctx, DL_Error, "Can't initialize the SSL client, HTTPS in remap rules will not function"); } } @@ -614,13 +615,15 @@ SSLConfig::startup() void SSLConfig::reconfigure(ConfigContext ctx) { - Dbg(dbg_ctl_ssl_load, "Reload SSLConfig"); - SSLConfigParams *params; - params = new SSLConfigParams; + CfgLoadLog(ctx, DL_Note, "SSLConfig loading ..."); + CfgLoadDbg(ctx, dbg_ctl_ssl_load, "Reload SSLConfig"); + + SSLConfigParams *params = new SSLConfigParams; // start loading the next config - int loading_config_index = get_loading_config_index(); + int loading_config_index = get_loading_config_index(); + configids[loading_config_index] = configProcessor.set(configids[loading_config_index], params); - params->initialize(); // re-read configuration + params->initialize(ctx); // re-read configuration // Make the new config available for use. commit_config_id(); ctx.complete("SSLConfig reloaded"); @@ -670,6 +673,8 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx) SSLConfig::scoped_config params; SSLCertLookup *lookup = new SSLCertLookup(); + CfgLoadLog(ctx, DL_Note, "(ssl) %s loading ...", params->configFilePath); + // Test SSL certificate loading startup. With large numbers of certificates, reloading can take time, so delay // twice the healthcheck period to simulate a loading a large certificate set. if (is_action_tag_set("test.multicert.delay")) { @@ -692,19 +697,13 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx) } if (!errata.empty()) { - errata.assign_annotation_glue_text("\n "); - errata.assign_severity_glue_text(" -> \n "); - bwprint(ts::bw_dbg, "\n{}", errata); - } else { - ts::bw_dbg = ""; + ctx.log(errata); } if (retStatus) { - Note("(ssl) %s finished loading%s", params->configFilePath, ts::bw_dbg.c_str()); - ctx.complete("SSLCertificateConfig loaded {}", ts::bw_dbg.c_str()); + CfgLoadComplete(ctx, "(ssl) %s finished loading", params->configFilePath); } else { - Error("(ssl) %s failed to load%s", params->configFilePath, ts::bw_dbg.c_str()); - ctx.fail("SSLCertificateConfig failed to load {}", ts::bw_dbg.c_str()); + CfgLoadFail(ctx, DL_Error, "(ssl) %s failed to load", params->configFilePath); } return retStatus; @@ -726,7 +725,7 @@ SSLCertificateConfig::release(SSLCertLookup *lookup) } bool -SSLTicketParams::LoadTicket(bool &nochange) +SSLTicketParams::LoadTicket(bool &nochange, ConfigContext ctx) { cleanup(); nochange = true; @@ -772,13 +771,13 @@ SSLTicketParams::LoadTicket(bool &nochange) return true; } if (!keyblock) { - Error("Could not load ticket key from %s", ticket_key_filename); + CfgLoadFail(ctx, DL_Error, "Could not load ticket key from %s", ticket_key_filename); return false; } default_global_keyblock = keyblock; load_time = time(nullptr); - Dbg(dbg_ctl_ssl_load, "ticket key reloaded from %s", ticket_key_filename); + CfgLoadDbg(ctx, dbg_ctl_ssl_load, "ticket key reloaded from %s", ticket_key_filename); #endif return true; } @@ -807,8 +806,7 @@ SSLTicketKeyConfig::startup() { config::ConfigRegistry::Get_Instance().register_record_config("ssl_ticket_key", // key [](ConfigContext ctx) { // handler callback - // eventually ctx should be passed through to the reconfigure fn - // and the loaders so it can show more details. + CfgLoadLog(ctx, DL_Note, "SSL ticket key loading ..."); if (SSLTicketKeyConfig::reconfigure(ctx)) { ctx.complete("SSL ticket key reloaded"); } else { @@ -824,13 +822,13 @@ SSLTicketKeyConfig::startup() } bool -SSLTicketKeyConfig::reconfigure([[maybe_unused]] ConfigContext ctx) +SSLTicketKeyConfig::reconfigure(ConfigContext ctx) { SSLTicketParams *ticketKey = new SSLTicketParams(); if (ticketKey) { bool nochange = false; - if (!ticketKey->LoadTicket(nochange)) { + if (!ticketKey->LoadTicket(nochange, ctx)) { delete ticketKey; return false; } diff --git a/src/iocore/net/SSLSNIConfig.cc b/src/iocore/net/SSLSNIConfig.cc index 341b489f222..6607e08d757 100644 --- a/src/iocore/net/SSLSNIConfig.cc +++ b/src/iocore/net/SSLSNIConfig.cc @@ -33,6 +33,7 @@ #include "P_SSLConfig.h" #include "iocore/net/SSLSNIConfig.h" #include "iocore/net/SNIActionItem.h" +#include "mgmt/config/ConfigContextDiags.h" #include "tscore/Diags.h" #include "tscore/Layout.h" #include "tscore/TSSystemState.h" @@ -262,22 +263,20 @@ SNIConfigParams::get(std::string_view servername, in_port_t dest_incoming_port) } bool -SNIConfigParams::initialize() +SNIConfigParams::initialize(ConfigContext ctx) { std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); - return initialize(sni_filename); + return initialize(sni_filename, ctx); } bool -SNIConfigParams::initialize(std::string const &sni_filename) +SNIConfigParams::initialize(std::string const &sni_filename, ConfigContext ctx) { - Note("%s loading ...", sni_filename.c_str()); + CfgLoadInProgress(ctx, "%s loading ...", sni_filename.c_str()); struct stat sbuf; if (stat(sni_filename.c_str(), &sbuf) == -1 && errno == ENOENT) { - Note("%s failed to load", sni_filename.c_str()); - Warning("Loading SNI configuration - filename: %s doesn't exist", sni_filename.c_str()); - + CfgLoadLog(ctx, DL_Warning, "Loading SNI configuration - %s doesn't exist", sni_filename.c_str()); return true; } @@ -289,7 +288,7 @@ SNIConfigParams::initialize(std::string const &sni_filename) if (TSSystemState::is_initializing()) { Emergency("%s failed to load: %s", sni_filename.c_str(), errMsg.str().c_str()); } else { - Error("%s failed to load: %s", sni_filename.c_str(), errMsg.str().c_str()); + CfgLoadFail(ctx, DL_Error, "%s failed to load: %s", sni_filename.c_str(), errMsg.str().c_str()); } return false; } @@ -321,27 +320,20 @@ SNIConfig::startup() int SNIConfig::reconfigure(ConfigContext ctx) { - Dbg(dbg_ctl_ssl, "Reload SNI file"); - SNIConfigParams *params = new SNIConfigParams; - bool retStatus = params->initialize(); + bool retStatus = params->initialize(ctx); if (retStatus) { _configid = configProcessor.set(_configid, params); if (SNIConfig::on_reconfigure) { SNIConfig::on_reconfigure(); } + std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); + CfgLoadComplete(ctx, "%s finished loading", sni_filename.c_str()); } else { delete params; - } - - std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); - if (retStatus || TSSystemState::is_initializing()) { - Note("%s finished loading", sni_filename.c_str()); - ctx.complete("Loading finished"); - } else { - Error("%s failed to load", sni_filename.c_str()); - ctx.fail("Failed to load"); + std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); + CfgLoadFail(ctx, DL_Error, "%s failed to load", sni_filename.c_str()); } return retStatus ? 1 : 0; diff --git a/src/iocore/net/SSLUtils.cc b/src/iocore/net/SSLUtils.cc index ee2231615a1..ac96354f78c 100644 --- a/src/iocore/net/SSLUtils.cc +++ b/src/iocore/net/SSLUtils.cc @@ -1917,7 +1917,8 @@ SSLMultiCertConfigLoader::_load_items(SSLCertLookup *lookup, config::SSLMultiCer if (sslMultiCertSettings->cert || sslMultiCertSettings->opt == SSLCertContextOption::OPT_TUNNEL) { if (!this->_store_ssl_ctx(lookup, sslMultiCertSettings)) { std::lock_guard lock(_loader_mutex); - errata.note(ERRATA_ERROR, "Failed to load certificate at item {}", item_num); + errata.note(ERRATA_ERROR, "Failed to load certificate '{}' at item {}", + sslMultiCertSettings->cert ? sslMultiCertSettings->cert : "(unnamed)", item_num); } } else { std::lock_guard lock(_loader_mutex); diff --git a/src/iocore/net/quic/QUICConfig.cc b/src/iocore/net/quic/QUICConfig.cc index 6790e25b205..6c39eb17f85 100644 --- a/src/iocore/net/quic/QUICConfig.cc +++ b/src/iocore/net/quic/QUICConfig.cc @@ -25,6 +25,7 @@ #include +#include "mgmt/config/ConfigContextDiags.h" #include "records/RecHttp.h" #include "../P_SSLConfig.h" @@ -57,7 +58,7 @@ quic_new_ssl_ctx() ALPN and SNI should be set to SSL object with NETVC_OPTIONS **/ static shared_SSL_CTX -quic_init_client_ssl_ctx(const QUICConfigParams *params) +quic_init_client_ssl_ctx(const QUICConfigParams *params, ConfigContext ctx) { std::unique_ptr ssl_ctx(nullptr, &SSL_CTX_free); ssl_ctx.reset(quic_new_ssl_ctx()); @@ -69,7 +70,7 @@ quic_init_client_ssl_ctx(const QUICConfigParams *params) #else if (SSL_CTX_set1_curves_list(ssl_ctx.get(), params->client_supported_groups()) != 1) { #endif - Error("SSL_CTX_set1_groups_list failed"); + CfgLoadLog(ctx, DL_Error, "SSL_CTX_set1_groups_list failed"); } } #endif @@ -99,7 +100,7 @@ QUICConfigParams::~QUICConfigParams() }; void -QUICConfigParams::initialize() +QUICConfigParams::initialize(ConfigContext ctx) { RecEstablishStaticConfigUInt32(this->_instance_id, "proxy.config.quic.instance_id"); RecEstablishStaticConfigInt32(this->_connection_table_size, "proxy.config.quic.connection_table.size"); @@ -171,7 +172,7 @@ QUICConfigParams::initialize() RecEstablishStaticConfigUInt32(this->_disable_http_0_9, "proxy.config.quic.disable_http_0_9"); RecEstablishStaticConfigUInt32(this->_cc_algorithm, "proxy.config.quic.cc_algorithm"); - this->_client_ssl_ctx = quic_init_client_ssl_ctx(this); + this->_client_ssl_ctx = quic_init_client_ssl_ctx(this, ctx); } uint32_t @@ -456,7 +457,7 @@ QUICConfig::reconfigure(ConfigContext ctx) QUICConfigParams *params; params = new QUICConfigParams; // re-read configuration - params->initialize(); + params->initialize(ctx); _config_id = configProcessor.set(_config_id, params); QUICConnectionId::SCID_LEN = params->scid_len(); diff --git a/src/mgmt/config/ConfigContext.cc b/src/mgmt/config/ConfigContext.cc index c23f3e27604..7dd81a58ca3 100644 --- a/src/mgmt/config/ConfigContext.cc +++ b/src/mgmt/config/ConfigContext.cc @@ -63,7 +63,7 @@ ConfigContext::in_progress(std::string_view text) if (auto p = _task.lock()) { p->set_in_progress(); if (!text.empty()) { - p->log(std::string{text}); + p->log(DL_Note, std::string{text}); } } } @@ -76,13 +76,31 @@ ConfigContext::log(std::string_view text) } } +void +ConfigContext::log(DiagsLevel level, std::string_view text) +{ + if (auto p = _task.lock()) { + p->log(level, std::string{text}); + } +} + +void +ConfigContext::log(swoc::Errata const &errata) +{ + if (auto p = _task.lock()) { + for (auto const &annotation : errata) { + p->log(static_cast(static_cast(annotation.severity())), std::string{annotation.text()}); + } + } +} + void ConfigContext::complete(std::string_view text) { if (auto p = _task.lock()) { p->set_completed(); if (!text.empty()) { - p->log(std::string{text}); + p->log(DL_Note, std::string{text}); } } } @@ -93,7 +111,7 @@ ConfigContext::fail(std::string_view reason) if (auto p = _task.lock()) { p->set_failed(); if (!reason.empty()) { - p->log(std::string{reason}); + p->log(DL_Error, std::string{reason}); } } } @@ -105,11 +123,11 @@ ConfigContext::fail(swoc::Errata const &errata, std::string_view summary) p->set_failed(); // Log the summary first if (!summary.empty()) { - p->log(std::string{summary}); + p->log(DL_Error, std::string{summary}); } // Log each error from the errata for (auto const &err : errata) { - p->log(std::string{err.text()}); + p->log(static_cast(static_cast(err.severity())), std::string{err.text()}); } } } diff --git a/src/mgmt/config/ConfigReloadTrace.cc b/src/mgmt/config/ConfigReloadTrace.cc index aa058433884..131f7c2be6c 100644 --- a/src/mgmt/config/ConfigReloadTrace.cc +++ b/src/mgmt/config/ConfigReloadTrace.cc @@ -24,6 +24,7 @@ #include "mgmt/config/ConfigReloadTrace.h" #include "mgmt/config/ConfigContext.h" #include "records/RecCore.h" +#include "tsutil/ts_diag_levels.h" #include #include "tsutil/Metrics.h" @@ -91,7 +92,15 @@ ConfigReloadTask & ConfigReloadTask::log(std::string const &text) { std::unique_lock lock(_mutex); - _info.logs.push_back(text); + _info.logs.push_back({DL_Undefined, text}); + return *this; +} + +ConfigReloadTask & +ConfigReloadTask::log(DiagsLevel level, std::string const &text) +{ + std::unique_lock lock(_mutex); + _info.logs.push_back({level, text}); return *this; } @@ -163,7 +172,7 @@ ConfigReloadTask::mark_as_bad_state(std::string_view reason) _atomic_last_updated_ms.store(now_ms(), std::memory_order_release); if (!reason.empty()) { // Push directly to avoid deadlock (log() would try to acquire same mutex) - _info.logs.emplace_back(reason); + _info.logs.push_back({DL_Undefined, std::string{reason}}); } } @@ -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); + } else { + Warning("Config reload [%s] finished with failures: %d succeeded, %d failed (%d total) " + "— run: traffic_ctl config status -t %s", + _info.token.c_str(), success_count, fail_count, total, _info.token.c_str()); + } + + dump_subtask_tree(_info.sub_tasks, 2); +} + +void +ConfigReloadTask::dump_subtask_tree(const std::vector &tasks, int indent) +{ + static constexpr int MAX_DEPTH = 10; + if (indent / 2 > MAX_DEPTH) { + return; + } + for (const auto &sub : tasks) { + auto sub_state = sub->get_state(); + auto sub_info = sub->get_info(); + Dbg(dbg_ctl_config, "%*s[%.*s] %s", indent, "", static_cast(state_to_string(sub_state).size()), + state_to_string(sub_state).data(), sub_info.description.c_str()); + for (const auto &entry : sub_info.logs) { + if (entry.level != DL_Undefined) { + static constexpr const char *tags[] = { + "[Diag] ", "[Dbg] ", "[Stat] ", "[Note] ", "[Warn] ", "[Err] ", "[Fatal] ", "[Alert] ", "[Emrg] ", + }; + int idx = std::min(std::max(static_cast(entry.level), 0), static_cast(DL_Emergency)); + Dbg(dbg_ctl_config, "%*s %s%s", indent, "", tags[idx], entry.text.c_str()); + } else { + Dbg(dbg_ctl_config, "%*s %s", indent, "", entry.text.c_str()); + } + } + if (!sub_info.sub_tasks.empty()) { + dump_subtask_tree(sub_info.sub_tasks, indent + 2); + } + } +} + int64_t ConfigReloadTask::get_last_updated_time_ms() const { @@ -336,6 +404,7 @@ ConfigReloadProgress::check_progress(int /* etype */, void * /* data */) // Confirmed terminal — safe to stop. Dbg(dbg_ctl_config, "Reload task %s confirmed %.*s after grace period, stopping progress check.", _reload->get_token().c_str(), static_cast(state_str.size()), state_str.data()); + _reload->log_reload_summary(current_state); return EVENT_DONE; } // First observation of terminal state — reschedule once more to confirm diff --git a/src/proxy/CacheControl.cc b/src/proxy/CacheControl.cc index ae0bc70e76c..3889c6887dc 100644 --- a/src/proxy/CacheControl.cc +++ b/src/proxy/CacheControl.cc @@ -33,6 +33,7 @@ #include "tscore/Filenames.h" #include "proxy/CacheControl.h" #include "proxy/ControlMatcher.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include "proxy/http/HttpConfig.h" namespace @@ -125,16 +126,16 @@ initCacheControl() void reloadCacheControl(ConfigContext ctx) { - Note("%s loading ...", ts::filename::CACHE); - CC_table *newTable; - + CfgLoadInProgress(ctx, "%s loading ...", ts::filename::CACHE); Dbg(dbg_ctl_cache_control, "%s updated, reloading", ts::filename::CACHE); + eventProcessor.schedule_in(new CC_FreerContinuation(CacheControlTable), CACHE_CONTROL_TIMEOUT, ET_CALL); - newTable = new CC_table("proxy.config.cache.control.filename", modulePrefix, &http_dest_tags); + CC_table *newTable = + new CC_table("proxy.config.cache.control.filename", modulePrefix, &http_dest_tags, + ALLOW_HOST_TABLE | ALLOW_IP_TABLE | ALLOW_REGEX_TABLE | ALLOW_HOST_REGEX_TABLE | ALLOW_URL_TABLE, ctx); ink_atomic_swap(&CacheControlTable, newTable); - Note("%s finished loading", ts::filename::CACHE); - ctx.complete("Finished loading"); + CfgLoadComplete(ctx, "%s finished loading", ts::filename::CACHE); } void diff --git a/src/proxy/ControlMatcher.cc b/src/proxy/ControlMatcher.cc index 8e6b6b7ee5e..4c5271bf304 100644 --- a/src/proxy/ControlMatcher.cc +++ b/src/proxy/ControlMatcher.cc @@ -31,6 +31,7 @@ #include "swoc/bwf_ip.h" #include "swoc/swoc_file.h" +#include "mgmt/config/ConfigContextDiags.h" #include "tscore/MatcherUtils.h" #include "tscore/Tokenizer.h" #include "proxy/ControlMatcher.h" @@ -639,7 +640,8 @@ IpMatcher::Print() const } template -ControlMatcher::ControlMatcher(const char *file_var, const char *name, const matcher_tags *tags, int flags_in) +ControlMatcher::ControlMatcher(const char *file_var, const char *name, const matcher_tags *tags, int flags_in, + ConfigContext ctx) { flags = flags_in; ink_assert(flags & (ALLOW_HOST_TABLE | ALLOW_REGEX_TABLE | ALLOW_URL_TABLE | ALLOW_IP_TABLE)); @@ -664,7 +666,7 @@ ControlMatcher::ControlMatcher(const char *file_var, const ch hrMatch = nullptr; if (!(flags & DONT_BUILD_TABLE)) { - m_numEntries = this->BuildTable(); + m_numEntries = this->BuildTable(ctx); } else { m_numEntries = 0; } @@ -731,7 +733,7 @@ ControlMatcher::Match(RequestData *rdata, MatchResult *result // template int -ControlMatcher::BuildTableFromString(char *file_buf) +ControlMatcher::BuildTableFromString(char *file_buf, ConfigContext ctx) { // Table build locals Tokenizer bufTok("\n"); @@ -776,7 +778,7 @@ ControlMatcher::BuildTableFromString(char *file_buf) if (config_tags != &socks_server_tags) { Result error = Result::failure("%s discarding %s entry at line %d : %s", matcher_name, config_file_path, line_num, errptr); - Error("%s", error.message()); + CfgLoadLog(ctx, DL_Error, "%s", error.message()); } ats_free(current); } else { @@ -874,7 +876,7 @@ ControlMatcher::BuildTableFromString(char *file_buf) // Check to see if there was an error in creating the NewEntry if (error.failed()) { - Error("%s", error.message()); + CfgLoadLog(ctx, DL_Error, "%s", error.message()); } // Deallocate the parsing structure @@ -893,22 +895,23 @@ ControlMatcher::BuildTableFromString(char *file_buf) template int -ControlMatcher::BuildTable() +ControlMatcher::BuildTable(ConfigContext ctx) { std::error_code ec; std::string content{swoc::file::load(swoc::file::path{config_file_path}, ec)}; + if (ec) { switch (ec.value()) { case ENOENT: - Warning("ControlMatcher - Cannot open config file: %s - %s", config_file_path, strerror(ec.value())); + CfgLoadLog(ctx, DL_Warning, "ControlMatcher - Cannot open config file: %s - %s", config_file_path, strerror(ec.value())); break; default: - Error("ControlMatcher - %s failed to load: %s", config_file_path, strerror(ec.value())); + CfgLoadFail(ctx, DL_Error, "ControlMatcher - %s failed to load: %s", config_file_path, strerror(ec.value())); return 1; } } - return BuildTableFromString(content.data()); + return BuildTableFromString(content.data(), ctx); } /**************************************************************** diff --git a/src/proxy/IPAllow.cc b/src/proxy/IPAllow.cc index 24c7dd67bee..6cb0815ed59 100644 --- a/src/proxy/IPAllow.cc +++ b/src/proxy/IPAllow.cc @@ -27,6 +27,7 @@ #include #include "proxy/IPAllow.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include "records/RecCore.h" #include "swoc/Errata.h" @@ -123,37 +124,28 @@ IpAllow::startup() void IpAllow::reconfigure(ConfigContext ctx) { - self_type *new_table; - std::string text; - std::string errata_text; - Note("%s loading ...", ts::filename::IP_ALLOW); - ctx.in_progress(); + CfgLoadInProgress(ctx, "%s loading ...", ts::filename::IP_ALLOW); + + auto *new_table = new self_type("proxy.config.cache.ip_allow.filename", "proxy.config.cache.ip_categories.filename"); - new_table = new self_type("proxy.config.cache.ip_allow.filename", "proxy.config.cache.ip_categories.filename"); // IP rules need categories, so load them first (if they exist). if (auto errata = new_table->BuildCategories(); !errata.is_ok()) { - swoc::bwprint(text, "{} failed to load", new_table->ip_categories_config_file); - swoc::bwprint(errata_text, "{}", errata); - Error("%s\n%s", text.c_str(), errata_text.c_str()); - ctx.fail(errata, "{} failed to load", new_table->ip_categories_config_file); + CfgLoadFail(ctx, DL_Error, "%s failed to load", new_table->ip_categories_config_file.c_str()); + ctx.fail(errata); delete new_table; return; } if (auto errata = new_table->BuildTable(); !errata.is_ok()) { - swoc::bwprint(text, "{} failed to load", ts::filename::IP_ALLOW); - swoc::bwprint(errata_text, "{}", errata); - if (errata.severity() <= ERRATA_ERROR) { - Error("%s\n%s", text.c_str(), errata_text.c_str()); - } else { - Fatal("%s\n%s", text.c_str(), errata_text.c_str()); + if (errata.severity() > ERRATA_ERROR) { + Fatal("%s failed to load", ts::filename::IP_ALLOW); } - ctx.fail(errata, "{} failed to load", ts::filename::IP_ALLOW); + CfgLoadFail(ctx, DL_Error, "%s failed to load", ts::filename::IP_ALLOW); + ctx.fail(errata); delete new_table; return; } configid = configProcessor.set(configid, new_table); - Note("%s finished loading", ts::filename::IP_ALLOW); - ctx.complete("Finished loading"); + CfgLoadComplete(ctx, "%s finished loading", ts::filename::IP_ALLOW); } IpAllow * diff --git a/src/proxy/ParentSelection.cc b/src/proxy/ParentSelection.cc index b1826a1e00a..60a3b3a5f2b 100644 --- a/src/proxy/ParentSelection.cc +++ b/src/proxy/ParentSelection.cc @@ -24,6 +24,7 @@ #include "proxy/ParentConsistentHash.h" #include "proxy/ParentRoundRobin.h" #include "proxy/ControlMatcher.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include "proxy/HostStatus.h" #include "proxy/hdrs/HTTP.h" @@ -303,14 +304,13 @@ ParentConfig::startup() void ParentConfig::reconfigure(ConfigContext ctx) { - Note("%s loading ...", ts::filename::PARENT); - - ParentConfigParams *params = nullptr; + CfgLoadInProgress(ctx, "%s loading ...", ts::filename::PARENT); // Allocate parent table - P_table *pTable = new P_table(file_var, modulePrefix, &http_dest_tags); - - params = new ParentConfigParams(pTable); + P_table *pTable = + new P_table(file_var, modulePrefix, &http_dest_tags, + ALLOW_HOST_TABLE | ALLOW_IP_TABLE | ALLOW_REGEX_TABLE | ALLOW_HOST_REGEX_TABLE | ALLOW_URL_TABLE, ctx); + ParentConfigParams *params = new ParentConfigParams(pTable); ink_assert(params != nullptr); m_id = configProcessor.set(m_id, params); @@ -319,8 +319,7 @@ ParentConfig::reconfigure(ConfigContext ctx) ParentConfig::print(); } - Note("%s finished loading", ts::filename::PARENT); - ctx.complete("Finished loading"); + CfgLoadComplete(ctx, "%s finished loading", ts::filename::PARENT); } void diff --git a/src/proxy/ReverseProxy.cc b/src/proxy/ReverseProxy.cc index a7add1f7967..7c4b721f503 100644 --- a/src/proxy/ReverseProxy.cc +++ b/src/proxy/ReverseProxy.cc @@ -32,6 +32,7 @@ #include #include "iocore/cache/Cache.h" #include "proxy/ReverseProxy.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #include "tscore/MatcherUtils.h" #include "tscore/Tokenizer.h" @@ -158,44 +159,27 @@ urlRewriteVerify() bool reloadUrlRewrite(ConfigContext ctx) { - std::string msg_buffer; - msg_buffer.reserve(1024); - UrlRewrite *newTable, *oldTable; - - Note("%s loading (checking first) ...", ts::filename::REMAP_YAML); - Note("%s loading ...", ts::filename::REMAP); - Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP_YAML); + CfgLoadInProgress(ctx, "%s loading ...", ts::filename::REMAP); Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP); - newTable = new UrlRewrite(); - bool status = newTable->load(); - bool is_yaml = (newTable->is_remap_yaml()); + UrlRewrite *newTable = new UrlRewrite(); - if (status) { - swoc::bwprint(msg_buffer, "{} finished loading", is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP); + bool status = newTable->load(ctx); + bool is_yaml = newTable->is_remap_yaml(); + const char *filename = is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP; - // Hold at least one lease, until we reload the configuration + if (status) { newTable->acquire(); - // Swap configurations - oldTable = rewrite_table.exchange(newTable); - + UrlRewrite *oldTable = rewrite_table.exchange(newTable); ink_assert(oldTable != nullptr); - - // Release the old one oldTable->release(); - Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); - Note("%s", msg_buffer.c_str()); - ctx.complete(msg_buffer); + CfgLoadComplete(ctx, "%s finished loading", filename); return true; } else { - swoc::bwprint(msg_buffer, "{} failed to load", is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP); - delete newTable; - Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); - Error("%s", msg_buffer.c_str()); - ctx.fail(msg_buffer); + CfgLoadFail(ctx, DL_Error, "%s failed to load", filename); return false; } } diff --git a/src/proxy/http/PreWarmConfig.cc b/src/proxy/http/PreWarmConfig.cc index 4adab6e2120..dbf79ce1021 100644 --- a/src/proxy/http/PreWarmConfig.cc +++ b/src/proxy/http/PreWarmConfig.cc @@ -58,7 +58,7 @@ PreWarmConfig::reconfigure(ConfigContext ctx) _config_id = configProcessor.set(_config_id, params); prewarmManager.reconfigure(); - ctx.complete("PreWarm config published."); + ctx.complete("PreWarm config published"); } PreWarmConfigParams * diff --git a/src/proxy/http/remap/RemapConfig.cc b/src/proxy/http/remap/RemapConfig.cc index bbefa27c68f..bb7bc915f5e 100644 --- a/src/proxy/http/remap/RemapConfig.cc +++ b/src/proxy/http/remap/RemapConfig.cc @@ -24,6 +24,7 @@ #include "proxy/http/remap/AclFiltering.h" #include "swoc/swoc_file.h" +#include "mgmt/config/ConfigContextDiags.h" #include "proxy/http/remap/RemapConfig.h" #include "proxy/http/remap/UrlRewrite.h" #include "proxy/ReverseProxy.h" @@ -1070,7 +1071,7 @@ process_regex_mapping_config(const char *from_host_lower, url_mapping *new_mappi } bool -remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti) +remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti, ConfigContext ctx) { char errBuf[1024]; char errStrBuf[1024]; @@ -1109,7 +1110,7 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti) return true; } if (ec.value()) { - Warning("Failed to open remapping configuration file %s - %s", path, strerror(ec.value())); + CfgLoadLog(ctx, DL_Warning, "Failed to open remapping configuration file %s - %s", path, strerror(ec.value())); return false; } @@ -1117,7 +1118,7 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti) ACLBehaviorPolicy behavior_policy = ACLBehaviorPolicy::ACL_BEHAVIOR_LEGACY; if (!UrlRewrite::get_acl_behavior_policy(behavior_policy)) { - Warning("Failed to get ACL matching policy."); + CfgLoadLog(ctx, DL_Warning, "Failed to get ACL matching policy."); return false; } bti->behavior_policy = behavior_policy; @@ -1538,7 +1539,7 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti) MAP_ERROR: snprintf(errBuf, sizeof(errBuf), "%s failed to add remap rule at %s line %d: %s", modulePrefix, path, cln + 1, errStr); - Error("%s", errBuf); + CfgLoadLog(ctx, DL_Error, "%s", errBuf); delete reg_map; delete new_mapping; @@ -1550,7 +1551,7 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti) } bool -remap_parse_config(const char *path, UrlRewrite *rewrite) +remap_parse_config(const char *path, UrlRewrite *rewrite, ConfigContext ctx) { BUILD_TABLE_INFO bti; @@ -1559,7 +1560,7 @@ remap_parse_config(const char *path, UrlRewrite *rewrite) rewrite->pluginFactory.indicatePreReload(); bti.rewrite = rewrite; - bool status = remap_parse_config_bti(path, &bti); + bool status = remap_parse_config_bti(path, &bti, ctx); /* Now after we parsed the configuration and (re)loaded plugins and plugin instances * accordingly notify all plugins that we are done */ diff --git a/src/proxy/http/remap/RemapYamlConfig.cc b/src/proxy/http/remap/RemapYamlConfig.cc index 04070030e76..7a441434463 100644 --- a/src/proxy/http/remap/RemapYamlConfig.cc +++ b/src/proxy/http/remap/RemapYamlConfig.cc @@ -23,6 +23,8 @@ #include "proxy/http/remap/RemapYamlConfig.h" +#include "mgmt/config/ConfigContextDiags.h" + #include #include #include @@ -962,7 +964,7 @@ parse_yaml_remap_rule(const YAML::Node &node, BUILD_TABLE_INFO *bti) } bool -remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti) +remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti, ConfigContext ctx) { try { Dbg(dbg_ctl_remap_yaml, "Parsing YAML config file: %s", path); @@ -978,7 +980,7 @@ remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti) ACLBehaviorPolicy behavior_policy = ACLBehaviorPolicy::ACL_BEHAVIOR_LEGACY; if (!UrlRewrite::get_acl_behavior_policy(behavior_policy)) { - Warning("Failed to get ACL matching policy."); + CfgLoadLog(ctx, DL_Warning, "Failed to get ACL matching policy."); return false; } bti->behavior_policy = behavior_policy; @@ -988,14 +990,14 @@ remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti) for (const auto &filter_def : config["acl_filters"]) { auto errata = parse_yaml_define_directive(filter_def, bti); if (!errata.is_ok()) { - Error("Failed to parse acl_filters section"); + CfgLoadLog(ctx, DL_Error, "Failed to parse acl_filters section"); return false; } } } if (config["remap"].IsNull() || !config["remap"].IsSequence()) { - Error("Expected toplevel 'remap' key to be a sequence"); + CfgLoadLog(ctx, DL_Error, "Expected toplevel 'remap' key to be a sequence"); return false; } @@ -1006,7 +1008,7 @@ remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti) auto errata = parse_yaml_remap_rule(rule, bti); if (!errata.is_ok()) { - Error("Failed to parse remap rule"); + CfgLoadLog(ctx, DL_Error, "Failed to parse remap rule"); return false; } } @@ -1017,15 +1019,15 @@ remap_parse_yaml_bti(const char *path, BUILD_TABLE_INFO *bti) return true; } catch (YAML::Exception &ex) { - Error("YAML parsing error in %s: %s", path, ex.what()); + CfgLoadLog(ctx, DL_Error, "YAML parsing error in %s: %s", path, ex.what()); } catch (std::exception &ex) { - Error("Exception parsing YAML config %s: %s", path, ex.what()); + CfgLoadLog(ctx, DL_Error, "Exception parsing YAML config %s: %s", path, ex.what()); } return false; } bool -remap_parse_yaml(const char *path, UrlRewrite *rewrite) +remap_parse_yaml(const char *path, UrlRewrite *rewrite, ConfigContext ctx) { BUILD_TABLE_INFO bti; @@ -1034,7 +1036,7 @@ remap_parse_yaml(const char *path, UrlRewrite *rewrite) rewrite->pluginFactory.indicatePreReload(); bti.rewrite = rewrite; - bool status = remap_parse_yaml_bti(path, &bti); + bool status = remap_parse_yaml_bti(path, &bti, ctx); /* Now after we parsed the configuration and (re)loaded plugins and plugin instances * accordingly notify all plugins that we are done */ diff --git a/src/proxy/http/remap/UrlRewrite.cc b/src/proxy/http/remap/UrlRewrite.cc index 77ace34441a..a8909a661d9 100644 --- a/src/proxy/http/remap/UrlRewrite.cc +++ b/src/proxy/http/remap/UrlRewrite.cc @@ -25,6 +25,7 @@ #include "proxy/http/remap/UrlRewrite.h" #include "proxy/http/remap/RemapYamlConfig.h" #include "iocore/eventsystem/ConfigProcessor.h" +#include "mgmt/config/ConfigContextDiags.h" #include "proxy/ReverseProxy.h" #include "tscore/Layout.h" #include "tscore/Filenames.h" @@ -76,7 +77,7 @@ UrlRewrite::get_acl_behavior_policy(ACLBehaviorPolicy &policy) } bool -UrlRewrite::load() +UrlRewrite::load(ConfigContext ctx) { ats_scoped_str config_file_path; @@ -85,13 +86,12 @@ UrlRewrite::load() this->_remap_yaml = true; if (!config_file_path || !swoc::file::exists(swoc::file::path(config_file_path))) { - Dbg(dbg_ctl_url_rewrite, "%s failed to load, fall back to %s", ts::filename::REMAP_YAML, ts::filename::REMAP); - Note("%s failed to load, fall back to %s", ts::filename::REMAP_YAML, ts::filename::REMAP); + CfgLoadLog(ctx, DL_Note, "%s failed to load, fall back to %s", ts::filename::REMAP_YAML, ts::filename::REMAP); // Fall back to remap.config if remap.yaml not found this->_remap_yaml = false; config_file_path = RecConfigReadConfigPath("proxy.config.url_remap.filename", ts::filename::REMAP); if (!config_file_path) { - Warning("%s Unable to locate %s. No remappings in effect", modulePrefix, ts::filename::REMAP); + CfgLoadLog(ctx, DL_Warning, "%s Unable to locate %s. No remappings in effect", modulePrefix, ts::filename::REMAP); return false; } } @@ -101,7 +101,7 @@ UrlRewrite::load() this->ts_name = ats_stringdup(rec_str); } if (this->ts_name == nullptr) { - Warning("%s Unable to determine proxy name. Incorrect redirects could be generated", modulePrefix); + CfgLoadLog(ctx, DL_Warning, "%s Unable to determine proxy name. Incorrect redirects could be generated", modulePrefix); this->ts_name = ats_strdup(""); } @@ -110,7 +110,7 @@ UrlRewrite::load() this->http_default_redirect_url = ats_stringdup(rec_str); } if (this->http_default_redirect_url == nullptr) { - Warning("%s Unable to determine default redirect url for \"referer\" filter.", modulePrefix); + CfgLoadLog(ctx, DL_Warning, "%s Unable to determine default redirect url for \"referer\" filter.", modulePrefix); this->http_default_redirect_url = ats_strdup("http://www.apache.org"); } @@ -130,7 +130,7 @@ UrlRewrite::load() fs::file_status status = fs::status(compilerPath, ec); if (ec || !swoc::file::is_regular_file(status)) { - Error("Configured plugin compiler path '%s' is not a regular file", buf); + CfgLoadFail(ctx, DL_Error, "Configured plugin compiler path '%s' is not a regular file", buf); return false; } else { // This also adds the configuration directory (etc/trafficserver) to find Cripts etc. @@ -143,7 +143,7 @@ UrlRewrite::load() Dbg(dbg_ctl_url_rewrite_regex, "strategyFactory file: %s", sf.c_str()); strategyFactory = new NextHopStrategyFactory(sf.c_str()); - if (TS_SUCCESS == this->BuildTable(config_file_path)) { + if (TS_SUCCESS == this->BuildTable(config_file_path, ctx)) { int n_rules = this->rule_count(); // Minimum # of rules to be considered a valid configuration. int required_rules; required_rules = RecGetRecordInt("proxy.config.url_remap.min_rules_required").value_or(0); @@ -154,10 +154,11 @@ UrlRewrite::load() Print(); } } else { - Warning("%s %d rules defined but %d rules required, configuration is invalid.", modulePrefix, n_rules, required_rules); + CfgLoadLog(ctx, DL_Warning, "%s %d rules defined but %d rules required, configuration is invalid.", modulePrefix, n_rules, + required_rules); } } else { - Warning("something failed during BuildTable() -- check your remap plugins!"); + CfgLoadLog(ctx, DL_Warning, "something failed during BuildTable() -- check your remap plugins!"); } // ACL Matching Policy @@ -816,7 +817,7 @@ UrlRewrite::InsertForwardMapping(mapping_type maptype, url_mapping *mapping, con */ int -UrlRewrite::BuildTable(const char *path) +UrlRewrite::BuildTable(const char *path, ConfigContext ctx) { ink_assert(forward_mappings.empty()); ink_assert(reverse_mappings.empty()); @@ -837,9 +838,9 @@ UrlRewrite::BuildTable(const char *path) bool parse_success; if (is_remap_yaml()) { - parse_success = remap_parse_yaml(path, this); + parse_success = remap_parse_yaml(path, this, ctx); } else { - parse_success = remap_parse_config(path, this); + parse_success = remap_parse_config(path, this, ctx); } if (!parse_success) { diff --git a/src/proxy/logging/LogConfig.cc b/src/proxy/logging/LogConfig.cc index 9af71a7e407..e6129a83ad4 100644 --- a/src/proxy/logging/LogConfig.cc +++ b/src/proxy/logging/LogConfig.cc @@ -48,6 +48,7 @@ using namespace std::literals; #include "tscore/SimpleTokenizer.h" #include "proxy/logging/YamlLogConfig.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" #define DISK_IS_CONFIG_FULL_MESSAGE \ @@ -422,7 +423,7 @@ LogConfig::setup_log_objects() void LogConfig::reconfigure(ConfigContext ctx) { - Dbg(dbg_ctl_log_config, "Reconfiguration request accepted"); + CfgLoadDbg(ctx, dbg_ctl_log_config, "Reconfiguration request accepted"); Log::config->reconfiguration_needed = true; Log::config->reload_ctx = ctx; @@ -774,8 +775,7 @@ LogConfig::evaluate_config() ats_scoped_str path(RecConfigReadConfigPath("proxy.config.log.config.filename", ts::filename::LOGGING)); struct stat sbuf; if (stat(path.get(), &sbuf) == -1 && errno == ENOENT) { - Warning("logging configuration '%s' doesn't exist", path.get()); - reload_ctx.fail("logging configuration '{}' doesn't exist", path.get()); + CfgLoadFail(reload_ctx, DL_Warning, "logging configuration '%s' doesn't exist", path.get()); return false; } @@ -784,11 +784,9 @@ LogConfig::evaluate_config() bool zret = y.parse(path.get()); if (zret) { - Note("%s finished loading", path.get()); - reload_ctx.complete("{} finished loading", path.get()); + CfgLoadComplete(reload_ctx, "%s finished loading", path.get()); } else { - Note("%s failed to load", path.get()); - reload_ctx.fail("{} failed to load", path.get()); + CfgLoadFail(reload_ctx, DL_Error, "%s failed to load", path.get()); } return zret; diff --git a/src/proxy/logging/YamlLogConfig.cc b/src/proxy/logging/YamlLogConfig.cc index e554e8f9666..a9d4327f208 100644 --- a/src/proxy/logging/YamlLogConfig.cc +++ b/src/proxy/logging/YamlLogConfig.cc @@ -22,6 +22,7 @@ #include "proxy/logging/YamlLogConfig.h" #include "proxy/logging/YamlLogConfigDecoders.h" +#include "mgmt/config/ConfigContextDiags.h" #include "proxy/logging/LogConfig.h" #include "proxy/logging/LogObject.h" @@ -45,7 +46,7 @@ YamlLogConfig::parse(const char *cfgFilename) try { result = loadLogConfig(cfgFilename); } catch (std::exception &ex) { - Error("%s", ex.what()); + CfgLoadFail(cfg->reload_ctx, DL_Error, "%s", ex.what()); result = false; } return result; @@ -61,14 +62,14 @@ YamlLogConfig::loadLogConfig(const char *cfgFilename) } if (!config.IsMap()) { - Error("malformed %s file; expected a map", cfgFilename); + CfgLoadFail(cfg->reload_ctx, DL_Error, "malformed %s file; expected a map", cfgFilename); return false; } if (config["logging"]) { config = config["logging"]; } else { - Error("malformed %s file; expected a toplevel 'logging' node", cfgFilename); + CfgLoadFail(cfg->reload_ctx, DL_Error, "malformed %s file; expected a toplevel 'logging' node", cfgFilename); return false; } diff --git a/src/records/unit_tests/test_ConfigReloadTask.cc b/src/records/unit_tests/test_ConfigReloadTask.cc index 9fa1704b31c..9cc1acdb107 100644 --- a/src/records/unit_tests/test_ConfigReloadTask.cc +++ b/src/records/unit_tests/test_ConfigReloadTask.cc @@ -92,7 +92,7 @@ TEST_CASE("ConfigReloadTask state transitions", "[config][reload][state]") // Verify logs contain the reason auto logs = task->get_logs(); REQUIRE(!logs.empty()); - REQUIRE(logs.back().find("Test timeout") != std::string::npos); + REQUIRE(logs.back().text.find("Test timeout") != std::string::npos); } SECTION("Terminal states cannot be changed via mark_as_bad_state") @@ -109,8 +109,8 @@ TEST_CASE("ConfigReloadTask state transitions", "[config][reload][state]") // Verify the rejected reason was NOT added to logs auto logs = task->get_logs(); - for (const auto &log : logs) { - REQUIRE(log.find("Should not apply") == std::string::npos); + for (const auto &entry : logs) { + REQUIRE(entry.text.find("Should not apply") == std::string::npos); } } diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index 659bf4f1e7d..ec5d34baa8a 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -256,8 +256,9 @@ ConfigCommand::config_diff() void ConfigCommand::config_status() { - std::string token = get_parsed_arguments()->get("token").value(); - std::string count = get_parsed_arguments()->get("count").value(); + std::string token = get_parsed_arguments()->get("token").value(); + std::string count = get_parsed_arguments()->get("count").value(); + std::string min_level = get_parsed_arguments()->get("min-level").value(); if (!count.empty() && !token.empty()) { // can't use both. @@ -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 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()->set_min_level(it->second); + } else { + _printer->write_output("Invalid --min-level value. Use: debug, note, warning, error"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + } + auto resp = fetch_config_reload(token, count); if (resp.error.size()) { diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index 8e82fe77a9b..e4851c97115 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -308,7 +308,7 @@ dot_fill(int width) // @param content_width visual columns available for icon+name+dots+duration (shrinks per nesting) void print_task_tree(const ConfigReloadResponse::ReloadInfo &f, bool full_report, const std::string &prefix, - const std::string &child_prefix, int content_width = 55) + const std::string &child_prefix, DiagsLevel min_level = DL_Undefined, int content_width = 55) { std::string fname; if (f.filename.empty() || f.filename == "") { @@ -348,8 +348,19 @@ print_task_tree(const ConfigReloadResponse::ReloadInfo &f, bool full_report, con // Log lines: indented under the task, with tree continuation line if children follow. if (full_report && !f.logs.empty()) { std::string log_pfx = has_children ? (child_prefix + "\xe2\x94\x82 ") : (child_prefix + " "); - for (const auto &log : f.logs) { - std::cout << log_pfx << log << '\n'; + for (const auto &entry : f.logs) { + if (min_level != DL_Undefined && entry.level != DL_Undefined && entry.level < min_level) { + continue; + } + std::cout << log_pfx; + if (entry.level != DL_Undefined) { + static constexpr const char *severity_tags[] = { + "[Diag] ", "[Dbg] ", "[Stat] ", "[Note] ", "[Warn] ", "[Err] ", "[Fatal] ", "[Alert] ", "[Emrg] ", + }; + int idx = std::min(std::max(static_cast(entry.level), 0), static_cast(DL_Emergency)); + std::cout << severity_tags[idx]; + } + std::cout << entry.text << '\n'; } } @@ -358,7 +369,7 @@ print_task_tree(const ConfigReloadResponse::ReloadInfo &f, bool full_report, con bool is_last = (i == f.sub_tasks.size() - 1); std::string sub_prefix = child_prefix + (is_last ? "\xe2\x94\x94\xe2\x94\x80 " : "\xe2\x94\x9c\xe2\x94\x80 "); std::string sub_child_prefix = child_prefix + (is_last ? " " : "\xe2\x94\x82 "); - print_task_tree(f.sub_tasks[i], full_report, sub_prefix, sub_child_prefix, content_width - 3); + print_task_tree(f.sub_tasks[i], full_report, sub_prefix, sub_child_prefix, min_level, content_width - 3); } } @@ -454,7 +465,7 @@ ConfigReloadPrinter::print_reload_report(const ConfigReloadResponse::ReloadInfo } const std::string base_prefix(" "); for (const auto &sub : info.sub_tasks) { - print_task_tree(sub, full_report, base_prefix, base_prefix); + print_task_tree(sub, full_report, base_prefix, base_prefix, _min_level); } } diff --git a/src/traffic_ctl/CtrlPrinters.h b/src/traffic_ctl/CtrlPrinters.h index 050cb705e30..dcae30837f8 100644 --- a/src/traffic_ctl/CtrlPrinters.h +++ b/src/traffic_ctl/CtrlPrinters.h @@ -225,9 +225,22 @@ class ConfigReloadPrinter : public BasePrinter { void write_output(YAML::Node const &result) override; + DiagsLevel _min_level{DL_Undefined}; + public: ConfigReloadPrinter(BasePrinter::Options opt) : BasePrinter(opt) {} + void + set_min_level(DiagsLevel level) + { + _min_level = level; + } + DiagsLevel + min_level() const + { + return _min_level; + } + void print_reload_report(const ConfigReloadResponse::ReloadInfo &info, bool full_report = false); void write_progress_line(const ConfigReloadResponse::ReloadInfo &info); }; diff --git a/src/traffic_ctl/jsonrpc/CtrlRPCRequests.h b/src/traffic_ctl/jsonrpc/CtrlRPCRequests.h index 5644f288a17..f33218c61fd 100644 --- a/src/traffic_ctl/jsonrpc/CtrlRPCRequests.h +++ b/src/traffic_ctl/jsonrpc/CtrlRPCRequests.h @@ -23,6 +23,7 @@ // We base on the common client types. #include "shared/rpc/RPCRequests.h" +#include "tsutil/ts_diag_levels.h" #include @@ -61,13 +62,18 @@ struct ConfigReloadRequest : shared::rpc::ClientRequest { // Full list of reload tasks, could be nested. struct ConfigReloadResponse { // Existing reload task info, could be nested. + struct LogEntry { + DiagsLevel level{DL_Undefined}; ///< DL_Undefined for state-change messages + std::string text; + }; + struct ReloadInfo { - std::string config_token; - std::string status; - std::string description; - std::string filename; - std::vector logs; - std::vector sub_tasks; + std::string config_token; + std::string status; + std::string description; + std::string filename; + std::vector logs; + std::vector sub_tasks; struct Meta { // internal info. int64_t created_time_ms{0}; int64_t last_updated_time_ms{0}; diff --git a/src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h b/src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h index d128915e7d9..40daec816c3 100644 --- a/src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h +++ b/src/traffic_ctl/jsonrpc/ctrl_yaml_codecs.h @@ -103,7 +103,14 @@ template <> struct convert { info.description = helper::try_extract(from, "description", false, std::string{""}); info.filename = helper::try_extract(from, "filename", false, std::string{""}); for (auto &&log : from["logs"]) { - info.logs.push_back(log.as()); + if (log.IsMap()) { + ConfigReloadResponse::LogEntry entry; + entry.level = static_cast(log["level"].as(DL_Undefined)); + entry.text = log["text"].as(); + info.logs.push_back(entry); + } else { + info.logs.push_back({DL_Undefined, log.as()}); + } } for (auto &&sub : from["sub_tasks"]) { diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index 9ca08ef8ad8..bacbf78eec4 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -177,6 +177,7 @@ main([[maybe_unused]] int argc, const char **argv) config_command.add_command("status", "Check the configuration status", [&]() { command->execute(); }) .add_option("--token", "-t", "Configuration token to check status.", "", 1, "") .add_option("--count", "-c", "Number of status records to return. Use numeric or 'all' to get the full history", "", 1, "") + .add_option("--min-level", "", "Minimum severity level for log entries: debug, note, warning, error", "", 1, "") .add_example_usage("traffic_ctl config status"); config_command.add_command("set", "Set a configuration value", "", 2, Command_Execute) .add_option("--cold", "-c", diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 10945ecf246..aa4587f2793 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -92,8 +92,8 @@ extern "C" int plock(int); #include "iocore/eventsystem/RecProcess.h" #include "proxy/Transform.h" #include "iocore/eventsystem/ConfigProcessor.h" +#include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" -#include "mgmt/config/ConfigContext.h" #include "proxy/http/HttpProxyServerMain.h" #include "proxy/http/HttpBodyFactory.h" #include "proxy/ProxySession.h" @@ -767,11 +767,11 @@ register_config_files() } else { ctx.log("{}", zret); if (zret.severity() >= ERRATA_ERROR) { - ctx.fail("Failed to reload records.yaml"); + CfgLoadFail(ctx, DL_Error, "Failed to reload %s", ts::filename::RECORDS); return; } } - ctx.complete(); + CfgLoadComplete(ctx, "%s finished loading", ts::filename::RECORDS); }, ConfigSource::FileOnly); From b730cd099a2018788953e220fc64a771619447d9 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 15 Apr 2026 11:41:42 +0200 Subject: [PATCH 2/6] cleanup --- .../command-line/traffic_ctl.en.rst | 2 - .../config-reload-framework.en.rst | 41 ++++++++-------- include/mgmt/config/ConfigContextDiags.h | 47 ++++++++++--------- src/iocore/cache/CacheHosting.cc | 4 +- src/iocore/dns/SplitDNS.cc | 4 +- src/iocore/net/QUICMultiCertConfigLoader.cc | 4 +- src/iocore/net/SSLConfig.cc | 12 ++--- src/iocore/net/SSLSNIConfig.cc | 10 ++-- src/proxy/CacheControl.cc | 2 +- src/proxy/ControlMatcher.cc | 2 +- src/proxy/IPAllow.cc | 8 ++-- src/proxy/ParentSelection.cc | 2 +- src/proxy/ReverseProxy.cc | 34 ++++++++++---- src/proxy/http/remap/UrlRewrite.cc | 2 +- src/proxy/logging/LogConfig.cc | 5 +- src/proxy/logging/YamlLogConfig.cc | 6 +-- src/traffic_ctl/CtrlPrinters.cc | 2 + src/traffic_server/traffic_server.cc | 2 +- 18 files changed, 106 insertions(+), 83 deletions(-) diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 9bd269b5dc4..4308e074775 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -678,7 +678,6 @@ Display the current value of a configuration record. │ [Note] SSL configs reloaded ├─ ✔ SSLConfig ·························· 10ms │ [Note] SSLConfig loading ... - │ [Dbg] Reload SSLConfig │ [Note] SSLConfig reloaded ├─ ✗ SNIConfig ·························· 12ms ✗ FAIL │ [Note] sni.yaml loading ... @@ -739,7 +738,6 @@ Display the current value of a configuration record. │ [Note] SSL configs reloaded ├─ ✔ SSLConfig ································· 1ms │ [Note] SSLConfig loading ... - │ [Dbg] Reload SSLConfig │ [Note] SSLConfig reloaded ├─ ✗ SNIConfig ································· 1ms ✗ FAIL │ [Note] sni.yaml loading ... diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index f38a115e437..fb428b5351e 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -699,17 +699,20 @@ Quick Reference - Want in task log? - Use * - Note - - yes - - ``CfgLoadInProgress`` / ``CfgLoadComplete`` - * - Error / Warning - - yes - - ``CfgLoadFail(ctx, DL_xxx, ...)`` + - yes + in_progress + - ``CfgLoadInProgress(ctx, ...)`` (subtasks) + * - Note + - yes + complete + - ``CfgLoadComplete(ctx, ...)`` + * - Error + - yes + fail + - ``CfgLoadFail(ctx, ...)`` * - Error + Errata - yes + fail - - ``CfgLoadFailWithErrata(ctx, ...)`` + - ``CfgLoadFailWithErrata(ctx, errata, ...)`` * - Note / Warning - yes (no state change) - - ``CfgLoadLog(ctx, DL_xxx, ...)`` + - ``CfgLoadLog(ctx, DL_Note|DL_Warning, ...)`` * - Dbg (conditional on tag) - yes - ``CfgLoadDbg(ctx, ctl, ...)`` @@ -727,8 +730,9 @@ Macro Details ------------- ``CfgLoadInProgress(ctx, fmt, ...)`` - Emits a ``Note()`` to ``diags.log`` and calls ``ctx.in_progress(msg)``. Use at the start - of a config load/reload: + 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 @@ -742,17 +746,16 @@ Macro Details CfgLoadComplete(ctx, "%s finished loading", filename); -``CfgLoadFail(ctx, level, fmt, ...)`` - Emits at the given ``DiagsLevel`` (typically ``DL_Error`` or ``DL_Warning``) to both - ``diags.log`` and the task log, then marks the task as FAIL (state-only, no additional - log entry): +``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, DL_Error, "%s failed to load", filename); - CfgLoadFail(ctx, DL_Warning, "No NAMEDs provided for %s", filename); + CfgLoadFail(ctx, "%s failed to load", filename); -``CfgLoadFailWithErrata(ctx, level, errata, fmt, ...)`` +``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. @@ -784,9 +787,9 @@ the diags summary, errata detail, and state change in a single call: .. code-block:: cpp - CfgLoadFailWithErrata(ctx, DL_Error, errata, "%s failed to load", filename); + CfgLoadFailWithErrata(ctx, errata, "%s failed to load", filename); -This logs the formatted message to ``diags.log`` at the given severity, appends it to +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. @@ -826,7 +829,6 @@ with a tag: │ [Note] SSL configs reloaded ├─ ✔ SSLConfig ································· 1ms │ [Note] SSLConfig loading ... - │ [Dbg] Reload SSLConfig │ [Note] SSLConfig reloaded ├─ ✗ SNIConfig ································· 1ms ✗ FAIL │ [Note] sni.yaml loading ... @@ -873,7 +875,6 @@ log entries is written to ``traffic.out`` / ``diags.log``: DIAG: (config.reload) [Note] SSL configs reloaded DIAG: (config.reload) [success] SSLConfig DIAG: (config.reload) [Note] SSLConfig loading ... - DIAG: (config.reload) [Dbg] Reload SSLConfig DIAG: (config.reload) [Note] SSLConfig reloaded DIAG: (config.reload) [fail] SNIConfig DIAG: (config.reload) [Note] sni.yaml loading ... diff --git a/include/mgmt/config/ConfigContextDiags.h b/include/mgmt/config/ConfigContextDiags.h index fbebef38827..96c09c5df5a 100644 --- a/include/mgmt/config/ConfigContextDiags.h +++ b/include/mgmt/config/ConfigContextDiags.h @@ -17,9 +17,10 @@ This is the common case for operational messages in config handlers: @code - CfgLoadInProgress(ctx, "%s loading ...", filename); + CfgLoadInProgress(ctx, "%s loading ...", filename); // subtasks + CfgLoadLog(ctx, DL_Note, "%s loading ...", filename); // top-level handlers CfgLoadComplete(ctx, "%s finished loading", filename); - CfgLoadFail(ctx, DL_Error, "%s failed to load", filename); + CfgLoadFail(ctx, "%s failed to load", filename); @endcode Use ctx methods directly when you only want the reload task log (no diags): @@ -46,10 +47,11 @@ | Want diags? | Want task log? | Use | |-------------|----------------|-----------------------------------| - | Note | yes | CfgLoadInProgress / Complete | - | Error/Warn | yes | CfgLoadFail(ctx, DL_xxx, ...) | - | Err + Errata| yes + fail | CfgLoadFailWithErrata(...) | - | Note/Warn | yes (no state) | CfgLoadLog(ctx, DL_xxx, ...) | + | Note | yes + in_progress| CfgLoadInProgress(ctx, ...) | + | Note | yes + complete | CfgLoadComplete(ctx, ...) | + | Error | yes + fail | CfgLoadFail(ctx, ...) | + | Err + Errata| yes + fail | CfgLoadFailWithErrata(...) | + | Note/Warn | yes (no state) | CfgLoadLog(ctx, DL_xxx, ...) | | Dbg(tag) | yes | CfgLoadDbg(ctx, ctl, ...) | | no | yes | ctx.log(...) | | no | yes + state | ctx.complete() / ctx.fail() | @@ -61,7 +63,7 @@ combine the diags summary, errata detail, and state change in one call: @code - CfgLoadFailWithErrata(ctx, DL_Error, errata, "%s failed to load", filename); + CfgLoadFailWithErrata(ctx, errata, "%s failed to load", filename); @endcode This logs the formatted message to diags and the task log at the given @@ -98,7 +100,9 @@ #include "tscore/Diags.h" /// Log a Note and mark the context as IN_PROGRESS. -/// Use at the start of a config load/reload operation. +/// The framework sets IN_PROGRESS on handler tasks automatically, so +/// CfgLoadLog(ctx, DL_Note, ...) is preferred for top-level "loading..." +/// messages. Use this macro for subtasks created via add_dependent_ctx(). /// /// CfgLoadInProgress(ctx, "%s loading ...", ts::filename::IP_ALLOW); /// @@ -123,32 +127,33 @@ (CTX).complete(_cfgctx_buf); \ } while (false) -/// Log at the given DiagsLevel and mark the context as FAIL. -/// Use when a config load/reload operation fails. +/// Log an Error and mark the context as FAIL. +/// Use when a config load/reload operation fails. Fail always implies +/// DL_Error — if the condition is merely degraded (not fatal to the load), +/// use CfgLoadLog(ctx, DL_Warning, ...) + CfgLoadComplete() instead. /// -/// CfgLoadFail(ctx, DL_Error, "%s failed to load", ts::filename::IP_ALLOW); -/// CfgLoadFail(ctx, DL_Warning, "No NAMEDs provided for %s", ts::filename::SPLITDNS); +/// CfgLoadFail(ctx, "%s failed to load", ts::filename::IP_ALLOW); /// -#define CfgLoadFail(CTX, LEVEL, FMT, ...) \ +#define CfgLoadFail(CTX, FMT, ...) \ do { \ char _cfgctx_buf[1024]; \ snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ - DiagsError(LEVEL, "%s", _cfgctx_buf); \ - (CTX).log(LEVEL, _cfgctx_buf); \ + DiagsError(DL_Error, "%s", _cfgctx_buf); \ + (CTX).log(DL_Error, _cfgctx_buf); \ (CTX).fail(); \ } while (false) -/// Log at the given DiagsLevel, append errata detail to the task log, and mark -/// the context as FAIL. Combines CfgLoadFail + ctx.fail(errata) in one call. +/// Log an Error, append errata detail to the task log, and mark the context +/// as FAIL. Combines CfgLoadFail + ctx.fail(errata) in one call. /// -/// CfgLoadFailWithErrata(ctx, DL_Error, errata, "%s failed to load", filename); +/// CfgLoadFailWithErrata(ctx, errata, "%s failed to load", filename); /// -#define CfgLoadFailWithErrata(CTX, LEVEL, ERRATA, FMT, ...) \ +#define CfgLoadFailWithErrata(CTX, ERRATA, FMT, ...) \ do { \ char _cfgctx_buf[1024]; \ snprintf(_cfgctx_buf, sizeof(_cfgctx_buf), FMT, ##__VA_ARGS__); \ - DiagsError(LEVEL, "%s", _cfgctx_buf); \ - (CTX).log(LEVEL, _cfgctx_buf); \ + DiagsError(DL_Error, "%s", _cfgctx_buf); \ + (CTX).log(DL_Error, _cfgctx_buf); \ (CTX).fail(ERRATA); \ } while (false) diff --git a/src/iocore/cache/CacheHosting.cc b/src/iocore/cache/CacheHosting.cc index 1a36f34f482..ce4f9a990bf 100644 --- a/src/iocore/cache/CacheHosting.cc +++ b/src/iocore/cache/CacheHosting.cc @@ -239,7 +239,7 @@ int fstat_wrapper(int fd, struct stat *s); int CacheHostTable::BuildTableFromString(const char *config_file_path, char *file_buf, ConfigContext ctx) { - CfgLoadInProgress(ctx, "%s loading ...", ts::filename::HOSTING); + CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::HOSTING); // Table build locals Tokenizer bufTok("\n"); @@ -398,7 +398,7 @@ CacheHostTable::BuildTable(const char *config_file_path, ConfigContext ctx) CfgLoadLog(ctx, DL_Warning, "Cannot open the config file: %s - %s", config_file_path, strerror(ec.value())); break; default: - CfgLoadFail(ctx, DL_Error, "%s failed to load: %s", config_file_path, strerror(ec.value())); + CfgLoadFail(ctx, "%s failed to load: %s", config_file_path, strerror(ec.value())); gen_host_rec.Init(type); return 0; } diff --git a/src/iocore/dns/SplitDNS.cc b/src/iocore/dns/SplitDNS.cc index bb017269962..1302b587ac5 100644 --- a/src/iocore/dns/SplitDNS.cc +++ b/src/iocore/dns/SplitDNS.cc @@ -137,7 +137,7 @@ SplitDNSConfig::reconfigure(ConfigContext ctx) return; } - CfgLoadInProgress(ctx, "%s loading ...", ts::filename::SPLITDNS); + CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::SPLITDNS); SplitDNS *params = new SplitDNS; @@ -149,7 +149,7 @@ SplitDNSConfig::reconfigure(ConfigContext ctx) if (nullptr == params->m_DNSSrvrTable || (0 == params->m_DNSSrvrTable->getEntryCount())) { gsplit_dns_enabled = 0; delete params; - CfgLoadFail(ctx, DL_Warning, "Failed to load %s - No NAMEDs provided! Disabling SplitDNS", ts::filename::SPLITDNS); + CfgLoadFail(ctx, "Failed to load %s - No NAMEDs provided! Disabling SplitDNS", ts::filename::SPLITDNS); return; } params->m_numEle = params->m_DNSSrvrTable->getEntryCount(); diff --git a/src/iocore/net/QUICMultiCertConfigLoader.cc b/src/iocore/net/QUICMultiCertConfigLoader.cc index 04d56c47542..2e7049643e8 100644 --- a/src/iocore/net/QUICMultiCertConfigLoader.cc +++ b/src/iocore/net/QUICMultiCertConfigLoader.cc @@ -45,6 +45,8 @@ QUICCertConfig::reconfigure(ConfigContext ctx) SSLConfig::scoped_config params; SSLCertLookup *lookup = new SSLCertLookup(); + CfgLoadInProgress(ctx, "(quic) %s loading ...", params->configFilePath); + QUICMultiCertConfigLoader loader(params); auto errata = loader.load(lookup, _config_id == 0); if (!lookup->is_valid || (errata.has_severity() && errata.severity() >= ERRATA_ERROR)) { @@ -64,7 +66,7 @@ QUICCertConfig::reconfigure(ConfigContext ctx) if (retStatus) { CfgLoadComplete(ctx, "(quic) %s finished loading", params->configFilePath); } else { - CfgLoadFail(ctx, DL_Error, "(quic) %s failed to load", params->configFilePath); + CfgLoadFail(ctx, "(quic) %s failed to load", params->configFilePath); } } diff --git a/src/iocore/net/SSLConfig.cc b/src/iocore/net/SSLConfig.cc index 4a0b6fdad96..bd3cd560c74 100644 --- a/src/iocore/net/SSLConfig.cc +++ b/src/iocore/net/SSLConfig.cc @@ -572,7 +572,8 @@ SSLConfigParams::initialize(ConfigContext ctx) if (this->clientCertExitOnLoadError) { Emergency("Can't initialize the SSL client, HTTPS in remap rules will not function"); } else { - CfgLoadFail(ctx, DL_Error, "Can't initialize the SSL client, HTTPS in remap rules will not function"); + SSLError("Can't initialize the SSL client, HTTPS in remap rules will not function"); + CfgLoadFail(ctx, "Can't initialize the SSL client, HTTPS in remap rules will not function"); } } @@ -615,8 +616,7 @@ SSLConfig::startup() void SSLConfig::reconfigure(ConfigContext ctx) { - CfgLoadLog(ctx, DL_Note, "SSLConfig loading ..."); - CfgLoadDbg(ctx, dbg_ctl_ssl_load, "Reload SSLConfig"); + CfgLoadInProgress(ctx, "SSLConfig loading ..."); SSLConfigParams *params = new SSLConfigParams; // start loading the next config @@ -673,7 +673,7 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx) SSLConfig::scoped_config params; SSLCertLookup *lookup = new SSLCertLookup(); - CfgLoadLog(ctx, DL_Note, "(ssl) %s loading ...", params->configFilePath); + CfgLoadInProgress(ctx, "(ssl) %s loading ...", params->configFilePath); // Test SSL certificate loading startup. With large numbers of certificates, reloading can take time, so delay // twice the healthcheck period to simulate a loading a large certificate set. @@ -703,7 +703,7 @@ SSLCertificateConfig::reconfigure(ConfigContext ctx) if (retStatus) { CfgLoadComplete(ctx, "(ssl) %s finished loading", params->configFilePath); } else { - CfgLoadFail(ctx, DL_Error, "(ssl) %s failed to load", params->configFilePath); + CfgLoadFail(ctx, "(ssl) %s failed to load", params->configFilePath); } return retStatus; @@ -771,7 +771,7 @@ SSLTicketParams::LoadTicket(bool &nochange, ConfigContext ctx) return true; } if (!keyblock) { - CfgLoadFail(ctx, DL_Error, "Could not load ticket key from %s", ticket_key_filename); + CfgLoadFail(ctx, "Could not load ticket key from %s", ticket_key_filename); return false; } default_global_keyblock = keyblock; diff --git a/src/iocore/net/SSLSNIConfig.cc b/src/iocore/net/SSLSNIConfig.cc index 6607e08d757..ce72e9d4bac 100644 --- a/src/iocore/net/SSLSNIConfig.cc +++ b/src/iocore/net/SSLSNIConfig.cc @@ -288,7 +288,7 @@ SNIConfigParams::initialize(std::string const &sni_filename, ConfigContext ctx) if (TSSystemState::is_initializing()) { Emergency("%s failed to load: %s", sni_filename.c_str(), errMsg.str().c_str()); } else { - CfgLoadFail(ctx, DL_Error, "%s failed to load: %s", sni_filename.c_str(), errMsg.str().c_str()); + CfgLoadFail(ctx, "%s failed to load: %s", sni_filename.c_str(), errMsg.str().c_str()); } return false; } @@ -322,18 +322,18 @@ SNIConfig::reconfigure(ConfigContext ctx) { SNIConfigParams *params = new SNIConfigParams; - bool retStatus = params->initialize(ctx); + std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); + bool retStatus = params->initialize(sni_filename, ctx); + if (retStatus) { _configid = configProcessor.set(_configid, params); if (SNIConfig::on_reconfigure) { SNIConfig::on_reconfigure(); } - std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); CfgLoadComplete(ctx, "%s finished loading", sni_filename.c_str()); } else { delete params; - std::string sni_filename = RecConfigReadConfigPath("proxy.config.ssl.servername.filename"); - CfgLoadFail(ctx, DL_Error, "%s failed to load", sni_filename.c_str()); + CfgLoadFail(ctx, "%s failed to load", sni_filename.c_str()); } return retStatus ? 1 : 0; diff --git a/src/proxy/CacheControl.cc b/src/proxy/CacheControl.cc index 3889c6887dc..12fff112537 100644 --- a/src/proxy/CacheControl.cc +++ b/src/proxy/CacheControl.cc @@ -126,7 +126,7 @@ initCacheControl() void reloadCacheControl(ConfigContext ctx) { - CfgLoadInProgress(ctx, "%s loading ...", ts::filename::CACHE); + CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::CACHE); Dbg(dbg_ctl_cache_control, "%s updated, reloading", ts::filename::CACHE); eventProcessor.schedule_in(new CC_FreerContinuation(CacheControlTable), CACHE_CONTROL_TIMEOUT, ET_CALL); diff --git a/src/proxy/ControlMatcher.cc b/src/proxy/ControlMatcher.cc index 4c5271bf304..4be7cf1880a 100644 --- a/src/proxy/ControlMatcher.cc +++ b/src/proxy/ControlMatcher.cc @@ -906,7 +906,7 @@ ControlMatcher::BuildTable(ConfigContext ctx) CfgLoadLog(ctx, DL_Warning, "ControlMatcher - Cannot open config file: %s - %s", config_file_path, strerror(ec.value())); break; default: - CfgLoadFail(ctx, DL_Error, "ControlMatcher - %s failed to load: %s", config_file_path, strerror(ec.value())); + CfgLoadFail(ctx, "ControlMatcher - %s failed to load: %s", config_file_path, strerror(ec.value())); return 1; } } diff --git a/src/proxy/IPAllow.cc b/src/proxy/IPAllow.cc index 6cb0815ed59..dc96c4a7fa6 100644 --- a/src/proxy/IPAllow.cc +++ b/src/proxy/IPAllow.cc @@ -124,14 +124,13 @@ IpAllow::startup() void IpAllow::reconfigure(ConfigContext ctx) { - CfgLoadInProgress(ctx, "%s loading ...", ts::filename::IP_ALLOW); + CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::IP_ALLOW); auto *new_table = new self_type("proxy.config.cache.ip_allow.filename", "proxy.config.cache.ip_categories.filename"); // IP rules need categories, so load them first (if they exist). if (auto errata = new_table->BuildCategories(); !errata.is_ok()) { - CfgLoadFail(ctx, DL_Error, "%s failed to load", new_table->ip_categories_config_file.c_str()); - ctx.fail(errata); + CfgLoadFailWithErrata(ctx, errata, "%s failed to load", new_table->ip_categories_config_file.c_str()); delete new_table; return; } @@ -139,8 +138,7 @@ IpAllow::reconfigure(ConfigContext ctx) if (errata.severity() > ERRATA_ERROR) { Fatal("%s failed to load", ts::filename::IP_ALLOW); } - CfgLoadFail(ctx, DL_Error, "%s failed to load", ts::filename::IP_ALLOW); - ctx.fail(errata); + CfgLoadFailWithErrata(ctx, errata, "%s failed to load", ts::filename::IP_ALLOW); delete new_table; return; } diff --git a/src/proxy/ParentSelection.cc b/src/proxy/ParentSelection.cc index 60a3b3a5f2b..c0070f195be 100644 --- a/src/proxy/ParentSelection.cc +++ b/src/proxy/ParentSelection.cc @@ -304,7 +304,7 @@ ParentConfig::startup() void ParentConfig::reconfigure(ConfigContext ctx) { - CfgLoadInProgress(ctx, "%s loading ...", ts::filename::PARENT); + CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::PARENT); // Allocate parent table P_table *pTable = diff --git a/src/proxy/ReverseProxy.cc b/src/proxy/ReverseProxy.cc index 7c4b721f503..893f421d7c0 100644 --- a/src/proxy/ReverseProxy.cc +++ b/src/proxy/ReverseProxy.cc @@ -159,27 +159,43 @@ urlRewriteVerify() bool reloadUrlRewrite(ConfigContext ctx) { - CfgLoadInProgress(ctx, "%s loading ...", ts::filename::REMAP); - Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP); + std::string msg_buffer; + + msg_buffer.reserve(1024); + UrlRewrite *newTable, *oldTable; - UrlRewrite *newTable = new UrlRewrite(); + CfgLoadLog(ctx, DL_Note, "%s loading (checking first) ...", ts::filename::REMAP_YAML); + CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::REMAP); + Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP_YAML); + Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP); + newTable = new UrlRewrite(); - bool status = newTable->load(ctx); - bool is_yaml = newTable->is_remap_yaml(); - const char *filename = is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP; + bool status = newTable->load(ctx); + bool is_yaml = (newTable->is_remap_yaml()); if (status) { + swoc::bwprint(msg_buffer, "{} finished loading", is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP); + + // Hold at least one lease, until we reload the configuration newTable->acquire(); - UrlRewrite *oldTable = rewrite_table.exchange(newTable); + // Swap configurations + oldTable = rewrite_table.exchange(newTable); + ink_assert(oldTable != nullptr); + + // Release the old one oldTable->release(); - CfgLoadComplete(ctx, "%s finished loading", filename); + Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); + CfgLoadComplete(ctx, "%s finished loading", is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP); return true; } else { + swoc::bwprint(msg_buffer, "{} failed to load", is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP); + delete newTable; - CfgLoadFail(ctx, DL_Error, "%s failed to load", filename); + Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); + CfgLoadFail(ctx, "%s failed to load", is_yaml ? ts::filename::REMAP_YAML : ts::filename::REMAP); return false; } } diff --git a/src/proxy/http/remap/UrlRewrite.cc b/src/proxy/http/remap/UrlRewrite.cc index a8909a661d9..7e82f8fbec4 100644 --- a/src/proxy/http/remap/UrlRewrite.cc +++ b/src/proxy/http/remap/UrlRewrite.cc @@ -130,7 +130,7 @@ UrlRewrite::load(ConfigContext ctx) fs::file_status status = fs::status(compilerPath, ec); if (ec || !swoc::file::is_regular_file(status)) { - CfgLoadFail(ctx, DL_Error, "Configured plugin compiler path '%s' is not a regular file", buf); + CfgLoadFail(ctx, "Configured plugin compiler path '%s' is not a regular file", buf); return false; } else { // This also adds the configuration directory (etc/trafficserver) to find Cripts etc. diff --git a/src/proxy/logging/LogConfig.cc b/src/proxy/logging/LogConfig.cc index e6129a83ad4..0aa16221db1 100644 --- a/src/proxy/logging/LogConfig.cc +++ b/src/proxy/logging/LogConfig.cc @@ -775,7 +775,8 @@ LogConfig::evaluate_config() ats_scoped_str path(RecConfigReadConfigPath("proxy.config.log.config.filename", ts::filename::LOGGING)); struct stat sbuf; if (stat(path.get(), &sbuf) == -1 && errno == ENOENT) { - CfgLoadFail(reload_ctx, DL_Warning, "logging configuration '%s' doesn't exist", path.get()); + // File doesn't exist — not a failure; ATS uses default logging. + CfgLoadComplete(reload_ctx, "logging configuration '%s' doesn't exist, using defaults", path.get()); return false; } @@ -786,7 +787,7 @@ LogConfig::evaluate_config() if (zret) { CfgLoadComplete(reload_ctx, "%s finished loading", path.get()); } else { - CfgLoadFail(reload_ctx, DL_Error, "%s failed to load", path.get()); + CfgLoadFail(reload_ctx, "%s failed to load", path.get()); } return zret; diff --git a/src/proxy/logging/YamlLogConfig.cc b/src/proxy/logging/YamlLogConfig.cc index a9d4327f208..ee88b2f724c 100644 --- a/src/proxy/logging/YamlLogConfig.cc +++ b/src/proxy/logging/YamlLogConfig.cc @@ -46,7 +46,7 @@ YamlLogConfig::parse(const char *cfgFilename) try { result = loadLogConfig(cfgFilename); } catch (std::exception &ex) { - CfgLoadFail(cfg->reload_ctx, DL_Error, "%s", ex.what()); + CfgLoadLog(cfg->reload_ctx, DL_Error, "%s", ex.what()); result = false; } return result; @@ -62,14 +62,14 @@ YamlLogConfig::loadLogConfig(const char *cfgFilename) } if (!config.IsMap()) { - CfgLoadFail(cfg->reload_ctx, DL_Error, "malformed %s file; expected a map", cfgFilename); + CfgLoadLog(cfg->reload_ctx, DL_Error, "malformed %s file; expected a map", cfgFilename); return false; } if (config["logging"]) { config = config["logging"]; } else { - CfgLoadFail(cfg->reload_ctx, DL_Error, "malformed %s file; expected a toplevel 'logging' node", cfgFilename); + CfgLoadLog(cfg->reload_ctx, DL_Error, "malformed %s file; expected a toplevel 'logging' node", cfgFilename); return false; } diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index e4851c97115..a52fc849d57 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -354,6 +354,8 @@ print_task_tree(const ConfigReloadResponse::ReloadInfo &f, bool full_report, con } std::cout << log_pfx; if (entry.level != DL_Undefined) { + // Indexed by DiagsLevel enum. In practice only [Dbg], [Note], [Warn], [Err] appear + // in task logs — Fatal/Alert/Emergency terminate the process before any task completes. static constexpr const char *severity_tags[] = { "[Diag] ", "[Dbg] ", "[Stat] ", "[Note] ", "[Warn] ", "[Err] ", "[Fatal] ", "[Alert] ", "[Emrg] ", }; diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index aa4587f2793..ae92b16cf01 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -767,7 +767,7 @@ register_config_files() } else { ctx.log("{}", zret); if (zret.severity() >= ERRATA_ERROR) { - CfgLoadFail(ctx, DL_Error, "Failed to reload %s", ts::filename::RECORDS); + CfgLoadFail(ctx, "Failed to reload %s", ts::filename::RECORDS); return; } } From 66fb7a3e0da2b516d0b971300aeb4caa161bee8b Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 15 Apr 2026 12:00:24 +0200 Subject: [PATCH 3/6] Fix log for the sslconfig module --- src/iocore/net/SSLConfig.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iocore/net/SSLConfig.cc b/src/iocore/net/SSLConfig.cc index bd3cd560c74..f6cdbf1647b 100644 --- a/src/iocore/net/SSLConfig.cc +++ b/src/iocore/net/SSLConfig.cc @@ -573,7 +573,7 @@ SSLConfigParams::initialize(ConfigContext ctx) 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"); - CfgLoadFail(ctx, "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"); } } From baed04329f5855655fb266d226063b579f9bd3bd Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 15 Apr 2026 17:48:37 +0200 Subject: [PATCH 4/6] Fix rocky ci --- src/iocore/net/quic/QUICConfig.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/iocore/net/quic/QUICConfig.cc b/src/iocore/net/quic/QUICConfig.cc index 6c39eb17f85..9905ae06889 100644 --- a/src/iocore/net/quic/QUICConfig.cc +++ b/src/iocore/net/quic/QUICConfig.cc @@ -58,7 +58,7 @@ quic_new_ssl_ctx() ALPN and SNI should be set to SSL object with NETVC_OPTIONS **/ static shared_SSL_CTX -quic_init_client_ssl_ctx(const QUICConfigParams *params, ConfigContext ctx) +quic_init_client_ssl_ctx(const QUICConfigParams *params, [[maybe_unused]] ConfigContext ctx) { std::unique_ptr ssl_ctx(nullptr, &SSL_CTX_free); ssl_ctx.reset(quic_new_ssl_ctx()); From 61504574a2f125716b6089353a47f764f5c6d52e Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 15 Apr 2026 23:14:22 +0200 Subject: [PATCH 5/6] Autest - Fix log text. --- src/proxy/IPAllow.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/proxy/IPAllow.cc b/src/proxy/IPAllow.cc index dc96c4a7fa6..c1ef179f8ca 100644 --- a/src/proxy/IPAllow.cc +++ b/src/proxy/IPAllow.cc @@ -136,7 +136,9 @@ IpAllow::reconfigure(ConfigContext ctx) } if (auto errata = new_table->BuildTable(); !errata.is_ok()) { if (errata.severity() > ERRATA_ERROR) { - Fatal("%s failed to load", ts::filename::IP_ALLOW); + std::string errata_text; + swoc::bwprint(errata_text, "{}", errata); + Fatal("%s failed to load\n%s", ts::filename::IP_ALLOW, errata_text.c_str()); } CfgLoadFailWithErrata(ctx, errata, "%s failed to load", ts::filename::IP_ALLOW); delete new_table; From 86c2507bd970b96bb40bf0d4fc956a5744cfa872 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 22 Apr 2026 14:03:11 +0200 Subject: [PATCH 6/6] Address PR review: fix race, double-log, case-insensitive --min-level --- .../config-reload-framework.en.rst | 2 +- include/mgmt/config/ConfigReloadTrace.h | 9 ++++---- src/iocore/net/SSLConfig.cc | 2 +- src/mgmt/config/ConfigReloadTrace.cc | 23 +++++++++++++------ src/traffic_ctl/CtrlCommands.cc | 9 +++++++- 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index fb428b5351e..550795be1bc 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -858,7 +858,7 @@ line is logged to ``diags.log``: .. code-block:: text - NOTE: Config reload [my-token] completed successfully: 3 tasks succeeded (3 total) + NOTE: Config reload [my-token] completed: 3/3 tasks succeeded **Failure:** diff --git a/include/mgmt/config/ConfigReloadTrace.h b/include/mgmt/config/ConfigReloadTrace.h index 396878e6085..7d9dadbca07 100644 --- a/include/mgmt/config/ConfigReloadTrace.h +++ b/include/mgmt/config/ConfigReloadTrace.h @@ -172,11 +172,12 @@ class ConfigReloadTask : public std::enable_shared_from_this } /// 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). + /// Entries from log(level, text) carry the supplied DiagsLevel. State-change + /// convenience methods attach an implicit level: in_progress() and complete() + /// log at DL_Note, fail() logs at DL_Error. The one-argument log(text) form + /// stores DL_Undefined and is always displayed (never filtered by --min-level). struct LogEntry { - DiagsLevel level{DL_Undefined}; ///< DL_Undefined for state-change messages + DiagsLevel level{DL_Undefined}; ///< DL_Undefined = always shown (filter bypass) std::string text; }; diff --git a/src/iocore/net/SSLConfig.cc b/src/iocore/net/SSLConfig.cc index f6cdbf1647b..065afbe7a14 100644 --- a/src/iocore/net/SSLConfig.cc +++ b/src/iocore/net/SSLConfig.cc @@ -573,7 +573,7 @@ SSLConfigParams::initialize(ConfigContext ctx) 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"); + ctx.log(DL_Warning, "Can't initialize the SSL client, HTTPS in remap rules will not function"); } } diff --git a/src/mgmt/config/ConfigReloadTrace.cc b/src/mgmt/config/ConfigReloadTrace.cc index 131f7c2be6c..bf26a6489f8 100644 --- a/src/mgmt/config/ConfigReloadTrace.cc +++ b/src/mgmt/config/ConfigReloadTrace.cc @@ -299,13 +299,22 @@ ConfigReloadTask::aggregate_status() void ConfigReloadTask::log_reload_summary(State final_state) { - if (!_info.main_task || !is_terminal(final_state) || _summary_logged) { - return; + // Snapshot token and sub_tasks under the lock to avoid races with RPC readers + // that also consult _info via get_info()/get_state() on other threads. + std::string token; + std::vector sub_tasks_snapshot; + { + std::unique_lock lock(_mutex); + if (!_info.main_task || !is_terminal(final_state) || _summary_logged) { + return; + } + _summary_logged = true; + token = _info.token; + sub_tasks_snapshot = _info.sub_tasks; } - _summary_logged = true; int success_count{0}, fail_count{0}, total{0}; - for (const auto &sub : _info.sub_tasks) { + for (const auto &sub : sub_tasks_snapshot) { State st = sub->get_state(); if (st == State::SUCCESS) { success_count++; @@ -316,14 +325,14 @@ ConfigReloadTask::log_reload_summary(State final_state) } if (final_state == State::SUCCESS) { - Note("Config reload [%s] completed: %d/%d tasks succeeded", _info.token.c_str(), success_count, total); + Note("Config reload [%s] completed: %d/%d tasks succeeded", token.c_str(), success_count, total); } else { Warning("Config reload [%s] finished with failures: %d succeeded, %d failed (%d total) " "— run: traffic_ctl config status -t %s", - _info.token.c_str(), success_count, fail_count, total, _info.token.c_str()); + token.c_str(), success_count, fail_count, total, token.c_str()); } - dump_subtask_tree(_info.sub_tasks, 2); + dump_subtask_tree(sub_tasks_snapshot, 2); } void diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index ec5d34baa8a..ddb44957c4f 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -20,6 +20,8 @@ */ #include "CtrlCommands.h" +#include +#include #include #include #include @@ -275,7 +277,12 @@ ConfigCommand::config_status() {"warning", DL_Warning}, {"error", DL_Error }, }; - if (auto it = level_map.find(min_level); it != level_map.end()) { + + std::string lowered{min_level}; + std::transform(lowered.begin(), lowered.end(), lowered.begin(), + [](unsigned char c) { return static_cast(std::tolower(c)); }); + + if (auto it = level_map.find(lowered); it != level_map.end()) { _printer->as()->set_min_level(it->second); } else { _printer->write_output("Invalid --min-level value. Use: debug, note, warning, error");