fix(gpu): disable deprecated eBPF probes by default - #54291
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0132ce105
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
More details
The default change consistently disables the GPU eBPF path while preserving the explicitly enabled path, and the collector surface loses only gpu.process.core.usage as intended. The updated e2e configuration keeps probes-on coverage, while curl now correctly reports the disabled endpoint's HTTP failure instead of treating its error body as success.
📊 Validated against 6 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit a0132ce · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 50c5c23 | Docs | Datadog PR Page | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 9f1c7785: Results for datadog-agent_7.83.0~devel.git.521.50c5c23.pipeline.128769532-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: f93514b Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_idle | memory utilization | +0.30 | [+0.24, +0.35] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.24 | [+0.16, +0.32] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | +0.22 | [-0.78, +1.22] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.15 | [+0.11, +0.18] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | +0.02 | [-0.10, +0.15] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.04 | [-0.10, +0.02] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.11 | [-0.15, -0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.58 | [-0.83, -0.33] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 148.21MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 729.35KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 496.33MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.12MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 184.21MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.77MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 356.23 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 411.13MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 73.26MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 28.98 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 305.07MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 60.05 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 276.05MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 22.45 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 287.73MiB ≤ 320MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
Re-lands #54053, which was auto-reverted in #54180 because it broke new-e2e-gpu on main: TestVectorAddProgramDetected asserts on gpu.process.core.usage, the one GPU metric that only the eBPF collector can produce. The GPU e2e suite relied on enable_ebpf_probes defaulting to true, so it now asks for the probes explicitly and keeps covering that path. The new default is covered instead by a unit test that pins the metric surface: disabling the probes must cost exactly the metrics declared eBPF-only and nothing else, so the next accidental drop fails in its own PR rather than on main. Also make QuerySysprobe fail on HTTP error statuses. Without --fail, curl returned the 503 "GPU eBPF probes are disabled" body as a successful response, which is why TestGPUSysprobeEndpointIsResponding kept passing with the probes disabled. Known gap: with the e2e suite pinned to probes-on, nothing at the e2e level exercises the shipped default. Covering that needs a second GPU suite variant and one more GPU runner slot. Co-Authored-By: Claude <noreply@anthropic.com>
TestMetricNamesWithoutEBPFProbes calls collectMetricNames twice, and each call installs a fresh global NVML mock via setupMockDevices. The events gatherer was stopped from a t.Cleanup, which does not run until the whole test ends, so the first call's asyncFetchWorker was still reading the global through EventSetWait while the second call wrote it. The race detector flagged it in CI (both the go test and bazel jobs run -race); it did not reproduce locally because I ran without -race. Stop the gatherer with a defer inside the helper instead. Stop() joins the worker via wg.Wait(), so the worker is gone before the next setup. Co-Authored-By: Claude <noreply@anthropic.com>
The upgrade section restated what the deprecation notice already says, so keep only the deprecation notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the probes off by default, the gpu_monitoring docs were wrong on two counts: they said enabling the module loads the eBPF programs, and they never mentioned the flag that now controls them. A user following the deprecation note could not find enable_ebpf_probes in their config file, because the setting was not public and so never rendered into the template. - describe the module in terms of the NVML metrics it always collects - expose enable_ebpf_probes as a public setting, marked deprecated, so it appears in the generated system-probe.yaml pkg/config/system-probe_template.yaml is the source the schema enrichment reads (schema.generate -> parse_template), and check_config_templates.sh asserts both render identically, so the two files move together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@gjulianm one thing I hit while flipping the default that I'd like your read on, since it sits outside what this PR touches. Two gates next to
That consumer's only reader is This isn't new — it was already reachable for anyone who set the flag manually — but this PR makes it the shipped default, so every GPU host now loads the event monitor's eBPF machinery for nothing. Two questions:
|
@mbertrone I think we can safely disable the event monitor if eBPF probes are disabled, yes, we can update the guards |
9f57b57 to
e3a0ca0
Compare
@gjulianm great I'll address that in another PR |
Co-authored-by: Guillermo Julián <gjulianm@users.noreply.github.com>
Co-authored-by: Guillermo Julián <gjulianm@users.noreply.github.com>
The two "Apply suggestion from @gjulianm" commits edited the gpu_monitoring comments in system-probe_template.yaml but left the matching descriptions in system-probe_schema.yaml untouched, so the two files drifted. schema.generate regenerates the schema from the template (parse_template overwrites each node's description), so the drift fails check_schema_refresh, and check_config_templates independently diffs the schema-rendered and template-rendered configs. Copy the template wording into the schema for the gpu_monitoring section and its enabled setting so both checks pass again. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hand-edit that documented enable_ebpf_probes left the node in its old position in system-probe_schema.yaml. schema.generate emits enable_ebpf_probes right after enabled (the documented settings come first), so check_schema_refresh in the generate_config_schema-linux job failed: the committed schema no longer matched the generated output. Move the node to match. The resulting file is byte-identical to the output of dda inv schema.generate (verified against the generator run in CI: same git blob hash c823f61). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
### What does this PR do? Gates the system-probe event monitor on `gpu_monitoring.enable_ebpf_probes` in addition to `gpu_monitoring.enabled`, in both the module-enablement logic (`pkg/system-probe/config`) and the GPU process-consumer creation (`cmd/system-probe/modules/eventmonitor.go`). The GPU monitoring module itself remains gated on `gpu_monitoring.enabled`, so NVML-based collection is unchanged. ### Motivation The event monitor exists only to feed process exec/exit events to the GPU eBPF probes. Keyed on `gpu_monitoring.enabled` alone, a host running GPU monitoring through NVML only still loaded the event monitor's eBPF programs and built a process consumer that nothing read. Follow-up to the discussion on #54291. ### Describe how you validated your changes Extended `TestEventMonitor` with the `enabled` × `enable_ebpf_probes` matrix, setting both explicitly rather than relying on the default (which is changing as the probes are deprecated). Verified that with the probes disabled the `gpu` module is still enabled while `event_monitor` is not: | `gpu_monitoring.enabled` | `enable_ebpf_probes` | enabled modules | | --- | --- | --- | | true | true | `discovery, event_monitor, gpu` | | true | false | `discovery, gpu` | | false | true | `discovery` | Also built `cmd/system-probe/modules` with `linux_bpf,nvml` to cover the configuration that includes both `eventmonitor.go` and the real `gpu.go`. ### Additional Notes Both gates had to move together to stay consistent with the `EnableEBPFProbes && processEventConsumer == nil` guard in the GPU module factory. Note that `gpu_monitoring.enable_ebpf_probes` still defaults to `true` on `main`, so this is a no-op for default configurations until #54291 lands; it currently only affects deployments that set it to `false` explicitly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: matteo.bertrone <matteo.bertrone@datadoghq.com>
### What does this PR do? Gates the system-probe event monitor on `gpu_monitoring.enable_ebpf_probes` in addition to `gpu_monitoring.enabled`, in both the module-enablement logic (`pkg/system-probe/config`) and the GPU process-consumer creation (`cmd/system-probe/modules/eventmonitor.go`). The GPU monitoring module itself remains gated on `gpu_monitoring.enabled`, so NVML-based collection is unchanged. ### Motivation The event monitor exists only to feed process exec/exit events to the GPU eBPF probes. Keyed on `gpu_monitoring.enabled` alone, a host running GPU monitoring through NVML only still loaded the event monitor's eBPF programs and built a process consumer that nothing read. Follow-up to the discussion on #54291. ### Describe how you validated your changes Extended `TestEventMonitor` with the `enabled` × `enable_ebpf_probes` matrix, setting both explicitly rather than relying on the default (which is changing as the probes are deprecated). Verified that with the probes disabled the `gpu` module is still enabled while `event_monitor` is not: | `gpu_monitoring.enabled` | `enable_ebpf_probes` | enabled modules | | --- | --- | --- | | true | true | `discovery, event_monitor, gpu` | | true | false | `discovery, gpu` | | false | true | `discovery` | Also built `cmd/system-probe/modules` with `linux_bpf,nvml` to cover the configuration that includes both `eventmonitor.go` and the real `gpu.go`. ### Additional Notes Both gates had to move together to stay consistent with the `EnableEBPFProbes && processEventConsumer == nil` guard in the GPU module factory. Note that `gpu_monitoring.enable_ebpf_probes` still defaults to `true` on `main`, so this is a no-op for default configurations until #54291 lands; it currently only affects deployments that set it to `false` explicitly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: matteo.bertrone <matteo.bertrone@datadoghq.com> 8657403
|
Backport evaluation: View Slack thread |
|
This usually happens when the cherry-pick has merge conflicts and needs manual resolution. To backport manually, run: git fetch
git worktree add .worktrees/backport-7.82.x 7.82.x
cd .worktrees/backport-7.82.x
git switch --create backport-54291-to-7.82.x
git cherry-pick -x --mainline 1 1316bc4f3ee3e6de03431e9b504d2fcd24366a28
git push --set-upstream origin backport-54291-to-7.82.xWorkflow logs: https://github.com/DataDog/datadog-agent/actions/runs/31006163118 |
What does this PR do?
Re-lands #54053, which flipped
system_probe.gpu_monitoring.enable_ebpf_probesfromtruetofalse. The two behavioral lines are identical to the original; everything else here is the test coverage that was missing the first time.pkg/config/setup/system_probe_settings.go— defaulttrue→falsepkg/config/schema/yaml/system-probe_schema.yaml— defaulttrue→falsetest/new-e2e/tests/gpu/testdata/config/system_probe_config.yaml— the GPU e2e suite now setsenable_ebpf_probes: trueexplicitly instead of inheriting itpkg/collector/corechecks/gpu/nvidia/collector_test.go— newTestMetricNamesWithoutEBPFProbes, pinning the metric surface of the default configurationtest/new-e2e/tests/gpu/capabilities.go—curl --failinQuerySysprobeupgradesection naming the metric that stops being collectedMotivation
The eBPF probes for GPU Monitoring are deprecated. Some customers still have the previously recommended
gpu.privilegedModeenabled, and these probes have caused crashes on NVIDIA GB300 Grace Blackwell Ultra machines: the arm64 uprobe single-step handler intermittently resumes the probed process at the wrong PC, corrupting whatever GPU process the probe fired in.#54053 was auto-reverted in #54180 because it broke
new-e2e-gpuonmain:gpu.process.core.usageis the one GPU metric produced only by the eBPF collector — every other metric has an NVML source. The suite relied on the default beingtrue, so it now asks for the probes by name and keeps covering that path.The default configuration is covered instead by a unit test that asserts turning the probes off costs exactly the metrics declared eBPF-only and nothing else. That inverts the failure mode: the next accidental metric drop fails in its own PR rather than on
main.The job didn't run on #54053 because
.on_gpu_or_e2e_changesdoesn't includepkg/config/**, and that PR touched nothing else. This PR touchestest/new-e2e/tests/gpu/**, so the GPU suite runs here and validates the new default before merge.While in
capabilities.go:QuerySysprobeusedcurl -swithout--fail, so the/gpu/checkendpoint's 503"GPU eBPF probes are disabled"body came back as a successful response.TestGPUSysprobeEndpointIsRespondingwas therefore passing green with the probes disabled — it could not fail. Fixed for both the host and Kubernetes paths.Describe how you validated your changes
TestMetricNamesWithoutEBPFProbes— passes. Confirmsprocess.core.usageis emitted with the probes on, absent with them off, and that no other metric disappears.pkg/collector/corechecks/gpu/nvidiapackage — passes, 27.3s.TestConfigurePRMCacheRequiresPRMEndpoint— passes with the new default, confirming an explicitenable_ebpf_probes: truestill builds the eBPF collector. This is a default change, not a forced disable.nvml,testtags; the GPU e2e suite runs in this PR's pipeline.Additional Notes
Known coverage gap. With the e2e suite pinned to probes-on, nothing at the e2e level exercises the shipped default — the unit test proves the metric set is right, but not that system-probe starts and stays healthy with the GPU module loaded probe-less. Closing that needs a second GPU suite variant and one more GPU runner slot. Deliberately deferred.
Cgroup permissions are unaffected.
configure_cgroup_permsis a separate flag, and incmd/system-probe/modules/gpu.gothe cgroup call sits outside theEnableEBPFProbesblock, so permission setup still runs with the probes off.gpu.process.core.usagehas consumers. Several dashboards query it. Theupgraderelease note points atgpu.process.sm_activeas the closest NVML equivalent. Whether the out-of-the-box GPU integration dashboard uses it has not been checked — that asset lives outside this repo.The deprecation is prose-only. Nothing in the code marks the flag or the probes as deprecated: no warning when a user explicitly opts back in, no removal target. Worth doing, but it belongs with the root-cause decision on the arm64 uprobe bug rather than here.