[HPA] Add v1alpha1.MetricSpec to support for Pod custom metrics #1651

moh-osman3 · 2023-04-13T02:56:50Z

Replaces #1561

This PR adds a new object v1alpha1.MetricSpec which is a subset of the metric types in https://pkg.go.dev/k8s.io/api/autoscaling/v2#MetricSpec

Currently v1alpha1.MetricSpec only supports PodsMetricsSource but can support more metric types in the metric array in the future. For example supporting ResourceMetricsSource in the future would allow resources to be configured in the HPA's metric array and allow us to deprecate targetCPUUtilization and targetMemoryUtilization

Resolves #1560

apis/v1alpha1/opentelemetrycollector_webhook.go

config/manager/kustomization.yaml

jaronoff97 · 2023-04-13T19:51:40Z

pkg/collector/horizontalpodautoscaler.go

@@ -86,11 +86,17 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al
 			},
 		}

-		if otelcol.Spec.Autoscaler != nil && otelcol.Spec.Autoscaler.Behavior != nil {
+		if otelcol.Spec.Autoscaler.Behavior != nil {


have we confirmed that Autoscaler isn't nil by this point?

Hmm there are several references of using otelcol.Spec.Autoscaler without a nil check in this function. In fact this is the only place that nil checks. If it is unset, otelcol.Spec.Autoscaler is created by the defaulting webhook at https://github.com/open-telemetry/opentelemetry-operator/blob/main/apis/v1alpha1/opentelemetrycollector_webhook.go#L72

So it should never be nil, but now I'm wondering if there cases where the defaulting webhook is never called and this causes a segfault?

thats a good q... maybe we add the guard at the start of the function?

Added the nil check at the start and returning nil if it is unset!

jaronoff97 · 2023-04-13T19:52:50Z

pkg/collector/horizontalpodautoscaler.go

 			behavior := ConvertToV2beta2Behavior(*otelcol.Spec.Autoscaler.Behavior)
 			autoscaler.Spec.Behavior = &behavior
 		}

+		// check for custom metrics
+		if len(otelcol.Spec.Autoscaler.Metrics) > 0 {
+			metrics := ConvertToV2Beta2PodMetrics(otelcol.Spec.Autoscaler.Metrics)


are we able to make this addition to confirm to the v2 spec to start with? Or given it's already using something similar to the v1alpha we want to keep it that way?

hm maybe i'm not understanding what this should be doing. What's the type of autoscaler.Spec.Metrics and how does that diff from what you're adding in?

otelcol.Spec.Autoscaler.Metrics has type v1alpha1.MetricSpec this supports only the PodsMetricSource

The HPA is either expecting a v2.MetricSpec or a v2beta2.MetricSpec so this does the necessary conversion for the HPA object depending on the version. This is something similar done for .Spec.Autoscaler.Behavior

The reason I created v1alpha1.MetricSpec instead of directly using a v2.MetricSpec is because v2.MetricSpec is way too large of an object for the use cases of our HPA (see https://pkg.go.dev/k8s.io/api/autoscaling/v2#MetricSpec). It supports 5 different metric types which would require a lot of validation and the deprecation of targetCPUUtilization and targetMemoryUtilization.

v1alpha1.MetricSpec is much smaller and can be updated as more functionality is required. I'm totally open to suggestions to change the type of otelcol.Spec.Autoscaler.Metrics though if that's still unclear!

nope, makes a ton of sense. thanks for explaining!

apis/v1alpha1/opentelemetrycollector_webhook.go

iblancasa · 2023-05-03T08:52:35Z

apis/v1alpha1/opentelemetrycollector_webhook.go

@@ -184,6 +185,12 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error {
 		}
 	}

+	if r.Spec.Ingress.Type == IngressTypeNginx && r.Spec.Mode == ModeSidecar {
+		return fmt.Errorf("the OptenTelemetry Spec Ingress configuiration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s",


Suggested change

return fmt.Errorf("the OptenTelemetry Spec Ingress configuiration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s",

return fmt.Errorf("the OptenTelemetry Spec Ingress configuration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s",

Thanks for the review! Fixed.

…-telemetry#1651) * add to api types and validate webhook * creation of metrics * chloggen and make generate * new MetricSpec * make bundle and api-docs * lint * add webhook test case * make bundle * updates * address review feedback * gofmt * add nil check and reorder validation * fix typo * lint

moh-osman3 marked this pull request as ready for review April 13, 2023 09:33

moh-osman3 requested a review from a team April 13, 2023 09:33

jaronoff97 reviewed Apr 13, 2023

View reviewed changes

jaronoff97 reviewed Apr 14, 2023

View reviewed changes

apis/v1alpha1/opentelemetrycollector_webhook.go Outdated Show resolved Hide resolved

jaronoff97 approved these changes Apr 18, 2023

View reviewed changes

iblancasa approved these changes May 3, 2023

View reviewed changes

moh-osman3 mentioned this pull request May 5, 2023

[HPA] Add support for Pod custom metrics #1561

Closed

moh-osman3 force-pushed the mohosman/issue-1560/HPA-pod-metrics branch from f5e0250 to 23109cd Compare May 17, 2023 18:54

moh-osman3 added 13 commits May 22, 2023 15:15

add to api types and validate webhook

f10cdad

creation of metrics

4d540b2

chloggen and make generate

79d1ca7

new MetricSpec

c75f488

make bundle and api-docs

09c903f

lint

a19b89f

add webhook test case

0ca95a9

make bundle

1e65294

updates

78649a3

address review feedback

b2bc5cb

gofmt

a61ca02

add nil check and reorder validation

54af3ac

fix typo

e178a4f

moh-osman3 force-pushed the mohosman/issue-1560/HPA-pod-metrics branch from 23109cd to e178a4f Compare May 22, 2023 19:15

lint

4b71f31

jaronoff97 approved these changes May 22, 2023

View reviewed changes

jaronoff97 merged commit 88022b4 into open-telemetry:main May 22, 2023

moh-osman3 mentioned this pull request Aug 15, 2023

REQUEST: New membership for moh-osman3 open-telemetry/community#1647

Closed

6 tasks

jaronoff97 mentioned this pull request Oct 24, 2023

Add metrics field to autoscaler #1203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HPA] Add v1alpha1.MetricSpec to support for Pod custom metrics #1651

[HPA] Add v1alpha1.MetricSpec to support for Pod custom metrics #1651

moh-osman3 commented Apr 13, 2023 •

edited

Loading

jaronoff97 Apr 13, 2023

moh-osman3 Apr 13, 2023

jaronoff97 Apr 14, 2023

moh-osman3 Apr 14, 2023

jaronoff97 Apr 13, 2023

jaronoff97 Apr 13, 2023

moh-osman3 Apr 13, 2023

moh-osman3 Apr 13, 2023

moh-osman3 Apr 13, 2023

moh-osman3 Apr 13, 2023

jaronoff97 Apr 14, 2023

iblancasa May 3, 2023

moh-osman3 May 3, 2023

	return fmt.Errorf("the OptenTelemetry Spec Ingress configuiration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s",
	return fmt.Errorf("the OptenTelemetry Spec Ingress configuration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s",

[HPA] Add v1alpha1.MetricSpec to support for Pod custom metrics #1651

[HPA] Add v1alpha1.MetricSpec to support for Pod custom metrics #1651

Conversation

moh-osman3 commented Apr 13, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moh-osman3 commented Apr 13, 2023 •

edited

Loading