-
Notifications
You must be signed in to change notification settings - Fork 712
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
Disable XML in conntrack parsing #2095
Conversation
0aba533
to
5c02dfc
Compare
After running this in WeaveCloud dev I can see a parsing improvement of 1-(1.74/1.01)*100 ~= 40% This is a rough estimation and I should make further tests but it's promising. Before (look for After (look for |
I have made some more thorough testing: multiple runs, using a more loaded machine (similar to the one seen on #1985 (comment)) and making sure to compare probe pods running in the same machine. The results are even better than I thought: 1-(1.25/4.23) ~= 70% improvement. Before: After (focusing on So, I will clean this up, fix the tests and merge it. |
cd14b62
to
13ad063
Compare
13ad063
to
d22d64c
Compare
|
||
// Now loop on the output stream | ||
decoder := xml.NewDecoder(reader) | ||
// Lop on the output stream |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
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.
I don't have a good intuition on performance of scanf, but as you've shown this is Better Than Live so we can work on tweaking the perf later.
Have you double-checked this output against various verisons of the conntrack CLI to ensure it will work with older versions? Is this format guarenteed to not change in the future?
These questions are non-blocking.
We control the version (i.e. the CLI included in the Scope container).
It hasn't changed in a long time. The last conntrack-tools release is from 2012 http://conntrack-tools.netfilter.org/ |
I was wrong, http://www.netfilter.org/projects/conntrack-tools/downloads.html says otherwise. So, no guarantees but I would doubt it will change (it hasn't been heavily modified in the past years https://git.netfilter.org/conntrack-tools/log/src/conntrack.c ) |
@2opremio unfortunately, the output depends not only on the version of conntrack CLI but also on the kernel and on the sysctl options configured (see #2117) |
With net.netfilter.nf_conntrack_acct = 1, conntrack adds the following fields in the output: packets=3 bytes=164 And with SELinux (e.g. Fedora), conntrack adds: secctx=... The parsing with fmt.Sscanf introduced in weaveworks#2095 was unfortunately rejecting lines with those fields. This patch fixes that by adding more complicated parsing in decodeTwoTuples() with FieldsFunc and SplitN. Fixes weaveworks#2117 Regression from weaveworks#2095
With net.netfilter.nf_conntrack_acct = 1, conntrack adds the following fields in the output: packets=3 bytes=164 And with SELinux (e.g. Fedora), conntrack adds: secctx=... The parsing with fmt.Sscanf introduced in weaveworks#2095 was unfortunately rejecting lines with those fields. This patch fixes that by adding more complicated parsing in decodeFlowKeyValues() with FieldsFunc and SplitN. Fixes weaveworks#2117 Regression from weaveworks#2095
This attempts to improve the performance problems from #1991
Parsing the default output of conntrack instead of XML turns out to be more difficult than we thought since some fields are optional and context-based.
I put together this hacky non-optimal parser to see if it's better than XML. If there's a considerable difference I will improve it and write a proper parser, otherwise I may just bite the bullet and fix vishvananda/netlink#171