Skip to content

Fix proc location for conntrack files#4150

Merged
AlexandreYang merged 2 commits into
masterfrom
alex/fix_conntrack_location
Jul 31, 2019
Merged

Fix proc location for conntrack files#4150
AlexandreYang merged 2 commits into
masterfrom
alex/fix_conntrack_location

Conversation

@AlexandreYang

@AlexandreYang AlexandreYang commented Jul 18, 2019

Copy link
Copy Markdown
Member

What does this PR do?

Avoid suffixing with /1 when reading conntrack proc files.

In a docker container the proc_location is suffixed with /1 for net files.

But there is not need to suffix with /1 for conntrack files

Conntrack files always reside in /proc/sys/net/*. Commands executed inside a container:

root@ffcad4dd91a8:/# cat /host/proc/sys/net/netfilter/nf_conntrack_events
1
root@ffcad4dd91a8:/# cat /proc/sys/net/netfilter/nf_conntrack_events
1

Note: nf_conntrack_count has a different value inside a container. See additional notes below.

Motivation

User getting this error Unable to read /host/proc/1/sys/net/nf_conntrack_max

datadog [ AGENT ] 2019-06-25 02:04:46 UTC | DEBUG | (pkg/collector/py/datadog_agent.go:152 in LogMessage) | (network.py:463) | Unable to read /host/proc/1/sys/net/nf_conntrack_max. Skipping nf_conntrack_max metrics pull.

Additional Notes

It seems that some host conntrack stats are not available inside a container.

Inside container:

root@ffcad4dd91a8:/# cat /host/proc/sys/net/netfilter/nf_conntrack_count
0
root@ffcad4dd91a8:/# cat /host/proc/sys/net/netfilter/nf_conntrack_max
262144

On host:

➜  ~ cat /proc/sys/net/netfilter/nf_conntrack_count
24
➜  ~ cat /proc/sys/net/netfilter/nf_conntrack_max
262144

But it seems that nf_conntrack_count has a different value inside a container (the only one):
conntrack_metrics.txt

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@AlexandreYang
AlexandreYang requested a review from a team as a code owner July 18, 2019 17:10
@AlexandreYang AlexandreYang changed the title Fix proc location Fix proc location for conntrack Jul 18, 2019
@AlexandreYang AlexandreYang changed the title Fix proc location for conntrack Fix proc location for conntrack files Jul 18, 2019
@codecov

codecov Bot commented Jul 18, 2019

Copy link
Copy Markdown

Codecov Report

Merging #4150 into master will decrease coverage by 13.13%.
The diff coverage is 92.85%.

@@             Coverage Diff             @@
##           master    #4150       +/-   ##
===========================================
- Coverage   86.71%   73.58%   -13.14%     
===========================================
  Files         743        7      -736     
  Lines       39233      458    -38775     
  Branches     4677       81     -4596     
===========================================
- Hits        34020      337    -33683     
+ Misses       3942       99     -3843     
+ Partials     1271       22     -1249

ofek
ofek previously approved these changes Jul 18, 2019
coignetp
coignetp previously approved these changes Jul 24, 2019
@AlexandreYang

Copy link
Copy Markdown
Member Author

On hold, waiting feedback from user.

@AlexandreYang AlexandreYang changed the title Fix proc location for conntrack files [HOLD] Fix proc location for conntrack files Jul 24, 2019
@AlexandreYang
AlexandreYang dismissed stale reviews from coignetp and ofek via b931ecb July 24, 2019 11:17
@AlexandreYang AlexandreYang changed the title [HOLD] Fix proc location for conntrack files Fix proc location for conntrack files Jul 31, 2019
@AlexandreYang

Copy link
Copy Markdown
Member Author

QAed, conntrack metrics retrieved correctly in container with new code.

@FlorianVeaux FlorianVeaux left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is good and has been tested correctly. Let's :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants