fix(failover): increase timeout for dns failover test - #1039
Conversation
Signed-off-by: averevki <sandyverevkin@gmail.com>
📝 WalkthroughWalkthroughThe DNS failover test now waits 60 seconds after removing the first cluster from active groups before it verifies failover to the second cluster. ChangesDNS failover validation
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testsuite/tests/multicluster/failover/test_dns_failover.py (1)
80-80: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftPrefer a bounded DNS-state wait over a fixed 60-second delay.
time.sleep(60)remains an arbitrary readiness condition. If the provider removes thegw1_iprecord after 60 seconds, the assertion at Line 86 can still fail. If removal completes earlier, every successful run waits unnecessarily.Poll
dns.resolver.resolve(hostname.hostname)until the result is{gw2_ip}, with a bounded timeout based on the existingDNS_PROPAGATION_WAITvalue intestsuite/utils/constants.py. Keep the final assertion for failure diagnostics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@testsuite/tests/multicluster/failover/test_dns_failover.py` at line 80, Replace the fixed time.sleep(60) in the failover test with bounded polling of dns.resolver.resolve(hostname.hostname) until the resolved addresses equal {gw2_ip}, using DNS_PROPAGATION_WAIT to define the timeout. Retain the final assertion so failures still provide diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@testsuite/tests/multicluster/failover/test_dns_failover.py`:
- Line 80: Replace the fixed time.sleep(60) in the failover test with bounded
polling of dns.resolver.resolve(hostname.hostname) until the resolved addresses
equal {gw2_ip}, using DNS_PROPAGATION_WAIT to define the timeout. Retain the
final assertion so failures still provide diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f901f11b-578a-4bbe-8471-f01ec4798a54
📒 Files selected for processing (1)
testsuite/tests/multicluster/failover/test_dns_failover.py
silvi-t
left a comment
There was a problem hiding this comment.
LGTM. With 60s sleep the test passes. Consider moving this value to testsuite/utils/constants.py alongside the existing DNS_PROPAGATION_WAIT constant.
Description
Test is failing for the last couple of days because the dns record for the first cluster doesn't have enough time to clean up from a provider, after cluster with this dns record is set as inactive during the test execution. Increasing the timeout slightly for the test to work on nightly runs
Summary by CodeRabbit