Revert "Revert "[WP] Fix isolation re-apply mechanism"" and fix the related test - #55631
Revert "Revert "[WP] Fix isolation re-apply mechanism"" and fix the related test#55631theop-dd wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Found one correctness issue.
Overall verdict: patch is incorrect because a failed filter installation can still activate an incomplete router buffer.
| if err := p.setupRawPacketProgs(progSpecs, probes.TCRawPacketDropActionKey, probes.RawPacketMaxTailCall, &p.rawPacketActionCollection, true); err != nil { | ||
| errs = multierror.Append(errs, err) | ||
| } | ||
| // all the filters are ready so we can flip | ||
| if err = p.flipRawPacketRouterBuffer(); err != nil { |
There was a problem hiding this comment.
Do not flip when setupRawPacketProgs fails. That function can fail after partially updating the inactive buffer; activating it could leave network-isolation filters missing or incomplete. Return the setup error and keep the current buffer active.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 2c618e3 | Docs | View more details | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 14a1e606: Results for datadog-agent_7.85.0~devel.git.15.2c618e3.pipeline.135976808-1_amd64.deb:No change detected Results for datadog-iot-agent_7.85.0~devel.git.15.2c618e3.pipeline.135976808-1_amd64.deb:No change detected |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 14a1e60 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | +0.28 | [+0.06, +0.50] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.16 | [+0.11, +0.21] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.04 | [+0.00, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.26 | [-0.33, -0.19] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.27 | [-0.30, -0.23] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.27 | [-0.40, -0.14] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.28 | [-0.32, -0.24] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | -0.45 | [-0.69, -0.21] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -0.55 | [-1.43, +0.33] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.69 | [-0.92, -0.46] | 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 | 4 ≤ 5 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 175.24MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 750.69KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 5 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 473.65MiB ≤ 537MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 212.24MiB ≤ 228MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.15MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 364.21 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 16 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 446.42MiB ≤ 455MiB | 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 | 71.95MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 31.54 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 328.08MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 64.64 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 302.83MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 25.19 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 310.12MiB ≤ 345MiB | 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".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 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 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_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 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 memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 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 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 missed_bytes: 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_logs, 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 total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
17 successful checks with minimal change (< 2 KiB)
|
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Patch is incorrect. The test remains timing-dependent and does not reliably verify that snapshot replay and buffer flipping completed.
| // Give time so the snapshot ends and the flip is done | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
A fixed sleep preserves the race: slow CI can still read the selector before replay completes, while every run now incurs a one-second delay. More importantly, this does not establish the claimed teardown safety because reloadPolicies still returns before the callback finishes. Add an explicit completion signal and wait for it deterministically.
f2b57f4 to
13b8c0b
Compare
3a36693 to
3d17b46
Compare
This reverts commit 49f0aaa.
3d17b46 to
53d6258
Compare
53d6258 to
915f7b3
Compare
915f7b3 to
b0387ee
Compare
Reverts #55598 with the required fixes.
Fix a race condition in
TestRawPacketRouterSelFlipOnRulesetReloadThis race condition caused CI delays and intermittent test failures.
The isolation reapplication change moved the
raw_packet_router_selflip out ofApplyRuleSet. The flip now occurs later, in the eBPF event-reader goroutine, after the snapshot has been replayed.However,
reloadPolicies()returns as soon as a no-op event is queued. The test then immediately reads the selector and expects it to have already flipped.Depending on the timing, this could result in either of the following:
StopReadersthen holds the manager lock whilehandleEventattempts to callGetMap, causing a deadlock. As a result, the test suite does not exit and the CI job eventually times out.This issue only affected tests. In production, the selector is still flipped correctly, isolations remain on the active buffer during reloads, and the probe is not stopped while a snapshot is being replayed.
The fix ensures that snapshot replay completes before
reloadPolicies()returns. It also moves the selector flip outside the event-reader callback, preventing teardown from deadlocking.The underlying deadlock is related to
ebpf-managerand was addressed for this use case in DataDog/ebpf-manager#288. This PR bumps theebpf-managerversion to include that fix.Use separate eBPF collections for each raw-packet router buffer
This issue did not affect CI, but it also needed to be fixed. Each raw-packet router buffer now uses its own eBPF collection.