fix(e2e): fix checkPolicy logic bug and improve underlay test diagnostics#6346
fix(e2e): fix checkPolicy logic bug and improve underlay test diagnostics#6346
Conversation
The waitSubnetU2OStatus() function waits for the Subnet.Status.U2OInterconnectionVPC field to be populated, which requires coordination between the Subnet Controller and VPC Controller. This is a cross-controller operation that involves: 1. Subnet Controller setting U2OInterconnectionIP (100-500ms) 2. VPC Controller updating VPC.Status.Router (100-500ms) 3. Subnet Controller reading VPC.Status.Router and setting U2OInterconnectionVPC (500-2000ms) The total operation can take 500-2500ms, which exceeds the original 3 second timeout. This causes intermittent test failures, especially after controller restarts. The fix increases the timeout to 30 seconds, consistent with waitSubnetStatusUpdate() and the framework's default timeout of 2 minutes. This provides adequate buffer for normal operation while still detecting real failures. Fixes intermittent timeouts in "should support underlay to overlay subnet interconnection" test. Co-Authored-By: Mengxin Liu <liumengxinfly@gmail.com>
…tics 1. Fix checkPolicy() logic bug: when expectPolicyExist=false, the original code returned true as soon as the first line did not contain the policy, without checking remaining lines. Now it scans all lines to determine whether the policy exists, then compares against the expectation. 2. Increase checkPolicy() timeout from 10s to 30s, consistent with other wait functions in the test. 3. Add meaningful condDesc to waitSubnetStatusUpdate(), waitSubnetU2OStatus(), and checkPolicy() so timeout failures produce actionable error messages instead of empty descriptions. 4. Replace ginkgo.By() with framework.Logf() inside polling loops in waitSubnetU2OStatus() to avoid excessive test output. 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>
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 enhances the reliability and diagnostic capabilities of end-to-end tests for Kube-OVN's underlay-to-overlay subnet interconnection. It addresses a critical logic flaw in the 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a logic bug in the checkPolicy function and significantly improves the diagnostics and stability of the underlay e2e tests. The fix in checkPolicy is correct and ensures proper behavior when checking for policy absence. Increasing timeouts to 30s for wait functions is a good step towards reducing test flakiness. The addition of descriptive messages for wait conditions and the cleanup of logging within polling loops are excellent improvements for maintainability and debugging. Overall, these are solid enhancements to the test suite.
I am having trouble creating individual review comments. Click here to see my feedback.
test/e2e/kube-ovn/underlay/underlay.go (94-95)
Removing this redundant log statement is a good cleanup. However, similar redundant framework.Logf calls still exist within the if enableU2O block (lines 79-80 and 85-86 in the file). These logs are also triggered when the wait condition is met. For consistency and to further reduce log noise, consider removing those logging statements as well.
Summary
checkPolicy()logic bug: whenexpectPolicyExist=false, the original code returnedtrueas soon as the first line did not contain the policy string, without scanning remaining lines. Now it correctly scans all lines before comparing against the expectation.checkPolicy()timeout from 10s to 30s, consistent with other wait functions (waitSubnetStatusUpdate,waitSubnetU2OStatus).condDesctowaitSubnetStatusUpdate(),waitSubnetU2OStatus(), andcheckPolicy()so timeout failures produce actionable error messages instead of empty strings liketimed out while waiting for the condition to be met:.ginkgo.By()withframework.Logf()inside polling loops inwaitSubnetU2OStatus()to reduce noisy test output.Test plan
checkPolicy()correctly detects both policy presence and absence🤖 Generated with Claude Code