Extract and optimize group member calculation #1937
Conversation
|
@jianjuns @antoninbas @Dyanngg Please let me know if the change makes sense to you. And whether it can help nested group feature or make it harder? @Dyanngg |
Codecov Report
@@ Coverage Diff @@
## main #1937 +/- ##
==========================================
+ Coverage 64.35% 66.69% +2.34%
==========================================
Files 193 197 +4
Lines 16967 17174 +207
==========================================
+ Hits 10919 11455 +536
+ Misses 4899 4551 -348
- Partials 1149 1168 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I'm still reviewing the |
I removed the field as it was consumed by |
There was a problem hiding this comment.
@Dyanngg and @antoninbas thanks for your quick review and feedback.
| labelItems map[string]*labelItem | ||
| // labelIndex is nested map from entityType to Namespace to keys of labelItems. | ||
| // It's used to filter potential labelItems when matching a namespace scoped selectorItem. | ||
| labelIndex map[entityType]map[string]sets.String |
There was a problem hiding this comment.
Is it more efficient to create two separate maps for Pods and ExternalEntities?
There was a problem hiding this comment.
Or to share code, we can convert them to one internal type?
There was a problem hiding this comment.
The code of matching Pods and ExtenalEntities is already shared and they are treated in same way when stored in labelItems.
Separating them in labelIndex is to reduce the potential labelItems a selector needs to match. This is because a selector can either select ExternalEntity or Pod. If it's a Pod selector, this is no need to try ExternalEntity labelItems. The same reason applies to selectorIndex.
tnqn
force-pushed
the
grouping
branch
2 times, most recently
from
fcf9851
to
a9126a2
Compare
|
@jianjuns @antoninbas I updated benchmark test result in PR description. To get accurate cpu and memory metrics, I used benchmark test and execute the processing in serial manner. |
| // GetEntities returns the selected Pods or ExternalEntities for the given group. | ||
| GetEntities(groupType string, name string) ([]*v1.Pod, []*v1alpha2.ExternalEntity) | ||
| // GetGroupsForPod returns the groups that select the given Pod. | ||
| GetGroupsForPod(namespace, name string) (map[string][]string, bool) |
There was a problem hiding this comment.
do you think we could define a new type for the group type: type GroupType string? It would help clarify what the returned values are if we have map[GroupType][]string instead of map[string][]string.
There was a problem hiding this comment.
I defined GroupType and listed all values here at the begining, then found it makes consumers must add their own type here. Maybe I should define GroupType (using string) here and let consumers define their own values in their own packages, does it make sense to you?
There was a problem hiding this comment.
I feel enum (int) also works, but I guess your concern is harder to track the int values when they are not in the same package?
There was a problem hiding this comment.
Maybe I should define GroupType (using string) here and let consumers define their own values in their own packages, does it make sense to you?
That works for me as a middle ground between using a plain string and defining named constants for all group types in this package.
There was a problem hiding this comment.
@jianjuns Yes, if using enum(int), we should define them together in this package to avoid conflict.
There was a problem hiding this comment.
I think it is just like string is easier to remember and has less chance to conflict (but can still). I do not like long string lookup, but I understand your point and no strong opinion.
| @@ -81,6 +82,10 @@ const ( | |||
| PriorityIndex = "priority" | |||
| // ClusterGroupIndex is used to index ClusterNetworkPolicies by ClusterGroup names. | |||
| ClusterGroupIndex = "clustergroup" | |||
|
|
|||
| appliedToGroupType = "appliedToGroup" | |||
There was a problem hiding this comment.
Then do you like to change this one to enum too?
| // GetEntities returns the selected Pods or ExternalEntities for the given group. | ||
| GetEntities(groupType string, name string) ([]*v1.Pod, []*v1alpha2.ExternalEntity) | ||
| // GetGroupsForPod returns the groups that select the given Pod. | ||
| GetGroupsForPod(namespace, name string) (map[string][]string, bool) |
|
/test-all |
tnqn
changed the title
tnqn
force-pushed
the
grouping
branch
4 times, most recently
from
def159c
to
17f15eb
Compare
tnqn
changed the title
|
/test-all |
|
/test-all |
|
@jianjuns @antoninbas @Dyanngg I added more unit tests and a method |
|
/test-all |
|
/test-e2e |
tnqn
dismissed stale reviews from jianjuns, Dyanngg, and antoninbas
via
1ea74f5
|
/test-all |
Previously grouping member calculation was coupled with the NetworkPolicyController, and there were two kinds of heavy calculation in the process: For each event like Pod add, it needed to find all affected groups by matching the new Pod's labels with all groups that can potentially select it. If it was an update event, it even needed to perform the process one more time for its old labels. Then it triggered the sync process of the affected groups. For the sync process of a group, it needed to find all Pods or ExternalEntities by matching its selectors with all entities that can potentially match it. For each pair of Pod and Group, they would be matched at least twice. Besides, whenever the group was processed another time, it would again scan all Pods. The repeated calculation can be eliminated by caching the result of the first matching process when processing Pod events. Then the matching process will only occur when there's a new Pod or a new Group. Besides, most Pods actually share same labels with others when they are managed by same controller. The cache can be further optimized to maintain the relationship between label set and label selector, making Pods that having same labels and Groups that having same selectors share the cache. This patch introduces an interface which is responsible for group member calculation and an implementation implementing it with the above optimization. The interface is then consumed by NetworkPolicyController and features like EndpointQuerier and ClusterGroup to retrieve Pods/ExternalEntities selected by a given Group or Groups that select a given Pod. Besides, other controllers that have the group logic like EgressPolicy can consume it directly without having redundant code with NetworkPolicyController.
|
/test-conformance |
|
/test-conformance |
|
/test-networkpolicy |
Previously grouping member calculation was coupled with the NetworkPolicyController, and there were two kinds of heavy calculation in the process:
For each event like Pod add, it needed to find all affected groups by matching the new Pod's labels with all groups that can potentially select it. If it was an update event, it even needed to perform the process one more time for its old labels. Then it triggered the sync process of the affected groups.
For the sync process of a group, it needed to find all Pods or ExternalEntities by matching its selectors with all entities that can potentially match it.
For each pair of Pod and Group, they would be matched at least twice. Besides, whenever the group was processed another time, it would again scan all Pods.
The repeated calculation can be eliminated by caching the result of the first matching process when processing Pod events. Then the matching process will only occur when there's a new Pod or a new Group.
Besides, most Pods actually share same labels with others when they are managed by same controller. The cache can be further optimized to maintain the relationship between label set and label selector, making Pods that having same labels and Groups that having same selectors share the cache.
This patch introduces an interface which is responsible for group member calculation and an implementation implementing it with the above optimization. The interface is then consumed by NetworkPolicyController and features like EndpointQuerier and ClusterGroup to retrieve Pods/ExternalEntities selected by a given Group or Groups that select a given Pod. Besides, other controllers that have the group logic like EgressPolicy can consume it directly without having redundant code with NetworkPolicyController.
Performance impact:
Explanation:
InitXLargeScaleWithSmallNamespaceshas only 4 Pods in each Namespace and all NetworkPolicies are namespace scoped, so even without the index, each policy just needs to scan 4 Pods at most. Maintaing the index adds few overhead, hence the minor increment in time and memory.InitXLargeScaleWithLargeNamespaceshas 1000 Pods and 100 NetworkPolicies in each Namespace, each NetworkPolicy applies to 10 Pods. Pods that have same labels will only be scaned once so many calculation is saved. When syncing groups, the result can be got directly without listing all Pods in the Namespace first, so calculation and memory is saved.InitXLargeScaleWithOneNamespacejust has many Pods but only 1 namespace, 1 AppliedToGroup and 1 AddressGroup in total, so having the index doesn't help anything.InitXLargeScaleNetpolPerPodhas only 1 namespace, 10000 Pods and 1 NetworkPolicy per Pod. It can benefit from the index greatly for the same reason asInitXLargeScaleWithLargeNamespaces.