-
Notifications
You must be signed in to change notification settings - Fork 359
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
Use netip.Addr for FlowExporter #5532
Use netip.Addr for FlowExporter #5532
Conversation
Benchmark results:
|
afb208b
to
07a20f7
Compare
I'm putting this on hold. While the synthetic benchmarks look good, I think real-life usage won't show too much improvement, given that https://github.com/ti-mo/conntrack uses net.IP. I will open an issue to see if there is interest in switching to netip.Addr, but I am not sure the project is actively maintained. |
@@ -26,15 +27,24 @@ import ( | |||
|
|||
// InitializeConnTrackDumper initializes the ConnTrackDumper interface for different OS and datapath types. | |||
func InitializeConnTrackDumper(nodeConfig *config.NodeConfig, serviceCIDRv4 *net.IPNet, serviceCIDRv6 *net.IPNet, ovsDatapathType ovsconfig.OVSDatapathType, isAntreaProxyEnabled bool) ConnTrackDumper { | |||
var svcCIDRv4, svcCIDRv6 netip.Prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it is better to change svcCIDRv4/v6 in agent.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to. While I think that in the long term, we could replace all occurrences of net.IP
with netip.Addr
(and net.IPNet
with netip.Prefix
), it's better to do it incrementally with small PRs, starting with places providing the bigger benefits (in terms of performance). The configuration CIDRs for the Agent (podCIDR, svcCIDR) are individual values and don't account for any memory usage (while the FlowExporter connection store can potentially holds 1000s of IP addresses).
07a20f7
to
9a67f0b
Compare
@yuntanghsu I'm re-opening this for review. I worked with the maintainer from the conntrack library and it now uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only one nit
9a67f0b
to
ec1239b
Compare
/test-all |
pkg/agent/flowexporter/connections/conntrack_connections_perf_test.go
Outdated
Show resolved
Hide resolved
Also add BenchmarkConnStore, meant to measure memory usage of the connection store (and all the connections). Signed-off-by: Antonin Bas <abas@vmware.com>
The net/netip package was introduced in Go 1.18. Compared to the existing net.IP type, the netip.Addr type takes less memory (especially for IPv6 addresses), is immutable, and is comparable so it supports == and can be used as a map key. In our case, this means that the flowexporter.Connection type takes a little less memory, and that the flowexporter.Tuple can be used directly as the flowexporter.ConnectionKey (no need to generate a string for use as the map key). Overall, this leads to higher performance (decreased CPU and memory usage) for the FlowExporter. There is potential for further improvement, as several key functions take as parameters both the flowexporter.Connection and flowexporter.ConnectionKey objects, which is no longer really required. See antrea-io#5271 Signed-off-by: Antonin Bas <abas@vmware.com>
This release uses netip.Addr instead of net.IP to represent connections. Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
ec1239b
to
eb02dae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
I added the |
Replace net.IP with netip.Addr in FlowExporter implementation
The net/netip package was introduced in Go 1.18. Compared to the
existing net.IP type, the netip.Addr type takes less memory (especially
for IPv6 addresses), is immutable, and is comparable so it supports ==
and can be used as a map key.
In our case, this means that the flowexporter.Connection type takes a
little less memory, and that the flowexporter.Tuple can be used directly
as the flowexporter.ConnectionKey (no need to generate a string for use
as the map key). Overall, this leads to higher performance (decreased
CPU and memory usage) for the FlowExporter.
There is potential for further improvement, as several key functions
take as parameters both the flowexporter.Connection and
flowexporter.ConnectionKey objects, which is no longer really required.