fix(e2e): poll for OVS bridge addresses in ovn-vpc-nat-gw test#6355
fix(e2e): poll for OVS bridge addresses in ovn-vpc-nat-gw test#6355
Conversation
The "Test ovn eip fip snat dnat" e2e test intermittently fails because the OVS bridge has no IP addresses when checked. This race occurs when the daemon re-processes the provider network event (triggered by the controller's periodic status update) and briefly reconfigures the bridge via OVS commands, right as the test queries `ip addr show`. Replace the one-shot bridge address assertion with a polling loop that waits up to 30s for the bridge addresses to stabilize before proceeding with the remaining validations. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
dosubot
bot
added
the
size:M
Summary of ChangesHello @oilbeater, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical flakiness issue in an end-to-end test related to OVN VPC NAT Gateway. The core problem stemmed from timing discrepancies during network configuration, leading to unstable bridge address reporting. The solution introduces a more resilient validation approach, ensuring test stability and reliability without altering core functionality. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a flaky e2e test by introducing a polling mechanism to wait for OVS bridge addresses to stabilize. The implementation is sound and directly targets the described race condition. I have one minor suggestion to improve the clarity of the framework.WaitUntil call, as its current usage could be misleading for future maintenance.
| ginkgo.By("get provider bridge v4 ip " + addr) | ||
| *bridgeIps = append(*bridgeIps, addr) | ||
| } | ||
| framework.WaitUntil(2*time.Second, 30*time.Second, func(_ context.Context) (bool, error) { |
There was a problem hiding this comment.
The framework.WaitUntil function appears to have a hardcoded polling interval of 2 seconds and ignores its first argument. While the value you've passed (2*time.Second) coincidentally matches the hardcoded interval, this is misleading and could cause confusion or bugs during future maintenance. The function signature in test/e2e/framework/wait.go is func WaitUntil(_, timeout time.Duration, ...) where the first argument is explicitly ignored.
To avoid confusion, it would be better to pass a zero value for the ignored interval. Ideally, framework.WaitUntil would be updated to accept and use the interval parameter, but since that's outside the scope of this PR, making the call less ambiguous is a good step.
| framework.WaitUntil(2*time.Second, 30*time.Second, func(_ context.Context) (bool, error) { | |
| framework.WaitUntil(0, 30*time.Second, func(_ context.Context) (bool, error) { |
…ling Three E2E tests had flaky patterns identified during analysis of recent fixes (#6343, #6345, #6347, #6349, #6355, #6358): node/node.go: - "should access overlay pods using node ip": RunHostCmdOrDie with no retry; OVN join subnet routes may not yet be installed on the host network stack when the pod turns Ready. Replace with WaitUntil (30s) and increase curl timeout from 2s to 5s. - "should access overlay services using node ip": same issue plus no endpoint readiness wait before the curl. Add endpoint WaitUntil (1m) then wrap the connectivity check with WaitUntil (30s). underlay/underlay.go: - "should be able to detect conflict vlan subnet": two time.Sleep(10s) calls used fixed waits instead of condition-based polling. Replace the first sleep with WaitUntil waiting for conflictVlan1 to be processed (non-conflicting), and the second with WaitUntil waiting for conflictVlan2.Status.Conflict to become true. - checkU2OFilterOpenFlowExist: manual "for range 3" retry loop with a hard 5s sleep. Replace with a deadline-based loop (30s, 2s interval) using time.Now().After for a clean timeout boundary. subnet/subnet.go: - "should detect MAC address conflict": time.Sleep(2s) before a one-shot event list is too short on a loaded cluster. Replace with WaitUntil (500ms interval, 15s timeout) polling for the AddressConflict Warning event. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ling (#6362) * fix(e2e): replace hard sleeps and unretried checks with WaitUntil polling Three E2E tests had flaky patterns identified during analysis of recent fixes (#6343, #6345, #6347, #6349, #6355, #6358): node/node.go: - "should access overlay pods using node ip": RunHostCmdOrDie with no retry; OVN join subnet routes may not yet be installed on the host network stack when the pod turns Ready. Replace with WaitUntil (30s) and increase curl timeout from 2s to 5s. - "should access overlay services using node ip": same issue plus no endpoint readiness wait before the curl. Add endpoint WaitUntil (1m) then wrap the connectivity check with WaitUntil (30s). underlay/underlay.go: - "should be able to detect conflict vlan subnet": two time.Sleep(10s) calls used fixed waits instead of condition-based polling. Replace the first sleep with WaitUntil waiting for conflictVlan1 to be processed (non-conflicting), and the second with WaitUntil waiting for conflictVlan2.Status.Conflict to become true. - checkU2OFilterOpenFlowExist: manual "for range 3" retry loop with a hard 5s sleep. Replace with a deadline-based loop (30s, 2s interval) using time.Now().After for a clean timeout boundary. subnet/subnet.go: - "should detect MAC address conflict": time.Sleep(2s) before a one-shot event list is too short on a loaded cluster. Replace with WaitUntil (500ms interval, 15s timeout) polling for the AddressConflict Warning event. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): replace per-node sleep with WaitUntil in vlan_subinterfaces test After patching ProviderNetwork with AutoCreateVlanSubinterfaces=false, the test verified existing subinterfaces were not deleted by sleeping 5 seconds inside the per-node loop (N nodes × 5s wasted time) and then doing an immediate assertion. This has two problems: 1. The sleep runs once per node, wasting N*5s even when the daemon reconciles quickly. 2. If the controller deletes a subinterface after the 5s sleep window, ExpectTrue produces a false-positive pass. Replace with WaitUntil (2s interval, 30s timeout) per node: the check passes on the first poll if subinterfaces are stable (common case), and retries up to 30s if there is any transient disruption, eliminating both issues. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(e2e): eliminate race between VIP finalizer addition and WaitToBeReady Root cause: handleAddVirtualIP called createOrUpdateVipCR to set Status.V4ip (triggering WaitToBeReady), but the finalizer was only added later in handleUpdateVirtualIP (triggered by a subsequent update event). This created a race window where CreateSync could return a VIP object with V4ip set but without the finalizer. Fix 1 (controller): In createOrUpdateVipCR's else branch, add the finalizer atomically in the same Update() call that sets spec/status, so the VIP is fully initialized in one API operation. Fix 2 (test framework): Update WaitToBeReady to require both an IP address AND the controller finalizer before declaring a VIP ready, ensuring tests only proceed with a fully-initialized VIP. Fix 3 (test): Add ginkgo.DeferCleanup for testVipName inside the It block so the VIP is deleted even on test failure, preventing the AfterEach subnetClient.DeleteSync from timing out. Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> --------- Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
[group:ovn-vpc-nat-gw] Test ovn eip fip snat dnatthat intermittently fails at line 341 withbridge.NonLinkLocalAddresses()returningnilinitProviderNetworkcall (triggered by controller's periodic status update every 30s) and the test'sip addr showcheck — the OVS bridge may briefly lose its addresses during the OVSDB transaction processingTest plan
ovn-vpc-nat-gwe2e test suite multiple times to verify the flaky failure no longer occursitFncalls (main provider network and extra provider network) benefit from the fix🤖 Generated with Claude Code