fix(cloudfoundry): avoid typed-nil DCAClient in cf_vm workloadmeta collector - #54929
fix(cloudfoundry): avoid typed-nil DCAClient in cf_vm workloadmeta collector#54929Kyle-Neale wants to merge 1 commit into
Conversation
…llector GetClusterAgentClient() returns a concrete *DCAClient on the error path (PR #50814 changed the return type from DCAClientInterface). cf_vm's getDCAClient assigned that into the interface field c.dcaClient before checking err, poisoning the field with a non-nil interface wrapping a nil pointer. The next Pull() short-circuited past the nil check and called a method on the nil *DCAClient receiver, crash-looping the node agent whenever the cluster agent is unreachable. Only assign to the field on success, and have Pull() use the guarded local instead of the field. Also add a mutex since getDCAClient is reachable from concurrent Pull calls.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 9a8a2b8 | Docs | Datadog PR Page | Give us feedback! |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 3b2688f Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_private_action_runner | memory utilization | +0.67 | [+0.55, +0.79] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.19 | [+0.04, +0.33] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.17 | [-0.08, +0.41] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.14 | [+0.10, +0.18] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.03 | [-0.09, +0.15] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.03 | [-0.05, +0.10] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | -0.22 | [-0.33, -0.12] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -0.85 | [-1.70, -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 | 171.28MiB ≤ 178MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 742.35KiB ≤ 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 | 509.60MiB ≤ 538MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 207.77MiB ≤ 229MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 263.78MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 369.87 ≤ 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 | 417.35MiB ≤ 439MiB | 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 | 72.25MiB ≤ 76MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 27.52 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 326.46MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 62.77 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 300.95MiB ≤ 314MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 21.90 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 313.52MiB ≤ 343MiB | 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_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 memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, 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_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_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_metrics_logs, bounds check total_bytes_received: 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_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_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_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_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_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
Summary
GetClusterAgentClient()returns a concrete*DCAClienton its error path (changed fromDCAClientInterfacein [CONTP-1679] feat(Autodiscovery): Add Instrumentation check AD provider #50814).cf_vm.go'sgetDCAClientassigned that into the interface fieldc.dcaClientbefore checking the error, which poisons the field with a non-nil interface wrapping a nil pointer.Pull()cycle short-circuits past the (now useless) nil check onc.dcaClientand callsGetCFAppsMetadataForNodeon the nil*DCAClientreceiver, panicking with a nil-pointer dereference.cf apply-changesentirely.c.dcaClientafter confirmingerr == nil, and havePull()use the already-guarded local value instead of re-reading the field. Also added a mutex aroundgetDCAClient, since it's reachable from concurrentPullcalls and was previously unlocked.Considered reverting
GetClusterAgentClient()'s return type back toDCAClientInterfaceinstead, but that would breakcomp/core/autodiscovery/providers/instrumentationchecks.go(added in the same #50814), which needs the assignment to satisfy a different, narrower interface thatDCAClientInterfacedoesn't cover. Fixing the consumer is the safe path.Test plan
TestPullDCAUnreachableTwice, which reproduces the exact panic (buildURL→doQuery→GetCFAppsMetadataForNodeon a nil receiver) on the secondPull()when the cluster agent is unreachable — verified it panics against the old logic and passes against this fix.go test -race ./comp/core/workloadmeta/collectors/internal/cloudfoundry/vm/...passes (all 9 tests).GetClusterAgentClient()for the same poison-before-check pattern; only this one was vulnerable.