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

probe: conntrack: fix output parsing #2118

Merged

Conversation

alban
Copy link
Contributor

@alban alban commented Jan 6, 2017

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 #2095 was unfortunately
rejecting lines with those fields. This patch fixes that by adding more
complicated parsing in decodeTwoTuples() with FieldsFunc and SplitN.

Fixes #2117
Regression from #2095


  • test under different configuration (SELinux, nf_conntrack_acct)
  • check performances compared to sscanf and xml

/cc @iaguis @2opremio

@2opremio
Copy link
Contributor

2opremio commented Jan 7, 2017

Thanks @alban . Regexps tend to perform pretty badly, let's confirm with a test but I suspect a simple "key=value" parser would be a better option.

@alban
Copy link
Contributor Author

alban commented Jan 9, 2017

tl;dr: decodeStreamedFlow() took

  • 0.56s in the scanf version
  • 1.73s in th regexp version

So regexp is significantly slower. I will try with a simple "key=value" parser. Note that there is several keys with the same string, so we have to read them in order.

Long version:

pprof couldn't work for me on go 1.7, so I had to revert to go 1.6 with:

git revert d1cf9f60fced05ccb9283d281c9405209bb4b0b0
git revert 002770d3949b52bcc0c0dba444e74090b97b53e8
git revert 28213a00a463ca68bd32cf972ebe74280f427885

Then, the perf check is done with:

sudo ./scope launch --probe.http.listen :4041
go tool pprof http://localhost:4041/debug/pprof/profile
web

The test I did was:

sudo docker run -ti --rm busybox
while date ; do echo GET | nc $IP 80 > /dev/null ; done

This was not a good idea to pick an external $IP for the test because that server latency changed when I tried the xml version. DoS mitigation? I'll use an internal server for the next test.

@alban alban force-pushed the alban/fix-conntrack-parsing branch from 8b82a9d to a57360a Compare January 11, 2017 17:37
@alban
Copy link
Contributor Author

alban commented Jan 11, 2017

Patch updated with a manual parser instead of regexp. It seems to work at a first glance but I still need to check the performances.

@alban
Copy link
Contributor Author

alban commented Jan 12, 2017

I checked the performance again, but this time:

  • without using an external server, but using nginx in Docker instead
  • running 4 clients in parallel with the following commands:
docker run -ti --rm busybox
while date ; do (sleep 0.05 ; echo GET / ) | nc 172.17.0.3 80 > /dev/null ; done

Then I compare the perfs from pprof:

  1. xml parsing: DecodeElement: 4.9s
  2. sscanf parsing (as it is on master, but with a local workaround for parsing the SELinux attributes): decodeStreamedFlow: 2.87s
  3. manual parsing in this PR: decodeStreamedFlow: 1.34s

I am not sure why my code is faster than the scanf, but that seems good.

@2opremio
Copy link
Contributor

I am not sure why my code is faster than the scanf, but that seems good.

Awesome, great job!

case key == "id":
f.Independent.ID, err = strconv.ParseInt(value, 10, 64)
}
}

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@alban alban force-pushed the alban/fix-conntrack-parsing branch from a57360a to e640d2b Compare January 13, 2017 12:08
@alban alban changed the title [WIP] probe: conntrack: fix output parsing probe: conntrack: fix output parsing Jan 13, 2017
@alban
Copy link
Contributor Author

alban commented Jan 13, 2017

Updated. I moved the 60 lines in the helper function, and updated the description of the PR. It worked for me on Fedora (with the SELinux field). I tried with and without "net.netfilter.nf_conntrack_acct". PTAL.

@2opremio 2opremio mentioned this pull request Jan 16, 2017
@@ -250,6 +252,53 @@ func removeInplace(s, sep []byte) []byte {
return s[:len(s)-len(sep)]
}

func decodeTwoTuples(line []byte, f *flow) error {

This comment was marked as abuse.

@2opremio
Copy link
Contributor

@alban Can you please a few examples of the lines which use to fail in conntrack_test.go?

@2opremio
Copy link
Contributor

Also, please extend the commented format examples in conntrack.go or remove them altogether.

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
@alban alban force-pushed the alban/fix-conntrack-parsing branch from e640d2b to f1e2b5d Compare January 17, 2017 18:32
@alban
Copy link
Contributor Author

alban commented Jan 17, 2017

Updated and rebased. Only the dumped flows have the extra fields, so I updated the examples and the tests for the dumped flows only.

@2opremio 2opremio merged commit 87c15a1 into weaveworks:master Jan 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conntrack fails if net.netfilter.nf_conntrack_acct = 1
3 participants