NetworkPolicy: bulk-add pods to new policies (or on restart) #2249
Conversation
|
@jcaamano @JacobTanenbaum could you take a look? |
squeed
changed the title
|
All green. I manually ran the new 1.21 netpol test matrix against this and it passed, so there's no regressions there. |
Signed-off-by: Casey Callendrello <cdc@redhat.com>
Updates mocks to reflect change in go-ovn, updates test cases. Update helper functions to allow for bulk operations. Signed-off-by: Casey Callendrello <cdc@redhat.com>
This adds a factory sync function for network policy that bulk-sets all pods on policy create. This means that creating a network policy that selects a large number of pods is much more efficient. Additionally, this means that ovn-kube startup time for large policies should be much faster. Signed-off-by: Casey Callendrello <cdc@redhat.com>
|
/lgtm |
|
Test failure was:
which is unrelated to the NP changes here... |
| if err = ovnNBClient.Execute(cmd); err != nil { | ||
| return fmt.Errorf("execute error adding port %s to port group %s (%v)", portInfo.name, portGroupName, err) | ||
| func addToPortGroup(ovnNBClient goovn.Client, portGroupName string, ports ...*lpInfo) error { | ||
| cmds := make([]*goovn.OvnCommand, 0, len(ports)) |
There was a problem hiding this comment.
Does the size of this need to be limited? I know the services controller ran into an issue where the command batch was too large for nbdb to process
There was a problem hiding this comment.
that was cause we are using bash nbctl calls there, since this is using go-bindings it should be more robust
There was a problem hiding this comment.
and im not sure if there is any size limit to number of transactions in an ovsdb call...should find out
| if err == goovn.ErrorNotFound { | ||
| return nil | ||
| } else { | ||
| return fmt.Errorf("error preparing removing port %v from port group %s (%v)", portInfo.name, portGroupName, err) |
There was a problem hiding this comment.
if we get stuck in a situation where one port has a problem, we are returning out of this function immediately, rather than trying to delete what we can and returning an error for the ports that failed. In ovn-kube delete is usually best effort, and in most of the cases where deleteFromPortGroup is called, the error is simply logged. Would it make more sense to try to remove everything we can, and still return an error?
There was a problem hiding this comment.
So, there are only two cases that return an error when preparing the command:
1 - the PortGroup itself doesn't exist (or the db is down) - so clearly we can return
2 - the UUID can't be parsed as a valid UUID - so something is very broken.
So, we don't really need to worry about the case where a single prepare fails.
Bumps go-ovn to pick up eBay/go-ovn#149
This adds a factory sync function for network policy that bulk-sets all pods on policy create. This means that creating a network policy that selects a large number of pods is much more efficient.
Additionally, this means that ovn-kube startup time for large policies should be much faster.