[autoscaling] HPA migration controller for DPA workload autoscaler#50210
[autoscaling] HPA migration controller for DPA workload autoscaler#50210clamoriniere wants to merge 4 commits into
Conversation
dd-octo-sts
Bot
added
internal
This comment has been minimized.
This comment has been minimized.
Files inventory check summaryFile checks results against ancestor 9d5f741f: Results for datadog-agent_7.80.0~devel.git.502.79ae686.pipeline.111614420-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
30 successful checks with minimal change (< 2 KiB)
On-wire sizes (compressed)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 9d5f741 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +1.98 | [-0.99, +4.94] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +1.98 | [-0.99, +4.94] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | +1.57 | [+0.60, +2.54] | 1 | Logs bounds checks dashboard |
| ➖ | docker_containers_memory | memory utilization | +1.16 | [+1.03, +1.30] | 1 | Logs |
| ➖ | file_tree | memory utilization | +0.68 | [+0.63, +0.73] | 1 | Logs |
| ➖ | ddot_metrics_sum_delta | memory utilization | +0.26 | [+0.07, +0.44] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.23 | [-0.01, +0.48] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_logs | memory utilization | +0.15 | [+0.09, +0.20] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | +0.08 | [-0.07, +0.24] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.04 | [-0.36, +0.44] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.01 | [-0.08, +0.11] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.00 | [-0.20, +0.20] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | -0.01 | [-0.20, +0.18] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.04 | [-0.48, +0.41] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | -0.06 | [-0.62, +0.51] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_v3 | ingress throughput | -0.06 | [-0.25, +0.13] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | -0.06 | [-0.11, -0.02] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | -0.08 | [-0.21, +0.06] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.14 | [-0.18, -0.10] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | -0.31 | [-0.55, -0.07] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -0.32 | [-0.37, -0.27] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | -0.37 | [-0.47, -0.27] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.53 | [-0.73, -0.32] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | -0.56 | [-0.72, -0.41] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | 721 ≥ 26 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | 246.33MiB ≤ 370MiB | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | 696 ≥ 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.18GiB ≤ 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 | 141.95MiB ≤ 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 | 471.57MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 178.09MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 354.24 ≤ 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 | 380.18MiB ≤ 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_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_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 missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_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_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.
clamoriniere
added
the
qa/done
clamoriniere
force-pushed
the
clamoriniere/hpa-migration
branch
from
8da5acd to
bbbf9b2
Compare
clamoriniere
force-pushed
the
clamoriniere/hpa-migration
branch
from
05fe08e to
315ade0
Compare
Adds a new HPA migration controller that detects existing HPAs for a DPA target, imports their configuration (metrics, replica bounds, custom queries) into the DPA spec as objectives/constraints, neutralises the HPA by setting its replica count to a very high value, and adds an HPAMigrationFinalizer to the DPA. On DPA deletion the finalizer handler restores the HPA to its original state before completing the deletion. Two correctness fixes are included: - Preserve HPA-imported objectives and constraints across profile template updates: when a ClusterProfile template hash changes, UpdateFromProfile() replaces the full DPA spec with the profile-derived one (which has no objectives). RestoreHPAImportedSpec() is now called before writing the updated spec to Kubernetes so the one-shot-imported fields survive. - Resolve %%tag_kube_cluster_name%% and %%env_VAR%% placeholders in DatadogMetric queries before storing them in the DPA spec. The Datadog backend cannot resolve cluster-agent-side template variables, so ResolveMetricQuery() (exported from externalmetrics/model) is called at import time inside resolveExternalMetricConfig(). Assisted-by: Claude:claude-sonnet-4-6
Assisted-by: Claude:claude-sonnet-4-6
clamoriniere
force-pushed
the
clamoriniere/hpa-migration
branch
from
315ade0 to
c4a69bd
Compare
…heck logic Two bugs prevented the HPA admission webhook from working correctly during HPA migration: 1. apiVersions hardcoded to ["v1"] — the MutatingWebhookConfiguration rule only targeted autoscaling/v1, so the API server never forwarded autoscaling/v2 HPA UPDATE requests to the webhook. Fixed by introducing an optional webhookWithResourceAPIVersions interface; HPAWebhook returns ["v1","v2"]. 2. Webhook blocked the cluster-agent's own disableHPA patch — the admission handler was checking only the incoming object for the managed-by-dpa annotation. Since disableHPA adds the annotation in the same patch, the webhook saw the annotation, treated it as an external edit, and reverted the spec — leaving the HPA with no selectPolicy:Disabled. Fixed by checking BOTH oldObject and object: disableHPA (old has no annotation) and restoreHPA (new has no annotation) pass through; external edits (annotation on both sides) are correctly reverted. MatchConditions (CEL) removed from the webhook registration — all filtering is now done in the Go handler (revertHPASpec), keeping the logic in one place and avoiding CEL evaluation overhead on the API server. Assisted-by: Claude:claude-sonnet-4-6
clamoriniere
force-pushed
the
clamoriniere/hpa-migration
branch
from
c4a69bd to
79ae686
Compare
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
…migration Extend the HPA→DPA migration to accept Resource/ContainerResource CPU metrics with target.type: AverageValue (UC9). Instead of refusing them, the controller resolves the workload referenced by targetRef (Deployment, StatefulSet, or Argo Rollout), reads the per-container resources.requests.cpu from the pod template, and converts the absolute target into the equivalent Utilization percentage using the existing PodCPUUtilization / ContainerCPUTargets fields. applyHPAConfigToDPASpec already emits a Utilization objective from those fields, so the DPA spec shape is unchanged. Pod-level Resource CPU sums the requests across all containers in the pod template (init containers excluded, matching Kubernetes' HPA arithmetic); ContainerResource CPU uses the named container's request. The lookup is a one-shot live GET via the dynamic client; no new informer is introduced. A new ConditionReasonMissingCPURequest reports the failure mode when the HPA's AverageValue target references a container without resources.requests.cpu or when the pod-level sum is zero; the HPA is left untouched in that case. Unknown workload kinds map to the existing UnsupportedHPAMetric reason. The kind→GVR table is built in provider.go (it already knew Argo Rollouts via discovery) and passed into workload.NewController so the migration controller and the profile WorkloadWatcher share the same supported-kinds view. Tests cover validation (AverageValue accepted, Value still rejected), conversion arithmetic (cpuPercentage rounding edge cases), per-kind lookup (Deployment / StatefulSet / Rollout / unknown), init-container exclusion, and end-to-end (HPA → extractHPAConfig → applyHPAConfigToDPASpec produces both Constraints and the Utilization objective). Assisted-by: Claude:claude-sonnet-4-6
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdfca85cda
ℹ️ 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".
| return nil, nil | ||
| } | ||
|
|
||
| return []string{unstrHPA.GetNamespace() + "/" + targetName}, nil |
There was a problem hiding this comment.
The migration index keys HPAs only by namespace/scaleTargetRef.name, so reconciliation can pick the wrong HPA (or report false ambiguity) when different workload kinds share the same name in one namespace (for example Deployment/web and StatefulSet/web). In that case a DPA targeting one kind may disable/restore the other kind’s HPA, which is a functional regression in migration safety; the key should include scaleTargetRef.kind (and ideally apiVersion) and lookup should match on the full target identity.
Useful? React with 👍 / 👎.
| parts := strings.SplitN(ref, ":", 2) | ||
| ddNS, ddName := parts[0], parts[1] |
There was a problem hiding this comment.
resolveExternalMetricConfig assumes every external metric name is datadogmetric@<ns>:<name> and directly reads parts[1] after SplitN, but validateHPAMetrics only checks the prefix. An input like datadogmetric@foo passes validation and then panics with an index-out-of-range during migration, which can break reconciliation for that DPA instead of returning a condition error.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
AlexanderYastrebov
left a comment
AlexanderYastrebov
left a comment
There was a problem hiding this comment.
Looks OK with a couple of concerns to verify:
- the HPA update flow does not lock controller out from updates (i.e. there is no possible state where controller tries to update/restore HPA and webhook prevents it)
- annotation-based state management for HPA and DPA works with git-flow (I think depending on how gitflow is implemented, it might wipe git-managed HPA/DPA annotations)
Also it might be worth splitting into two parts to simplify review and testing:
- Managed HPA lifecycle for the common standard case (cpu, memory scaling)
- Support HPA DD external metrics, etc.
| // Only act when the DPA-management annotation is present on BOTH objects. | ||
| // - disableHPA (first adds the annotation): old has no annotation → skip. | ||
| // - restoreHPA (removes the annotation): new has no annotation → skip. | ||
| // - External user edits: annotation present on both → revert. | ||
| dpaRef := oldHPA.Annotations[model.HPAManagedByDPAAnnotation] | ||
| if dpaRef == "" || incomingHPA.Annotations[model.HPAManagedByDPAAnnotation] == "" { | ||
| return admissionAllowed() | ||
| } |
There was a problem hiding this comment.
How does this work for the gitflow? I think updated version won't have an annotation added in runtime...
| return admissionAllowed() | ||
| } | ||
|
|
||
| // Build a JSON merge-patch that replaces the spec with the old (disabled) spec. |
There was a problem hiding this comment.
Why not reject the update completely since we reset the spec anyways?
The webhook may tell controller-initiated updates (disable or restore HPA) by a presense of a private annotation like:
if updated.annotations[controllerUpdateKey] == "true" {
return applyUpdate(old, new)
} else {
return rejectUpdate()
}| // Unless explicitly stated otherwise all files in this repository are licensed | ||
| // under the Apache License Version 2.0. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| // Copyright 2016-present Datadog, Inc. |
| // IsHPAMigrationEnabled returns true if the hpa-migration preview option is enabled. | ||
| // When true the controller will detect an existing HPA for the same target, neutralise it, | ||
| // and take over horizontal scaling. | ||
| func (p *PodAutoscalerInternal) IsHPAMigrationEnabled() bool { | ||
| return p.previewOptions.HPAMigration | ||
| } |
There was a problem hiding this comment.
Does it work well for reverse scenario: user discovers something is off and disables preview feature expecting the controller to restore the HPA?
2 participants
Summary
controller_hpa_migration.go) that detects existing HPAs targeting the same workload as a DPA, imports their configuration (metrics, replica bounds, custom queries) as DPA objectives/constraints, neutralises the HPA by setting its replica count to a sentinel value, and adds anHPAMigrationFinalizerto the DPA. On DPA deletion the finalizer handler restores the HPA to its original state.hpa_webhook.go) that intercepts UPDATE operations on HPA resources managed by a DPA, reverts any spec change to keep the HPA neutralised, and surfaces a warning to the user.RestoreHPAImportedSpec()is called before writing an updated profile spec to Kubernetes so one-shot-imported fields survive profile hash changes.%%tag_kube_cluster_name%%/%%env_VAR%%placeholders being stored raw in the DPA spec:ResolveMetricQuery()(exported fromexternalmetrics/model) is now called at import time insideresolveExternalMetricConfig().Test plan
HPAWebhookinhpa_webhook_test.go(8 tests, all passing)RestoreHPAImportedSpecinpod_autoscaler_test.goResolveMetricQueryinutils_test.goand template variable resolution incontroller_hpa_migration_test.gokindcluster (clamoriniere-hpa-migration) with the Datadog Operator and a real HPA targeting an nginx deployment🤖 PR description and code assisted by Claude:claude-sonnet-4-6