Skip to content

Ignore /proc/sys/fs/binfmt_misc by default#7650

Merged
mgarabed merged 3 commits intomasterfrom
ofek/d
Sep 24, 2020
Merged

Ignore /proc/sys/fs/binfmt_misc by default#7650
mgarabed merged 3 commits intomasterfrom
ofek/d

Conversation

@ofek
Copy link
Copy Markdown
Contributor

@ofek ofek commented Sep 23, 2020

@ofek ofek requested review from a team as code owners September 23, 2020 19:39
@ofek ofek mentioned this pull request Sep 23, 2020
Closed
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 24, 2020 •
edited
Loading

Codecov Report

Merging #7650 into master will increase coverage by 5.11%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
disk/datadog_checks/disk/disk.py 86.92% <100.00%> (+2.69%) ⬆️
disk/tests/test_filter.py 100.00% <100.00%> (+3.54%) ⬆️
disk/tests/test_unit.py 100.00% <100.00%> (ø)
apache/tests/common.py
confluent_platform/tests/common.py
istio/tests/common.py
...datadog_checks/kube_apiserver_metrics/__about__.py
clickhouse/datadog_checks/clickhouse/__init__.py
nagios/tests/conftest.py
teamcity/tests/common.py
... and 798 more

mx-psi
mx-psi previously approved these changes Sep 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM as long as we are sure that this is not a breaking change for any user. Note that #7378 already adds an option to ignore this and other non-physical mount points.

pducolin
pducolin previously approved these changes Sep 24, 2020
View reviewed changes
hithwen
hithwen requested changes Sep 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hithwen hithwen left a comment

Choose a reason for hiding this comment

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

Can you rename blacklist to exclude?

@mx-psi
Copy link
Copy Markdown
Member

mx-psi commented Sep 24, 2020 •
edited
Loading

Can you rename blacklist to exclude?

@hithwen we are taking care of that on a separate PR: #7627

albertvaka
albertvaka suggested changes Sep 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@albertvaka albertvaka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This won't work on the containerized Agent. It should be /host/proc/sys/fs/binfmt_misc instead there.

About the naming: I would keep blacklist here so it's consistent with the other config options, and would rename them all for the next release as part of the PR mentioned above ^.

@mx-psi
Copy link
Copy Markdown
Member

mx-psi commented Sep 24, 2020

This won't work on the containerized Agent. It should be /host/proc/sys/fs/binfmt_misc instead there.

Maybe we should exclude by filesystem instead of mountpoint then?

mgarabed
mgarabed reviewed Sep 24, 2020
View reviewed changes
Comment thread disk/datadog_checks/disk/data/conf.yaml.default Outdated
#
# mount_point_global_blacklist: []
# mount_point_global_blacklist:
# - /proc/sys/fs/binfmt_misc$
Copy link
Copy Markdown
Member

@mgarabed mgarabed Sep 24, 2020

Choose a reason for hiding this comment

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

Should we add a comment that overriding this value will introduce possible problems on systems using systemd?

Copy link
Copy Markdown
Contributor Author

@ofek ofek Sep 24, 2020

Choose a reason for hiding this comment

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

Hmm, dunno if a comment in description for each would scale long term. WDYT?

@ofek ofek dismissed stale reviews from pducolin and mx-psi via 54a400d September 24, 2020 15:51
@mgarabed mgarabed merged commit 91888a4 into master Sep 24, 2020
@mgarabed mgarabed deleted the ofek/d branch September 24, 2020 18:47
mgarabed pushed a commit that referenced this pull request Sep 24, 2020
@ofek @mgarabed
Ignore /proc/sys/fs/binfmt_misc by default (#7650)
6ec1340 
* Ignore `/proc/sys/fs/binfmt_misc` by default

* address

* add warning
@sarcasticadmin sarcasticadmin mentioned this pull request Dec 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hithwen hithwen hithwen requested changes

@mgarabed mgarabed mgarabed approved these changes

@mx-psi mx-psi mx-psi left review comments

@pducolin pducolin pducolin left review comments

+1 more reviewer

@albertvaka albertvaka albertvaka requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation integration/disk

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ofek @mx-psi @hithwen @mgarabed @albertvaka @pducolin
⚡