Add AuTest for connect_attempts rr_retries and max_retries#12932
Add AuTest for connect_attempts rr_retries and max_retries#12932masaori335 merged 4 commits intoapache:masterfrom
Conversation
ab39cfa to
3b069bc
Compare
There was a problem hiding this comment.
Pull request overview
Adds new AuTest coverage for origin connect-attempt retry behaviors (round-robin retries vs max retries) and extends the ATS replay test harness to support DNS record injection and error.log validation via replay YAML.
Changes:
- Added three new Proxy Verifier replay scenarios covering
proxy.config.http.connect_attempts_rr_retriesandproxy.config.http.connect_attempts_max_retries. - Added new
dns/connect_attempts.test.pyto run the new replay tests and gold files forerror.logexpectations. - Extended
tests/gold_tests/autest-site/ats_replay.test.extto support DNSrecordsinjection anderror_logvalidation (including gold-file based validation).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/dns/replay/connect_attempts_rr_retries.replay.yaml | New replay scenario for round-robin retry behavior and down-server cache behavior. |
| tests/gold_tests/dns/replay/connect_attempts_rr_no_retry.replay.yaml | New replay scenario covering baseline behavior with no retries. |
| tests/gold_tests/dns/replay/connect_attempts_rr_max_retries.replay.yaml | New replay scenario covering connect_attempts_max_retries behavior. |
| tests/gold_tests/dns/gold/connect_attempts_rr_retries_error_log.gold | Expected error.log output for rr-retries scenario. |
| tests/gold_tests/dns/gold/connect_attempts_rr_no_error_log.gold | Expected error.log output for no-retry scenario. |
| tests/gold_tests/dns/gold/connect_attempts_rr_max_retries_error_log.gold | Expected error.log output for max-retries scenario. |
| tests/gold_tests/dns/connect_attempts.test.py | Test driver that runs the three new replay tests. |
| tests/gold_tests/autest-site/ats_replay.test.ext | Adds error_log validation support and DNS records wiring for replay-based tests. |
tests/gold_tests/dns/replay/connect_attempts_rr_no_retry.replay.yaml
Outdated
Show resolved
Hide resolved
tests/gold_tests/dns/replay/connect_attempts_rr_no_retry.replay.yaml
Outdated
Show resolved
Hide resolved
tests/gold_tests/dns/replay/connect_attempts_rr_max_retries.replay.yaml
Outdated
Show resolved
Hide resolved
tests/gold_tests/dns/replay/connect_attempts_rr_max_retries.replay.yaml
Outdated
Show resolved
Hide resolved
tests/gold_tests/dns/replay/connect_attempts_rr_retries.replay.yaml
Outdated
Show resolved
Hide resolved
tests/gold_tests/dns/replay/connect_attempts_rr_retries.replay.yaml
Outdated
Show resolved
Hide resolved
|
Somehow, the |
|
I found difference on my mac and docker env. We're hitting An idea of speed up this test is adding some features to proxy-verifer to refuse connection immediately. |
|
[approve ci autest 0of4] |
|
[approve ci autest 0] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
You can also share your feedback on Copilot code review. Take the survey.
| else: | ||
| dns = tr.MakeDNServer(name, default='127.0.0.1') | ||
| if 'records' in dns_config: | ||
| dns.addRecords(dns_config['records']) |
There was a problem hiding this comment.
For consistency with the rest of the test suite, consider calling dns.addRecords using the named argument (records=...). Every other caller in tests/gold_tests uses the keyword form, which makes the call site clearer and consistent.
| dns.addRecords(dns_config['records']) | |
| dns.addRecords(records=dns_config['records']) |
| proxy.config.diags.debug.tags: 'http|hostdb|dns' | ||
| proxy.config.http.connect_attempts_rr_retries: 2 | ||
| proxy.config.http.connect_attempts_max_retries: 0 | ||
| proxy.config.http.connect_attempts_max_retries_down_server: 0 |
There was a problem hiding this comment.
The PR description says this covers the combination of connect_attempts_rr_retries and connect_attempts_max_retries, but in this replay config connect_attempts_max_retries is set to 0 so the retry path that uses connect_attempts_rr_retries to rotate RR targets is never exercised. Consider adding (or adjusting) a scenario where both settings are non-zero so the combined behavior is actually tested.
| fields: | ||
| - [Host, example.com] | ||
| - [uuid, 20] | ||
| delay: 10s | ||
|
|
There was a problem hiding this comment.
This request uses a 10s delay to wait for proxy.config.http.down_server.cache_time (10s) to expire. Using an equal delay makes the test timing-sensitive, and the fixed 10s sleep significantly slows the gold test suite. Consider reducing the cache_time for the test and using a delay slightly greater than it (or keep cache_time=10 but make the delay >10s).
| fields: | ||
| - [Host, example.com] | ||
| - [uuid, 20] | ||
| delay: 10s |
There was a problem hiding this comment.
This request uses a 10s delay to wait for proxy.config.http.down_server.cache_time (5s) to expire. That makes the test unnecessarily slow; consider shrinking the delay to just over the cache_time (or lowering cache_time + delay together) to keep the suite fast while still ensuring the cache has expired.
| delay: 10s | |
| delay: 6s |
| fields: | ||
| - [Host, example.com] | ||
| - [uuid, 20] | ||
| delay: 10s |
There was a problem hiding this comment.
This request uses a 10s delay to wait for proxy.config.http.down_server.cache_time (10s) to expire. Using an equal delay can be timing-sensitive, and the fixed 10s sleep slows the suite. Consider reducing cache_time for the test and using a delay slightly greater than it (or keep cache_time=10 but make delay >10s).
| delay: 10s | |
| delay: 11s |
bryancall
left a comment
There was a problem hiding this comment.
Nice work on this, @masaori335. The test design is solid — using sm_id in the gold files to verify round-robin behavior across retries is clever, and the DNS record injection + error_log validation extensions to the autest harness are well done. Built and ran the test on an ASAN build and it passes cleanly. 👍
|
I'll merge this ignoring Copilot's suggestions. I'll revisit this if we find this is flaky. |
Covers combination of
proxy.config.http.connect_attempts_rr_retriesandproxy.config.http.connect_attempts_max_retries.The
connect_attempts_max_retries_down_serverwill be covered by #12922