CASCL-1724: requeue DPA status write on conflict - #55608
Conversation
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
|
I can only run on private repositories. |
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Patch is correct. The conflict retry paths refresh the live object before retrying, preserve concurrent metadata/status state, remain bounded, and include focused regression coverage. I found no actionable issues introduced by this change.
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: 0649698 | Docs | View more details | Give us feedback! |
Files inventory check summaryFile checks results against ancestor c2fb6573: Results for datadog-agent_7.84.0~devel.git.575.0649698.pipeline.134011019-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.575.0649698.pipeline.134011019-1_amd64.deb:No change detected |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: c2fb657 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +1.77 | [+0.90, +2.64] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.83 | [+0.60, +1.06] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.30 | [+0.27, +0.34] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.23 | [+0.14, +0.31] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.12 | [+0.08, +0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.04 | [+0.00, +0.07] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.15 | [-0.20, -0.10] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.18 | [-0.31, -0.06] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | -0.78 | [-0.99, -0.57] | 1 | Logs |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | -1.31 | [-1.55, -1.07] | 1 | Logs |
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 | 173.69MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 751.32KiB ≤ 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 | 512.13MiB ≤ 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 | 19 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 215.60MiB ≤ 220MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.53MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 385.89 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 419.87MiB ≤ 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.82MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 29.19 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 325.37MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 73.46 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 303.86MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 22.30 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 309.33MiB ≤ 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_logs, bounds check intake_connections: 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_security_no_fs_load, 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_metrics_logs, bounds check missed_bytes: 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_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_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_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_idle, 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_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
patch is correct — status-write failures now propagate a requeue result through both reconciliation paths, while preserving existing internal-state upserts and retry limits. Tests cover conflict and success cases.
What does this PR do?
Makes the Cluster Agent
DatadogPodAutoscaler(DPA) controller requeue the reconcile when a status write fails instead of silently dropping the update.updateAutoscalerStatusAndUpsertnow returns anautoscaling.ProcessResultand setsRequeuewhenupdatePodAutoscalerStatusreturns an error (e.g. an HTTP 409 conflict from a stale cachedresourceVersion). The two call sites propagate that result. The spec-write path already requeued on error, so no change was needed there.Motivation
CASCL-1724. A customer reported a DPA that stopped honoring its configured metric and continuously scaled down, with a 409 conflict logged on the leader every ~5 minutes.
Root cause: the status write reuses the
resourceVersionread from the informer cache (ObjectMeta: podAutoscaler.ObjectMeta); when that value is stale,UpdateStatusreturns 409. In the normal scaling path the resulting error was returned but the reconcile result stayedNoRequeue, so the key was forgotten and only retried on the next informer event (the ~5-minute resync explains the cadence).Fix approach (idiomatic controller pattern): on a status-write error, requeue and let the reconcile restart. The conflict is caused by a concurrent successful write to the object, which is itself delivered to the informer as a watch event, so a subsequent reconcile reads the up-to-date object from the cache and the write succeeds. Retries are bounded by the workqueue rate-limiter and
Process()'smaxRetry. This avoids adding live API reads inside the write path.Scope note validated during investigation: the 409 does not by itself cause the reported "CPU-only fallback". On a failed status write the controller still upserts the fresh recommendation into its internal store, and horizontal scaling runs before the status write, so the configured metric is still applied. The fallback is driven by main-recommendation staleness and is tracked separately; this PR fixes the conflict handling only.
Describe how you validated your changes
Added unit tests in
controller_test.godrivingupdateAutoscalerStatusAndUpsertagainst a fake dynamic client:TestUpdateAutoscalerStatusAndUpsertRequeuesOnConflict: a 409 on the status subresource makes the reconcile returnRequeueand surface the error, and the fresh recommendation is still persisted to the store.TestUpdateAutoscalerStatusAndUpsertNoConflict: a successful status write does not requeue (steady state preserved).Ran the full
pkg/clusteragent/autoscaling/workloadpackage test suite (go test -tags 'test kubeapiserver') — all green — andgofmt.Additional Notes
retry.RetryOnConflict+ live-read implementation on this branch, per review feedback.PR by Bits - View session in Datadog
Comment @DataDog to request changes