You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2026/04/09 11:07:02 UTC
[PR] Add `Test.AddConfigReload()` autest extension [trafficserver]
brbzull0 opened a new pull request, #13075:
URL: https://github.com/apache/trafficserver/pull/13075
Replace the fragile pattern of fire-and-forget `traffic_ctl config reload` followed by sleep/log-grepping with a deterministic helper that uses monitor mode (`-m`) to block until completion and validates reload tasks via the get_reload_config_status JSONRPC endpoint.
The extension supports expected exit codes (success/fail/timeout/any), task presence and absence assertions, per-task status validation, custom tokens, and a delay_start parameter for filesystem timestamp sensitivity.
Migrate 21 existing tests to use the new helper, removing manual sleep synchronization and When.FileContains polling. Add an optional filename parameter to ConfigReloadTask so sub-tasks carry their associated config file for precise identification in test assertions.
#### Tiny change to the reload core:
Reload sub-tasks previously only carried a description (e.g."SNIConfig"), making it impossible to assert which config file triggereda task. The C++ change adds an optional `filename` parameter to`ConfigReloadTask::add_child()` and `ConfigContext::add_dependent_ctx()`so the JSONRPC status endpoint returns the actual file path (e.g.`sni.yaml`, `remap.yaml`) for each sub-task. This allows tests to validate that the right config handlers ran -- or didn't run.
**Includes RST documentation for the new extension API.**
Fixes https://github.com/apache/trafficserver/issues/12965
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4260931160
[approve ci]
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3091603157
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -0,0 +1,228 @@
+'''
+AuTest extension for config reload operations.
+
+Provides Test.AddConfigReload() to replace the legacy pattern of
+fire-and-forget `traffic_ctl config reload` followed by log grepping via
+When.FileContains("finished loading", N).
+
+Test.AddConfigReload(ts, ...)
+ Triggers a reload via `traffic_ctl config reload -m` (monitor mode),
+ which blocks until the reload reaches a terminal state (default timeout
+ 30s), then optionally validates the task tree via the
+ get_reload_config_status JSONRPC endpoint.
+
+Note: standalone record-triggered reloads (via traffic_ctl config set, with
+no explicit config reload) do not create tasks in the reload framework and
+cannot be verified with this extension.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import jsonrpc
+
+_reload_counter = 0
+
+_EXPECT_EXIT_CODES = {
+ "success": 0,
+ "fail": 2,
+ "timeout": 75,
+}
+
+
+def _next_token():
+ global _reload_counter
+ _reload_counter += 1
+ return f"autest-reload-{_reload_counter}"
+
+
+def _shell_quote(s):
+ """Single-quote a string for shell, escaping embedded single quotes."""
+ return "'" + s.replace("'", "'\\''") + "'"
+
+
+def _task_matches(info, name):
+ """Check if a task matches the given name by description, filename, or basename."""
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if not val or val == "<none>":
+ continue
+ if val == name or os.path.basename(val) == name:
+ return True
+ return False
+
+
+def _collect_task_names(info, names=None):
+ """Recursively collect human-friendly task identifiers from the task tree."""
+ if names is None:
+ names = set()
+ if isinstance(info, dict):
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if val and val != "<none>":
+ basename = os.path.basename(val)
+ names.add(basename if basename != val else val)
+ for sub in info.get("sub_tasks", []):
+ _collect_task_names(sub, names)
+ return names
+
+
+def _find_task(info, name):
+ """Find a task by description or filename (supports basename matching)."""
+ if isinstance(info, dict):
+ if _task_matches(info, name):
+ return info
+ for sub in info.get("sub_tasks", []):
+ found = _find_task(sub, name)
+ if found:
+ return found
+ return None
+
+
+def _make_task_validator(expect_tasks, expect_absent_tasks):
+ """Build a CustomJSONRPCResponse callback for task validation."""
+
+ def validator(resp):
+ if resp.is_error():
+ return (False, f"RPC error: {resp.error_as_str()}")
+
+ result = resp.__dict__.get("result", {})
+ tasks_list = result.get("tasks", [])
+ if not tasks_list:
+ if expect_tasks:
+ return (False, "No reload tasks in response")
+ if expect_absent_tasks:
+ return (True, "No tasks present, absence confirmed")
+ return (True, "No tasks expected, none found")
+
+ info = tasks_list[0]
+ all_names = _collect_task_names(info)
+
+ if expect_tasks:
+ items = expect_tasks.items() if isinstance(expect_tasks, dict) else [(t, None) for t in expect_tasks]
+ for task_name, task_status in items:
+ if task_name not in all_names:
+ return (False, f"Task '{task_name}' not found in reload. Found: {sorted(all_names)}")
+ if task_status is not None:
+ task = _find_task(info, task_name)
+ actual = task.get("status", "unknown") if task else "unknown"
+ if actual != task_status:
+ return (False, f"Task '{task_name}' status is '{actual}', expected '{task_status}'")
+
+ if expect_absent_tasks:
+ for task_name in expect_absent_tasks:
+ if task_name in all_names:
+ return (False, f"Task '{task_name}' should NOT be present but was found in: {sorted(all_names)}")
+
+ return (True, "All task assertions passed")
+
+ return validator
+
+
+def AddConfigReload(
+ self,
+ ts,
+ expect="success",
+ token=None,
+ data=None,
+ force=False,
+ timeout="30s",
+ initial_wait=1.0,
+ refresh_int=0.5,
+ expect_tasks=None,
+ expect_absent_tasks=None,
+ delay_start=None,
+ description=None,
+):
+ """Trigger a config reload, block until completion, and validate the result.
+
+ Creates one or two TestRuns:
+ 1. Runs `traffic_ctl config reload -m` to trigger and monitor the reload.
+ Asserts the exit code based on the `expect` parameter.
+ 2. (Optional) If expect_tasks or expect_absent_tasks is set, queries the
+ get_reload_config_status JSONRPC endpoint and validates the task tree
+ using CustomJSONRPCResponse.
+
+ Args:
+ ts: ATS process object.
+ expect: "success" (exit 0), "fail" (exit 2), "timeout" (exit 75),
+ or "any" (exit 0 or 2).
+ token: Custom reload token. Auto-generated if None.
+ data: Inline YAML string or @file path for --data flag.
+ force: If True, adds --force.
+ timeout: Timeout duration string for --timeout (e.g. "30s", "1m").
+ Set to None to disable.
+ initial_wait: Seconds before first poll (--initial-wait).
+ refresh_int: Seconds between polls (--refresh-int).
+ expect_tasks: List of task names expected in the reload, or a dict
+ mapping task names to expected statuses (e.g. {"sni.yaml": "fail"}).
+ expect_absent_tasks: List of task names that must NOT appear in the reload.
+ delay_start: Seconds to wait before starting the reload TestRun.
+ Useful when the previous step writes a config file and CI may
+ run fast enough that the filesystem timestamp hasn't changed.
+ description: Custom TestRun description.
+
+ Returns:
+ The reload TestRun object (first test run).
+ """
+ if expect not in _EXPECT_EXIT_CODES and expect != "any":
+ raise ValueError(f"expect must be one of {list(_EXPECT_EXIT_CODES) + ['any']}, got '{expect}'")
+
+ if token is None:
+ token = _next_token()
+
+ if description is None:
+ description = f"Reload config [{token}]"
+ if expect != "success":
+ description += f" (expect {expect})"
+
+ tr = self.AddTestRun(description)
+ if delay_start is not None:
+ tr.DelayStart = delay_start
+ tr.Processes.Default.Env = ts.Env
+ tr.StillRunningAfter = ts
+
+ cmd = f"traffic_ctl config reload -m -t {token}"
+ cmd += f" -w {initial_wait} -r {refresh_int}"
+
+ if timeout is not None:
+ cmd += f" -T {timeout}"
+ if force:
+ cmd += " --force"
+ if data is not None:
+ if data.startswith("@"):
+ cmd += f" --data {data}"
+ else:
+ cmd += f" --data {_shell_quote(data)}"
Review Comment:
Good catch. While `token` is auto-generated (no metacharacters) and `data` paths come from test code, it's cheap to make this defensive. Fixed — both `token` and `data` (including `@path` form) are now shell-quoted via `_shell_quote()`.
##########
tests/gold_tests/tls/tls_sni_yaml_reload.test.py:
##########
@@ -108,20 +106,13 @@
trupd.Processes.Default.Env = ts.Env
trupd.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["sni.yaml"], description="Reload config")
Review Comment:
`expect="fail"` already asserts the overall reload exit code is 2 (failure), and `expect_tasks=["sni.yaml"]` confirms the task was present in the reload tree. Since the overall status is derived from sub-task statuses, adding `{"sni.yaml": "fail"}` would double-assert the same condition.
The dict form is useful when you need to verify mixed per-task statuses independently of the overall result (e.g. `{"sni.yaml": "fail", "SSLConfig": "success"}` to prove the failure was isolated). For this simple single-task case the list form is intentional.
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4261206796
[approve ci]
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4216585176
[approve ci autest 3]
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4543898892
@moonchen just a reminder 😄
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3089676448
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -0,0 +1,228 @@
+'''
+AuTest extension for config reload operations.
+
+Provides Test.AddConfigReload() to replace the legacy pattern of
+fire-and-forget `traffic_ctl config reload` followed by log grepping via
+When.FileContains("finished loading", N).
+
+Test.AddConfigReload(ts, ...)
+ Triggers a reload via `traffic_ctl config reload -m` (monitor mode),
+ which blocks until the reload reaches a terminal state (default timeout
+ 30s), then optionally validates the task tree via the
+ get_reload_config_status JSONRPC endpoint.
+
+Note: standalone record-triggered reloads (via traffic_ctl config set, with
+no explicit config reload) do not create tasks in the reload framework and
+cannot be verified with this extension.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import jsonrpc
+
+_reload_counter = 0
+
+_EXPECT_EXIT_CODES = {
+ "success": 0,
+ "fail": 2,
+ "timeout": 75,
+}
+
+
+def _next_token():
+ global _reload_counter
+ _reload_counter += 1
+ return f"autest-reload-{_reload_counter}"
+
+
+def _shell_quote(s):
+ """Single-quote a string for shell, escaping embedded single quotes."""
+ return "'" + s.replace("'", "'\\''") + "'"
+
+
+def _task_matches(info, name):
+ """Check if a task matches the given name by description, filename, or basename."""
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if not val or val == "<none>":
+ continue
+ if val == name or os.path.basename(val) == name:
+ return True
+ return False
+
+
+def _collect_task_names(info, names=None):
+ """Recursively collect human-friendly task identifiers from the task tree."""
+ if names is None:
+ names = set()
+ if isinstance(info, dict):
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if val and val != "<none>":
+ basename = os.path.basename(val)
+ names.add(basename if basename != val else val)
+ for sub in info.get("sub_tasks", []):
+ _collect_task_names(sub, names)
+ return names
+
+
+def _find_task(info, name):
+ """Find a task by description or filename (supports basename matching)."""
+ if isinstance(info, dict):
+ if _task_matches(info, name):
+ return info
+ for sub in info.get("sub_tasks", []):
+ found = _find_task(sub, name)
+ if found:
+ return found
+ return None
+
+
+def _make_task_validator(expect_tasks, expect_absent_tasks):
+ """Build a CustomJSONRPCResponse callback for task validation."""
+
+ def validator(resp):
+ if resp.is_error():
+ return (False, f"RPC error: {resp.error_as_str()}")
+
+ result = resp.__dict__.get("result", {})
+ tasks_list = result.get("tasks", [])
+ if not tasks_list:
+ if expect_tasks:
+ return (False, "No reload tasks in response")
+ if expect_absent_tasks:
+ return (True, "No tasks present, absence confirmed")
+ return (True, "No tasks expected, none found")
+
+ info = tasks_list[0]
+ all_names = _collect_task_names(info)
+
+ if expect_tasks:
+ items = expect_tasks.items() if isinstance(expect_tasks, dict) else [(t, None) for t in expect_tasks]
+ for task_name, task_status in items:
+ if task_name not in all_names:
+ return (False, f"Task '{task_name}' not found in reload. Found: {sorted(all_names)}")
+ if task_status is not None:
+ task = _find_task(info, task_name)
+ actual = task.get("status", "unknown") if task else "unknown"
+ if actual != task_status:
+ return (False, f"Task '{task_name}' status is '{actual}', expected '{task_status}'")
+
+ if expect_absent_tasks:
+ for task_name in expect_absent_tasks:
+ if task_name in all_names:
+ return (False, f"Task '{task_name}' should NOT be present but was found in: {sorted(all_names)}")
+
+ return (True, "All task assertions passed")
+
+ return validator
+
+
+def AddConfigReload(
+ self,
+ ts,
+ expect="success",
+ token=None,
+ data=None,
+ force=False,
+ timeout="30s",
+ initial_wait=1.0,
+ refresh_int=0.5,
+ expect_tasks=None,
+ expect_absent_tasks=None,
+ delay_start=None,
+ description=None,
+):
+ """Trigger a config reload, block until completion, and validate the result.
+
+ Creates one or two TestRuns:
+ 1. Runs `traffic_ctl config reload -m` to trigger and monitor the reload.
+ Asserts the exit code based on the `expect` parameter.
+ 2. (Optional) If expect_tasks or expect_absent_tasks is set, queries the
+ get_reload_config_status JSONRPC endpoint and validates the task tree
+ using CustomJSONRPCResponse.
+
+ Args:
+ ts: ATS process object.
+ expect: "success" (exit 0), "fail" (exit 2), "timeout" (exit 75),
+ or "any" (exit 0 or 2).
+ token: Custom reload token. Auto-generated if None.
+ data: Inline YAML string or @file path for --data flag.
+ force: If True, adds --force.
+ timeout: Timeout duration string for --timeout (e.g. "30s", "1m").
+ Set to None to disable.
+ initial_wait: Seconds before first poll (--initial-wait).
+ refresh_int: Seconds between polls (--refresh-int).
+ expect_tasks: List of task names expected in the reload, or a dict
+ mapping task names to expected statuses (e.g. {"sni.yaml": "fail"}).
+ expect_absent_tasks: List of task names that must NOT appear in the reload.
+ delay_start: Seconds to wait before starting the reload TestRun.
+ Useful when the previous step writes a config file and CI may
+ run fast enough that the filesystem timestamp hasn't changed.
+ description: Custom TestRun description.
+
+ Returns:
+ The reload TestRun object (first test run).
+ """
+ if expect not in _EXPECT_EXIT_CODES and expect != "any":
+ raise ValueError(f"expect must be one of {list(_EXPECT_EXIT_CODES) + ['any']}, got '{expect}'")
+
+ if token is None:
+ token = _next_token()
+
+ if description is None:
+ description = f"Reload config [{token}]"
+ if expect != "success":
+ description += f" (expect {expect})"
+
+ tr = self.AddTestRun(description)
+ if delay_start is not None:
+ tr.DelayStart = delay_start
+ tr.Processes.Default.Env = ts.Env
+ tr.StillRunningAfter = ts
+
+ cmd = f"traffic_ctl config reload -m -t {token}"
+ cmd += f" -w {initial_wait} -r {refresh_int}"
+
+ if timeout is not None:
+ cmd += f" -T {timeout}"
+ if force:
+ cmd += " --force"
+ if data is not None:
+ if data.startswith("@"):
+ cmd += f" --data {data}"
+ else:
+ cmd += f" --data {_shell_quote(data)}"
Review Comment:
The `token` and `data` (when passed as `@path`) are interpolated into a shell command without quoting. If they contain spaces or shell metacharacters the command can break or be interpreted unexpectedly. Shell-quote `token` and any `--data` argument (including `@...` paths), or build the command using an argv-style API if available.
##########
tests/gold_tests/tls/tls_sni_yaml_reload.test.py:
##########
@@ -108,20 +106,13 @@
trupd.Processes.Default.Env = ts.Env
trupd.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["sni.yaml"], description="Reload config")
Review Comment:
This reload is expected to fail specifically due to `sni.yaml`, but `expect_tasks` is provided as a list, which only asserts presence (not per-task status). To preserve the earlier intent (previously asserted via diags.log grep), consider using the dict form to assert `sni.yaml` has status `fail` (e.g. `expect_tasks={"sni.yaml": "fail"}`) or adding an explicit per-task status check.
```suggestion
tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks={"sni.yaml": "fail"}, description="Reload config")
```
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -0,0 +1,228 @@
+'''
+AuTest extension for config reload operations.
+
+Provides Test.AddConfigReload() to replace the legacy pattern of
+fire-and-forget `traffic_ctl config reload` followed by log grepping via
+When.FileContains("finished loading", N).
+
+Test.AddConfigReload(ts, ...)
+ Triggers a reload via `traffic_ctl config reload -m` (monitor mode),
+ which blocks until the reload reaches a terminal state (default timeout
+ 30s), then optionally validates the task tree via the
+ get_reload_config_status JSONRPC endpoint.
+
+Note: standalone record-triggered reloads (via traffic_ctl config set, with
+no explicit config reload) do not create tasks in the reload framework and
+cannot be verified with this extension.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import jsonrpc
+
+_reload_counter = 0
+
+_EXPECT_EXIT_CODES = {
+ "success": 0,
+ "fail": 2,
+ "timeout": 75,
+}
+
+
+def _next_token():
+ global _reload_counter
+ _reload_counter += 1
+ return f"autest-reload-{_reload_counter}"
+
+
+def _shell_quote(s):
+ """Single-quote a string for shell, escaping embedded single quotes."""
+ return "'" + s.replace("'", "'\\''") + "'"
+
+
+def _task_matches(info, name):
+ """Check if a task matches the given name by description, filename, or basename."""
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if not val or val == "<none>":
+ continue
+ if val == name or os.path.basename(val) == name:
+ return True
+ return False
+
+
+def _collect_task_names(info, names=None):
+ """Recursively collect human-friendly task identifiers from the task tree."""
+ if names is None:
+ names = set()
+ if isinstance(info, dict):
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if val and val != "<none>":
+ basename = os.path.basename(val)
+ names.add(basename if basename != val else val)
+ for sub in info.get("sub_tasks", []):
+ _collect_task_names(sub, names)
+ return names
+
+
+def _find_task(info, name):
+ """Find a task by description or filename (supports basename matching)."""
+ if isinstance(info, dict):
+ if _task_matches(info, name):
+ return info
+ for sub in info.get("sub_tasks", []):
+ found = _find_task(sub, name)
+ if found:
+ return found
+ return None
+
+
+def _make_task_validator(expect_tasks, expect_absent_tasks):
+ """Build a CustomJSONRPCResponse callback for task validation."""
+
+ def validator(resp):
+ if resp.is_error():
+ return (False, f"RPC error: {resp.error_as_str()}")
+
+ result = resp.__dict__.get("result", {})
+ tasks_list = result.get("tasks", [])
+ if not tasks_list:
+ if expect_tasks:
+ return (False, "No reload tasks in response")
+ if expect_absent_tasks:
+ return (True, "No tasks present, absence confirmed")
+ return (True, "No tasks expected, none found")
+
+ info = tasks_list[0]
+ all_names = _collect_task_names(info)
+
+ if expect_tasks:
+ items = expect_tasks.items() if isinstance(expect_tasks, dict) else [(t, None) for t in expect_tasks]
+ for task_name, task_status in items:
+ if task_name not in all_names:
+ return (False, f"Task '{task_name}' not found in reload. Found: {sorted(all_names)}")
+ if task_status is not None:
+ task = _find_task(info, task_name)
+ actual = task.get("status", "unknown") if task else "unknown"
+ if actual != task_status:
+ return (False, f"Task '{task_name}' status is '{actual}', expected '{task_status}'")
+
+ if expect_absent_tasks:
+ for task_name in expect_absent_tasks:
+ if task_name in all_names:
Review Comment:
`_collect_task_names()` stores only basenames for `filename` values, so `expect_tasks`/`expect_absent_tasks` presence checks will fail if a caller passes the full path returned by JSONRPC (e.g. `/opt/ats/etc/trafficserver/ip_allow.yaml`). Consider validating presence via `_find_task()`/`_task_matches()` for each expected name (or adding both the full `filename` and its basename to `all_names`).
```suggestion
task = _find_task(info, task_name)
if task is None:
return (False, f"Task '{task_name}' not found in reload. Found: {sorted(all_names)}")
if task_status is not None:
actual = task.get("status", "unknown")
if actual != task_status:
return (False, f"Task '{task_name}' status is '{actual}', expected '{task_status}'")
if expect_absent_tasks:
for task_name in expect_absent_tasks:
if _find_task(info, task_name) is not None:
```
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -78,19 +78,13 @@
tr2.Processes.Default.Env = ts.Env
tr2.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Reload config")
Review Comment:
This test expects the reload to fail because `ssl_multicert.yaml` is invalid, but `expect_tasks` is a list so it only asserts the task exists. Consider using the dict form (e.g. `expect_tasks={"ssl_multicert.yaml": "fail"}`) so the test verifies the intended handler failure rather than just any reload failure with that task present.
```suggestion
tr2reload = Test.AddConfigReload(
ts, expect="fail", expect_tasks={"ssl_multicert.yaml": "fail"}, description="Reload config")
```
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3114123764
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -0,0 +1,225 @@
+'''
+AuTest extension for config reload operations.
+
+Provides Test.AddConfigReload() to replace the legacy pattern of
+fire-and-forget `traffic_ctl config reload` followed by log grepping via
+When.FileContains("finished loading", N).
+
+Test.AddConfigReload(ts, ...)
+ Triggers a reload via `traffic_ctl config reload -m` (monitor mode),
+ which blocks until the reload reaches a terminal state (default timeout
+ 30s), then optionally validates the task tree via the
+ get_reload_config_status JSONRPC endpoint.
+
+Note: standalone record-triggered reloads (via traffic_ctl config set, with
+no explicit config reload) do not create tasks in the reload framework and
+cannot be verified with this extension.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import jsonrpc
+
+_reload_counter = 0
+
+_EXPECT_EXIT_CODES = {
+ "success": 0,
+ "fail": 2,
+ "timeout": 75,
+}
+
+
+def _next_token():
+ global _reload_counter
+ _reload_counter += 1
+ return f"autest-reload-{_reload_counter}"
+
+
+def _shell_quote(s):
+ """Single-quote a string for shell, escaping embedded single quotes."""
+ return "'" + s.replace("'", "'\\''") + "'"
+
+
+def _task_matches(info, name):
+ """Check if a task matches the given name by description, filename, or basename."""
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if not val or val == "<none>":
+ continue
+ if val == name or os.path.basename(val) == name:
+ return True
+ return False
+
+
+def _collect_task_names(info, names=None):
+ """Recursively collect human-friendly task identifiers from the task tree."""
+ if names is None:
+ names = set()
+ if isinstance(info, dict):
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if val and val != "<none>":
+ basename = os.path.basename(val)
+ names.add(basename if basename != val else val)
+ for sub in info.get("sub_tasks", []):
+ _collect_task_names(sub, names)
+ return names
+
+
+def _find_task(info, name):
+ """Find a task by description or filename (supports basename matching)."""
+ if isinstance(info, dict):
+ if _task_matches(info, name):
+ return info
+ for sub in info.get("sub_tasks", []):
+ found = _find_task(sub, name)
+ if found:
+ return found
+ return None
+
+
+def _make_task_validator(expect_tasks, expect_absent_tasks):
+ """Build a CustomJSONRPCResponse callback for task validation."""
+
+ def validator(resp):
+ if resp.is_error():
+ return (False, f"RPC error: {resp.error_as_str()}")
+
+ result = resp.__dict__.get("result", {})
+ tasks_list = result.get("tasks", [])
+ if not tasks_list:
+ if expect_tasks:
+ return (False, "No reload tasks in response")
+ if expect_absent_tasks:
+ return (True, "No tasks present, absence confirmed")
+ return (True, "No tasks expected, none found")
+
+ info = tasks_list[0]
+ all_names = _collect_task_names(info)
+
+ if expect_tasks:
+ items = expect_tasks.items() if isinstance(expect_tasks, dict) else [(t, None) for t in expect_tasks]
+ for task_name, task_status in items:
+ if task_name not in all_names:
+ return (False, f"Task '{task_name}' not found in reload. Found: {sorted(all_names)}")
+ if task_status is not None:
+ task = _find_task(info, task_name)
+ actual = task.get("status", "unknown") if task else "unknown"
+ if actual != task_status:
+ return (False, f"Task '{task_name}' status is '{actual}', expected '{task_status}'")
+
+ if expect_absent_tasks:
+ for task_name in expect_absent_tasks:
+ if task_name in all_names:
+ return (False, f"Task '{task_name}' should NOT be present but was found in: {sorted(all_names)}")
+
+ return (True, "All task assertions passed")
+
+ return validator
+
+
+def AddConfigReload(
+ self,
+ ts,
+ expect="success",
+ token=None,
+ data=None,
+ force=False,
+ timeout="30s",
+ initial_wait=1.0,
+ refresh_int=0.5,
+ expect_tasks=None,
+ expect_absent_tasks=None,
+ delay_start=None,
+ description=None,
+):
+ """Trigger a config reload, block until completion, and validate the result.
+
+ Creates one or two TestRuns:
+ 1. Runs `traffic_ctl config reload -m` to trigger and monitor the reload.
+ Asserts the exit code based on the `expect` parameter.
+ 2. (Optional) If expect_tasks or expect_absent_tasks is set, queries the
+ get_reload_config_status JSONRPC endpoint and validates the task tree
+ using CustomJSONRPCResponse.
+
+ Args:
+ ts: ATS process object.
+ expect: "success" (exit 0), "fail" (exit 2), "timeout" (exit 75),
+ or "any" (exit 0 or 2).
+ token: Custom reload token. Auto-generated if None.
+ data: Inline YAML string or @file path for --data flag.
+ force: If True, adds --force.
+ timeout: Timeout duration string for --timeout (e.g. "30s", "1m").
+ Set to None to disable.
+ initial_wait: Seconds before first poll (--initial-wait).
+ refresh_int: Seconds between polls (--refresh-int).
+ expect_tasks: List of task names expected in the reload, or a dict
+ mapping task names to expected statuses (e.g. {"sni.yaml": "fail"}).
+ expect_absent_tasks: List of task names that must NOT appear in the reload.
+ delay_start: Seconds to wait before starting the reload TestRun.
+ Useful when the previous step writes a config file and CI may
+ run fast enough that the filesystem timestamp hasn't changed.
+ description: Custom TestRun description.
+
+ Returns:
+ The reload TestRun object (first test run).
+ """
+ if expect not in _EXPECT_EXIT_CODES and expect != "any":
+ raise ValueError(f"expect must be one of {list(_EXPECT_EXIT_CODES) + ['any']}, got '{expect}'")
+
+ if token is None:
+ token = _next_token()
+
+ if description is None:
+ description = f"Reload config [{token}]"
+ if expect != "success":
+ description += f" (expect {expect})"
+
+ tr = self.AddTestRun(description)
+ if delay_start is not None:
+ tr.DelayStart = delay_start
+ tr.Processes.Default.Env = ts.Env
+ tr.StillRunningAfter = ts
+
+ cmd = f"traffic_ctl config reload -m -t {_shell_quote(token)}"
+ cmd += f" -w {initial_wait} -r {refresh_int}"
+
+ if timeout is not None:
+ cmd += f" -T {timeout}"
Review Comment:
`timeout` is interpolated into the shell command without quoting (`cmd += f" -T {timeout}"`). `traffic_ctl config reload` explicitly supports duration strings with spaces (e.g. `"1 hour 30min"`), which will be split into multiple argv tokens here and cause the command to fail. Shell-quote `timeout` (and/or build the command using an argv-style API) so multi-word durations work reliably.
```suggestion
cmd += f" -T {_shell_quote(timeout)}"
```
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4214122078
[approve ci]
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3347870204
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -78,19 +78,13 @@
tr2.Processes.Default.Env = ts.Env
tr2.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Reload config")
tr2reload.StillRunningAfter = server
-tr2reload.Processes.Default.Command = 'traffic_ctl config reload'
-tr2reload.Processes.Default.Env = ts.Env
-tr2reload.Processes.Default.ReturnCode = 0
-ts.Disk.diags_log.Content = Testers.ContainsExpression('ERROR: ', 'ERROR')
# Reload of ssl_multicert.yaml should fail, BUT the old config structure
# should be in place to successfully answer for the test domain
tr3 = Test.AddTestRun("Make request again for $sni_domain")
-# Wait for the reload to complete
-tr3.Processes.Default.StartBefore(server2, ready=When.FileContains(ts.Disk.diags_log.Name, 'failed to load certificate ', 1))
Review Comment:
Yes. That `When.FileContains` was sort of a sync gate, it waited for the log line before running the next curl. Now `traffic_ctl config reload -m` blocks until the reload reaches a terminal state, so by the time tr3 runs the reload is definitely done.
##########
tests/gold_tests/remap_yaml/remap_reload_yaml.test.py:
##########
@@ -106,15 +105,10 @@ def update_remap_yaml(path: str, lines: list) -> None:
url: http://bravo.ex:{pv_port}
'''.split("\n")))
-tr = Test.AddTestRun("remap_yaml reload, fails")
-tr.Processes.Default.Env = tm.Env
-tr.Processes.Default.Command = 'sleep 2; traffic_ctl config reload'
+tr = Test.AddConfigReload(tm, expect="fail", expect_tasks=["remap.yaml"], delay_start=2, description="remap_yaml reload, fails")
tr = Test.AddTestRun("after first reload")
-await_config_reload = tr.Processes.Process('config_reload_failed', 'sleep 30')
-await_config_reload.Ready = When.FileContains(tm.Disk.diags_log.Name, "configuration is invalid")
Review Comment:
This is the same as above: the `await_config_reload` was a sleep 30 placeholder. This was used because the previous reload run was fire-and-forget. Now with the new `-m` the reload run only completes after the reload terminates, so the next run can start the verifier client straight away. The removed string validation is now asserted by `expect="fail"`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4611592763
> @brbzull0 thanks for the PR. I think it's a good change. Just a few small areas where I thought it was worth a second look.
Thanks @moonchen for the review. Pushed some changes already. 👍
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4707076439
[approve ci autest 1]
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3091599667
##########
tests/gold_tests/autest-site/config_reload.test.ext:
##########
@@ -0,0 +1,228 @@
+'''
+AuTest extension for config reload operations.
+
+Provides Test.AddConfigReload() to replace the legacy pattern of
+fire-and-forget `traffic_ctl config reload` followed by log grepping via
+When.FileContains("finished loading", N).
+
+Test.AddConfigReload(ts, ...)
+ Triggers a reload via `traffic_ctl config reload -m` (monitor mode),
+ which blocks until the reload reaches a terminal state (default timeout
+ 30s), then optionally validates the task tree via the
+ get_reload_config_status JSONRPC endpoint.
+
+Note: standalone record-triggered reloads (via traffic_ctl config set, with
+no explicit config reload) do not create tasks in the reload framework and
+cannot be verified with this extension.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import jsonrpc
+
+_reload_counter = 0
+
+_EXPECT_EXIT_CODES = {
+ "success": 0,
+ "fail": 2,
+ "timeout": 75,
+}
+
+
+def _next_token():
+ global _reload_counter
+ _reload_counter += 1
+ return f"autest-reload-{_reload_counter}"
+
+
+def _shell_quote(s):
+ """Single-quote a string for shell, escaping embedded single quotes."""
+ return "'" + s.replace("'", "'\\''") + "'"
+
+
+def _task_matches(info, name):
+ """Check if a task matches the given name by description, filename, or basename."""
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if not val or val == "<none>":
+ continue
+ if val == name or os.path.basename(val) == name:
+ return True
+ return False
+
+
+def _collect_task_names(info, names=None):
+ """Recursively collect human-friendly task identifiers from the task tree."""
+ if names is None:
+ names = set()
+ if isinstance(info, dict):
+ for field in ("description", "filename"):
+ val = info.get(field)
+ if val and val != "<none>":
+ basename = os.path.basename(val)
+ names.add(basename if basename != val else val)
+ for sub in info.get("sub_tasks", []):
+ _collect_task_names(sub, names)
+ return names
+
+
+def _find_task(info, name):
+ """Find a task by description or filename (supports basename matching)."""
+ if isinstance(info, dict):
+ if _task_matches(info, name):
+ return info
+ for sub in info.get("sub_tasks", []):
+ found = _find_task(sub, name)
+ if found:
+ return found
+ return None
+
+
+def _make_task_validator(expect_tasks, expect_absent_tasks):
+ """Build a CustomJSONRPCResponse callback for task validation."""
+
+ def validator(resp):
+ if resp.is_error():
+ return (False, f"RPC error: {resp.error_as_str()}")
+
+ result = resp.__dict__.get("result", {})
+ tasks_list = result.get("tasks", [])
+ if not tasks_list:
+ if expect_tasks:
+ return (False, "No reload tasks in response")
+ if expect_absent_tasks:
+ return (True, "No tasks present, absence confirmed")
+ return (True, "No tasks expected, none found")
+
+ info = tasks_list[0]
+ all_names = _collect_task_names(info)
+
+ if expect_tasks:
+ items = expect_tasks.items() if isinstance(expect_tasks, dict) else [(t, None) for t in expect_tasks]
+ for task_name, task_status in items:
+ if task_name not in all_names:
+ return (False, f"Task '{task_name}' not found in reload. Found: {sorted(all_names)}")
+ if task_status is not None:
+ task = _find_task(info, task_name)
+ actual = task.get("status", "unknown") if task else "unknown"
+ if actual != task_status:
+ return (False, f"Task '{task_name}' status is '{actual}', expected '{task_status}'")
+
+ if expect_absent_tasks:
+ for task_name in expect_absent_tasks:
+ if task_name in all_names:
Review Comment:
`_collect_task_names()` already normalizes to basenames via `os.path.basename()`, and all call-sites pass short names (e.g. `"sni.yaml"`, not full paths). The JSONRPC endpoint returns whatever was stored on the C++ side, which in this PR is always a filename constant (e.g. `ts::filename::SNI`), not an absolute path. The basename fallback is defensive.
Using `_find_task()` instead would work equivalently but isn't a fix — both approaches resolve to the same result for the inputs this API accepts. Keeping the `set` lookup for presence and reserving `_find_task()` for when we need the actual task object (i.e. status checks) feels cleaner.
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3091604895
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -78,19 +78,13 @@
tr2.Processes.Default.Env = ts.Env
tr2.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Reload config")
Review Comment:
Same reasoning as the `tls_sni_yaml_reload` comment — `expect="fail"` covers the failure assertion and the list form confirms the right handler was involved. The dict form would be redundant here. It's available for more complex multi-task scenarios where per-task status matters independently.
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 merged PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3337096264
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -78,19 +78,13 @@
tr2.Processes.Default.Env = ts.Env
tr2.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Reload config")
tr2reload.StillRunningAfter = server
-tr2reload.Processes.Default.Command = 'traffic_ctl config reload'
-tr2reload.Processes.Default.Env = ts.Env
-tr2reload.Processes.Default.ReturnCode = 0
-ts.Disk.diags_log.Content = Testers.ContainsExpression('ERROR: ', 'ERROR')
Review Comment:
Was this check removed on purpose?
##########
tests/gold_tests/traffic_ctl/remap_inc/remap_inc.test.py:
##########
@@ -14,16 +14,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-import os
-
Test.Summary = '''
Test traffic_ctl config reload with remap.config .include directive
'''
Test.ContinueOnFail = False
-Test.Setup.Copy("wait_reload.sh")
Review Comment:
Since you're removing this code, can you please also remove the file `wait_reload.sh`
##########
tests/gold_tests/remap_yaml/remap_reload_yaml.test.py:
##########
@@ -106,15 +105,10 @@ def update_remap_yaml(path: str, lines: list) -> None:
url: http://bravo.ex:{pv_port}
'''.split("\n")))
-tr = Test.AddTestRun("remap_yaml reload, fails")
-tr.Processes.Default.Env = tm.Env
-tr.Processes.Default.Command = 'sleep 2; traffic_ctl config reload'
+tr = Test.AddConfigReload(tm, expect="fail", expect_tasks=["remap.yaml"], delay_start=2, description="remap_yaml reload, fails")
tr = Test.AddTestRun("after first reload")
-await_config_reload = tr.Processes.Process('config_reload_failed', 'sleep 30')
-await_config_reload.Ready = When.FileContains(tm.Disk.diags_log.Name, "configuration is invalid")
Review Comment:
Could this check stay in place?
##########
tests/gold_tests/tls/tls_sni_yaml_reload.test.py:
##########
@@ -84,8 +84,6 @@
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression("Could Not Connect", "Verify curl could successfully connect")
tr.Processes.Default.Streams.stderr = Testers.IncludesExpression(f"CN={sni_domain}", f"Verify curl used the {sni_domain} SNI")
-ts.Disk.diags_log.Content = Testers.IncludesExpression(
- "SSL negotiation finished successfully", "Verify that the TLS handshake was successful")
Review Comment:
Was this check removed on purpose?
##########
src/iocore/net/SSLClientCoordinator.cc:
##########
@@ -37,8 +37,8 @@ SSLClientCoordinator::reconfigure(ConfigContext reconf_ctx)
// The SSLConfig owns the client cert context storage and the SNIConfig will load
// into it.
SSLConfig::reconfigure(reconf_ctx.add_dependent_ctx("SSLConfig"));
- SNIConfig::reconfigure(reconf_ctx.add_dependent_ctx("SNIConfig"));
- SSLCertificateConfig::reconfigure(reconf_ctx.add_dependent_ctx("SSLCertificateConfig"));
+ SNIConfig::reconfigure(reconf_ctx.add_dependent_ctx("SNIConfig", ts::filename::SNI));
+ SSLCertificateConfig::reconfigure(reconf_ctx.add_dependent_ctx("SSLCertificateConfig", ts::filename::SSL_MULTICERT));
#if TS_USE_QUIC == 1
QUICCertConfig::reconfigure(reconf_ctx.add_dependent_ctx("QUICCertConfig"));
Review Comment:
should this also get `ts::filename::SSL_MULTICERT`?
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -78,19 +78,13 @@
tr2.Processes.Default.Env = ts.Env
tr2.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Reload config")
tr2reload.StillRunningAfter = server
-tr2reload.Processes.Default.Command = 'traffic_ctl config reload'
-tr2reload.Processes.Default.Env = ts.Env
-tr2reload.Processes.Default.ReturnCode = 0
-ts.Disk.diags_log.Content = Testers.ContainsExpression('ERROR: ', 'ERROR')
# Reload of ssl_multicert.yaml should fail, BUT the old config structure
# should be in place to successfully answer for the test domain
tr3 = Test.AddTestRun("Make request again for $sni_domain")
-# Wait for the reload to complete
-tr3.Processes.Default.StartBefore(server2, ready=When.FileContains(ts.Disk.diags_log.Name, 'failed to load certificate ', 1))
Review Comment:
Was this check removed on purpose?
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4596480904
@brbzull0 thanks for the PR. I think it's a good change. Just a few small areas where I thought it was worth a second look.
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4633045335
[approve ci]
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3347878087
##########
tests/gold_tests/tls/tls_sni_yaml_reload.test.py:
##########
@@ -84,8 +84,6 @@
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression("Could Not Connect", "Verify curl could successfully connect")
tr.Processes.Default.Streams.stderr = Testers.IncludesExpression(f"CN={sni_domain}", f"Verify curl used the {sni_domain} SNI")
-ts.Disk.diags_log.Content = Testers.IncludesExpression(
- "SSL negotiation finished successfully", "Verify that the TLS handshake was successful")
Review Comment:
Yes. Now that I was re-checking this It turns out it's a dead check on master, the string itself doesn't appear anywhere in the ATS source.
My understanding is that the successful handshake is already proven by the `CN={sni_domain}` check on curl stderr right above.
##########
src/iocore/net/SSLClientCoordinator.cc:
##########
@@ -37,8 +37,8 @@ SSLClientCoordinator::reconfigure(ConfigContext reconf_ctx)
// The SSLConfig owns the client cert context storage and the SNIConfig will load
// into it.
SSLConfig::reconfigure(reconf_ctx.add_dependent_ctx("SSLConfig"));
- SNIConfig::reconfigure(reconf_ctx.add_dependent_ctx("SNIConfig"));
- SSLCertificateConfig::reconfigure(reconf_ctx.add_dependent_ctx("SSLCertificateConfig"));
+ SNIConfig::reconfigure(reconf_ctx.add_dependent_ctx("SNIConfig", ts::filename::SNI));
+ SSLCertificateConfig::reconfigure(reconf_ctx.add_dependent_ctx("SSLCertificateConfig", ts::filename::SSL_MULTICERT));
#if TS_USE_QUIC == 1
QUICCertConfig::reconfigure(reconf_ctx.add_dependent_ctx("QUICCertConfig"));
Review Comment:
Yes!! thanks.
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#discussion_r3347869026
##########
tests/gold_tests/traffic_ctl/remap_inc/remap_inc.test.py:
##########
@@ -14,16 +14,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-import os
-
Test.Summary = '''
Test traffic_ctl config reload with remap.config .include directive
'''
Test.ContinueOnFail = False
-Test.Setup.Copy("wait_reload.sh")
Review Comment:
Good catch, removed.
##########
tests/gold_tests/tls/ssl_multicert_loader.test.py:
##########
@@ -78,19 +78,13 @@
tr2.Processes.Default.Env = ts.Env
tr2.Processes.Default.ReturnCode = 0
-tr2reload = Test.AddTestRun("Reload config")
-tr2reload.StillRunningAfter = ts
+tr2reload = Test.AddConfigReload(ts, expect="fail", expect_tasks=["ssl_multicert.yaml"], description="Reload config")
tr2reload.StillRunningAfter = server
-tr2reload.Processes.Default.Command = 'traffic_ctl config reload'
-tr2reload.Processes.Default.Env = ts.Env
-tr2reload.Processes.Default.ReturnCode = 0
-ts.Disk.diags_log.Content = Testers.ContainsExpression('ERROR: ', 'ERROR')
Review Comment:
Yeah, on purpose. `expect="fail"` already asserts the reload exited with code `2` via monitor mode, and `expect_tasks=["ssl_multicert.yaml"]` confirms the right handler actually ran.
--
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] Add `Test.AddConfigReload()` autest extension [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13075:
URL: https://github.com/apache/trafficserver/pull/13075#issuecomment-4669159516
[approve ci]
--
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