[CONTP-1546] feat: Datadog instrumentation controller with handler event dispatching#50206
Conversation
Go Package Import DifferencesBaseline: 8ae8fee
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: 432dfb5 | Docs | Datadog PR Page | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 8ae8fee5: Results for datadog-agent_7.80.0~devel.git.592.432dfb5.pipeline.112082528-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
19 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 8cf3b53 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | -1.09 | [-4.00, +1.81] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | file_tree | memory utilization | +0.40 | [+0.36, +0.45] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.32 | [+0.07, +0.57] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.17 | [+0.12, +0.22] | 1 | Logs bounds checks dashboard |
| ➖ | otlp_ingest_metrics | memory utilization | +0.16 | [+0.00, +0.31] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | +0.12 | [-0.04, +0.28] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | +0.09 | [-0.15, +0.33] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.03 | [-0.50, +0.55] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.01 | [-0.13, +0.15] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.10, +0.10] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_v3 | ingress throughput | +0.00 | [-0.20, +0.20] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | -0.00 | [-0.21, +0.20] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | -0.01 | [-0.08, +0.06] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.01 | [-0.21, +0.19] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | -0.03 | [-0.43, +0.37] | 1 | Logs |
| ➖ | ddot_metrics_sum_delta | memory utilization | -0.03 | [-0.21, +0.15] | 1 | Logs |
| ➖ | docker_containers_memory | memory utilization | -0.05 | [-0.15, +0.05] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.07 | [-0.52, +0.39] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -0.09 | [-0.14, -0.04] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | -0.32 | [-0.42, -0.23] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.65 | [-0.69, -0.61] | 1 | Logs bounds checks dashboard |
| ➖ | docker_containers_cpu | % cpu utilization | -1.09 | [-4.00, +1.81] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -1.15 | [-1.35, -0.94] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -1.32 | [-2.29, -0.34] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | 673 ≥ 26 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | 244.26MiB ≤ 370MiB | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | 695 ≥ 26 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | 0.16GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_0ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | 0.20GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_1000ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | 0.17GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_100ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | 0.19GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_500ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 140.57MiB ≤ 147MiB | 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 | 467.98MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 180.32MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 364.64 ≤ 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 | 383.96MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | 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_metrics_logs, bounds check cpu_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 memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_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_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_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_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.
7773a6b to
2f7eda6
Compare
a055d92 to
18d2f2e
Compare
18d2f2e to
6bfdb10
Compare
| statusClient dynamic.Interface | ||
| lister cache.GenericLister | ||
| synced cache.InformerSynced | ||
| workqueue workqueue.TypedRateLimitingInterface[string] |
There was a problem hiding this comment.
ℹ️ The workqueue uses namespace/name, not the CR object, as the key. On reconcile we read current state from the informer cache and diff agains the lastSeen map. This gives us automatic deduplication avoiding stale-snapshots from being processed.
| lastSeenMu sync.Mutex | ||
| lastSeen map[string]*datadoghq.DatadogInstrumentation |
There was a problem hiding this comment.
ℹ️ We're storing the last successful reconciled state. We need this to reconstruct the old/new diff for event classification.
| "k8s.io/client-go/tools/cache" | ||
| ) | ||
|
|
||
| func classifySectionEvent(handler Handler, oldObj, newObj *datadoghq.DatadogInstrumentation) (EventType, bool) { |
There was a problem hiding this comment.
ℹ️ Event classification is per-handler, not per-CR. A single CR update might be a 'create' for one handler (section added) and a 'delete' for another (section removed).
| // Only the leader writes status conditions back to the CR. All replicas run handlers because | ||
| // some handlers must act on events regardless of leadership. Known gap: a handler error on a | ||
| // follower will not be reflected in the CR status if the leader's handler succeeded. | ||
| if !c.isLeader() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
❓ This is an obvious gap in the controller + handler design. I want all DCA replicas to process instrumentation CRs regardless of leadership, but there's only one CR status to update.
Any suggestions on how this could be improved?
28f3097 to
3edd5f8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3edd5f853b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err := UnstructuredFromDatadogInstrumentation(statusObj, unstructuredStatusObj); err != nil { | ||
| return err | ||
| } | ||
| _, err = client.Resource(gvrDatadogInstrumentation).Namespace(latest.Namespace).UpdateStatus(ctx, unstructuredStatusObj, metav1.UpdateOptions{}) |
There was a problem hiding this comment.
Avoid unconditional status writes in reconcile loop
This path always performs UpdateStatus even when the computed conditions are unchanged. Since handleUpdate enqueues every update and classifySectionEvent maps any resource that still has the section to EventUpdate, a leader instance can repeatedly reconcile the same object and write identical status forever, creating sustained API churn and repeated handler execution for instrumented CRs. Please only call UpdateStatus when at least one condition actually changed (or ignore status-only updates before dispatch).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I've added a check in Controller#handleUpdate to drop update events if the object's generation hasn't changed (.spec hasn't changed).
dustmop
left a comment
There was a problem hiding this comment.
LGTM for agent-configuration owned files
triviajon
left a comment
There was a problem hiding this comment.
lgtm for container-integrations owned files
3edd5f8 to
bac4c9b
Compare
bac4c9b to
575a4cd
Compare
| // admission webhooks, should be added here rather than constructed in the generic | ||
| // controller startup path. | ||
| type Deps struct { | ||
| IsLeader func() bool |
There was a problem hiding this comment.
nit: it could be worth to explicitly calling out the impact of the isLeader method here. If the handler doesn't drop events when the cluster-agent is not leader, they could theoretically be wasting time doing work, and in the worst case, mess things up if it's not idempotent
f5c7062 to
432dfb5
Compare
…ent dispatching (#50206) ### What does this PR do? Introduces a `DatadogInstrumentation` CRD controller in the Cluster Agent that watches `DatadogInstrumentation` custom resources and dispatches section-level events (`EventCreate`, `EventUpdate`, `EventDelete`) to product-specific handlers. - **Controller** (`pkg/clusteragent/instrumentation/`): Uses a key-only workqueue with a `lastSeen` store to diff current state (from the informer cache) against the last successfully reconciled state. This gives natural deduplication and prevents stale-snapshot bugs on requeue. - **`Handler` interface**: Product handlers implement `HasSection`, `Handle`, `Validate`, and `SupportsTarget`. Each handler owns a section of the CR spec and only receives events when its section changes. - **Status conditions**: After handlers run, the leader writes `metav1.Condition` entries back to `.status.conditions` with conflict retry. - **Autodiscovery handler stub**: Skeleton handler that checks for `spec.config.checks` presence. Returns `ConditionUnknown` until check delivery is implemented in a follow-up. - **Registration**: Gated behind `datadog_instrumentation.enabled: true` in the Cluster Agent controller catalog. ### Motivation Part of the [DatadogInstrumentation CRD initiative](https://datadoghq.atlassian.net/wiki/spaces/CONTP/pages/6564659495/DatadogInstrumentation+CRD+for+Workload-Scoped+Product+Enablement) to enable workload-scoped product configuration through a Kubernetes-native CRD. This PR builds the shared controller platform that all product handlers (Autodiscovery check delivery, APM SSI, etc.) will plug into. Depends on: DataDog/datadog-operator#2962 ### Describe how you validated your changes Unit tests covering reconciliation dispatch, section-level event classification, leader-only status writes, `lastSeen` lifecycle, event deduplication, and status condition updates with a fake dynamic client. #### CR status is updated on each create/update/delete event <img width="696" height="173" alt="image" src="https://github.com/user-attachments/assets/37310f64-caf8-4167-b305-331a6ead76c9" /> <img width="793" height="610" alt="image" src="https://github.com/user-attachments/assets/5f884b70-a71e-4621-92b2-f5299fd62d68" /> Co-authored-by: mathew.estafanous <[email protected]>
…ok (#50428) ### What does this PR do? Depends on #50206 Adds a validating admission webhook for `DatadogInstrumentation` CRs and refactors the `Webhook.Resources()` interface to support non-v1 API versions. The validation webhook listens to CREATE/UPDATE events from `datadoginstrumentations.datadoghq.com` then runs three step validation: 1. **Unique targetRef per namespace**. Only one CR can target a given workload in a namespace. If a CR is submitted that matches an existing targetRef, it is rejected. 2. **Target compatibility.** For each handler where HasSection returns true, the controller calls SupportsTarget to verify the CR's targetRef kind is supported by that handler. For example, if a CR targets a Service but has spec.config.apm populated, the SSI handler rejects it because it only supports Deployments and StatefulSets. 3. **Product-specific validation.** Each relevant handler's Validate is called to check its own fields (check names, tracer version formats, etc.). ### Motivation The validation webhook gives users immediate feedback on invalid CRs (duplicate targets, unsupported workload kinds, bad config) instead of silently ignoring bad configuration like annotations did. #### Additional Notes The `Webhook#Resources()` interface was refactored to return `[]common.WebhookResourceRule` to give each webhook full flexibility on the group/version/kind to intercept. Previously, `Resources()` used a group to many kinds map, and hardcoded the version to `v1`. Unfortunately the DatadogInstrumentation CRD versioning isn't `v1` but `v1alpha1`, so CR events were never intercepted. A large in-place refactor had to be made to replace the hardcoded string with a configurable version. ### Describe how you validated your changes Unit tests covering webhook validation (duplicate targetRef on create/update, target compatibility, product validation errors, multi-handler scenarios, fail-open on nil lister, section skipping) and the `WebhookResourceRule` refactor across existing webhook controller tests. ```bash go test -tags kubeapiserver,test ./pkg/clusteragent/admission/validate/datadoginstrumentation/ ``` #### Manual QA 1. Start the agent with instrumentation CRD + admission controller enabled ```yaml platform: type: "minikube" name: "instrumentation-crd" reset: false helm: versions: agent: build: {} cluster_agent: build: {} injector: "0.52.0" manifests: - path: "rbac.yaml" namespace: "system" config: datadog: operator: enabled: false clusterChecks: enabled: true kubelet: tlsVerify: false clusterAgent: enabled: true admissionController: enabled: true env: - name: DD_INSTRUMENTATION_CRD_CONTROLLER_ENABLED value: "true" ``` 2. Attempt to deploy an invalid CR instance (ie. wrong targetRef kind `SomethingElse`) ```yaml apiVersion: datadoghq.com/v1alpha1 kind: DatadogInstrumentation metadata: name: example-instrumentation namespace: default spec: targetRef: apiVersion: apps/v1 kind: Something-Else name: example-workload config: checks: - integration: redisdb containerImage: - redis initConfig: {} instances: - host: "%%host%%" port: 6379 password: "12345678" tags: - service:redis logs: - type: file path: /var/log/redis/*.log source: redis service: redis tags: - service:redis ``` 3. Validation webhook rejects CR create/update. ``` > admission webhook "datadog.webhook.datadog.instrumentation.validation" denied the request: handler "autodiscovery" does not support target kind "Something-Else" ``` 4. Duplicate CR rejected ``` > admission webhook "datadog.webhook.datadog.instrumentation.validation" denied the request: DatadogInstrumentation "example-instrumentation" in namespace "default" already targets Deployment/example-workload ``` Co-authored-by: mathew.estafanous <[email protected]>
### What does this PR do? Bump SGC on DCA to counteract increase from new DatadogInstrumentation CRD handler feature: #50206. Note: no new external dependencies were included, just update of the Datadog Operator dep and code for the handlers Co-authored-by: gabe.dossantos <[email protected]>
What does this PR do?
Introduces a
DatadogInstrumentationCRD controller in the Cluster Agent that watchesDatadogInstrumentationcustom resources and dispatches section-level events (EventCreate,EventUpdate,EventDelete) to product-specific handlers.pkg/clusteragent/instrumentation/): Uses a key-only workqueue with alastSeenstore to diff current state (from the informer cache) against the last successfully reconciled state. This gives natural deduplication and prevents stale-snapshot bugs on requeue.Handlerinterface: Product handlers implementHasSection,Handle,Validate, andSupportsTarget. Each handler owns a section of the CR spec and only receives events when its section changes.metav1.Conditionentries back to.status.conditionswith conflict retry.spec.config.checkspresence. ReturnsConditionUnknownuntil check delivery is implemented in a follow-up.datadog_instrumentation.enabled: truein the Cluster Agent controller catalog.Motivation
Part of the DatadogInstrumentation CRD initiative to enable workload-scoped product configuration through a Kubernetes-native CRD. This PR builds the shared controller platform that all product handlers (Autodiscovery check delivery, APM SSI, etc.) will plug into.
Depends on: DataDog/datadog-operator#2962
Describe how you validated your changes
Unit tests covering reconciliation dispatch, section-level event classification, leader-only status writes,
lastSeenlifecycle, event deduplication, and status condition updates with a fake dynamic client.CR status is updated on each create/update/delete event