test(e2e): fixed trafficTestResult interval for (systemd_)resiliency - #670
test(e2e): fixed trafficTestResult interval for (systemd_)resiliency#670andreaskaris wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe resiliency tests now verify traffic after router recovery. Traffic polling uses shorter intervals and curl timeouts. Evaluation rejects empty measurements, permits up to 40 failures, and reports failure timestamps. ChangesResiliency traffic validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ResiliencyTest
participant RouterPod
participant curl
participant trafficTestResult
ResiliencyTest->>RouterPod: Delete or restart router pod
RouterPod-->>ResiliencyTest: Complete recovery
ResiliencyTest->>curl: Verify stretched-L2 traffic
ResiliencyTest->>trafficTestResult: Evaluate traffic disruption
trafficTestResult-->>ResiliencyTest: Return result or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2etests/tests/resiliency.go (1)
697-700: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the threshold comment to the code.
The comment states that the test permits 10 failures.
maxAllowedFailurespermits 40 failures. Update the comment to prevent an incorrect disruption budget from being maintained later.🤖 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 `@e2etests/tests/resiliency.go` around lines 697 - 700, Update the comment above eval to state that the test permits up to 40 failures, and revise the corresponding downtime calculation to match the 500ms tick interval.
🤖 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.
Inline comments:
In `@e2etests/tests/resiliency.go`:
- Around line 671-685: Update the executor calls at
e2etests/tests/resiliency.go:671-685, e2etests/tests/resiliency.go:642-646, and
e2etests/tests/systemd_resiliency.go:454-458 to use the deadline-aware
Executor.Exec variant, passing the relevant collector or test context so the
enclosing kubectl exec is cancelled when the deadline or spec ends. Preserve the
existing curl arguments and result handling.
In `@e2etests/tests/systemd_resiliency.go`:
- Around line 460-466: Update the failure message in the stopAndCount result
assertion to describe routerpod-pod.service restart and recovery instead of
router pod deletion and recovery, while preserving the existing threshold and
result details.
---
Nitpick comments:
In `@e2etests/tests/resiliency.go`:
- Around line 697-700: Update the comment above eval to state that the test
permits up to 40 failures, and revise the corresponding downtime calculation to
match the 500ms tick interval.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 408a2ccc-8c91-487b-b05a-cde71eab239c
📒 Files selected for processing (2)
e2etests/tests/resiliency.goe2etests/tests/systemd_resiliency.go
1b074bc to
5c4c550
Compare
|
@CodeRabbit review |
|
| } | ||
| } | ||
|
|
||
| // eval evaluates the trafficTestResult. We are conservative and allow for up to |
There was a problem hiding this comment.
doesn't this mean we are more forgiving now?
There was a problem hiding this comment.
I rephrased that comment
5c4c550 to
1c76038
Compare
|
|
||
| // eval evaluates the trafficTestResult. In order to reduce flakes, we allow for up to 40 failures, | ||
| // Meaning that with a tick interval of 500ms, we allow up to ca. 20 seconds of downtime. | ||
| // We need to be a bit more lenient here as depending on the reconcile timing, BGP sessions might take a while to |
There was a problem hiding this comment.
@maiqueb is this too much for your migration target?
If so, we need to understand if either we have a bug or we need to tighten up the tests
There was a problem hiding this comment.
20 seconds of downtime is not acceptable for virt.
I think we need to better understand the error pattern before allowing the tests to be more lenient.
There was a problem hiding this comment.
the test did allow for 2.3 * 5 = ca. 12 seconds, so I'll tweak it to that value
1c76038 to
99c4966
Compare
measureTrafficLoss did not use a fixed interval for when it ran its tests. Meaning that if a curl hung, each run would happen at ca. 2 seconds + 300 ms. Use a 500ms ticker instead and cap the curl at 400ms. Set maxAllowedFailures to 26 to allow for a maximum ~12 seconds of downtime which is close to the max downtime from the earlier version. Also make sure that traffic actually works again before stopping and counting. Signed-off-by: Andreas Karis <ak.karis@gmail.com> Reported-at: openperouter#664
99c4966 to
eaf6d1a
Compare
3 participants
Is this a BUG FIX or a FEATURE ?:
/kind flake
What this PR does / why we need it:
measureTrafficLoss did not use a fixed interval for when it ran its
tests. Meaning that if a curl hung, each run would happen at ca.
2 seconds + 300 ms. Use a 500ms ticker instead and cap the curl at
400ms. Set maxAllowedFailures to 40 to allow for a maximum ~20 seconds
of downtime.
Also make sure that traffic actually works again before stopping and
counting.
Special notes for your reviewer:
Related to: #664
This PR adds a few improvements and increases the timeout by a few seconds.
However, the underlying RC for the issue seem to be repeated reconciles which in turn delay BGP session establishment and I'm not sure how to address this.
Release note:
AI Guidelines Acknowledgment:
Summary by CodeRabbit