Conversation
maycmlee
left a comment
There was a problem hiding this comment.
Just a tiny nit, but approving
…nto mackjmr/host-profiler-feature
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2403 +/- ##
==========================================
+ Coverage 37.98% 38.09% +0.11%
==========================================
Files 298 299 +1
Lines 25029 25182 +153
==========================================
+ Hits 9508 9594 +86
- Misses 14796 14853 +57
- Partials 725 735 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| // If a user disabled HostPID manually, error out rather than enabling it for them. | ||
| if nodeAgent, ok := ddaSpec.Override[v2alpha1.NodeAgentComponentName]; ok { | ||
| if nodeAgent.HostPID != nil && apiutils.BoolValue(nodeAgent.HostPID) == false { | ||
| o.logger.Error(errHostPIDDisabledManually, "Host PID is required to run the host profiler. Please enable host PID or disable the host profiler") |
There was a problem hiding this comment.
nit - this won't be logged unless someone has node agent override configured. Probably log error only if hostProfilerEnabled == true and there is no override or no HostPID is set.
There was a problem hiding this comment.
nit - this won't be logged unless someone has node agent override configured
This is the goal. My assumption is that if hostPID was disabled manually by the user, then this was voluntary and we shouldn't enable it for the purpose of the host profiler feature. Instead I want to error out. That said, based on #2403 (comment) I guess I can just force enable it always ?
There was a problem hiding this comment.
sorry, I misunderstood intent here. What you say makes sense and could go either way so we can leave it as is.
There was a problem hiding this comment.
Ill remove it to stay consistent with other paths that do this.
| if nodeAgent, ok := ddaSpec.Override[v2alpha1.NodeAgentComponentName]; ok { | ||
| if nodeAgent.HostPID != nil && apiutils.BoolValue(nodeAgent.HostPID) == false { | ||
| o.logger.Error(errHostPIDDisabledManually, "Host PID is required to run the host profiler. Please enable host PID or disable the host profiler") | ||
| o.hostPIDDisabledManually = true |
There was a problem hiding this comment.
nit - recently there was a change #2365 in npm, usm features to enabled host pid when system probe is enabled. I think same can be done here.
There was a problem hiding this comment.
I do enable it
. But based on the other PR, I guess I can skip the check that hostPID is disabled manually check althogether and just force enable ?|
|
||
| // GetLatestAgentImage returns the latest host profiler image | ||
| func GetLatestHostProfilerImage() string { | ||
| image := newImage(DockerHubContainerRegistry, DefaultHostProfilerDevImageName, DefaultHostProfilerDevImageLatestTag, false, false, false) |
There was a problem hiding this comment.
This doesn't really work and isn't really needed at all. This is called when Daemonset is initialized and is overwritted by global defaults so it's reset to gcr here.
To override container level image you can include this annotation with the one which enabled profile (this mechanism was added by #1730)
agent.datadoghq.com/host-profiler-enabled: "true"
experimental.agent.datadoghq.com/image-override-config: |
{
"host-profiler": {
"name": "docker.io/datadog/ddot-ebpf-dev:nightly-latest"
}
} There was a problem hiding this comment.
This doesn't really work and isn't really needed at all.
Yeah, I noticed that and had to set registry: docker.io/datadog
I'm fine with using annotation until we get official image. SHould I just unset the image field:
in the container struct in that case ?There was a problem hiding this comment.
you can remove docker.io/GetLatestHostProfilerImage and use default image.
| // If a user disabled HostPID manually, error out rather than enabling it for them. | ||
| if nodeAgent, ok := ddaSpec.Override[v2alpha1.NodeAgentComponentName]; ok { | ||
| if nodeAgent.HostPID != nil && apiutils.BoolValue(nodeAgent.HostPID) == false { | ||
| o.logger.Error(errHostPIDDisabledManually, "Host PID is required to run the host profiler. Please enable host PID or disable the host profiler") |
There was a problem hiding this comment.
sorry, I misunderstood intent here. What you say makes sense and could go either way so we can leave it as is.
| if nodeAgent, ok := ddaSpec.Override[v2alpha1.NodeAgentComponentName]; ok { | ||
| if nodeAgent.HostPID != nil && apiutils.BoolValue(nodeAgent.HostPID) == false { | ||
| o.logger.Error(errHostPIDDisabledManually, "Host PID is required to run the host profiler. Please enable host PID or disable the host profiler") | ||
| o.hostPIDDisabledManually = true |
|
|
||
| // GetLatestAgentImage returns the latest host profiler image | ||
| func GetLatestHostProfilerImage() string { | ||
| image := newImage(DockerHubContainerRegistry, DefaultHostProfilerDevImageName, DefaultHostProfilerDevImageLatestTag, false, false, false) |
There was a problem hiding this comment.
you can remove docker.io/GetLatestHostProfilerImage and use default image.
What does this PR do?
Add support for full host profiler.
Motivation
OTAGENT-711
Additional Notes
Anything else we should know when reviewing?
Minimum Agent Versions
Are there minimum versions of the Datadog Agent and/or Cluster Agent required?
Describe your test plan
Prereqs:
continuous_profiler_readpermissionTest 1. Configmap
Ensure:
host:test1in staging.Test 2. Inline config
Test 3. HostPID manually disabled
Ensure:
Host PID is required to run the host profiler. Please enable host PID or disable the host profilerChecklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel