You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/09/29 20:38:51 UTC
[PR] autest: disable time-based log rolling by default [trafficserver]
bryancall opened a new pull request, #13749:
URL: https://github.com/apache/trafficserver/pull/13749
ATS rolls its logs daily at 00:00 UTC by default (`proxy.config.log.rolling_enabled: 1`, `rolling_interval_sec: 86400`, `rolling_offset_hr: 0`). When an autest is running at midnight, its logs are renamed to `*.old`, and its end-of-test `Disk.File` content checks then read the fresh, empty file and fail.
This is the cause of the `jax_fingerprint` and `log-filenames` failures on the AuTest 1of4 shard. Over the last two weeks, 5 of the 6 AuTest 1of4 shard runs that spanned 00:00 UTC (apache and ATSUnderground CI together) failed one or both tests. None of the 107 runs that did not span midnight failed either one. The failing assertions were:
- `Verify the jax_fingerprint log contains a JA4H fingerprint.` (also JA4 and JA3)
- `error.log should contain connection error messages`
- `my_custom_log.log should contain the custom transaction logs`
This change turns time-based log rolling off once, in `MakeATSProcess`, so every autest ATS instance gets it. `stale_response.test.py` already did this for the same reason in #13410. Tests that exercise rolling (`log_retention`, `sigusr2`) set `proxy.config.log.rolling_enabled` themselves, and that still overrides this default.
## Testing
Built master (a2416f2826) with the `ci-fedora-autest` preset in `ci.trafficserver.apache.org/ats/fedora:44`, then used libfaketime to start the container clock just before midnight UTC:
| | jax_fingerprint | log-filenames |
|---|---|---|
| Across midnight, before this change | fails with the CI assertions above; the logs are rolled to `*.old` | fails with the CI assertions above; the logs are rolled to `*.old` |
| Across midnight, with this change | 3 of 3 pass, nothing rolled | 2 of 2 pass, nothing rolled |
| Normal clock, 16 workers in parallel, before this change | 80 of 80 pass | 80 of 80 pass |
The rest of `gold_tests/logging` passes with this change (16 passed, 4 skipped), as do `sigusr2` and `stale_response`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] autest: turn log rolling off by default [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749#discussion_r4147860930
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
Review Comment:
Fixed the wording in f096d78df1: it now says this sets `proxy.config.log.rolling_enabled` to 0, which turns rolling off. The default, 1, is time-based rolling only, so the daily roll is all it removes; changing the interval or offset instead would only move the roll to another hour inside some test's run.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
"""Create a traffic server process.
:param block_for_debug: if True, causes traffic_server to run with the
--block option enabled, and effectively disables timeouts that could be
triggered by running traffic_server under a debugger. In the debugger,
`set cmd_block = 0`, set any desired break points, then `c` to continue
to let the test proceed.
+
+ :param disable_log_rolling: if True (the default), turn off time-based log
+ rolling, so that a test running across 00:00 UTC does not have its logs
+ renamed before its content checks read them. Pass False to leave the
Review Comment:
Aligned in f096d78df1: the docstring now says it sets `rolling_enabled` to 0, and that the default (1) rolls on a timer only, which is why turning it off is what stops the midnight roll.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +400,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms': 20})
+ # The default time-based log rolling renames every log at 00:00 UTC. A test that is running
+ # across midnight then checks the fresh, empty file and fails. Tests that exercise rolling
+ # set this themselves, which overrides the default here.
Review Comment:
Aligned in f096d78df1: the inline comment now describes the default of 1 as a daily timer at 00:00 UTC and says that 0 turns rolling off.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +400,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms': 20})
+ # The default time-based log rolling renames every log at 00:00 UTC. A test that is running
+ # across midnight then checks the fresh, empty file and fails. Tests that exercise rolling
+ # set this themselves, which overrides the default here.
+ if disable_log_rolling:
+ p.Disk.records_config.update({'proxy.config.log.rolling_enabled': 0})
Review Comment:
Same fix as the docstring thread, in f096d78df1: the comment says 0 turns rolling off, and since the default (1) is time-based only, nothing but the daily roll changes.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] autest: turn log rolling off by default [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749#discussion_r4148124926
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +402,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms': 20})
+ # The default rolling_enabled of 1 rolls every log on a timer, daily at 00:00 UTC. A test
+ # that is running across midnight then checks the fresh, empty file and fails. 0 turns
+ # rolling off; tests that exercise rolling set rolling_enabled themselves, overriding this.
Review Comment:
Agreed, and thanks. I will add it as a separate PR rather than here: a Catch2 test that calls `LogObject::roll_files()` with synthetic times (23:59:59, then 00:00:01) and checks that the log is renamed to `*.old` and that the next record lands in the new file. `roll_files()` takes the current time as an argument, so the test needs no wall-clock wait, where an AuTest would sit through a real rolling interval.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] autest: disable time-based log rolling by default [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749#discussion_r4147835806
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
Review Comment:
The PR description/title emphasize disabling *time-based* log rolling, but setting `proxy.config.log.rolling_enabled` to 0 disables log rolling entirely (not just midnight-based rolling). If the intent is truly to disable all rolling in AuTests, please update the wording (including parameter/docstring) to remove ambiguity. If the intent is only to stop time-based rolling while keeping other rolling behaviors, consider leaving `rolling_enabled` on and instead adjusting the time-based controls (e.g., interval/offset) so midnight doesn’t trigger a rename.
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
"""Create a traffic server process.
:param block_for_debug: if True, causes traffic_server to run with the
--block option enabled, and effectively disables timeouts that could be
triggered by running traffic_server under a debugger. In the debugger,
`set cmd_block = 0`, set any desired break points, then `c` to continue
to let the test proceed.
+
+ :param disable_log_rolling: if True (the default), turn off time-based log
+ rolling, so that a test running across 00:00 UTC does not have its logs
+ renamed before its content checks read them. Pass False to leave the
Review Comment:
The docstring and inline comment describe the behavior as turning off *time-based* rolling, but the implementation disables rolling globally via `proxy.config.log.rolling_enabled`. Please align the documentation with the actual behavior (either clarify that all rolling is disabled, or adjust implementation to only disable the time-based aspect).
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
Review Comment:
Given the documentation focuses on time-based rolling, `disable_log_rolling` is potentially ambiguous (it reads like it disables all rolling, which is what the current implementation does). Consider renaming to a more explicit parameter name (e.g., `disable_all_log_rolling` if that’s intended, or `disable_time_based_log_rolling` if the intent is only time-based behavior) to reduce misuse and keep call sites self-documenting.
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +400,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms': 20})
+ # The default time-based log rolling renames every log at 00:00 UTC. A test that is running
+ # across midnight then checks the fresh, empty file and fails. Tests that exercise rolling
+ # set this themselves, which overrides the default here.
Review Comment:
The docstring and inline comment describe the behavior as turning off *time-based* rolling, but the implementation disables rolling globally via `proxy.config.log.rolling_enabled`. Please align the documentation with the actual behavior (either clarify that all rolling is disabled, or adjust implementation to only disable the time-based aspect).
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +400,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms': 20})
+ # The default time-based log rolling renames every log at 00:00 UTC. A test that is running
+ # across midnight then checks the fresh, empty file and fails. Tests that exercise rolling
+ # set this themselves, which overrides the default here.
+ if disable_log_rolling:
+ p.Disk.records_config.update({'proxy.config.log.rolling_enabled': 0})
Review Comment:
The PR description/title emphasize disabling *time-based* log rolling, but setting `proxy.config.log.rolling_enabled` to 0 disables log rolling entirely (not just midnight-based rolling). If the intent is truly to disable all rolling in AuTests, please update the wording (including parameter/docstring) to remove ambiguity. If the intent is only to stop time-based rolling while keeping other rolling behaviors, consider leaving `rolling_enabled` on and instead adjusting the time-based controls (e.g., interval/offset) so midnight doesn’t trigger a rename.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] autest: turn log rolling off by default [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall merged PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] autest: turn log rolling off by default [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749#discussion_r4147862296
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -60,14 +60,20 @@ def MakeATSProcess(
dump_runroot=True,
enable_proxy_protocol=False,
enable_proxy_protocol_cp_src=False,
- disable_log_checks=False):
+ disable_log_checks=False,
+ disable_log_rolling=True):
Review Comment:
Kept `disable_log_rolling`: with the corrected docstring the name says exactly what it does (`rolling_enabled` 0 turns rolling off), and because the default only rolls on a timer there is no separate time-based-only option to tell it apart from.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] autest: turn log rolling off by default [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13749:
URL: https://github.com/apache/trafficserver/pull/13749#discussion_r4148072535
##########
tests/gold_tests/autest-site/trafficserver.test.ext:
##########
@@ -394,6 +402,12 @@ def MakeATSProcess(
# For config reload tests, no need to wait the default 3 seconds to detect the need for a reload.
p.Disk.records_config.update({'proxy.config.config_update_interval_ms': 20})
+ # The default rolling_enabled of 1 rolls every log on a timer, daily at 00:00 UTC. A test
+ # that is running across midnight then checks the fresh, empty file and fails. 0 turns
+ # rolling off; tests that exercise rolling set rolling_enabled themselves, overriding this.
Review Comment:
[P3] Keep an active test for explicitly enabled automatic rolling
Non-blocking coverage gap: `log_retention.test.py` has an unconditional `Test.SkipIf(Condition.true(...))`, and `sigusr2.test.py` explicitly sets rolling_enabled to 0 and exercises external rotation. Thus those tests do not verify automatic rolling in a normal suite run. With this default, ordinary log assertions will also no longer encounter the automatic-rollover path. Consider a small active regression that explicitly enables automatic rolling after MakeATSProcess(), drives a controlled roll, and checks both the renamed log and a subsequent record in the new file. That would preserve intentional coverage without bringing midnight dependence back into unrelated tests; the existing tests can remain unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org