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

[system-probe] Add missing system probe settings for parity with helm chart #149

Merged
merged 1 commit into from
Sep 7, 2020

Conversation

xornivore
Copy link
Contributor

What does this PR do?

Adds missing toggles for OOMKill and TCPQueueLength modules as well as collectDNSStats setting.

Motivation

Parity with the helm chart in terms of full set of system probe features.

Additional Notes

Adding unit tests for these and similar features (hello security agent!) becomes rather tedious and results in rather repetitive code, there might be some quick wins in terms of refactoring to use more helper functions and injecting additional volumes, mounts. It would be ideal to keep that refactoring separate from these changes here.

Describe your test plan

Configure DatadogAgent CR with desired features and make sure they are properly enabled:

apiVersion: datadoghq.com/v1alpha1
kind: DatadogAgent
metadata:
  name: datadog-agent
spec:
# ...
  agent:
    # ...
    systemProbe:
      enabled: true
      enableOOMKill: true
      enableTCPQueueLength: true
   # ...

… chart

Adds missing toggles for OOMKill and TCPQueueLength modules as well as collectDNSStats setting.
@xornivore xornivore added the enhancement New feature or request label Sep 4, 2020
@xornivore xornivore added this to the v0.4 milestone Sep 4, 2020
@xornivore xornivore requested a review from a team September 4, 2020 02:19
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #149 into master will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   58.68%   58.96%   +0.27%     
==========================================
  Files          31       31              
  Lines        4909     4942      +33     
==========================================
+ Hits         2881     2914      +33     
  Misses       1814     1814              
  Partials      214      214              
Flag Coverage Δ
#unittests 58.96% <100.00%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/datadogagent/systemprobe.go 100.00% <100.00%> (ø)
pkg/controller/datadogagent/utils.go 82.59% <100.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4faeb97...870ea9a. Read the comment docs.

Comment on lines +148 to +149
SystemProbeLibModulesVolumeName = "modules"
SystemProbeLibModulesVolumePath = "/lib/modules"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are missing the "kernelHeader folder"
@L3n41c could you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Well, the kernel headers are looked for in /lib/modules/$(uname -r)/build which is generally a symlink to /usr/src/kernel-$(uname-r).
So, mounting the modules and src volumes defined here should be fine.
👍🏻

Comment on lines +148 to +149
SystemProbeLibModulesVolumeName = "modules"
SystemProbeLibModulesVolumePath = "/lib/modules"
Copy link
Member

Choose a reason for hiding this comment

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

Well, the kernel headers are looked for in /lib/modules/$(uname -r)/build which is generally a symlink to /usr/src/kernel-$(uname-r).
So, mounting the modules and src volumes defined here should be fine.
👍🏻

@clamoriniere clamoriniere merged commit 20bdcff into master Sep 7, 2020
@clamoriniere clamoriniere deleted the xornivore/missing-sysprobe-settings branch September 7, 2020 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants