Skip to content

Add tests for VIP finalizer handling and subnet status updates#6068

Merged
zbb88888 merged 2 commits intokubeovn:masterfrom
zbb88888:fix-other-ip-count
Dec 18, 2025
Merged

Add tests for VIP finalizer handling and subnet status updates#6068
zbb88888 merged 2 commits intokubeovn:masterfrom
zbb88888:fix-other-ip-count

Conversation

@zbb88888
Copy link
Copy Markdown
Collaborator

@zbb88888 zbb88888 commented Dec 18, 2025

  • Introduced a new test to verify that the subnet status is correctly updated when a VIP is created and deleted, ensuring that finalizers are properly handled.
  • Added checks for both IPv4 and IPv6 protocols, including dual stack scenarios, to confirm that available and using IP counts and ranges are updated as expected.
  • Enhanced the existing VIP creation test to wait for the finalizer to be added before proceeding with subnet status verification.
  • Updated sleep durations to ensure sufficient time for status updates after VIP operations.

Pull Request

What type of this PR

Examples of user facing changes:

  • Bug fixes

  • 将其他三种从 kube-ovn-controller 外部的 ip 的 finalizer 添加和删除调整为基本一致,避免出现subnet status 统计不准导致 subnet 一直触发更新入队的问题

  • 调整一部分 CRd del equeue 的指针指向 copy 后的资源(不需要,已回滚)

  • 将 iptables 的 e2e 拆分为两个,qos 的测试流程耗时较长 > 15分钟。 便于快速验证 iptables-eip CR 业务问题和 qos 功能问题

Which issue(s) this PR fixes

Fixes #(issue-number)

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @zbb88888, 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 significantly enhances the stability and observability of network resource management by refining how finalizers are handled across various IP-related Custom Resources (CRs) like VIPs, EIPs, FIPs, DNATs, and SNATs. The changes ensure that resources are properly cleaned up and that subnet status accurately reflects IP allocation and release, even in complex scenarios involving dependencies or race conditions. Additionally, new end-to-end tests have been added to validate these improvements and introduce testing for EIP QoS policies.

Highlights

  • Refactored Finalizer Handling: Improved the finalizer logic for Virtual IPs (VIPs), External IPs (EIPs), Floating IPs (FIPs), Destination NAT (DNAT) rules, and Source NAT (SNAT) rules to ensure robust resource cleanup and prevent premature deletion of dependent resources.
  • Enhanced Subnet Status Updates: Implemented more precise and timely updates to subnet status metrics (available and using IP counts/ranges) during the creation and deletion of VIPs and EIPs, covering IPv4, IPv6, and dual-stack scenarios.
  • New E2E Tests for EIP QoS and Finalizers: Introduced a new end-to-end test suite specifically for EIP Quality of Service (QoS) policies, including testing default, EIP-specific, and IP-specific QoS rules, and added comprehensive tests for finalizer behavior and subnet status updates.
  • VPC NAT Gateway NoDefaultEIP Support: Added functionality and tests for creating VPC NAT Gateways with the NoDefaultEIP option, allowing users to prevent automatic EIP creation and manage EIPs manually.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

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 a significant and valuable refactoring of the finalizer and deletion logic for several CRDs, including OvnEip, Vip, and IptablesEip. The changes align with standard controller patterns by handling deletions through the update handler when a DeletionTimestamp is present. This makes the resource cleanup process more robust. The addition of comprehensive e2e tests to validate this new behavior is also a great improvement.

My main concern is the repeated use of time.Sleep in the controller logic to handle synchronization. This approach is brittle and can lead to race conditions. I've left specific comments with suggestions to replace these sleeps with more robust, event-driven mechanisms.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 18, 2025

Pull Request Test Coverage Report for Build 20331778203

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 312 (0.0%) changed or added relevant lines in 10 files are covered.
  • 21 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.06%) to 22.603%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controller/ippool.go 0 1 0.0%
pkg/controller/controller.go 0 2 0.0%
pkg/controller/subnet.go 0 2 0.0%
pkg/controller/pod.go 0 3 0.0%
pkg/controller/ovn_fip.go 0 34 0.0%
pkg/controller/ovn_snat.go 0 34 0.0%
pkg/controller/ovn_dnat.go 0 36 0.0%
pkg/controller/vip.go 0 60 0.0%
pkg/controller/vpc_nat_gw_eip.go 0 60 0.0%
pkg/controller/ovn_eip.go 0 80 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/controller/ovn_dnat.go 2 0.0%
pkg/controller/ovn_fip.go 2 0.0%
pkg/controller/ovn_snat.go 2 0.0%
pkg/controller/vpc_nat_gw_eip.go 4 0.0%
pkg/controller/vip.go 5 0.0%
pkg/controller/ovn_eip.go 6 0.0%
Totals Coverage Status
Change from base Build 20330664063: -0.06%
Covered Lines: 12052
Relevant Lines: 53320

💛 - Coveralls

@zbb88888 zbb88888 requested a review from oilbeater December 18, 2025 06:20
@zbb88888 zbb88888 marked this pull request as ready for review December 18, 2025 06:20
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. test automation tests labels Dec 18, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 18, 2025
- Introduced a new test to verify that the subnet status is correctly updated when a VIP is created and deleted, ensuring that finalizers are properly handled.
- Added checks for both IPv4 and IPv6 protocols, including dual stack scenarios, to confirm that available and using IP counts and ranges are updated as expected.
- Enhanced the existing VIP creation test to wait for the finalizer to be added before proceeding with subnet status verification.
- Updated sleep durations to ensure sufficient time for status updates after VIP operations.

Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
@zbb88888 zbb88888 merged commit 2346e35 into kubeovn:master Dec 18, 2025
8 checks passed
@zbb88888 zbb88888 deleted the fix-other-ip-count branch December 18, 2025 09:10
zbb88888 added a commit to zbb88888/kube-ovn that referenced this pull request Dec 18, 2025
…vn#6068)

* Add tests for VIP finalizer handling and subnet status updates

- Introduced a new test to verify that the subnet status is correctly updated when a VIP is created and deleted, ensuring that finalizers are properly handled.
- Added checks for both IPv4 and IPv6 protocols, including dual stack scenarios, to confirm that available and using IP counts and ranges are updated as expected.
- Enhanced the existing VIP creation test to wait for the finalizer to be added before proceeding with subnet status verification.
- Updated sleep durations to ensure sufficient time for status updates after VIP operations.

Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>

* fix after review

Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>

---------

Signed-off-by: zbb88888 <jmdxjsjgcxy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files. test automation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants