Skip to content

fix: add retry logic to vpc-egress-gateway e2e curl check#6345

Merged
oilbeater merged 1 commit intomasterfrom
fix/veg-e2e-curl-retry
Feb 26, 2026
Merged

fix: add retry logic to vpc-egress-gateway e2e curl check#6345
oilbeater merged 1 commit intomasterfrom
fix/veg-e2e-curl-retry

Conversation

@oilbeater
Copy link
Copy Markdown
Collaborator

Summary

  • Replace RunHostCmdOrDie with WaitUntil + RunHostCmd in checkEgressAccess() to add retry logic (2s interval, 30s timeout) for the curl connectivity check
  • Fixes flaky e2e test "should be able to create vpc-egress-gateway with underlay subnet" which intermittently fails with curl exit code 28 (connection timeout)
  • Root cause: VEG Status.Ready only indicates control plane readiness; OVN dataplane convergence (NB→SB propagation, ovn-controller flow installation, BFD session establishment) is asynchronous and may take several seconds

Test plan

  • Run vpc-egress-gateway e2e tests multiple times to verify flakiness is resolved
  • Verify the retry pattern matches existing CheckPodEgressRoutes usage in the same test suite

🤖 Generated with Claude Code

The curl command in checkEgressAccess() used RunHostCmdOrDie which fails
immediately without retry. This causes flaky failures because VEG
Status.Ready only indicates control plane readiness, while the OVN
dataplane (NB→SB propagation, ovn-controller flow installation, BFD
session establishment) may still be converging.

Replace RunHostCmdOrDie with WaitUntil + RunHostCmd to retry for up to
30 seconds, matching the pattern already used by CheckPodEgressRoutes.

Signed-off-by: Mengxin Liu <liumengxinfly@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. test automation tests labels Feb 26, 2026
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @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 enhances the robustness of VPC Egress Gateway end-to-end tests by introducing a retry mechanism for network connectivity checks. This change specifically targets a flaky test that intermittently failed due to the asynchronous nature of network dataplane convergence, ensuring that tests wait for network readiness before asserting connectivity.

Highlights

  • E2E Test Stability: Implemented retry logic for curl connectivity checks within the checkEgressAccess() function to mitigate intermittent failures in VPC Egress Gateway e2e tests.
  • Retry Mechanism: Replaced RunHostCmdOrDie with a WaitUntil loop wrapping RunHostCmd, allowing for retries every 2 seconds with a total timeout of 30 seconds.
  • Root Cause Addressed: Addressed test flakiness caused by asynchronous OVN dataplane convergence, which can lead to temporary network unavailability even after the control plane indicates readiness.
Changelog
  • test/e2e/vpc-egress-gateway/e2e_test.go
    • Replaced a direct curl command execution with a WaitUntil retry loop to handle transient network issues.
    • Updated error handling for RunHostCmd within the retry block.
Activity
  • The author plans to run vpc-egress-gateway e2e tests multiple times to verify flakiness is resolved.
  • The author plans to verify the retry pattern matches existing CheckPodEgressRoutes usage in the same test suite.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces retry logic to an e2e test to address flakiness, which is a solid improvement. The use of WaitUntil is appropriate for handling the asynchronous nature of network convergence. My review includes a suggestion to enhance debuggability by adding logging within the retry loop. I've also noted a point of confusion in the WaitUntil helper function that could be addressed in a future refactoring to improve the test framework's clarity.

clientIP, _, err := net.SplitHostPort(strings.TrimSpace(output))
framework.ExpectNoError(err)
var clientIP string
framework.WaitUntil(2*time.Second, 30*time.Second, func(_ context.Context) (bool, error) {
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.

medium

This retry logic is a good improvement for the flaky test. I have two suggestions for further improvement:

  1. WaitUntil function signature: The framework.WaitUntil function is a bit misleading. It accepts an interval as its first argument, but this argument is ignored, and a hardcoded 2-second interval is used instead. Your call works because you've passed 2*time.Second, matching the hardcoded value. This is worth noting as it has caused issues elsewhere (e.g., CheckPodEgressRoutes passes a 3-second interval that is ignored). A follow-up to refactor WaitUntil to use the interval parameter would improve the framework's clarity.

  2. Debugging: To make this test easier to debug if it times out, consider adding logging within the retry loop to show why a retry is happening. For example:

    func(_ context.Context) (bool, error) {
        output, err := e2epodoutput.RunHostCmd(pod.Namespace, pod.Name, cmd)
        if err != nil {
            framework.Logf("curl command failed, will retry: %v", err)
            return false, nil
        }
        clientIP, _, err = net.SplitHostPort(strings.TrimSpace(output))
        if err != nil {
            framework.Logf("failed to parse curl output, will retry. output: %q, error: %v", output, err)
        }
        return err == nil, nil
    }

@oilbeater oilbeater merged commit 8e592b3 into master Feb 26, 2026
75 of 76 checks passed
@oilbeater oilbeater deleted the fix/veg-e2e-curl-retry branch February 26, 2026 05:33
oilbeater added a commit that referenced this pull request Feb 27, 2026
…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>
oilbeater added a commit that referenced this pull request Feb 27, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files. test automation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant