-
Notifications
You must be signed in to change notification settings - Fork 484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add autoscale option to enable support for Horizontal Pod Autoscaling #746
Changes from 6 commits
449b51b
e9a09b2
2ba820c
0c3f204
5600157
93d8131
a477345
6fed4dc
03435ba
2505026
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,10 +33,22 @@ type OpenTelemetryCollectorSpec struct { | |
// +optional | ||
Args map[string]string `json:"args,omitempty"` | ||
|
||
// Autoscale turns on/off the autoscale feature. By default, it's enabled if the Replicas field is not set. | ||
// +optional | ||
Autoscale *bool `json:"autoscale,omitempty"` | ||
|
||
// Replicas is the number of pod instances for the underlying OpenTelemetry Collector | ||
// +optional | ||
Replicas *int32 `json:"replicas,omitempty"` | ||
|
||
// MinReplicas sets a lower bound to the autoscaling feature. | ||
// +optional | ||
MinReplicas *int32 `json:"minReplicas,omitempty"` | ||
|
||
// MaxReplicas sets an upper bound to the autoscaling feature. When autoscaling is enabled and no value is provided, a default value is used. | ||
// +optional | ||
MaxReplicas *int32 `json:"maxReplicas,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a proposal to make the config/CR simpler. Could we add only If the max replicas is set the operator would create HPA. In the validating webhook it would check if maxReplicas is higher than replicas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably we can do It (remove autoscale), https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/horizontal-pod-autoscaler-v1/#HorizontalPodAutoscalerSpec maxReplicas (int32), required |
||
|
||
// ImagePullPolicy indicates the pull policy to be used for retrieving the container image (Always, Never, IfNotPresent) | ||
// +optional | ||
ImagePullPolicy v1.PullPolicy `json:"imagePullPolicy,omitempty"` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,5 +109,20 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { | |
} | ||
} | ||
|
||
// validate autoscale with horizontal pod autoscaler | ||
if r.Spec.Autoscale != nil && *r.Spec.Autoscale { | ||
if r.Spec.Replicas != nil { | ||
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas should be nil") | ||
} | ||
|
||
if r.Spec.MaxReplicas == nil || *r.Spec.MaxReplicas < int32(1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, maaxReplicas should be defined and more than one") | ||
} | ||
|
||
if r.Spec.MinReplicas != nil && *r.Spec.MinReplicas > *r.Spec.MaxReplicas { | ||
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas") | ||
} | ||
} | ||
|
||
return nil | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package collector | ||
|
||
import ( | ||
"github.com/go-logr/logr" | ||
autoscalingv1 "k8s.io/api/autoscaling/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
||
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" | ||
"github.com/open-telemetry/opentelemetry-operator/internal/config" | ||
"github.com/open-telemetry/opentelemetry-operator/pkg/naming" | ||
) | ||
|
||
func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) autoscalingv1.HorizontalPodAutoscaler { | ||
labels := Labels(otelcol) | ||
labels["app.kubernetes.io/name"] = naming.Collector(otelcol) | ||
|
||
annotations := Annotations(otelcol) | ||
var cpuTarget int32 = 90 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The value of the cpuTarget is hardcoded here. I think we should take the input of TargetCPUUtilizationPercentage as a part of OpenTelemetryCollectorSpec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, that make sense to do, but I initially tried to be consistent with jaeger operator https://github.com/jaegertracing/jaeger-operator/pull/856/files where it also hardcoded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine for the first version. I would recommend adding a comment and extracting the value to a constant. |
||
|
||
return autoscalingv1.HorizontalPodAutoscaler{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: naming.Collector(otelcol), | ||
Namespace: otelcol.Namespace, | ||
Labels: labels, | ||
Annotations: annotations, | ||
}, | ||
Spec: autoscalingv1.HorizontalPodAutoscalerSpec{ | ||
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{ | ||
APIVersion: "apps/v1", | ||
Kind: "Deployment", | ||
Name: naming.Collector(otelcol), | ||
}, | ||
MinReplicas: otelcol.Spec.MinReplicas, | ||
MaxReplicas: *otelcol.Spec.MaxReplicas, | ||
TargetCPUUtilizationPercentage: &cpuTarget, | ||
}, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package collector_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
||
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" | ||
"github.com/open-telemetry/opentelemetry-operator/internal/config" | ||
. "github.com/open-telemetry/opentelemetry-operator/pkg/collector" | ||
) | ||
|
||
func TestHPA(t *testing.T) { | ||
// prepare | ||
enable := true | ||
var minReplicas int32 = 3 | ||
var maxReplicas int32 = 5 | ||
|
||
otelcol := v1alpha1.OpenTelemetryCollector{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "my-instance", | ||
}, | ||
Spec: v1alpha1.OpenTelemetryCollectorSpec{ | ||
MinReplicas: &minReplicas, | ||
MaxReplicas: &maxReplicas, | ||
Autoscale: &enable, | ||
}, | ||
} | ||
|
||
cfg := config.New() | ||
hpa := HorizontalPodAutoscaler(cfg, logger, otelcol) | ||
|
||
// verify | ||
assert.Equal(t, "my-instance-collector", hpa.Name) | ||
assert.Equal(t, "my-instance-collector", hpa.Labels["app.kubernetes.io/name"]) | ||
assert.Equal(t, int32(3), *hpa.Spec.MinReplicas) | ||
assert.Equal(t, int32(5), hpa.Spec.MaxReplicas) | ||
assert.Equal(t, int32(90), *hpa.Spec.TargetCPUUtilizationPercentage) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be set to true in the defaulting webhook if
replicas
is nil?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure, right now the default is one replica, if you don't specify replicas, I tried to not change default behaviour