fix(ssi): wait for RC before inject-all - #55376
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: caa859b | Docs | View more details | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 237a93bf: Results for datadog-agent_7.84.0~devel.git.634.caa859b.pipeline.134556036-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.634.caa859b.pipeline.134556036-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
8 successful checks with minimal change (< 2 KiB)
|
e2e3081 to
a8fb51b
Compare
3a1389f to
f8e4966
Compare
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 237a93b Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_security_idle | memory utilization | +0.45 | [+0.40, +0.50] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.41 | [+0.33, +0.50] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | +0.23 | [-0.01, +0.48] | 1 | Logs |
| ➖ | quality_gate_private_action_runner | memory utilization | +0.16 | [+0.03, +0.29] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.12 | [+0.08, +0.15] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | +0.05 | [-0.16, +0.26] | 1 | Logs |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.04 | [+0.00, +0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.21 | [-0.44, +0.02] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.29 | [-0.34, -0.25] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -0.87 | [-1.75, -0.00] | 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 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 172.00MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 747.65KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 515.45MiB ≤ 537MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.15MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 17 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 209.71MiB ≤ 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.09MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 354.88 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 418.32MiB ≤ 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.72MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 28.08 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 328.91MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 62.25 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 307.55MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.46 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 313.96MiB ≤ 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_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_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.
- quality_gate_metrics_logs, bounds check memory_usage: 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 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_metrics_logs, bounds check intake_connections: 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_logs, bounds check total_bytes_received: 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 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 intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, 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_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
f8e4966 to
8711468
Compare
Empty GetConfigs at Cluster Agent start is not a round-trip. Applying inject-all in that window instruments pods a remote deny would skip.
8711468 to
d9bf966
Compare
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Patch is incorrect: it waits for a global RC sync before registering APM_POLICIES, so the sync can complete without fetching that product and incorrectly enable inject-all.
There was a problem hiding this comment.
A failed first backend Remote Config refresh can still mark the local client as synchronized. This enables inject-all from an empty cache and can ignore an unseen remote deny.
🤖 Datadog Autotest · Commit d9bf966 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| if client.HasSynced() { | ||
| m.applySnapshotAndSubscribe(client) | ||
| return | ||
| } |
There was a problem hiding this comment.
If nothing has called client.Subscribe(state.ProductApmPolicies, ) then this sync is pretty meaningless for you as you won't get ApmPolicies products without the subscribe. (which I think is what the AI model is calling out below)
If you know something has called subscribe already, then I'm confused about the intent of this code.
There was a problem hiding this comment.
APM_POLICIES is already in WithProducts when the cluster-agent builds the client, before Start() and before this subscribe. Subscribe would register it if it weren't already there; here it just attaches the callback, the product is already on the wire. HasSynced was just "has that first poll finished?", so we wouldn't treat an empty cache as "no policies" too early.
A global sync flag is still the wrong signal though. It doesn't say which products were actually requested. Dropped it. The mutator now uses SubscribeAll(..., WithInitialUpdate()): you get called once there is an answer for that product (including empty). If a poll that requested it has already completed, that happens during Subscribe; otherwise on the first poll that does.
And the wait is bounded, after a minute we instrument anyway, rather than silently withhold SSI from a configuration that asked for it.
WDYT?
A plain subscription only fires on config changes, so a subscriber cannot tell "the backend has no configs for me" from "the client has not asked yet". WithInitialUpdate delivers the product's current state once: during Subscribe if a poll already carried the product, otherwise on the first poll that does. Opt-in per subscription, so subscribers that do not pass it see no change in behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Subscribe with WithInitialUpdate so a confirmed-empty APM_POLICIES snapshot reaches onRemoteConfigUpdate, which is what releases inject-all, and so a snapshot the client already holds is applied without waiting for another poll. A successful poll of the local RC service is not proof that it reached the backend, so "no answer" cannot be told apart from "no policies". Bound the wait at one minute and instrument, rather than silently withhold SSI from a configuration that asked for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 381f7d09e8
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caa859b877
ℹ️ 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".
Description
Do not apply Single Step Instrumentation inject-all until the Cluster Agent has completed a Remote Config round-trip for
APM_POLICIES. An empty local cache at subscribe time is not a backend snapshot, so treating it as "no rules" could instrument pods that a remote deny would have skipped. The admission webhook does not block: unmatched pods pass through unchanged until the first snapshot arrives (or is confirmed empty).Follow-up to #55104.
Related Issue
#55104
Type of Change
Checklist
How Has This Been Tested?
Unit tests in
pkg/clusteragent/admission/mutate/autoinstrumentation(dda inv test --targets=./pkg/clusteragent/admission/mutate/autoinstrumentation).Additional Notes
Depends on the matching/RC layering from #55104. This PR is opened against that branch.
Static Helm/Operator targets still match immediately while RC is pending. If Remote Config never syncs, inject-all stays withheld (fail-closed).