Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ovn snat and fip delete #2584

Merged
merged 2 commits into from Apr 3, 2023
Merged

Conversation

ShaPoHun
Copy link
Contributor

What type of this PR

Examples of user facing changes:

  • Bug fixes
    If fip or snat is not ready, fip or snat cannot be deleted

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should explain what the patch does and why it is necessary.
  • There are some typos in the code comments, such as "delte" instead of "delete".
  • In both ovn_fip.go and ovn_snat.go, there is a potential bug where the cachedFip.Status.Ready or cachedSnat.Status.Ready condition may not be sufficient to ensure that the FIP or SNAT rule exists before attempting to delete it. It would be better to check for the existence of the rule explicitly before attempting to delete it.
  • There are some formatting errors in the code, such as inconsistent indentation and spacing. These should be fixed to improve readability.
  • There are no obvious performance issues or ways to improve the code.

pkg/controller/ovn_fip.go Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2023

  • The commit message is missing. It should provide a brief description of the changes made in the patch.
  • There are no potential bugs or format errors in the code patch diff.
  • There are no performance issues in the code patch diff.
  • In handleAddOvnDnatRule and handleAddOvnFip, there is a new call to patchOvnDnatStatus and patchOvnFipStatus respectively, but it is not clear what this function does. It would be helpful to add comments explaining the purpose of these functions.
  • In handleAddOvnSnatRule, there is a new call to patchOvnSnatStatus, but it is not clear what this function does. It would be helpful to add comments explaining the purpose of this function.
  • In handleDelOvnDnatRule, handleDelOvnFip, and handleUpdateOvnSnatRule, there is a check for cachedDnat.Status.Ready, cachedFip.Status.Vpc != "" && cachedFip.Status.V4Eip != "" && cachedFip.Status.V4Ip != "", and cachedSnat.Status.Vpc != "" && cachedSnat.Status.V4Eip != "" && cachedSnat.Status.V4IpCidr != "" respectively. It is not clear why these checks are necessary and what they are trying to achieve. It would be helpful to add comments explaining the purpose of these checks.
  • In handleDelOvnDnatRule, handleDelOvnFip, and handleUpdateOvnSnatRule, there is a call to klog.Errorf with the message "failed to delete dnat/fip/snat, %v". The error message should be more specific and provide more information about the error that occurred.

@bobz965 bobz965 merged commit da323a5 into kubeovn:master Apr 3, 2023
52 checks passed
@ShaPoHun ShaPoHun deleted the fix_ovn_delete branch April 6, 2023 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants