Support extra DCA RC clients - #55606
Draft
clamoriniere wants to merge 15 commits into
Draft
Conversation
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
Contributor
|
I can only run on private repositories. |
dd-octo-sts
Bot
added
team/container-platform
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
AI review by Codex (OpenAI) - workflow run
Patch is incorrect. Additional clients can use the wrong TUF director root, and cache collision validation compares unnormalized paths.
| if spec.DatabaseFileName == "" { | ||
| spec.DatabaseFileName = fmt.Sprintf("remote-config-%s.db", safeRemoteConfigInstanceName(spec.Name)) | ||
| } | ||
| if owner, found := databaseFileOwners[spec.DatabaseFileName]; found { |
Contributor
There was a problem hiding this comment.
Collision detection compares raw filenames, but NewService later resolves them with path.Join(run_path, filename). Values such as x/../remote-config.db or two differently normalized paths therefore bypass this check and share a cache, allowing one service to corrupt or clear another's state. Validate basename-only filenames or compare cleaned resolved paths.
Contributor
Author
There was a problem hiding this comment.
it is fixed now
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: bcc073b | Docs | View more details | Give us feedback! |
Contributor
Files inventory check summaryFile checks results against ancestor c2fb6573: Results for datadog-agent_7.84.0~devel.git.587.bcc073b.pipeline.134076030-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.587.bcc073b.pipeline.134076030-1_amd64.deb:No change detected |
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: c2fb657 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_cpu | % cpu utilization | +1.29 | [+1.02, +1.56] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | +0.48 | [-0.40, +1.35] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.16 | [+0.13, +0.19] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.16 | [+0.12, +0.20] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.15 | [+0.10, +0.20] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.14 | [+0.10, +0.19] | 1 | Logs bounds checks dashboard |
| ➖ | dsd_uds_10mb_3k_timestamped_contexts_memory | memory utilization | +0.11 | [-0.10, +0.33] | 1 | Logs |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.05 | [-0.13, +0.03] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.50 | [-0.62, -0.39] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.54 | [-0.78, -0.31] | 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 | 174.18MiB ≤ 179MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 753.64KiB ≤ 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 | 522.78MiB ≤ 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 | 209.74MiB ≤ 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.50MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 372.75 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 20 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 426.34MiB ≤ 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 | 72.03MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 28.43 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 328.73MiB ≤ 355MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 64.25 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 305.23MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 22.64 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 308.86MiB ≤ 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_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_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_idle, bounds check intake_connections: 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_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_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_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_private_action_runner, 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 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_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.
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
Contributor
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
9 successful checks with minimal change (< 2 KiB)
|
The secrets status template ended with `{{- end -}}`, whose trailing `-}}`
swallowed the file's final newline, so the section emitted no trailing
newline at all. The status renderer only appends a single newline after each
section and printHeader writes its `===` rule immediately, so the next
section's header landed flush against the last line of the secrets output.
This was latent: secrets was the last rendered section, so nothing ever
followed it. Every other status template already ends with a newline.
Assisted-by: Claude:claude-opus-5
Per-instance remote configuration status was already exported under remoteConfigStatus.instances, but the status templates only rendered the default client. A Cluster Agent with extra RC clients showed no sign they existed, and a failing extra client was invisible to `status`. Render every instance, skipping the default one so it is not listed twice. Also record each instance's endpoint URL, which is the point of extra clients: they can target a different backend than the default. Userinfo is stripped from the URL before it reaches the status output, since rc_dd_url is user supplied and status output is routinely pasted into tickets. Assisted-by: Claude:claude-opus-5
There was no way to see, from `datadog-cluster-agent status`, whether workload autoscaling was receiving anything over remote configuration. Report the number of DatadogPodAutoscalers, leader state, and per RC product the last received config version, last update time and age, configs in the last update, cumulative update count, and last apply error. The update is recorded before the configs are processed, so a config that fails to apply still shows as received and its error is surfaced separately rather than looking like a connectivity problem. Three states are distinguished so the section is never ambiguous: disabled in config, enabled but not started (StartWorkloadAutoscaling failing is logged, not fatal), and running. The section is named "Autoscaling" because status sections are ordered alphabetically with only "collector" special cased; this places it after "Autodiscovery" and leaves the group for cluster autoscaling to join later. Only the workload products are reported for now. The status also names the RC client serving the autoscaling products, using the same instance key as the Remote Configuration section so the two can be cross-referenced. Assisted-by: Claude:claude-opus-5
Each extra client was rendered as its own free-standing block, which read as a series of unrelated stanzas rather than one collection. Group them under an "Additional clients" heading and render one indented list entry per client, so the number of extra clients is obvious at a glance. The default client is now filtered out by the provider rather than by a name comparison inside the templates. That removes the duplicated magic string from both templates and lets the heading appear only when at least one extra client exists. Assisted-by: Claude:claude-opus-5
Extra clients reported status under "cluster-agent:<name>", so a client configured as "autoscaling" showed up as "cluster-agent:autoscaling" in both the Remote Configuration and Autoscaling status sections. The prefix carried no information the section heading did not already give. Report them under the configured name instead. The prefix did guarantee that an extra client could never collide with the default client's status key, so reject "Remote Config" as a client name: without the prefix such a client would silently share the default client's status entry, with the two overwriting each other's fields and no error anywhere. Assisted-by: Claude:claude-opus-5
Every additional client had to spell out the Remote Configuration products it owned, but the only useful value was always the full product set of the subsystem the client serves. Drop the "products" setting and derive the set from the client name instead, so an entry needs only an endpoint and a key. Names match the configuration that enables each subsystem and cover exactly the products that subsystem resolves through ClientForProducts. An unrecognised name is rejected: the name is the only thing selecting products, so a client that owned nothing would never be used. The autoscaling set includes the cluster autoscaling product on purpose. command.go resolves all enabled autoscaling products in one call, so leaving CLUSTER_AUTOSCALING_VALUES on the default client would split the subsystem across two clients as soon as autoscaling.cluster.enabled is turned on. Products with no ClientForProducts consumer are deliberately excluded. Additional clients are created lazily when one of their products is requested, so an unconsumed product would leave that product subscribed nowhere at all. K8S_INJECTION_DD is the current example and stays on the default client. A test pins the sets to that consumer list so the two cannot drift apart. Assisted-by: Claude:claude-opus-5
Follow-up cleanup now that the feature works, with no behaviour change. Export DefaultStatusInstance from pkg/config/remote/service and use it everywhere. The "Remote Config" literal was repeated across four files, two of them carrying "keep in sync" comments. The rcType argument to NewService keeps its own literal: it is a log prefix, not a status key, and only happens to share the string. Drop the two options the default RC service passed that already matched the service defaults, along with the constant and variable that only existed to feed them. What remains is the case that differs: when the key comes from remote_configuration.api_key, watch that setting for runtime updates instead of the process-wide api_key. Delete validateAdditionalRemoteConfigProducts. Products now come only from curated presets, so it could never fire, and its messages still named the removed "products" setting. The invariants it checked are asserted against the preset table by TestRemoteConfigClientPresetsMatchConsumers. Build the product routing map once instead of keeping extraProductOwners alongside byProduct with the same key set. Delete safeRemoteConfigInstanceName. Client names are now a closed set of preset keys, all filename safe, so it can no longer change anything. Assisted-by: Claude:claude-opus-5
Contributor
Go Package Import DifferencesBaseline: c2fb657
|
This change was also adjusting the default and MRF remote config services: pointing their API key watcher at the setting their key actually comes from, and giving MRF its own status entry. Both are improvements to pre-existing behaviour, not requirements of additional clients. The API key watcher is hardcoded to "api_key" today, so the MRF service reacts to the wrong setting and ignores updates to its own. The status expvars were package-level globals that every service instance wrote to, so MRF has always overwritten the default client's entry; leaving it on the shared entry keeps exactly that behaviour. Dropping both makes this change strictly additive for existing deployments and leaves the two files identical to main. Each is worth fixing on its own, where it can be reviewed as the behaviour change it is. Assisted-by: Claude:claude-opus-5
Additional clients exposed an "api_key_setting" so their key could be read from, and followed at, another config path. Nothing used it: every client sets api_key directly, with ENC[] handling secrets, and the only caller passed an empty string, which the service read as "do not follow updates". Drop the setting and express what the feature actually needs. WithoutAPIKeyUpdates replaces WithAPIKeyUpdateSetting: a client given its own static key must not have it replaced when the process-wide api_key changes. Which setting the watcher follows is no longer configurable, so the callback goes back to its original comparison, and the unrelated trimming of updated keys goes with it. Also drops a TrimSpace in WithStatusInstance that could never fire, since both callers pass an already-trimmed value. Assisted-by: Claude:claude-opus-5
The Cluster Agent kept its own "remote-config.db" literal to seed the cache filename collision map, duplicating the service package's default. The name is not configurable: there is no setting for it, and the default client never calls WithDatabaseFileName, so the compile-time default is the whole story. Export it and have both sides use it. That map is what stops an additional client from claiming the default client's cache, so the duplication was a latent bug: changing the service default would have left the Cluster Agent seeding the map with a stale name, and an extra client could then quietly share the default cache. Assisted-by: Claude:claude-opus-5
clamoriniere
added
the
qa/done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Allows the Cluster Agent to run several Remote Configuration clients in one process, each with its own endpoint, API key, TUF roots and cache database.
Extra clients are declared under
cluster_agent.remote_configuration.additional_clients. The client name selects the subsystem it serves, and with it the RC products it owns —autoscaling,kubeactions,private_action_runnerorapm_instrumentation. Any product not claimed by an extra client keeps using the default client and the default cache, so existing deployments are unaffected. Clients are created lazily, only when a product they own is actually requested.The setting is not published in the generated
datadog.yamldocumentation: it is intended for internal testing, not for customer configuration.datadog-cluster-agent statusreports each client, so a misconfigured extra client is visible rather than silent:A new
Autoscalingstatus section reports workload autoscaling health: the number ofDatadogPodAutoscalers, which RC client serves it, and per product the last received config version and update time.Both samples are taken from a Cluster Agent whose autoscaling client targets a different Remote Configuration backend than its default client, with the extra endpoint anonymized.
Motivation
The Cluster Agent assumed a single Remote Configuration client per process, which made endpoint and API key ownership process-wide: every service watched the top-level
api_key, status expvars were shared, and instances would have reused the same cache database.Running autoscaling against a different Remote Configuration backend than the rest of the Cluster Agent requires those resources to be per-instance.
Describe how you validated your changes
Unit tests covering spec parsing and validation, product routing, lazy client creation, API key isolation, per-instance status, and the new status sections.
Also validated end to end on a local kind cluster, with the autoscaling client pointed at the staging Remote Configuration backend while the default client stayed on production: both clients authenticated independently, kept separate cache databases, and the autoscaling client received live configuration.
Additional Notes
Configuration is rejected at startup rather than degrading silently when it cannot work: process-level products (
AGENT_CONFIG,AGENT_TASK) on an extra client, a product claimed by two clients, products used by one subsystem split across clients, colliding or path-like cache database filenames, and a client namedRemote Config, which is reserved for the default client's status entry.PR by Bits - View session in Datadog
Comment @DataDog to request changes
🤖 PR description and code assisted by Claude:claude-opus-5