-
Notifications
You must be signed in to change notification settings - Fork 452
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 9 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 |
---|---|---|
|
@@ -109,5 +109,16 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { | |
} | ||
} | ||
|
||
// validate autoscale with horizontal pod autoscaler | ||
if r.Spec.MaxReplicas != 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, maxReplicas should be defined and more than one") | ||
} | ||
|
||
if r.Spec.Replicas != nil && *r.Spec.Replicas > *r.Spec.MaxReplicas { | ||
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. while we are at this, could you please set the 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. not sure that I got this right, do you ask to set Spec.Replicas equal to 1 in Default() if r.Spec.Replicas == nil? Because I can see where we set Spec.Replicas outside of it, only in tests and here: // if autoscale is enabled, use replicas from current Status
if params.Instance.Spec.MaxReplicas != nil {
currentReplicas := existing.Status.Replicas
// if replicas (minReplicas from HPA perspective) is bigger than
// current status use it.
if *params.Instance.Spec.Replicas > currentReplicas {
currentReplicas = *params.Instance.Spec.Replicas
}
updated.Spec.Replicas = ¤tReplicas
} 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 replicas should be set to 1 in deployment if it is nil. It's fine I will take a look at it in another PR. |
||
return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas 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,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 | ||
|
||
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" | ||
) | ||
|
||
const defaultCPUTarget int32 = 90 | ||
|
||
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) | ||
cpuTarget := defaultCPUTarget | ||
|
||
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.Replicas, | ||
MaxReplicas: *otelcol.Spec.MaxReplicas, | ||
TargetCPUUtilizationPercentage: &cpuTarget, | ||
}, | ||
} | ||
} |
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_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 | ||
var minReplicas int32 = 3 | ||
var maxReplicas int32 = 5 | ||
|
||
otelcol := v1alpha1.OpenTelemetryCollector{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "my-instance", | ||
}, | ||
Spec: v1alpha1.OpenTelemetryCollectorSpec{ | ||
Replicas: &minReplicas, | ||
MaxReplicas: &maxReplicas, | ||
}, | ||
} | ||
|
||
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.
I have a proposal to make the config/CR simpler.
Could we add only
MaxReplicas
field in this PR to support autostacling?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 comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that
MaxReplicas
can be nil (e.g. for infinite scaling) in this case we needAutoscale
flag to enable the HPA.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.
Probably we can do It (remove autoscale), https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/horizontal-pod-autoscaler-v1/#HorizontalPodAutoscalerSpec
because maxReplicas is required:
maxReplicas (int32), required
upper limit for the number of pods that can be set by the autoscaler; cannot be smaller than MinReplicas.