[clusteragent/autoscaling] Add local fallback fields to `DatadogPodAutoscalerInternal` #30776

jennchenn · 2024-11-05T15:59:07Z

What does this PR do?

Add new fields to DatadogPodAutoscalerInternal to support local fallback. This PR adds the following to DatadogPodAutoscalerInternal:

customRecommenderConfiguration to track annotation configuring a custom recommender on a DatadogPodAutoscaler resource (autoscaling.datadoghq.com/custom-recommender)
mainScalingValues, of ScalingValues type to represent scaling values from RC
fallbackScalingValues, of ScalingValues type to represent scaling values from local fallback
-> scalingValues is now a representation of the active scaling values that should be used (either mainScalingValues or fallbackScalingValues)

and some helper functions. Existing functions that build/read from status are updated to account for local scaling values.

ScalingValues definitions are also moved out of rc_schema.go

Configuration fields will likely need to be added later.

Motivation

First of changes to support local fallback

Describe how to test/QA your changes

Behavior should be unchanged right now

Possible Drawbacks / Trade-offs

both sets of scaling values need to be stored at all times, and one set is duplicated to scalingValues so we know which one is active
customRecommenderConfiguration is only updated when scaling from DPA (i.e. not from UI)

Additional Notes

Note: currently scalingValues is the only set of values being used - this will need to be changed in subsequent PRs (storing values from RC in mainScalingValues and choosing the correct set for scalingValues).

Adding new source type requires DataDog/datadog-operator#1513

agent-platform-auto-pr · 2024-11-05T16:17:35Z

[Fast Unit Tests Report]

On pipeline 50566539 (CI Visibility). The following jobs did not run any unit tests:

Jobs:

tests_flavor_dogstatsd_deb-x64
tests_flavor_heroku_deb-x64
tests_flavor_iot_deb-x64

If you modified Go files and expected unit tests to run in these jobs, please double check the job logs. If you think tests should have been executed reach out to #agent-devx-help

agent-platform-auto-pr · 2024-11-05T16:33:44Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv aws.create-vm --pipeline-id=50566539 --os-family=ubuntu

Note: This applies to commit 5dd8695

cit-pr-commenter · 2024-11-05T17:15:05Z

Regression Detector

Regression Detector Results

Metrics dashboard
Target profiles
Run ID: 4f507e13-b5b7-4637-b1ec-cb8c256e47fe

Baseline: 88a8e04
Comparison: 5dd8695
Diff

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	tcp_syslog_to_blackhole	ingress throughput	+1.51	[+1.43, +1.59]	1	Logs
➖	quality_gate_logs	% cpu utilization	+1.28	[-1.67, +4.23]	1	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.50	[-0.23, +1.23]	1	Logs
➖	file_tree	memory utilization	+0.26	[+0.12, +0.39]	1	Logs
➖	file_to_blackhole_300ms_latency	egress throughput	+0.17	[-0.47, +0.80]	1	Logs
➖	file_to_blackhole_0ms_latency	egress throughput	+0.09	[-0.75, +0.92]	1	Logs
➖	file_to_blackhole_1000ms_latency	egress throughput	+0.07	[-0.70, +0.84]	1	Logs
➖	file_to_blackhole_0ms_latency_http2	egress throughput	+0.03	[-0.83, +0.89]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.00	[-0.01, +0.01]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	-0.01	[-0.11, +0.09]	1	Logs
➖	file_to_blackhole_100ms_latency	egress throughput	-0.02	[-0.77, +0.74]	1	Logs
➖	file_to_blackhole_0ms_latency_http1	egress throughput	-0.03	[-0.80, +0.74]	1	Logs
➖	file_to_blackhole_1000ms_latency_linear_load	egress throughput	-0.15	[-0.61, +0.30]	1	Logs
➖	quality_gate_idle	memory utilization	-0.20	[-0.24, -0.16]	1	Logs bounds checks dashboard
➖	file_to_blackhole_500ms_latency	egress throughput	-0.26	[-1.03, +0.51]	1	Logs
➖	otel_to_otel_logs	ingress throughput	-0.93	[-1.58, -0.28]	1	Logs
➖	quality_gate_idle_all_features	memory utilization	-3.13	[-3.25, -3.00]	1	Logs bounds checks dashboard

Bounds Checks: ❌ Failed

perf	experiment	bounds_check_name	replicates_passed	links
❌	file_to_blackhole_100ms_latency	lost_bytes	9/10
✅	file_to_blackhole_0ms_latency	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency	memory_usage	10/10
✅	file_to_blackhole_0ms_latency_http1	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency_http1	memory_usage	10/10
✅	file_to_blackhole_0ms_latency_http2	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency_http2	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency_linear_load	memory_usage	10/10
✅	file_to_blackhole_100ms_latency	memory_usage	10/10
✅	file_to_blackhole_300ms_latency	lost_bytes	10/10
✅	file_to_blackhole_300ms_latency	memory_usage	10/10
✅	file_to_blackhole_500ms_latency	lost_bytes	10/10
✅	file_to_blackhole_500ms_latency	memory_usage	10/10
✅	quality_gate_idle	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_idle_all_features	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_logs	lost_bytes	10/10
✅	quality_gate_logs	memory_usage	10/10

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 lost_bytes: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check memory_usage: 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.

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go

vboulineau · 2024-12-04T17:26:38Z

pkg/clusteragent/autoscaling/workload/model/metadata.go

+)
+
+// Annotations represents the relevant annotations on a DatadogPodAutoscaler object
+type Annotations struct {


I don't think the fact that the info comes from annotations is important, the info itself is important. I would see it as a RecommenderConfiguration which happens to be from annotations in DPA parsing only.

That could directly be a field in DPAInt customRecommenderConfiguration *recommenderConfiguration (in which case the file may not be necessary)

vboulineau · 2024-12-04T17:27:04Z

pkg/clusteragent/autoscaling/workload/model/metadata.go

+// Annotations represents the relevant annotations on a DatadogPodAutoscaler object
+type Annotations struct {
+	Endpoint         string
+	FallbackEndpoint string


I don't think we want to support another endpoint for fallback (for now).

vboulineau · 2024-12-04T17:29:04Z

pkg/clusteragent/autoscaling/workload/model/metadata.go

+
+// exported for testing purposes
+const (
+	AnnotationsURLKey         = "autoscaling.datadoghq.com/url"


As you already have a JSON for settings, it can simplified as a single JSON object:

autoscaling.datadoghq.com/custom-recommender: {"url": <string>, "settings": {...} }

vboulineau · 2024-12-04T17:30:30Z

pkg/clusteragent/autoscaling/workload/model/metadata.go

+type Annotations struct {
+	Endpoint         string
+	FallbackEndpoint string
+	Settings         map[string]string


Suggested change

Settings map[string]string

Settings map[string]any

I think we can keep the types (int, string, etc.) so that the recommender receives typed info. This is what the JSON/Proto allows: https://github.com/DataDog/agent-payload/blob/master/jsonschema/WorkloadRecommendationsRequest.json#L51-L74

vboulineau · 2024-12-04T17:32:53Z

pkg/clusteragent/autoscaling/workload/model/metadata.go

+}
+
+// ParseAnnotations extracts the relevant autoscaling annotations from a kubernetes annotation map
+func ParseAnnotations(annotations map[string]string) Annotations {


I'm not sure the function needs to be exported. It should be call within the package and filled directly by the DatadogPodAutoscalerInternal operations (UpdateFromPodAutoscaler, so that we guarantee consistency of the object).

vboulineau · 2024-12-04T17:35:28Z

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go

 	scalingValues ScalingValues

+	// mainScalingValues represents the scaling values retrieved from the product


Suggested change

// mainScalingValues represents the scaling values retrieved from the product

// mainScalingValues represents the scaling values retrieved from the main recommender (product, optionally a custom endpoint).

vboulineau · 2024-12-04T17:49:08Z

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go

 // RemoveValues clears autoscaling values data from the PodAutoscalerInternal as we stopped autoscaling
 func (p *PodAutoscalerInternal) RemoveValues() {
 	p.scalingValues = ScalingValues{}
 }

+// RemoveMainValues clears main autoscaling values data from the PodAutoscalerInternal as we stopped autoscaling
+func (p *PodAutoscalerInternal) RemoveMainValues() {


Is there any difference with UpdateFromMainValues(ScalingValues{})

Good point, no difference, mostly added to stay consistent with the RemoveValues() function above but can remove these two

vboulineau · 2024-12-04T17:50:19Z

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go

@@ -148,16 +159,36 @@ func (p *PodAutoscalerInternal) UpdateFromSettings(podAutoscalerSpec *datadoghq.
 	p.horizontalEventsRetention = getHorizontalEventsRetention(podAutoscalerSpec.Policy, longestScalingRulePeriodAllowed)
 }

-// UpdateFromValues updates the PodAutoscalerInternal from a new scaling values
+// UpdateFromValues updates the PodAutoscalerInternal scaling values


Ideally we would need to refine this API so that it's not complete freedom from controller 🤔

Could this validation (ensuring that scalingValues is always either mainScalingValues or fallbackScalingValues) be done in one of the follow-up PRs to make use of fallbackScalingValues/mainScalingValues? There are no changes to the controller logic yet so scalingValues is the only field being used at the moment

vboulineau · 2024-12-04T17:50:35Z

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go

@@ -320,11 +356,21 @@ func (p *PodAutoscalerInternal) CreationTimestamp() time.Time {
 	return p.creationTimestamp
 }

-// ScalingValues returns the scaling values of the PodAutoscaler
+// ScalingValues returns a pointer to the active scaling values of the PodAutoscaler


Not a pointer

…dpai-local-fallback

…_adapt-dpai-local-fallback

…dpai-local-fallback

jennchenn added 4 commits November 1, 2024 17:50

Add annotations and local recommendations to pod autoscaler internal

2346774

Extend scaling values with local recommendations

bf45051

Track previous horizontal recommendations

c54a91e

Fix linter errors

0ca33ba

jennchenn added the team/containers label Nov 5, 2024

github-actions bot added the short review PR is simple enough to be reviewed quickly label Nov 5, 2024

jennchenn added 2 commits November 5, 2024 19:38

fixup! Track previous horizontal recommendations

2b491ac

Define bool to determine when local fallback active

1957a34

github-actions bot added medium review PR review might take time and removed short review PR is simple enough to be reviewed quickly labels Nov 5, 2024

Check target source prior to updating DPAI from existing status

f1305ff

jennchenn added changelog/no-changelog qa/done QA done before merge and regressions are covered by tests component/autoscaling labels Nov 5, 2024

jennchenn marked this pull request as ready for review November 5, 2024 21:26

jennchenn requested a review from a team as a code owner November 5, 2024 21:26

vboulineau reviewed Nov 6, 2024

View reviewed changes

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go Outdated Show resolved Hide resolved

pkg/clusteragent/autoscaling/workload/model/pod_autoscaler.go Outdated Show resolved Hide resolved

jennchenn added 2 commits November 6, 2024 13:12

Define annotation structure and parse directly

3e72df5

Use scalingValues to represent active scaling values

70ed026

jennchenn requested a review from vboulineau November 7, 2024 15:04

vboulineau reviewed Dec 4, 2024

View reviewed changes

github-actions bot added long review PR is complex, plan time to review it and removed medium review PR review might take time labels Dec 4, 2024

jennchenn added 4 commits December 4, 2024 13:45

Update function description comments

3afc059

Parse annotations to new custom recommender config type

5757b59

Rename type CustomRecommenderConfiguration to RecommenderConfiguration

92263a1

Merge remote-tracking branch 'origin/main' into jenn/CASCL-102_adapt-…

fffced9

…dpai-local-fallback

jennchenn added 5 commits December 5, 2024 19:33

Add recommender configuration type comment

b69bcfb

fixup! Merge remote-tracking branch 'origin/main' into jenn/CASCL-102…

8424eb9

…_adapt-dpai-local-fallback

Merge remote-tracking branch 'origin/main' into jenn/CASCL-102_adapt-…

5b04388

…dpai-local-fallback

Add custom recommender configuration to fake DPAI for tests

767066e

Set custom recommender configuration once

5dd8695

jennchenn requested a review from vboulineau December 12, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clusteragent/autoscaling] Add local fallback fields to `DatadogPodAutoscalerInternal` #30776

[clusteragent/autoscaling] Add local fallback fields to `DatadogPodAutoscalerInternal` #30776

jennchenn commented Nov 5, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 5, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 5, 2024 •

edited

Loading

cit-pr-commenter bot commented Nov 5, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

vboulineau Dec 4, 2024

vboulineau Dec 4, 2024

vboulineau Dec 4, 2024

vboulineau Dec 4, 2024

vboulineau Dec 4, 2024

vboulineau Dec 4, 2024

vboulineau Dec 4, 2024

jennchenn Dec 4, 2024

vboulineau Dec 4, 2024

jennchenn Dec 9, 2024

vboulineau Dec 4, 2024

		scalingValues ScalingValues

		// mainScalingValues represents the scaling values retrieved from the product

[clusteragent/autoscaling] Add local fallback fields to DatadogPodAutoscalerInternal #30776

Are you sure you want to change the base?

[clusteragent/autoscaling] Add local fallback fields to DatadogPodAutoscalerInternal #30776

Conversation

jennchenn commented Nov 5, 2024 • edited Loading

What does this PR do?

Motivation

Describe how to test/QA your changes

Possible Drawbacks / Trade-offs

Additional Notes

agent-platform-auto-pr bot commented Nov 5, 2024 • edited Loading

agent-platform-auto-pr bot commented Nov 5, 2024 • edited Loading

Test changes on VM

cit-pr-commenter bot commented Nov 5, 2024 • edited Loading

Regression Detector

Regression Detector Results

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

Bounds Checks: ❌ Failed

Explanation

CI Pass/Fail Decision

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[clusteragent/autoscaling] Add local fallback fields to `DatadogPodAutoscalerInternal` #30776

[clusteragent/autoscaling] Add local fallback fields to `DatadogPodAutoscalerInternal` #30776

jennchenn commented Nov 5, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 5, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Nov 5, 2024 •

edited

Loading

cit-pr-commenter bot commented Nov 5, 2024 •

edited

Loading