Skip to content

Pass PROC_ROOT as environment variable to ss#7095

Merged
L3n41c merged 9 commits into
masterfrom
lenaic/network_check_on_containers
Sep 7, 2020
Merged

Pass PROC_ROOT as environment variable to ss#7095
L3n41c merged 9 commits into
masterfrom
lenaic/network_check_on_containers

Conversation

@L3n41c

@L3n41c L3n41c commented Jul 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Pass the PROC_ROOT environment variable to ss

Motivation

When the agent is running inside a container, the host procfs is mounted under /host/proc

Additional Notes

Here is how ss uses an alternate path for procfs:
https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/misc/ss.c#n474

This PR requires DataDog/datadog-agent#5987 to support the extra env parameter passed to _util.subprocess_output.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • 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

@codecov

codecov Bot commented Jul 9, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #7095 into master will decrease coverage by 9.45%.
The diff coverage is 96.15%.

Impacted Files Coverage Δ
network/datadog_checks/network/network.py 66.97% <90.00%> (+1.67%) ⬆️
network/tests/test_network.py 97.67% <100.00%> (+4.08%) ⬆️
cassandra/tests/conftest.py
riakcs/datadog_checks/riakcs/__about__.py
datadog_checks_base/tests/test_agent_check.py
...ks_base/datadog_checks/base/utils/serialization.py
...orm/datadog_checks/confluent_platform/__about__.py
istio/tests/common.py
crio/datadog_checks/crio/crio.py
.../datadog_checks/openstack_controller/exceptions.py
... and 970 more

@L3n41c
[network] Pass PROC_ROOT to ss
1101fad 
`ss` can use a non-standard procfs location thanks to the `PROC_ROOT` environment variable.
This is useful when the agent runs inside a container where the host procfs is mounted in `/host/proc`.

See https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/misc/ss.c#n474
@L3n41c
L3n41c force-pushed the lenaic/network_check_on_containers branch from 56286cb to 1101fad Compare July 9, 2020 10:36
olivielpeau
olivielpeau reviewed Jul 15, 2020
View reviewed changes
Comment thread datadog_checks_base/datadog_checks/base/stubs/_util.py
@L3n41c L3n41c mentioned this pull request Jul 17, 2020
Merged
@L3n41c
L3n41c marked this pull request as ready for review August 14, 2020 20:22
@L3n41c
L3n41c requested review from a team as code owners August 14, 2020 20:22
@L3n41c
L3n41c requested a review from olivielpeau August 14, 2020 20:22
hush-hush
hush-hush previously approved these changes Aug 14, 2020
View reviewed changes
olivielpeau
olivielpeau reviewed Aug 20, 2020
View reviewed changes
Comment thread datadog_checks_base/datadog_checks/base/utils/subprocess_output.py
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/network.py Outdated
@L3n41c
L3n41c force-pushed the lenaic/network_check_on_containers branch from e82a509 to 73898f7 Compare September 1, 2020 14:58
@L3n41c
L3n41c force-pushed the lenaic/network_check_on_containers branch from 73898f7 to a691bf1 Compare September 1, 2020 15:28
AlexandreYang
AlexandreYang reviewed Sep 1, 2020
View reviewed changes
Comment thread network/datadog_checks/network/network.py Outdated
Comment thread network/datadog_checks/network/network.py Outdated
AlexandreYang
AlexandreYang reviewed Sep 1, 2020
View reviewed changes
Comment thread datadog_checks_base/datadog_checks/base/stubs/_util.py
Comment thread network/datadog_checks/network/network.py
olivielpeau
olivielpeau previously approved these changes Sep 1, 2020
View reviewed changes

@olivielpeau olivielpeau 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.

@AlexandreYang made relevant comments, overall the logic looks good to me 👍

@L3n41c L3n41c mentioned this pull request Sep 2, 2020
Merged
4 tasks
AlexandreYang
AlexandreYang reviewed Sep 2, 2020
View reviewed changes

@AlexandreYang AlexandreYang left a comment •
edited
Loading

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.

Thx for the changes ! 👍

One last thing :)

Comment thread network/datadog_checks/network/network.py
@L3n41c
L3n41c force-pushed the lenaic/network_check_on_containers branch from 94514b3 to 2290772 Compare September 3, 2020 15:47
@L3n41c
L3n41c requested a review from AlexandreYang September 3, 2020 15:49
AlexandreYang
AlexandreYang previously approved these changes Sep 4, 2020
View reviewed changes
@L3n41c
L3n41c requested a review from AlexandreYang September 4, 2020 08:14
@L3n41c
L3n41c force-pushed the lenaic/network_check_on_containers branch from 493d2cf to 5c864e4 Compare September 4, 2020 09:42
@AlexandreYang
AlexandreYang self-requested a review September 4, 2020 11:33
AlexandreYang
AlexandreYang requested changes Sep 4, 2020
View reviewed changes

@AlexandreYang AlexandreYang left a comment •
edited
Loading

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.

Last 2 things:

1/ Any reason the label is changelog/Changed (will trigger a major version bump, breaking changes) instead of changelog/Added (will trigger a minor version bump, enhancements) ?

Same comment for #7479 it's not too late to change since the base is not released yet.

We follow semver for versionning.

2/ We need to release datadog_checks_base, I can help for that, but we need to address 1/ first.

Once done, we need to update

integrations-core/network/setup.py

Line 29 in 7270935

CHECKS_BASE_REQ = 'datadog_checks_base'

to CHECKS_BASE_REQ = 'datadog_checks_base>=XXX' to the newly released version.

This helps preventing people installing network check (agent integration install datadog-network...) on older agents that doesn't support passing env to get_subprocess_output.

@AlexandreYang AlexandreYang changed the title [network] Pass PROC_ROOT to ss Pass PROC_ROOT to ss Sep 4, 2020
@AlexandreYang AlexandreYang changed the title Pass PROC_ROOT to ss Pass PROC_ROOT as environment variable to ss Sep 4, 2020
@L3n41c
L3n41c merged commit 2337e66 into master Sep 7, 2020
@L3n41c
L3n41c deleted the lenaic/network_check_on_containers branch September 7, 2020 07:49
@vboulineau vboulineau mentioned this pull request Sep 24, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olivielpeau olivielpeau olivielpeau left review comments
@AlexandreYang AlexandreYang AlexandreYang approved these changes
@hush-hush hush-hush Awaiting requested review from hush-hush

Assignees

No one assigned

Labels

integration/network

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@L3n41c @hush-hush @olivielpeau @AlexandreYang