Skip to content

fix(failover): increase timeout for dns failover test - #1039

Merged
averevki merged 1 commit into
Kuadrant:mainfrom
averevki:increase-failover-timeout
Aug 12, 2026
Merged

fix(failover): increase timeout for dns failover test#1039
averevki merged 1 commit into
Kuadrant:mainfrom
averevki:increase-failover-timeout

Conversation

@averevki

@averevki averevki commented Aug 11, 2026

Copy link
Copy Markdown
Member

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

  • Tests
    • Extended the DNS failover verification wait time to 60 seconds, improving test reliability when switching traffic to the secondary cluster.

Signed-off-by: averevki <sandyverevkin@gmail.com>
@averevki
averevki requested a review from a team August 11, 2026 13:56
@averevki averevki self-assigned this Aug 11, 2026
@averevki averevki added bug Something isn't working DNS Operator Changes related to DNS Operator and resources it manages labels Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The DNS failover test now waits 60 seconds after removing the first cluster from active groups before it verifies failover to the second cluster.

Changes

DNS failover validation

Layer / File(s) Summary
Post-removal failover wait
testsuite/tests/multicluster/failover/test_dns_failover.py
The test increases the post-failover wait from 40 to 60 seconds.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

  • Kuadrant/testsuite#1020: Modifies the same DNS failover test and extends its propagation wait from 40 to 60 seconds.

Suggested labels: test-case

Poem

A rabbit waits by the DNS gate,
Sixty seconds to validate.
The first cluster hops away,
The second serves the test today.
Failover clear, the burrow cheers.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the problem and rationale but omits the required Changes and Verification sections. Add the main changes and describe how the test or nightly run was verified.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the timeout increase and follows the required Conventional Commits format.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
testsuite/tests/multicluster/failover/test_dns_failover.py (1)

80-80: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

Prefer a bounded DNS-state wait over a fixed 60-second delay.

time.sleep(60) remains an arbitrary readiness condition. If the provider removes the gw1_ip record 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 existing DNS_PROPAGATION_WAIT value in testsuite/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

📥 Commits

Reviewing files that changed from the base of the PR and between 80a3ede and 32ad1a7.

📒 Files selected for processing (1)
  • testsuite/tests/multicluster/failover/test_dns_failover.py

@silvi-t silvi-t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. With 60s sleep the test passes. Consider moving this value to testsuite/utils/constants.py alongside the existing DNS_PROPAGATION_WAIT constant.

@averevki
averevki merged commit 5fc9fae into Kuadrant:main Aug 12, 2026
6 checks passed
@averevki
averevki deleted the increase-failover-timeout branch August 12, 2026 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working DNS Operator Changes related to DNS Operator and resources it manages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants