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

netpol: fix packet drop casued by incorrect address set deletion #2677

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

zhangzujian
Copy link
Member

@zhangzujian zhangzujian commented Apr 19, 2023

What type of this PR

  • Bug fixes

Which issue(s) this PR fixes:

Fixes #(issue-number)

WHAT

🤖 Generated by Copilot at 7ca8b88

Simplify network policy controller by removing address set deletion logic. This improves performance and consistency with the address set controller.

🤖 Generated by Copilot at 7ca8b88

The network policy controller was complex
It deleted address sets with side effects
But now it's more simple
It just sets a np label
And lets the address set controller do the rest

HOW

🤖 Generated by Copilot at 7ca8b88

  • Remove the code that deletes the ingress and egress address sets associated with the network policy when it is updated (link, link in network_policy.go). This avoids unnecessary deletion and recreation of address sets, which can cause performance issues and inconsistency in the OVN database. The address sets are now managed by a separate controller that handles the creation, update and deletion of address sets based on the pod and namespace labels, and the network policy controller only needs to reference the existing address sets by name.

@zhangzujian zhangzujian added bug Something isn't working need backport labels Apr 19, 2023
@github-actions
Copy link
Contributor

  • The commit message is missing. It should provide a brief description of the changes made in this patch.
  • There are two blocks of code that have been commented out instead of being removed. These should be removed to avoid confusion and unnecessary clutter.
  • There are multiple instances where the same error is returned without any additional information or context. This can make it difficult to debug issues later on. Consider adding more descriptive error messages.
  • There are some variables that are declared but not used, such as ingressAclCmd and egressAclCmd. These should be removed to improve readability and reduce clutter.
  • There are some repeated lines of code that could be refactored into functions to improve code reuse and maintainability. For example, the code for deleting address sets is repeated twice.

@zhangzujian zhangzujian marked this pull request as ready for review April 19, 2023 09:24
@github-actions
Copy link
Contributor

  • The commit message is missing. Please add a descriptive commit message.
  • There are two blocks of code that are identical and could be refactored into a function to avoid duplication.
  • The deletion of ingress and egress address sets is commented out, but it's not clear why. If this is intentional, please add a comment explaining why these lines were removed. Otherwise, please uncomment them.
  • There is a potential performance issue in the handleUpdateNp function. The function makes multiple calls to c.ovnClient.PortGroupExists, which can be expensive. Consider caching the result of the first call to PortGroupExists and reusing it for subsequent calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants