From 29518d920b56bc223c01845b54f4f447566e83a2 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Mon, 1 Aug 2022 11:13:43 +0200 Subject: [PATCH 01/18] Update to v2beta2 Signed-off-by: Kevin Earls --- apis/v1alpha1/zz_generated.deepcopy.go | 56 +++++++++---------- .../opentelemetrycollector_controller.go | 4 +- pkg/collector/horizontalpodautoscaler.go | 32 ++++++++--- pkg/collector/horizontalpodautoscaler_test.go | 5 +- .../reconcile/horizontalpodautoscaler.go | 12 ++-- .../reconcile/horizontalpodautoscaler_test.go | 12 ++-- 6 files changed, 70 insertions(+), 51 deletions(-) diff --git a/apis/v1alpha1/zz_generated.deepcopy.go b/apis/v1alpha1/zz_generated.deepcopy.go index cb60f0ca6e..cba3b24b99 100644 --- a/apis/v1alpha1/zz_generated.deepcopy.go +++ b/apis/v1alpha1/zz_generated.deepcopy.go @@ -65,9 +65,9 @@ func (in *Exporter) DeepCopy() *Exporter { func (in *Instrumentation) DeepCopyInto(out *Instrumentation) { *out = *in out.Status = in.Status - in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - in.Spec.DeepCopyInto(&out.Spec) out.TypeMeta = in.TypeMeta + in.Spec.DeepCopyInto(&out.Spec) + in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Instrumentation. @@ -275,6 +275,14 @@ func (in *OpenTelemetryCollectorList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSpec) { *out = *in + in.Resources.DeepCopyInto(&out.Resources) + if in.NodeSelector != nil { + in, out := &in.NodeSelector, &out.NodeSelector + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.Args != nil { in, out := &in.Args, &out.Args *out = make(map[string]string, len(*in)) @@ -297,7 +305,6 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp *out = new(int32) **out = **in } - out.TargetAllocator = in.TargetAllocator if in.SecurityContext != nil { in, out := &in.SecurityContext, &out.SecurityContext *out = new(v1.SecurityContext) @@ -308,13 +315,14 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp *out = new(v1.PodSecurityContext) (*in).DeepCopyInto(*out) } - if in.VolumeClaimTemplates != nil { - in, out := &in.VolumeClaimTemplates, &out.VolumeClaimTemplates - *out = make([]v1.PersistentVolumeClaim, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) + if in.PodAnnotations != nil { + in, out := &in.PodAnnotations, &out.PodAnnotations + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val } } + out.TargetAllocator = in.TargetAllocator if in.VolumeMounts != nil { in, out := &in.VolumeMounts, &out.VolumeMounts *out = make([]v1.VolumeMount, len(*in)) @@ -322,13 +330,6 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.Volumes != nil { - in, out := &in.Volumes, &out.Volumes - *out = make([]v1.Volume, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } if in.Ports != nil { in, out := &in.Ports, &out.Ports *out = make([]v1.ServicePort, len(*in)) @@ -350,7 +351,13 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp (*in)[i].DeepCopyInto(&(*out)[i]) } } - in.Resources.DeepCopyInto(&out.Resources) + if in.VolumeClaimTemplates != nil { + in, out := &in.VolumeClaimTemplates, &out.VolumeClaimTemplates + *out = make([]v1.PersistentVolumeClaim, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.Tolerations != nil { in, out := &in.Tolerations, &out.Tolerations *out = make([]v1.Toleration, len(*in)) @@ -358,18 +365,11 @@ func (in *OpenTelemetryCollectorSpec) DeepCopyInto(out *OpenTelemetryCollectorSp (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.NodeSelector != nil { - in, out := &in.NodeSelector, &out.NodeSelector - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val - } - } - if in.PodAnnotations != nil { - in, out := &in.PodAnnotations, &out.PodAnnotations - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val + if in.Volumes != nil { + in, out := &in.Volumes, &out.Volumes + *out = make([]v1.Volume, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) } } } diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 7854514df5..b2f01fd0ce 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -21,7 +21,7 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" - autoscalingv1 "k8s.io/api/autoscaling/v1" + autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" @@ -179,7 +179,7 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er Owns(&corev1.ServiceAccount{}). Owns(&corev1.Service{}). Owns(&appsv1.Deployment{}). - Owns(&autoscalingv1.HorizontalPodAutoscaler{}). + Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}). Owns(&appsv1.DaemonSet{}). Owns(&appsv1.StatefulSet{}). Complete(r) diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 7c7003fb48..31110d3c69 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -16,7 +16,8 @@ package collector import ( "github.com/go-logr/logr" - autoscalingv1 "k8s.io/api/autoscaling/v1" + autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -26,30 +27,45 @@ import ( const defaultCPUTarget int32 = 90 -func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) autoscalingv1.HorizontalPodAutoscaler { +func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) autoscalingv2beta2.HorizontalPodAutoscaler { labels := Labels(otelcol, cfg.LabelsFilter()) labels["app.kubernetes.io/name"] = naming.Collector(otelcol) annotations := Annotations(otelcol) cpuTarget := defaultCPUTarget - return autoscalingv1.HorizontalPodAutoscaler{ + targetCPUUtilization := autoscalingv2beta2.MetricSpec{ + Type: autoscalingv2beta2.ResourceMetricSourceType, + Resource: &autoscalingv2beta2.ResourceMetricSource{ + Name: corev1.ResourceCPU, + Target: autoscalingv2beta2.MetricTarget{ + Type: autoscalingv2beta2.UtilizationMetricType, + AverageUtilization: &cpuTarget, + }, + }, + ContainerResource: nil, + External: nil, + } + + metrics := []autoscalingv2beta2.MetricSpec{targetCPUUtilization} + + return autoscalingv2beta2.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: naming.HorizontalPodAutoscaler(otelcol), Namespace: otelcol.Namespace, Labels: labels, Annotations: annotations, }, - Spec: autoscalingv1.HorizontalPodAutoscalerSpec{ - ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{ + Spec: autoscalingv2beta2.HorizontalPodAutoscalerSpec{ + ScaleTargetRef: autoscalingv2beta2.CrossVersionObjectReference{ APIVersion: v1alpha1.GroupVersion.String(), Kind: "OpenTelemetryCollector", Name: naming.OpenTelemetryCollector(otelcol), }, - MinReplicas: otelcol.Spec.Replicas, - MaxReplicas: *otelcol.Spec.MaxReplicas, - TargetCPUUtilizationPercentage: &cpuTarget, + MinReplicas: otelcol.Spec.Replicas, + MaxReplicas: *otelcol.Spec.MaxReplicas, + Metrics: metrics, }, } } diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index b168816549..fcddedd8a7 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -48,5 +49,7 @@ func TestHPA(t *testing.T) { 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) + assert.Equal(t, 1, len(hpa.Spec.Metrics)) + assert.Equal(t, corev1.ResourceCPU, hpa.Spec.Metrics[0].Resource.Name) + assert.Equal(t, int32(90), *hpa.Spec.Metrics[0].Resource.Target.AverageUtilization) } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index 606f4dffce..1f574aa0b1 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -18,7 +18,7 @@ import ( "context" "fmt" - autoscalingv1 "k8s.io/api/autoscaling/v1" + autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -31,7 +31,7 @@ import ( // HorizontalPodAutoscaler reconciles HorizontalPodAutoscalers if autoscale is true and replicas is nil. func HorizontalPodAutoscalers(ctx context.Context, params Params) error { - desired := []autoscalingv1.HorizontalPodAutoscaler{} + desired := []autoscalingv2beta2.HorizontalPodAutoscaler{} // check if autoscale mode is on, e.g MaxReplicas is not nil if params.Instance.Spec.MaxReplicas != nil { @@ -51,7 +51,7 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { return nil } -func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []autoscalingv1.HorizontalPodAutoscaler) error { +func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []autoscalingv2beta2.HorizontalPodAutoscaler) error { one := int32(1) for _, obj := range expected { desired := obj @@ -60,7 +60,7 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect return fmt.Errorf("failed to set controller reference: %w", err) } - existing := &autoscalingv1.HorizontalPodAutoscaler{} + existing := &autoscalingv2beta2.HorizontalPodAutoscaler{} nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} err := params.Client.Get(ctx, nns, existing) if k8serrors.IsNotFound(err) { @@ -110,7 +110,7 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect return nil } -func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []autoscalingv1.HorizontalPodAutoscaler) error { +func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []autoscalingv2beta2.HorizontalPodAutoscaler) error { opts := []client.ListOption{ client.InNamespace(params.Instance.Namespace), client.MatchingLabels(map[string]string{ @@ -119,7 +119,7 @@ func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected }), } - list := &autoscalingv1.HorizontalPodAutoscalerList{} + list := &autoscalingv2beta2.HorizontalPodAutoscalerList{} if err := params.Client.List(ctx, list, opts...); err != nil { return fmt.Errorf("failed to list: %w", err) } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 62b901da37..5c9cd5ae9a 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -22,7 +22,7 @@ import ( "github.com/stretchr/testify/assert" v1 "k8s.io/api/apps/v1" - autoscalingv1 "k8s.io/api/autoscaling/v1" + autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -39,10 +39,10 @@ func TestExpectedHPA(t *testing.T) { expectedHPA := collector.HorizontalPodAutoscaler(params.Config, logger, params.Instance) t.Run("should create HPA", func(t *testing.T) { - err := expectedHorizontalPodAutoscalers(context.Background(), params, []autoscalingv1.HorizontalPodAutoscaler{expectedHPA}) + err := expectedHorizontalPodAutoscalers(context.Background(), params, []autoscalingv2beta2.HorizontalPodAutoscaler{expectedHPA}) assert.NoError(t, err) - exists, err := populateObjectIfExists(t, &autoscalingv1.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) assert.NoError(t, err) assert.True(t, exists) }) @@ -56,10 +56,10 @@ func TestExpectedHPA(t *testing.T) { updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) createObjectIfNotExists(t, "test-collector", &updatedHPA) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []autoscalingv1.HorizontalPodAutoscaler{updatedHPA}) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []autoscalingv2beta2.HorizontalPodAutoscaler{updatedHPA}) assert.NoError(t, err) - actual := autoscalingv1.HorizontalPodAutoscaler{} + actual := autoscalingv2beta2.HorizontalPodAutoscaler{} exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) assert.NoError(t, err) @@ -69,7 +69,7 @@ func TestExpectedHPA(t *testing.T) { }) t.Run("should delete HPA", func(t *testing.T) { - err := deleteHorizontalPodAutoscalers(context.Background(), params, []autoscalingv1.HorizontalPodAutoscaler{expectedHPA}) + err := deleteHorizontalPodAutoscalers(context.Background(), params, []autoscalingv2beta2.HorizontalPodAutoscaler{expectedHPA}) assert.NoError(t, err) actual := v1.Deployment{} From 1c4a35b2406e5b2cb5cb6cbd09862b15c267b4f6 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Mon, 1 Aug 2022 17:47:53 +0200 Subject: [PATCH 02/18] First attempt at supporting v2 and v2beta2 of autoscaling Signed-off-by: Kevin Earls --- .../opentelemetrycollector_controller.go | 19 +- .../opentelemetrycollector_controller_test.go | 30 ++- internal/config/main.go | 19 ++ internal/config/main_test.go | 4 + internal/config/options.go | 1 + pkg/autodetect/main.go | 22 ++ pkg/collector/horizontalpodautoscaler.go | 103 ++++++--- pkg/collector/horizontalpodautoscaler_test.go | 84 ++++++- .../reconcile/horizontalpodautoscaler.go | 218 +++++++++++++----- .../reconcile/horizontalpodautoscaler_test.go | 112 ++++++--- pkg/collector/reconcile/suite_test.go | 3 +- 11 files changed, 479 insertions(+), 136 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index b2f01fd0ce..ab539f8a55 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -21,6 +21,7 @@ import ( "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" + autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -173,7 +174,12 @@ func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params // SetupWithManager tells the manager what our controller is interested in. func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). + err := r.config.AutoDetect() + if err != nil { + return err + } + autoscalingVersion := r.config.AutoscalingVersion() + builder := ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.OpenTelemetryCollector{}). Owns(&corev1.ConfigMap{}). Owns(&corev1.ServiceAccount{}). @@ -181,6 +187,13 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er Owns(&appsv1.Deployment{}). Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}). Owns(&appsv1.DaemonSet{}). - Owns(&appsv1.StatefulSet{}). - Complete(r) + Owns(&appsv1.StatefulSet{}) + + if autoscalingVersion == config.AutoscalingVersionV2 { + builder = builder.Owns(&autoscalingv2.HorizontalPodAutoscaler{}) + } else { + builder = builder.Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}) + } + + return builder.Complete(r) } diff --git a/controllers/opentelemetrycollector_controller_test.go b/controllers/opentelemetrycollector_controller_test.go index 981449f7e5..9593e5f2ab 100644 --- a/controllers/opentelemetrycollector_controller_test.go +++ b/controllers/opentelemetrycollector_controller_test.go @@ -36,14 +36,22 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/controllers" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" "github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile" + "github.com/open-telemetry/opentelemetry-operator/pkg/platform" ) var logger = logf.Log.WithName("unit-tests") +var mockAutoDetector = &mockAutoDetect{ + HPAVersionFunc: func() (string, error) { + return config.AutoscalingVersionV2Beta2, nil // FIXME how do we test both? + }, +} func TestNewObjectsOnReconciliation(t *testing.T) { // prepare - cfg := config.New(config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator")) + + cfg := config.New(config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator"), config.WithAutoDetect(mockAutoDetector)) nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} reconciler := controllers.NewReconciler(controllers.Params{ Client: k8sClient, @@ -129,7 +137,7 @@ func TestNewObjectsOnReconciliation(t *testing.T) { func TestNewStatefulSetObjectsOnReconciliation(t *testing.T) { // prepare - cfg := config.New() + cfg := config.New(config.WithAutoDetect(mockAutoDetector)) nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} reconciler := controllers.NewReconciler(controllers.Params{ Client: k8sClient, @@ -341,3 +349,21 @@ func TestRegisterWithManager(t *testing.T) { // verify assert.NoError(t, err) } + +var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) + +type mockAutoDetect struct { + PlatformFunc func() (platform.Platform, error) + HPAVersionFunc func() (string, error) +} + +func (m *mockAutoDetect) HPAVersion() (string, error) { + return m.HPAVersionFunc() +} + +func (m *mockAutoDetect) Platform() (platform.Platform, error) { + if m.PlatformFunc != nil { + return m.PlatformFunc() + } + return platform.Unknown, nil +} diff --git a/internal/config/main.go b/internal/config/main.go index f20af554bb..f742e2bba1 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -30,6 +30,9 @@ const ( defaultAutoDetectFrequency = 5 * time.Second defaultCollectorConfigMapEntry = "collector.yaml" defaultTargetAllocatorConfigMapEntry = "targetallocator.yaml" + AutoscalingVersionV2 = "v2" + AutoscalingVersionV2Beta2 = "V2Beta2" + DefaultAutoscalingVersion = "fred" //FIXME change this back to AutoscalingVersionV2...or should it be v2beta2??? ) // Config holds the static configuration for this operator. @@ -45,6 +48,7 @@ type Config struct { targetAllocatorConfigMapEntry string autoInstrumentationNodeJSImage string autoInstrumentationJavaImage string + autoscalingVersion string onChange []func() error labelsFilter []string platform platform.Platform @@ -61,6 +65,7 @@ func New(opts ...Option) Config { logger: logf.Log.WithName("config"), platform: platform.Unknown, version: version.Get(), + autoscalingVersion: DefaultAutoscalingVersion, } for _, opt := range opts { opt(&o) @@ -81,6 +86,7 @@ func New(opts ...Option) Config { autoInstrumentationPythonImage: o.autoInstrumentationPythonImage, autoInstrumentationDotNetImage: o.autoInstrumentationDotNetImage, labelsFilter: o.labelsFilter, + autoscalingVersion: o.autoscalingVersion, } } @@ -132,6 +138,14 @@ func (c *Config) AutoDetect() error { } } + hpaVersion, err := c.autoDetect.HPAVersion() + if err != nil { + return err + } else { + c.autoscalingVersion = hpaVersion + c.logger.Info(">>>>>> In Autodetect, Set HPA version to [", c.autoscalingVersion, "] from [", hpaVersion, "]") // TODO set level + } + return nil } @@ -160,6 +174,11 @@ func (c *Config) Platform() platform.Platform { return c.platform } +// AutoscalingVersion represents the preferred version of autoscaling. +func (c *Config) AutoscalingVersion() string { + return c.autoscalingVersion +} + // AutoInstrumentationJavaImage returns OpenTelemetry Java auto-instrumentation container image. func (c *Config) AutoInstrumentationJavaImage() string { return c.autoInstrumentationJavaImage diff --git a/internal/config/main_test.go b/internal/config/main_test.go index ba981ab2dd..ceec3aa36e 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -102,6 +102,10 @@ type mockAutoDetect struct { PlatformFunc func() (platform.Platform, error) } +func (m *mockAutoDetect) HPAVersion() (string, error) { + return config.DefaultAutoscalingVersion, nil // TODO add a test? +} + func (m *mockAutoDetect) Platform() (platform.Platform, error) { if m.PlatformFunc != nil { return m.PlatformFunc() diff --git a/internal/config/options.go b/internal/config/options.go index beef2b4b83..9bb30f94d6 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -41,6 +41,7 @@ type options struct { collectorConfigMapEntry string targetAllocatorConfigMapEntry string targetAllocatorImage string + autoscalingVersion string onChange []func() error labelsFilter []string platform platform.Platform diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index 487d5b4a29..859d41b40b 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -27,6 +27,7 @@ var _ AutoDetect = (*autoDetect)(nil) // AutoDetect provides an assortment of routines that auto-detect traits based on the runtime. type AutoDetect interface { Platform() (platform.Platform, error) + HPAVersion() (string, error) } type autoDetect struct { @@ -64,3 +65,24 @@ func (a *autoDetect) Platform() (platform.Platform, error) { return platform.Kubernetes, nil } + +func (a *autoDetect) HPAVersion() (string, error) { + apiList, err := a.dcl.ServerGroups() + if err != nil { + return "", err + } + + for _, apiGroup := range apiList.Groups { + if apiGroup.Name == "autoscaling" { + for _, version := range apiGroup.Versions { + if version.Version == "v2" { // FIXME use constants here + return version.Version, nil + } + } + return "v2beta2", nil + } + } + + // FIXME we should never get here....what to do? Return v2beta2? + return "", nil +} diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 31110d3c69..3d868bc5d9 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -16,9 +16,11 @@ package collector import ( "github.com/go-logr/logr" + autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" @@ -27,45 +29,88 @@ import ( const defaultCPUTarget int32 = 90 -func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) autoscalingv2beta2.HorizontalPodAutoscaler { +func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) runtime.Object { + err := cfg.AutoDetect() + if err != nil { + logger.Error(err, "cfg.Autodetect failed") + } + autoscalingVersion := cfg.AutoscalingVersion() + logger.Info(">>>>>>>>>> In HorizontalPodAutoscaler, autoscaling version is", autoscalingVersion) // TODO update, change level, etc... + labels := Labels(otelcol, cfg.LabelsFilter()) labels["app.kubernetes.io/name"] = naming.Collector(otelcol) annotations := Annotations(otelcol) cpuTarget := defaultCPUTarget + var o runtime.Object - targetCPUUtilization := autoscalingv2beta2.MetricSpec{ - Type: autoscalingv2beta2.ResourceMetricSourceType, - Resource: &autoscalingv2beta2.ResourceMetricSource{ - Name: corev1.ResourceCPU, - Target: autoscalingv2beta2.MetricTarget{ - Type: autoscalingv2beta2.UtilizationMetricType, - AverageUtilization: &cpuTarget, - }, - }, - ContainerResource: nil, - External: nil, + objectMeta := metav1.ObjectMeta{ + Name: naming.HorizontalPodAutoscaler(otelcol), + Namespace: otelcol.Namespace, + Labels: labels, + Annotations: annotations, } - metrics := []autoscalingv2beta2.MetricSpec{targetCPUUtilization} + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + targetCPUUtilization := autoscalingv2beta2.MetricSpec{ + Type: autoscalingv2beta2.ResourceMetricSourceType, + Resource: &autoscalingv2beta2.ResourceMetricSource{ + Name: corev1.ResourceCPU, + Target: autoscalingv2beta2.MetricTarget{ + Type: autoscalingv2beta2.UtilizationMetricType, + AverageUtilization: &cpuTarget, + }, + }, + ContainerResource: nil, + External: nil, + } + metrics := []autoscalingv2beta2.MetricSpec{targetCPUUtilization} - return autoscalingv2beta2.HorizontalPodAutoscaler{ - ObjectMeta: metav1.ObjectMeta{ - Name: naming.HorizontalPodAutoscaler(otelcol), - Namespace: otelcol.Namespace, - Labels: labels, - Annotations: annotations, - }, - Spec: autoscalingv2beta2.HorizontalPodAutoscalerSpec{ - ScaleTargetRef: autoscalingv2beta2.CrossVersionObjectReference{ - APIVersion: v1alpha1.GroupVersion.String(), - Kind: "OpenTelemetryCollector", - Name: naming.OpenTelemetryCollector(otelcol), + autoscaler := autoscalingv2beta2.HorizontalPodAutoscaler{ + ObjectMeta: objectMeta, + Spec: autoscalingv2beta2.HorizontalPodAutoscalerSpec{ + ScaleTargetRef: autoscalingv2beta2.CrossVersionObjectReference{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "OpenTelemetryCollector", + Name: naming.OpenTelemetryCollector(otelcol), + }, + MinReplicas: otelcol.Spec.Replicas, + MaxReplicas: *otelcol.Spec.MaxReplicas, + Metrics: metrics, }, + } + o = &autoscaler + //return o + } else { + targetCPUUtilization := autoscalingv2.MetricSpec{ + Type: autoscalingv2.ResourceMetricSourceType, + Resource: &autoscalingv2.ResourceMetricSource{ + Name: corev1.ResourceCPU, + Target: autoscalingv2.MetricTarget{ + Type: autoscalingv2.UtilizationMetricType, + AverageUtilization: &cpuTarget, + }, + }, + ContainerResource: nil, + External: nil, + } + metrics := []autoscalingv2.MetricSpec{targetCPUUtilization} - MinReplicas: otelcol.Spec.Replicas, - MaxReplicas: *otelcol.Spec.MaxReplicas, - Metrics: metrics, - }, + autoscaler := autoscalingv2.HorizontalPodAutoscaler{ + ObjectMeta: objectMeta, + Spec: autoscalingv2.HorizontalPodAutoscalerSpec{ + ScaleTargetRef: autoscalingv2.CrossVersionObjectReference{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: "OpenTelemetryCollector", + Name: naming.OpenTelemetryCollector(otelcol), + }, + MinReplicas: otelcol.Spec.Replicas, + MaxReplicas: *otelcol.Spec.MaxReplicas, + Metrics: metrics, + }, + } + o = &autoscaler } + + return o } diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index fcddedd8a7..d4cda637bc 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -15,19 +15,31 @@ package collector_test import ( + "fmt" "testing" "github.com/stretchr/testify/assert" + autoscalingv2 "k8s.io/api/autoscaling/v2" + autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/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/autodetect" . "github.com/open-telemetry/opentelemetry-operator/pkg/collector" + "github.com/open-telemetry/opentelemetry-operator/pkg/platform" ) func TestHPA(t *testing.T) { - // prepare + type test struct { + name string + autoscalingVersion string + } + v2Test := test{"V2", "v2"} + v2beta2Test := test{"V2Beta2", config.AutoscalingVersionV2Beta2} + tests := []test{v2Test, v2beta2Test} + var minReplicas int32 = 3 var maxReplicas int32 = 5 @@ -41,15 +53,63 @@ func TestHPA(t *testing.T) { }, } - 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, 1, len(hpa.Spec.Metrics)) - assert.Equal(t, corev1.ResourceCPU, hpa.Spec.Metrics[0].Resource.Name) - assert.Equal(t, int32(90), *hpa.Spec.Metrics[0].Resource.Target.AverageUtilization) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + mockAutoDetector := &mockAutoDetect{ + HPAVersionFunc: func() (string, error) { + return test.autoscalingVersion, nil + }, + } + configuration := config.New(config.WithAutoDetect(mockAutoDetector)) + raw := HorizontalPodAutoscaler(configuration, logger, otelcol) + err := configuration.AutoDetect() + if err != nil { + t.Errorf("configuration.autodetect failed", err) + } + + logger.Info("Running ", t.Name(), "with autoscaling version", configuration.AutoscalingVersion()) // FIXME this print nothing + fmt.Printf("In test %s using autoscaling version %s\n", t.Name(), configuration.AutoscalingVersion()) + if configuration.AutoscalingVersion() == config.AutoscalingVersionV2Beta2 { + hpa := raw.(*autoscalingv2beta2.HorizontalPodAutoscaler) + + // 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, 1, len(hpa.Spec.Metrics)) + assert.Equal(t, corev1.ResourceCPU, hpa.Spec.Metrics[0].Resource.Name) + assert.Equal(t, int32(90), *hpa.Spec.Metrics[0].Resource.Target.AverageUtilization) + } else { + hpa := raw.(*autoscalingv2.HorizontalPodAutoscaler) + + // 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, 1, len(hpa.Spec.Metrics)) + assert.Equal(t, corev1.ResourceCPU, hpa.Spec.Metrics[0].Resource.Name) + assert.Equal(t, int32(90), *hpa.Spec.Metrics[0].Resource.Target.AverageUtilization) + } + }) + } +} + +var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) + +type mockAutoDetect struct { + PlatformFunc func() (platform.Platform, error) + HPAVersionFunc func() (string, error) +} + +func (m *mockAutoDetect) HPAVersion() (string, error) { + return m.HPAVersionFunc() +} + +func (m *mockAutoDetect) Platform() (platform.Platform, error) { + if m.PlatformFunc != nil { + return m.PlatformFunc() + } + return platform.Unknown, nil } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index 1f574aa0b1..403988c367 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -18,12 +18,15 @@ import ( "context" "fmt" + autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/pkg/collector" ) @@ -31,7 +34,7 @@ import ( // HorizontalPodAutoscaler reconciles HorizontalPodAutoscalers if autoscale is true and replicas is nil. func HorizontalPodAutoscalers(ctx context.Context, params Params) error { - desired := []autoscalingv2beta2.HorizontalPodAutoscaler{} + desired := []runtime.Object{} // check if autoscale mode is on, e.g MaxReplicas is not nil if params.Instance.Spec.MaxReplicas != nil { @@ -51,66 +54,134 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { return nil } -func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []autoscalingv2beta2.HorizontalPodAutoscaler) error { +func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { + err := params.Config.AutoDetect() // WTF + if err != nil { + params.Log.Error(err, "cfg.Autodetect failed") + } + autoscalingVersion := params.Config.AutoscalingVersion() + params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers, autoscaling version is", autoscalingVersion) // TODO update, change level, etc... one := int32(1) + for _, obj := range expected { - desired := obj + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + desired := *obj.(*autoscalingv2beta2.HorizontalPodAutoscaler) - if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } + if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference: %w", err) + } - existing := &autoscalingv2beta2.HorizontalPodAutoscaler{} - nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) - } - params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - continue - } else if err != nil { - return fmt.Errorf("failed to get %w", err) - } + existing := &autoscalingv2beta2.HorizontalPodAutoscaler{} + nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} + err := params.Client.Get(ctx, nns, existing) + if k8serrors.IsNotFound(err) { + if err := params.Client.Create(ctx, &desired); err != nil { + return fmt.Errorf("failed to create: %w", err) + } + params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + continue + } else if err != nil { + return fmt.Errorf("failed to get %w", err) + } - updated := existing.DeepCopy() - if updated.Annotations == nil { - updated.Annotations = map[string]string{} - } - if updated.Labels == nil { - updated.Labels = map[string]string{} - } + updated := existing.DeepCopy() + if updated.Annotations == nil { + updated.Annotations = map[string]string{} + } + if updated.Labels == nil { + updated.Labels = map[string]string{} + } - updated.OwnerReferences = desired.OwnerReferences - if params.Instance.Spec.MaxReplicas != nil { - updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas - } else { - updated.Spec.MinReplicas = &one + updated.OwnerReferences = desired.OwnerReferences + if params.Instance.Spec.MaxReplicas != nil { + updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas + if params.Instance.Spec.MinReplicas != nil { + updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas + } else { + updated.Spec.MinReplicas = &one + } } - } - for k, v := range desired.Annotations { - updated.Annotations[k] = v - } - for k, v := range desired.Labels { - updated.Labels[k] = v - } + for k, v := range desired.Annotations { + updated.Annotations[k] = v + } + for k, v := range desired.Labels { + updated.Labels[k] = v + } - patch := client.MergeFrom(existing) + patch := client.MergeFrom(existing) - if err := params.Client.Patch(ctx, updated, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) - } + if err := params.Client.Patch(ctx, updated, patch); err != nil { + return fmt.Errorf("failed to apply changes: %w", err) + } + + params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + } else { // FIXME is there a better way to do this than a giant copy/paste? + desired := *obj.(*autoscalingv2.HorizontalPodAutoscaler) + + if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference: %w", err) + } + + existing := &autoscalingv2.HorizontalPodAutoscaler{} + nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} + err := params.Client.Get(ctx, nns, existing) + if k8serrors.IsNotFound(err) { + if err := params.Client.Create(ctx, &desired); err != nil { + return fmt.Errorf("failed to create: %w", err) + } + params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + continue + } else if err != nil { + return fmt.Errorf("failed to get %w", err) + } - params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + updated := existing.DeepCopy() + if updated.Annotations == nil { + updated.Annotations = map[string]string{} + } + if updated.Labels == nil { + updated.Labels = map[string]string{} + } + + updated.OwnerReferences = desired.OwnerReferences + if params.Instance.Spec.MaxReplicas != nil { + updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas + if params.Instance.Spec.MinReplicas != nil { + updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas + } else { + updated.Spec.MinReplicas = &one + } + } + + for k, v := range desired.Annotations { + updated.Annotations[k] = v + } + for k, v := range desired.Labels { + updated.Labels[k] = v + } + + patch := client.MergeFrom(existing) + + if err := params.Client.Patch(ctx, updated, patch); err != nil { + return fmt.Errorf("failed to apply changes: %w", err) + } + + params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + } } return nil } -func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []autoscalingv2beta2.HorizontalPodAutoscaler) error { +func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { + err := params.Config.AutoDetect() + if err != nil { + params.Log.Error(err, "cfg.Autodetect failed") + } + autoscalingVersion := params.Config.AutoscalingVersion() + params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers, autoscaling version is", autoscalingVersion) // TODO update, change level, etc... + opts := []client.ListOption{ client.InNamespace(params.Instance.Namespace), client.MatchingLabels(map[string]string{ @@ -119,26 +190,53 @@ func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected }), } - list := &autoscalingv2beta2.HorizontalPodAutoscalerList{} - if err := params.Client.List(ctx, list, opts...); err != nil { - return fmt.Errorf("failed to list: %w", err) - } + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + list := &autoscalingv2beta2.HorizontalPodAutoscalerList{} + if err := params.Client.List(ctx, list, opts...); err != nil { + return fmt.Errorf("failed to list: %w", err) + } - for i := range list.Items { - existing := list.Items[i] - del := true - for _, keep := range expected { - if keep.Name == existing.Name && keep.Namespace == existing.Namespace { - del = false - break + for i := range list.Items { + existing := list.Items[i] + del := true + for _, k := range expected { + keep := k.(*autoscalingv2beta2.HorizontalPodAutoscaler) + if keep.Name == existing.Name && keep.Namespace == existing.Namespace { + del = false + break + } } + + if del { + if err := params.Client.Delete(ctx, &existing); err != nil { + return fmt.Errorf("failed to delete: %w", err) + } + params.Log.V(2).Info("deleted", "hpa.name", existing.Name, "hpa.namespace", existing.Namespace) + } + } + } else { + list := &autoscalingv2.HorizontalPodAutoscalerList{} + if err := params.Client.List(ctx, list, opts...); err != nil { + return fmt.Errorf("failed to list: %w", err) } - if del { - if err := params.Client.Delete(ctx, &existing); err != nil { - return fmt.Errorf("failed to delete: %w", err) + for i := range list.Items { + existing := list.Items[i] + del := true + for _, k := range expected { + keep := k.(*autoscalingv2.HorizontalPodAutoscaler) + if keep.Name == existing.Name && keep.Namespace == existing.Namespace { + del = false + break + } + } + + if del { + if err := params.Client.Delete(ctx, &existing); err != nil { + return fmt.Errorf("failed to delete: %w", err) + } + params.Log.V(2).Info("deleted", "hpa.name", existing.Name, "hpa.namespace", existing.Namespace) } - params.Log.V(2).Info("deleted", "hpa.name", existing.Name, "hpa.namespace", existing.Namespace) } } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 5c9cd5ae9a..411d88c311 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -22,63 +22,89 @@ import ( "github.com/stretchr/testify/assert" v1 "k8s.io/api/apps/v1" + autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/tools/record" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" "github.com/open-telemetry/opentelemetry-operator/pkg/collector" + "github.com/open-telemetry/opentelemetry-operator/pkg/platform" ) func TestExpectedHPA(t *testing.T) { - params := paramsWithHPA() - expectedHPA := collector.HorizontalPodAutoscaler(params.Config, logger, params.Instance) + params := paramsWithHPA(config.AutoscalingVersionV2Beta2) + err := params.Config.AutoDetect() + if err != nil { + params.Log.Error(err, "params.Config.Autodetect failed") + } + autoscalingVersion := params.Config.AutoscalingVersion() - t.Run("should create HPA", func(t *testing.T) { - err := expectedHorizontalPodAutoscalers(context.Background(), params, []autoscalingv2beta2.HorizontalPodAutoscaler{expectedHPA}) + expectedHPA := collector.HorizontalPodAutoscaler(params.Config, logger, params.Instance) + //t.Run("should create HPA", func(t *testing.T) { + err = expectedHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) + assert.NoError(t, err) + + exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + assert.NoError(t, err) + assert.True(t, exists) + //}) + + //t.Run("should update HPA", func(t *testing.T) { + minReplicas := int32(1) + maxReplicas := int32(3) + updateParms := paramsWithHPA(config.AutoscalingVersionV2Beta2) + updateParms.Instance.Spec.Replicas = &minReplicas + updateParms.Instance.Spec.MaxReplicas = &maxReplicas + updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) + + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) + createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) assert.NoError(t, err) - exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + actual := autoscalingv2beta2.HorizontalPodAutoscaler{} + exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + assert.NoError(t, err) assert.True(t, exists) - }) - - t.Run("should update HPA", func(t *testing.T) { - minReplicas := int32(1) - maxReplicas := int32(3) - updateParms := paramsWithHPA() - updateParms.Instance.Spec.Replicas = &minReplicas - updateParms.Instance.Spec.MaxReplicas = &maxReplicas - updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) - - createObjectIfNotExists(t, "test-collector", &updatedHPA) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []autoscalingv2beta2.HorizontalPodAutoscaler{updatedHPA}) + assert.Equal(t, int32(1), *actual.Spec.MinReplicas) + assert.Equal(t, int32(3), actual.Spec.MaxReplicas) + } else { + updatedAutoscaler := *updatedHPA.(*autoscalingv2.HorizontalPodAutoscaler) + createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) assert.NoError(t, err) - actual := autoscalingv2beta2.HorizontalPodAutoscaler{} + actual := autoscalingv2.HorizontalPodAutoscaler{} exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) assert.NoError(t, err) assert.True(t, exists) assert.Equal(t, int32(1), *actual.Spec.MinReplicas) assert.Equal(t, int32(3), actual.Spec.MaxReplicas) - }) + } - t.Run("should delete HPA", func(t *testing.T) { - err := deleteHorizontalPodAutoscalers(context.Background(), params, []autoscalingv2beta2.HorizontalPodAutoscaler{expectedHPA}) - assert.NoError(t, err) + //}) + + //t.Run("should delete HPA", func(t *testing.T) { + err = deleteHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) + assert.NoError(t, err) - actual := v1.Deployment{} - exists, _ := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collecto"}) - assert.False(t, exists) - }) + actual := v1.Deployment{} + exists, _ = populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collecto"}) + assert.False(t, exists) + //}) } -func paramsWithHPA() Params { +func paramsWithHPA(autoscalingVersion string) Params { configYAML, err := ioutil.ReadFile("../testdata/test.yaml") if err != nil { fmt.Printf("Error getting yaml file: %v", err) @@ -87,8 +113,19 @@ func paramsWithHPA() Params { minReplicas := int32(3) maxReplicas := int32(5) + mockAutoDetector := &mockAutoDetect{ + HPAVersionFunc: func() (string, error) { + return autoscalingVersion, nil + }, + } + configuration := config.New(config.WithAutoDetect(mockAutoDetector), config.WithCollectorImage(defaultCollectorImage), config.WithTargetAllocatorImage(defaultTaAllocationImage)) + err = configuration.AutoDetect() + if err != nil { + logger.Error(err, "configuration.autodetect failed") + } + return Params{ - Config: config.New(config.WithCollectorImage(defaultCollectorImage), config.WithTargetAllocatorImage(defaultTaAllocationImage)), + Config: configuration, Client: k8sClient, Instance: v1alpha1.OpenTelemetryCollector{ TypeMeta: metav1.TypeMeta{ @@ -120,3 +157,22 @@ func paramsWithHPA() Params { Recorder: record.NewFakeRecorder(10), } } + +// FIXME we need some way to make this global. +var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) + +type mockAutoDetect struct { + PlatformFunc func() (platform.Platform, error) + HPAVersionFunc func() (string, error) +} + +func (m *mockAutoDetect) HPAVersion() (string, error) { + return m.HPAVersionFunc() +} + +func (m *mockAutoDetect) Platform() (platform.Platform, error) { + if m.PlatformFunc != nil { + return m.PlatformFunc() + } + return platform.Unknown, nil +} diff --git a/pkg/collector/reconcile/suite_test.go b/pkg/collector/reconcile/suite_test.go index 6d8e011cbc..0ce59bc928 100644 --- a/pkg/collector/reconcile/suite_test.go +++ b/pkg/collector/reconcile/suite_test.go @@ -259,8 +259,7 @@ func createObjectIfNotExists(tb testing.TB, name string, object client.Object) { tb.Helper() err := k8sClient.Get(context.Background(), client.ObjectKey{Namespace: "default", Name: name}, object) if errors.IsNotFound(err) { - err := k8sClient.Create(context.Background(), - object) + err := k8sClient.Create(context.Background(), object) assert.NoError(tb, err) } } From e8ff6b5c498640f00ae87e53df1df9c55500998b Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Mon, 1 Aug 2022 18:00:38 +0200 Subject: [PATCH 03/18] Fix ErrorF statement Signed-off-by: Kevin Earls --- pkg/collector/horizontalpodautoscaler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index d4cda637bc..ae863defc3 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -64,7 +64,7 @@ func TestHPA(t *testing.T) { raw := HorizontalPodAutoscaler(configuration, logger, otelcol) err := configuration.AutoDetect() if err != nil { - t.Errorf("configuration.autodetect failed", err) + t.Errorf("configuration.autodetect failed %v", err) } logger.Info("Running ", t.Name(), "with autoscaling version", configuration.AutoscalingVersion()) // FIXME this print nothing From eb9ec445df6848bf577612230b3b4b58263f8aff Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Mon, 1 Aug 2022 19:42:27 +0200 Subject: [PATCH 04/18] Adding whitespace to rerun CI Signed-off-by: Kevin Earls --- pkg/collector/horizontalpodautoscaler_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index ae863defc3..932967b7f3 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -94,6 +94,7 @@ func TestHPA(t *testing.T) { } }) } + } var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) From 693a0994740b573ee3f06c706a21e36ecad52430 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Tue, 2 Aug 2022 09:44:38 +0200 Subject: [PATCH 05/18] Make the stupid linter happy Signed-off-by: Kevin Earls --- pkg/collector/horizontalpodautoscaler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index 932967b7f3..298b92ede5 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -94,7 +94,7 @@ func TestHPA(t *testing.T) { } }) } - + } var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) From ed148e6bbbf08829f30693833e29d617f460bee5 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 3 Aug 2022 14:47:18 +0200 Subject: [PATCH 06/18] Just run on 1.24 to get full test results Signed-off-by: Kevin Earls --- .github/workflows/e2e.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index ac32a4cb08..6cb5e1f81f 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -16,7 +16,7 @@ jobs: # All Kubernetes version in between expose the same APIs, hence the operator # should be compatible with them. kube-version: - - "1.19" + #- "1.19" - "1.24" steps: From 0be053d7186ed389a58f68c2f569570cd57e6723 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 3 Aug 2022 15:15:12 +0200 Subject: [PATCH 07/18] Just run on 1.19 to get full test results Signed-off-by: Kevin Earls --- .github/workflows/e2e.yaml | 4 ++-- .github/workflows/scorecard.yaml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 6cb5e1f81f..c55c638ed5 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -16,8 +16,8 @@ jobs: # All Kubernetes version in between expose the same APIs, hence the operator # should be compatible with them. kube-version: - #- "1.19" - - "1.24" + - "1.19" + #- "1.24" steps: diff --git a/.github/workflows/scorecard.yaml b/.github/workflows/scorecard.yaml index 23f4adab44..1095b22ee1 100644 --- a/.github/workflows/scorecard.yaml +++ b/.github/workflows/scorecard.yaml @@ -14,7 +14,7 @@ jobs: matrix: kube-version: - "1.19" - - "1.24" + #- "1.24" steps: From 04d8f881c665b8f832a2780273cce58d8b2781f0 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 3 Aug 2022 17:09:28 +0200 Subject: [PATCH 08/18] Cleanup, remove extraneous autodetect() calls, run only on 1.24 Signed-off-by: Kevin Earls --- .github/workflows/e2e.yaml | 4 ++-- .github/workflows/scorecard.yaml | 4 ++-- internal/config/main.go | 2 +- pkg/collector/horizontalpodautoscaler.go | 6 +----- pkg/collector/horizontalpodautoscaler_test.go | 1 + pkg/collector/reconcile/horizontalpodautoscaler.go | 12 ++---------- 6 files changed, 9 insertions(+), 20 deletions(-) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index c55c638ed5..6cb5e1f81f 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -16,8 +16,8 @@ jobs: # All Kubernetes version in between expose the same APIs, hence the operator # should be compatible with them. kube-version: - - "1.19" - #- "1.24" + #- "1.19" + - "1.24" steps: diff --git a/.github/workflows/scorecard.yaml b/.github/workflows/scorecard.yaml index 1095b22ee1..452613b04f 100644 --- a/.github/workflows/scorecard.yaml +++ b/.github/workflows/scorecard.yaml @@ -13,8 +13,8 @@ jobs: strategy: matrix: kube-version: - - "1.19" - #- "1.24" + #- "1.19" + - "1.24" steps: diff --git a/internal/config/main.go b/internal/config/main.go index f742e2bba1..19d9da9e7e 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -31,7 +31,7 @@ const ( defaultCollectorConfigMapEntry = "collector.yaml" defaultTargetAllocatorConfigMapEntry = "targetallocator.yaml" AutoscalingVersionV2 = "v2" - AutoscalingVersionV2Beta2 = "V2Beta2" + AutoscalingVersionV2Beta2 = "v2beta2" DefaultAutoscalingVersion = "fred" //FIXME change this back to AutoscalingVersionV2...or should it be v2beta2??? ) diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 3d868bc5d9..404d4765ab 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -30,12 +30,8 @@ import ( const defaultCPUTarget int32 = 90 func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) runtime.Object { - err := cfg.AutoDetect() - if err != nil { - logger.Error(err, "cfg.Autodetect failed") - } autoscalingVersion := cfg.AutoscalingVersion() - logger.Info(">>>>>>>>>> In HorizontalPodAutoscaler, autoscaling version is", autoscalingVersion) // TODO update, change level, etc... + logger.Info(">>>>>>>>>> In HorizontalPodAutoscaler", "autoscaling version is", autoscalingVersion) // TODO update, change level, etc... labels := Labels(otelcol, cfg.LabelsFilter()) labels["app.kubernetes.io/name"] = naming.Collector(otelcol) diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index 298b92ede5..aff0088da6 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -61,6 +61,7 @@ func TestHPA(t *testing.T) { }, } configuration := config.New(config.WithAutoDetect(mockAutoDetector)) + configuration.AutoDetect() raw := HorizontalPodAutoscaler(configuration, logger, otelcol) err := configuration.AutoDetect() if err != nil { diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index 403988c367..8a55661b38 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -55,12 +55,8 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { } func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { - err := params.Config.AutoDetect() // WTF - if err != nil { - params.Log.Error(err, "cfg.Autodetect failed") - } autoscalingVersion := params.Config.AutoscalingVersion() - params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers, autoscaling version is", autoscalingVersion) // TODO update, change level, etc... + params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers", "autoscaling version is", autoscalingVersion) // TODO update, change level, etc... one := int32(1) for _, obj := range expected { @@ -175,12 +171,8 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect } func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { - err := params.Config.AutoDetect() - if err != nil { - params.Log.Error(err, "cfg.Autodetect failed") - } autoscalingVersion := params.Config.AutoscalingVersion() - params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers, autoscaling version is", autoscalingVersion) // TODO update, change level, etc... + params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers", "autoscaling version is", autoscalingVersion) // TODO update, change level, etc... opts := []client.ListOption{ client.InNamespace(params.Instance.Namespace), From 3abfaccd9cd4aa5a52769be5dcaba1f9f62b912e Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Thu, 4 Aug 2022 11:06:13 +0200 Subject: [PATCH 09/18] Remove redundant builder.Owns call Signed-off-by: Kevin Earls --- .github/workflows/e2e.yaml | 2 +- .github/workflows/scorecard.yaml | 2 +- controllers/opentelemetrycollector_controller.go | 3 +-- pkg/collector/horizontalpodautoscaler_test.go | 3 +-- pkg/collector/reconcile/horizontalpodautoscaler_test.go | 2 +- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 6cb5e1f81f..ac32a4cb08 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -16,7 +16,7 @@ jobs: # All Kubernetes version in between expose the same APIs, hence the operator # should be compatible with them. kube-version: - #- "1.19" + - "1.19" - "1.24" steps: diff --git a/.github/workflows/scorecard.yaml b/.github/workflows/scorecard.yaml index 452613b04f..23f4adab44 100644 --- a/.github/workflows/scorecard.yaml +++ b/.github/workflows/scorecard.yaml @@ -13,7 +13,7 @@ jobs: strategy: matrix: kube-version: - #- "1.19" + - "1.19" - "1.24" steps: diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index ab539f8a55..0019fa90ae 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -174,7 +174,7 @@ func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params // SetupWithManager tells the manager what our controller is interested in. func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) error { - err := r.config.AutoDetect() + err := r.config.AutoDetect() // TODO explain if err != nil { return err } @@ -185,7 +185,6 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er Owns(&corev1.ServiceAccount{}). Owns(&corev1.Service{}). Owns(&appsv1.Deployment{}). - Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}). Owns(&appsv1.DaemonSet{}). Owns(&appsv1.StatefulSet{}) diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index aff0088da6..bb0e713c54 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -61,12 +61,11 @@ func TestHPA(t *testing.T) { }, } configuration := config.New(config.WithAutoDetect(mockAutoDetector)) - configuration.AutoDetect() - raw := HorizontalPodAutoscaler(configuration, logger, otelcol) err := configuration.AutoDetect() if err != nil { t.Errorf("configuration.autodetect failed %v", err) } + raw := HorizontalPodAutoscaler(configuration, logger, otelcol) logger.Info("Running ", t.Name(), "with autoscaling version", configuration.AutoscalingVersion()) // FIXME this print nothing fmt.Printf("In test %s using autoscaling version %s\n", t.Name(), configuration.AutoscalingVersion()) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 411d88c311..f945fffe50 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -40,7 +40,7 @@ import ( func TestExpectedHPA(t *testing.T) { params := paramsWithHPA(config.AutoscalingVersionV2Beta2) - err := params.Config.AutoDetect() + err := params.Config.AutoDetect() // Do I need to do this? if err != nil { params.Log.Error(err, "params.Config.Autodetect failed") } From 26414c9cecd8e47bc21c91ebdad9a63031df17e5 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Thu, 4 Aug 2022 12:12:42 +0200 Subject: [PATCH 10/18] Start of cleanup Signed-off-by: Kevin Earls --- .../opentelemetrycollector_controller_test.go | 2 +- internal/config/main.go | 2 +- internal/config/main_test.go | 2 +- pkg/autodetect/main.go | 6 +++--- pkg/collector/horizontalpodautoscaler.go | 13 ++++--------- pkg/collector/horizontalpodautoscaler_test.go | 12 +++--------- .../reconcile/horizontalpodautoscaler_test.go | 6 ++---- 7 files changed, 15 insertions(+), 28 deletions(-) diff --git a/controllers/opentelemetrycollector_controller_test.go b/controllers/opentelemetrycollector_controller_test.go index 9593e5f2ab..b4476fd32b 100644 --- a/controllers/opentelemetrycollector_controller_test.go +++ b/controllers/opentelemetrycollector_controller_test.go @@ -44,7 +44,7 @@ import ( var logger = logf.Log.WithName("unit-tests") var mockAutoDetector = &mockAutoDetect{ HPAVersionFunc: func() (string, error) { - return config.AutoscalingVersionV2Beta2, nil // FIXME how do we test both? + return config.AutoscalingVersionV2Beta2, nil // TODO do we need to test both? }, } diff --git a/internal/config/main.go b/internal/config/main.go index 19d9da9e7e..5a13cef07d 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -32,7 +32,7 @@ const ( defaultTargetAllocatorConfigMapEntry = "targetallocator.yaml" AutoscalingVersionV2 = "v2" AutoscalingVersionV2Beta2 = "v2beta2" - DefaultAutoscalingVersion = "fred" //FIXME change this back to AutoscalingVersionV2...or should it be v2beta2??? + DefaultAutoscalingVersion = AutoscalingVersionV2 ) // Config holds the static configuration for this operator. diff --git a/internal/config/main_test.go b/internal/config/main_test.go index ceec3aa36e..62f4e8d1a7 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -98,7 +98,7 @@ func TestAutoDetectInBackground(t *testing.T) { var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) -type mockAutoDetect struct { +type mockAutoDetect struct { // FIXME is there some way to only define this once? PlatformFunc func() (platform.Platform, error) } diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index 859d41b40b..fa07878b73 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -16,6 +16,7 @@ package autodetect import ( + "errors" "k8s.io/client-go/discovery" "k8s.io/client-go/rest" @@ -75,7 +76,7 @@ func (a *autoDetect) HPAVersion() (string, error) { for _, apiGroup := range apiList.Groups { if apiGroup.Name == "autoscaling" { for _, version := range apiGroup.Versions { - if version.Version == "v2" { // FIXME use constants here + if version.Version == "v2" { // We can't use the constants from internal/config/main.go as that would create an import cycle return version.Version, nil } } @@ -83,6 +84,5 @@ func (a *autoDetect) HPAVersion() (string, error) { } } - // FIXME we should never get here....what to do? Return v2beta2? - return "", nil + return "", errors.New("Failed to find apiGroup autoscaling") } diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 404d4765ab..06abd0b177 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -38,7 +38,7 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al annotations := Annotations(otelcol) cpuTarget := defaultCPUTarget - var o runtime.Object + var result runtime.Object objectMeta := metav1.ObjectMeta{ Name: naming.HorizontalPodAutoscaler(otelcol), @@ -57,8 +57,6 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al AverageUtilization: &cpuTarget, }, }, - ContainerResource: nil, - External: nil, } metrics := []autoscalingv2beta2.MetricSpec{targetCPUUtilization} @@ -75,8 +73,7 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al Metrics: metrics, }, } - o = &autoscaler - //return o + result = &autoscaler } else { targetCPUUtilization := autoscalingv2.MetricSpec{ Type: autoscalingv2.ResourceMetricSourceType, @@ -87,8 +84,6 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al AverageUtilization: &cpuTarget, }, }, - ContainerResource: nil, - External: nil, } metrics := []autoscalingv2.MetricSpec{targetCPUUtilization} @@ -105,8 +100,8 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al Metrics: metrics, }, } - o = &autoscaler + result = &autoscaler } - return o + return result } diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index bb0e713c54..9d65b4dce7 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -15,7 +15,6 @@ package collector_test import ( - "fmt" "testing" "github.com/stretchr/testify/assert" @@ -36,8 +35,8 @@ func TestHPA(t *testing.T) { name string autoscalingVersion string } - v2Test := test{"V2", "v2"} - v2beta2Test := test{"V2Beta2", config.AutoscalingVersionV2Beta2} + v2Test := test{config.AutoscalingVersionV2, config.AutoscalingVersionV2} + v2beta2Test := test{config.AutoscalingVersionV2Beta2, config.AutoscalingVersionV2Beta2} tests := []test{v2Test, v2beta2Test} var minReplicas int32 = 3 @@ -62,13 +61,9 @@ func TestHPA(t *testing.T) { } configuration := config.New(config.WithAutoDetect(mockAutoDetector)) err := configuration.AutoDetect() - if err != nil { - t.Errorf("configuration.autodetect failed %v", err) - } + assert.NoError(t, err) raw := HorizontalPodAutoscaler(configuration, logger, otelcol) - logger.Info("Running ", t.Name(), "with autoscaling version", configuration.AutoscalingVersion()) // FIXME this print nothing - fmt.Printf("In test %s using autoscaling version %s\n", t.Name(), configuration.AutoscalingVersion()) if configuration.AutoscalingVersion() == config.AutoscalingVersionV2Beta2 { hpa := raw.(*autoscalingv2beta2.HorizontalPodAutoscaler) @@ -94,7 +89,6 @@ func TestHPA(t *testing.T) { } }) } - } var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index f945fffe50..c2849301bf 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -40,10 +40,8 @@ import ( func TestExpectedHPA(t *testing.T) { params := paramsWithHPA(config.AutoscalingVersionV2Beta2) - err := params.Config.AutoDetect() // Do I need to do this? - if err != nil { - params.Log.Error(err, "params.Config.Autodetect failed") - } + err := params.Config.AutoDetect() + assert.NoError(t, err) autoscalingVersion := params.Config.AutoscalingVersion() expectedHPA := collector.HorizontalPodAutoscaler(params.Config, logger, params.Instance) From b9f8af164d5da42166ce635042149d927192302f Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Thu, 4 Aug 2022 12:48:44 +0200 Subject: [PATCH 11/18] Appease the stupid linter Signed-off-by: Kevin Earls --- pkg/autodetect/main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index fa07878b73..630dcb419a 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -17,6 +17,7 @@ package autodetect import ( "errors" + "k8s.io/client-go/discovery" "k8s.io/client-go/rest" From df711da141652002f683c9b90b07b1634fb5475b Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Thu, 4 Aug 2022 14:17:44 +0200 Subject: [PATCH 12/18] Cleanup Signed-off-by: Kevin Earls --- .../opentelemetrycollector_controller.go | 2 +- .../opentelemetrycollector_controller_test.go | 3 +- internal/config/main.go | 2 +- internal/config/main_test.go | 2 +- pkg/autodetect/main.go | 2 +- pkg/collector/horizontalpodautoscaler.go | 1 - .../reconcile/horizontalpodautoscaler.go | 2 - .../reconcile/horizontalpodautoscaler_test.go | 99 +++++++++---------- 8 files changed, 54 insertions(+), 59 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 0019fa90ae..6a59749eab 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -174,7 +174,7 @@ func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params // SetupWithManager tells the manager what our controller is interested in. func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) error { - err := r.config.AutoDetect() // TODO explain + err := r.config.AutoDetect() // We need to call this so we can get the correct autodetect version if err != nil { return err } diff --git a/controllers/opentelemetrycollector_controller_test.go b/controllers/opentelemetrycollector_controller_test.go index b4476fd32b..829f89c672 100644 --- a/controllers/opentelemetrycollector_controller_test.go +++ b/controllers/opentelemetrycollector_controller_test.go @@ -44,13 +44,12 @@ import ( var logger = logf.Log.WithName("unit-tests") var mockAutoDetector = &mockAutoDetect{ HPAVersionFunc: func() (string, error) { - return config.AutoscalingVersionV2Beta2, nil // TODO do we need to test both? + return config.AutoscalingVersionV2Beta2, nil }, } func TestNewObjectsOnReconciliation(t *testing.T) { // prepare - cfg := config.New(config.WithCollectorImage("default-collector"), config.WithTargetAllocatorImage("default-ta-allocator"), config.WithAutoDetect(mockAutoDetector)) nsn := types.NamespacedName{Name: "my-instance", Namespace: "default"} reconciler := controllers.NewReconciler(controllers.Params{ diff --git a/internal/config/main.go b/internal/config/main.go index 5a13cef07d..9b67587637 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -143,7 +143,7 @@ func (c *Config) AutoDetect() error { return err } else { c.autoscalingVersion = hpaVersion - c.logger.Info(">>>>>> In Autodetect, Set HPA version to [", c.autoscalingVersion, "] from [", hpaVersion, "]") // TODO set level + c.logger.Info("In Autodetect, Set HPA version to [", c.autoscalingVersion, "] from [", hpaVersion, "]") } return nil diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 62f4e8d1a7..ceec3aa36e 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -98,7 +98,7 @@ func TestAutoDetectInBackground(t *testing.T) { var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) -type mockAutoDetect struct { // FIXME is there some way to only define this once? +type mockAutoDetect struct { PlatformFunc func() (platform.Platform, error) } diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index 630dcb419a..6ded38e79a 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -77,7 +77,7 @@ func (a *autoDetect) HPAVersion() (string, error) { for _, apiGroup := range apiList.Groups { if apiGroup.Name == "autoscaling" { for _, version := range apiGroup.Versions { - if version.Version == "v2" { // We can't use the constants from internal/config/main.go as that would create an import cycle + if version.Version == "v2" { // Can't use the constants from internal/config/main.go as that would create an import cycle return version.Version, nil } } diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 06abd0b177..6f4199face 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -31,7 +31,6 @@ const defaultCPUTarget int32 = 90 func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) runtime.Object { autoscalingVersion := cfg.AutoscalingVersion() - logger.Info(">>>>>>>>>> In HorizontalPodAutoscaler", "autoscaling version is", autoscalingVersion) // TODO update, change level, etc... labels := Labels(otelcol, cfg.LabelsFilter()) labels["app.kubernetes.io/name"] = naming.Collector(otelcol) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index 8a55661b38..ff67d74038 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -56,7 +56,6 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() - params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers", "autoscaling version is", autoscalingVersion) // TODO update, change level, etc... one := int32(1) for _, obj := range expected { @@ -172,7 +171,6 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() - params.Log.Info(">>>>>>>>>> In expectedHorizontalPodAutoscalers", "autoscaling version is", autoscalingVersion) // TODO update, change level, etc... opts := []client.ListOption{ client.InNamespace(params.Instance.Namespace), diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index c2849301bf..ef95983af3 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -45,61 +45,61 @@ func TestExpectedHPA(t *testing.T) { autoscalingVersion := params.Config.AutoscalingVersion() expectedHPA := collector.HorizontalPodAutoscaler(params.Config, logger, params.Instance) - //t.Run("should create HPA", func(t *testing.T) { - err = expectedHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) - assert.NoError(t, err) - - exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) - assert.NoError(t, err) - assert.True(t, exists) - //}) - - //t.Run("should update HPA", func(t *testing.T) { - minReplicas := int32(1) - maxReplicas := int32(3) - updateParms := paramsWithHPA(config.AutoscalingVersionV2Beta2) - updateParms.Instance.Spec.Replicas = &minReplicas - updateParms.Instance.Spec.MaxReplicas = &maxReplicas - updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) - - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { - updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) - createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) + t.Run("should create HPA", func(t *testing.T) { + err = expectedHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) assert.NoError(t, err) - actual := autoscalingv2beta2.HorizontalPodAutoscaler{} - exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) - + exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) assert.NoError(t, err) assert.True(t, exists) - assert.Equal(t, int32(1), *actual.Spec.MinReplicas) - assert.Equal(t, int32(3), actual.Spec.MaxReplicas) - } else { - updatedAutoscaler := *updatedHPA.(*autoscalingv2.HorizontalPodAutoscaler) - createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) - assert.NoError(t, err) - - actual := autoscalingv2.HorizontalPodAutoscaler{} - exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) - + }) + + t.Run("should update HPA", func(t *testing.T) { + minReplicas := int32(1) + maxReplicas := int32(3) + updateParms := paramsWithHPA(config.AutoscalingVersionV2Beta2) + updateParms.Instance.Spec.Replicas = &minReplicas + updateParms.Instance.Spec.MaxReplicas = &maxReplicas + updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) + + t.Log("Eat me") + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) + createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) + assert.NoError(t, err) + + actual := autoscalingv2beta2.HorizontalPodAutoscaler{} + exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + + assert.NoError(t, err) + assert.True(t, exists) + assert.Equal(t, int32(1), *actual.Spec.MinReplicas) + assert.Equal(t, int32(3), actual.Spec.MaxReplicas) + } else { + updatedAutoscaler := *updatedHPA.(*autoscalingv2.HorizontalPodAutoscaler) + createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) + assert.NoError(t, err) + + actual := autoscalingv2.HorizontalPodAutoscaler{} + exists, err := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collector"}) + + assert.NoError(t, err) + assert.True(t, exists) + assert.Equal(t, int32(1), *actual.Spec.MinReplicas) + assert.Equal(t, int32(3), actual.Spec.MaxReplicas) + } + }) + + t.Run("should delete HPA", func(t *testing.T) { + err = deleteHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) assert.NoError(t, err) - assert.True(t, exists) - assert.Equal(t, int32(1), *actual.Spec.MinReplicas) - assert.Equal(t, int32(3), actual.Spec.MaxReplicas) - } - - //}) - - //t.Run("should delete HPA", func(t *testing.T) { - err = deleteHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) - assert.NoError(t, err) - actual := v1.Deployment{} - exists, _ = populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collecto"}) - assert.False(t, exists) - //}) + actual := v1.Deployment{} + exists, _ := populateObjectIfExists(t, &actual, types.NamespacedName{Namespace: "default", Name: "test-collecto"}) + assert.False(t, exists) + }) } func paramsWithHPA(autoscalingVersion string) Params { @@ -156,7 +156,6 @@ func paramsWithHPA(autoscalingVersion string) Params { } } -// FIXME we need some way to make this global. var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { From ceeaf37067fa5e6db6236bb2232410803d713aa6 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 10 Aug 2022 11:02:55 +0200 Subject: [PATCH 13/18] Check before returning v2beta2, reduce copy/pasted code Signed-off-by: Kevin Earls --- pkg/autodetect/main.go | 13 +- .../reconcile/horizontalpodautoscaler.go | 156 +++++++----------- 2 files changed, 71 insertions(+), 98 deletions(-) diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index 6ded38e79a..524e5ec6ba 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -17,6 +17,7 @@ package autodetect import ( "errors" + "sort" "k8s.io/client-go/discovery" "k8s.io/client-go/rest" @@ -76,12 +77,18 @@ func (a *autoDetect) HPAVersion() (string, error) { for _, apiGroup := range apiList.Groups { if apiGroup.Name == "autoscaling" { - for _, version := range apiGroup.Versions { - if version.Version == "v2" { // Can't use the constants from internal/config/main.go as that would create an import cycle + // Sort this so we can make sure to get v2 before v2beta2 + versions := apiGroup.Versions + sort.Slice(versions, func(i, j int) bool { + return versions[i].Version < versions[j].Version + }) + + for _, version := range versions { + if version.Version == "v2" || version.Version == "v2beta2" { // Can't use the constants from internal/config/main.go as that would create an import cycle return version.Version, nil } } - return "v2beta2", nil + return "", errors.New("Failed to find appropriate version of apiGroup autoscaling") } } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index ff67d74038..f8ce755c1d 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -21,11 +21,13 @@ import ( autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/pkg/collector" ) @@ -56,117 +58,81 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() - one := int32(1) + var existing client.Object + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + existing = &autoscalingv2beta2.HorizontalPodAutoscaler{} + } else { + existing = &autoscalingv2.HorizontalPodAutoscaler{} + } for _, obj := range expected { - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { - desired := *obj.(*autoscalingv2beta2.HorizontalPodAutoscaler) - - if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } + desired, _ := meta.Accessor(obj) - existing := &autoscalingv2beta2.HorizontalPodAutoscaler{} - nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) - } - params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - continue - } else if err != nil { - return fmt.Errorf("failed to get %w", err) - } - - updated := existing.DeepCopy() - if updated.Annotations == nil { - updated.Annotations = map[string]string{} - } - if updated.Labels == nil { - updated.Labels = map[string]string{} - } - - updated.OwnerReferences = desired.OwnerReferences - if params.Instance.Spec.MaxReplicas != nil { - updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas - } else { - updated.Spec.MinReplicas = &one - } - } + if err := controllerutil.SetControllerReference(¶ms.Instance, desired, params.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference: %w", err) + } - for k, v := range desired.Annotations { - updated.Annotations[k] = v - } - for k, v := range desired.Labels { - updated.Labels[k] = v + nns := types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()} + err := params.Client.Get(ctx, nns, existing) + if k8serrors.IsNotFound(err) { + if err := params.Client.Create(ctx, obj.(client.Object)); err != nil { + return fmt.Errorf("failed to create: %w", err) } + params.Log.V(2).Info("created", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) + continue + } else if err != nil { + return fmt.Errorf("failed to get %w", err) + } - patch := client.MergeFrom(existing) - - if err := params.Client.Patch(ctx, updated, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) - } + existing.SetOwnerReferences(desired.GetOwnerReferences()) + setAutoscalerSpec(autoscalingVersion, params.Instance, existing) - params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - } else { // FIXME is there a better way to do this than a giant copy/paste? - desired := *obj.(*autoscalingv2.HorizontalPodAutoscaler) + annotations := existing.GetAnnotations() + for k, v := range desired.GetAnnotations() { + annotations[k] = v + } + existing.SetAnnotations(annotations) + labels := existing.GetLabels() + for k, v := range desired.GetLabels() { + labels[k] = v + } + existing.SetLabels(labels) - if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } + patch := client.MergeFrom(existing) - existing := &autoscalingv2.HorizontalPodAutoscaler{} - nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) - } - params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - continue - } else if err != nil { - return fmt.Errorf("failed to get %w", err) - } + if err := params.Client.Patch(ctx, existing, patch); err != nil { + return fmt.Errorf("failed to apply changes: %w", err) + } - updated := existing.DeepCopy() - if updated.Annotations == nil { - updated.Annotations = map[string]string{} - } - if updated.Labels == nil { - updated.Labels = map[string]string{} - } + params.Log.V(2).Info("applied", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) + } - updated.OwnerReferences = desired.OwnerReferences - if params.Instance.Spec.MaxReplicas != nil { - updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas - } else { - updated.Spec.MinReplicas = &one - } - } + return nil +} - for k, v := range desired.Annotations { - updated.Annotations[k] = v - } - for k, v := range desired.Labels { - updated.Labels[k] = v +func setAutoscalerSpec(autoscalingVersion string, instance v1alpha1.OpenTelemetryCollector, autoscaler runtime.Object) { + one := int32(1) + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + existing := autoscaler.(*autoscalingv2beta2.HorizontalPodAutoscaler) + if instance.Spec.MaxReplicas != nil { + existing.Spec.MaxReplicas = *instance.Spec.MaxReplicas + if instance.Spec.MinReplicas != nil { + existing.Spec.MinReplicas = instance.Spec.MinReplicas + } else { + existing.Spec.MinReplicas = &one } - - patch := client.MergeFrom(existing) - - if err := params.Client.Patch(ctx, updated, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) + } + } else { + existing := autoscaler.(*autoscalingv2.HorizontalPodAutoscaler) + if instance.Spec.MaxReplicas != nil { + existing.Spec.MaxReplicas = *instance.Spec.MaxReplicas + if instance.Spec.MinReplicas != nil { + existing.Spec.MinReplicas = instance.Spec.MinReplicas + } else { + existing.Spec.MinReplicas = &one } - - params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) } } - - return nil } func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { From bed58fc39378912090317c191813f56d38f3f031 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 10 Aug 2022 15:17:34 +0200 Subject: [PATCH 14/18] Back out DRY changes Signed-off-by: Kevin Earls --- .../reconcile/horizontalpodautoscaler.go | 156 +++++++++++------- .../reconcile/horizontalpodautoscaler_test.go | 1 - 2 files changed, 95 insertions(+), 62 deletions(-) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index f8ce755c1d..ff67d74038 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -21,13 +21,11 @@ import ( autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" k8serrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/pkg/collector" ) @@ -58,81 +56,117 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() - var existing client.Object - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { - existing = &autoscalingv2beta2.HorizontalPodAutoscaler{} - } else { - existing = &autoscalingv2.HorizontalPodAutoscaler{} - } + one := int32(1) for _, obj := range expected { - desired, _ := meta.Accessor(obj) + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + desired := *obj.(*autoscalingv2beta2.HorizontalPodAutoscaler) - if err := controllerutil.SetControllerReference(¶ms.Instance, desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } + if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference: %w", err) + } - nns := types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()} - err := params.Client.Get(ctx, nns, existing) - if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, obj.(client.Object)); err != nil { - return fmt.Errorf("failed to create: %w", err) + existing := &autoscalingv2beta2.HorizontalPodAutoscaler{} + nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} + err := params.Client.Get(ctx, nns, existing) + if k8serrors.IsNotFound(err) { + if err := params.Client.Create(ctx, &desired); err != nil { + return fmt.Errorf("failed to create: %w", err) + } + params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + continue + } else if err != nil { + return fmt.Errorf("failed to get %w", err) } - params.Log.V(2).Info("created", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) - continue - } else if err != nil { - return fmt.Errorf("failed to get %w", err) - } - existing.SetOwnerReferences(desired.GetOwnerReferences()) - setAutoscalerSpec(autoscalingVersion, params.Instance, existing) + updated := existing.DeepCopy() + if updated.Annotations == nil { + updated.Annotations = map[string]string{} + } + if updated.Labels == nil { + updated.Labels = map[string]string{} + } - annotations := existing.GetAnnotations() - for k, v := range desired.GetAnnotations() { - annotations[k] = v - } - existing.SetAnnotations(annotations) - labels := existing.GetLabels() - for k, v := range desired.GetLabels() { - labels[k] = v - } - existing.SetLabels(labels) + updated.OwnerReferences = desired.OwnerReferences + if params.Instance.Spec.MaxReplicas != nil { + updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas + if params.Instance.Spec.MinReplicas != nil { + updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas + } else { + updated.Spec.MinReplicas = &one + } + } - patch := client.MergeFrom(existing) + for k, v := range desired.Annotations { + updated.Annotations[k] = v + } + for k, v := range desired.Labels { + updated.Labels[k] = v + } - if err := params.Client.Patch(ctx, existing, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) - } + patch := client.MergeFrom(existing) - params.Log.V(2).Info("applied", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) - } + if err := params.Client.Patch(ctx, updated, patch); err != nil { + return fmt.Errorf("failed to apply changes: %w", err) + } - return nil -} + params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + } else { // FIXME is there a better way to do this than a giant copy/paste? + desired := *obj.(*autoscalingv2.HorizontalPodAutoscaler) -func setAutoscalerSpec(autoscalingVersion string, instance v1alpha1.OpenTelemetryCollector, autoscaler runtime.Object) { - one := int32(1) - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { - existing := autoscaler.(*autoscalingv2beta2.HorizontalPodAutoscaler) - if instance.Spec.MaxReplicas != nil { - existing.Spec.MaxReplicas = *instance.Spec.MaxReplicas - if instance.Spec.MinReplicas != nil { - existing.Spec.MinReplicas = instance.Spec.MinReplicas - } else { - existing.Spec.MinReplicas = &one + if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference: %w", err) } - } - } else { - existing := autoscaler.(*autoscalingv2.HorizontalPodAutoscaler) - if instance.Spec.MaxReplicas != nil { - existing.Spec.MaxReplicas = *instance.Spec.MaxReplicas - if instance.Spec.MinReplicas != nil { - existing.Spec.MinReplicas = instance.Spec.MinReplicas - } else { - existing.Spec.MinReplicas = &one + + existing := &autoscalingv2.HorizontalPodAutoscaler{} + nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} + err := params.Client.Get(ctx, nns, existing) + if k8serrors.IsNotFound(err) { + if err := params.Client.Create(ctx, &desired); err != nil { + return fmt.Errorf("failed to create: %w", err) + } + params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) + continue + } else if err != nil { + return fmt.Errorf("failed to get %w", err) } + + updated := existing.DeepCopy() + if updated.Annotations == nil { + updated.Annotations = map[string]string{} + } + if updated.Labels == nil { + updated.Labels = map[string]string{} + } + + updated.OwnerReferences = desired.OwnerReferences + if params.Instance.Spec.MaxReplicas != nil { + updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas + if params.Instance.Spec.MinReplicas != nil { + updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas + } else { + updated.Spec.MinReplicas = &one + } + } + + for k, v := range desired.Annotations { + updated.Annotations[k] = v + } + for k, v := range desired.Labels { + updated.Labels[k] = v + } + + patch := client.MergeFrom(existing) + + if err := params.Client.Patch(ctx, updated, patch); err != nil { + return fmt.Errorf("failed to apply changes: %w", err) + } + + params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) } } + + return nil } func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index ef95983af3..98f5fca467 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -62,7 +62,6 @@ func TestExpectedHPA(t *testing.T) { updateParms.Instance.Spec.MaxReplicas = &maxReplicas updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) - t.Log("Eat me") if autoscalingVersion == config.AutoscalingVersionV2Beta2 { updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) From e69354936934cc50566823090be1ded6c6c0da72 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Thu, 11 Aug 2022 10:59:28 +0200 Subject: [PATCH 15/18] reduc copy/pasted code Signed-off-by: Kevin Earls --- .../reconcile/horizontalpodautoscaler.go | 154 +++++++----------- 1 file changed, 58 insertions(+), 96 deletions(-) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index ff67d74038..8f3deeacac 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -21,6 +21,7 @@ import ( autoscalingv2 "k8s.io/api/autoscaling/v2" autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -56,117 +57,78 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() - one := int32(1) + var existing client.Object + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + existing = &autoscalingv2beta2.HorizontalPodAutoscaler{} + } else { + existing = &autoscalingv2.HorizontalPodAutoscaler{} + } for _, obj := range expected { - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { - desired := *obj.(*autoscalingv2beta2.HorizontalPodAutoscaler) - - if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } - - existing := &autoscalingv2beta2.HorizontalPodAutoscaler{} - nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) - } - params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - continue - } else if err != nil { - return fmt.Errorf("failed to get %w", err) - } - - updated := existing.DeepCopy() - if updated.Annotations == nil { - updated.Annotations = map[string]string{} - } - if updated.Labels == nil { - updated.Labels = map[string]string{} - } - - updated.OwnerReferences = desired.OwnerReferences - if params.Instance.Spec.MaxReplicas != nil { - updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas - } else { - updated.Spec.MinReplicas = &one - } - } + desired, _ := meta.Accessor(obj) - for k, v := range desired.Annotations { - updated.Annotations[k] = v - } - for k, v := range desired.Labels { - updated.Labels[k] = v - } - - patch := client.MergeFrom(existing) + if err := controllerutil.SetControllerReference(¶ms.Instance, desired, params.Scheme); err != nil { + return fmt.Errorf("failed to set controller reference: %w", err) + } - if err := params.Client.Patch(ctx, updated, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) + nns := types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()} + err := params.Client.Get(ctx, nns, existing) + if k8serrors.IsNotFound(err) { + if err := params.Client.Create(ctx, obj.(client.Object)); err != nil { + return fmt.Errorf("failed to create: %w", err) } + params.Log.V(2).Info("created", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) + continue + } else if err != nil { + return fmt.Errorf("failed to get %w", err) + } - params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - } else { // FIXME is there a better way to do this than a giant copy/paste? - desired := *obj.(*autoscalingv2.HorizontalPodAutoscaler) + updated := existing.DeepCopyObject().(client.Object) + updated.SetOwnerReferences(desired.GetOwnerReferences()) + setAutoscalerSpec(params, autoscalingVersion, updated) - if err := controllerutil.SetControllerReference(¶ms.Instance, &desired, params.Scheme); err != nil { - return fmt.Errorf("failed to set controller reference: %w", err) - } + annotations := updated.GetAnnotations() + for k, v := range desired.GetAnnotations() { + annotations[k] = v + } + updated.SetAnnotations(annotations) + labels := updated.GetLabels() + for k, v := range desired.GetLabels() { + labels[k] = v + } + updated.SetLabels(labels) - existing := &autoscalingv2.HorizontalPodAutoscaler{} - nns := types.NamespacedName{Namespace: desired.Namespace, Name: desired.Name} - err := params.Client.Get(ctx, nns, existing) - if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, &desired); err != nil { - return fmt.Errorf("failed to create: %w", err) - } - params.Log.V(2).Info("created", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) - continue - } else if err != nil { - return fmt.Errorf("failed to get %w", err) - } + patch := client.MergeFrom(existing) - updated := existing.DeepCopy() - if updated.Annotations == nil { - updated.Annotations = map[string]string{} - } - if updated.Labels == nil { - updated.Labels = map[string]string{} - } - - updated.OwnerReferences = desired.OwnerReferences - if params.Instance.Spec.MaxReplicas != nil { - updated.Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas - if params.Instance.Spec.MinReplicas != nil { - updated.Spec.MinReplicas = params.Instance.Spec.MinReplicas - } else { - updated.Spec.MinReplicas = &one - } - } + if err := params.Client.Patch(ctx, updated, patch); err != nil { + return fmt.Errorf("failed to apply changes: %w", err) + } - for k, v := range desired.Annotations { - updated.Annotations[k] = v - } - for k, v := range desired.Labels { - updated.Labels[k] = v - } + params.Log.V(2).Info("applied", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) + } - patch := client.MergeFrom(existing) + return nil +} - if err := params.Client.Patch(ctx, updated, patch); err != nil { - return fmt.Errorf("failed to apply changes: %w", err) +func setAutoscalerSpec(params Params, autoscalingVersion string, updated client.Object) { + one := int32(1) + if params.Instance.Spec.MaxReplicas != nil { + if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas + if params.Instance.Spec.MinReplicas != nil { + updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.MinReplicas + } else { + updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MinReplicas = &one + } + } else { + updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas + if params.Instance.Spec.MinReplicas != nil { + updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.MinReplicas + } else { + updated.(*autoscalingv2.HorizontalPodAutoscaler).Spec.MinReplicas = &one } - - params.Log.V(2).Info("applied", "hpa.name", desired.Name, "hpa.namespace", desired.Namespace) } } - - return nil } func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { From 4791659319f7af7c89e3980c153773ffb94042ea Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Thu, 11 Aug 2022 11:25:05 +0200 Subject: [PATCH 16/18] Respond to comments Signed-off-by: Kevin Earls --- internal/config/main.go | 5 ++--- pkg/autodetect/main.go | 2 +- pkg/collector/horizontalpodautoscaler.go | 6 +++--- pkg/collector/reconcile/horizontalpodautoscaler.go | 9 ++++----- .../reconcile/horizontalpodautoscaler_test.go | 10 +++++----- 5 files changed, 15 insertions(+), 17 deletions(-) diff --git a/internal/config/main.go b/internal/config/main.go index 9b67587637..5e3d3e92a8 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -141,10 +141,9 @@ func (c *Config) AutoDetect() error { hpaVersion, err := c.autoDetect.HPAVersion() if err != nil { return err - } else { - c.autoscalingVersion = hpaVersion - c.logger.Info("In Autodetect, Set HPA version to [", c.autoscalingVersion, "] from [", hpaVersion, "]") } + c.autoscalingVersion = hpaVersion + c.logger.Info("In Autodetect, Set HPA version to [", c.autoscalingVersion, "] from [", hpaVersion, "]") return nil } diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index 524e5ec6ba..afd6dbd849 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -88,7 +88,7 @@ func (a *autoDetect) HPAVersion() (string, error) { return version.Version, nil } } - return "", errors.New("Failed to find appropriate version of apiGroup autoscaling") + return "", errors.New("Failed to find appropriate version of apiGroup autoscaling, only v2 and v2beta2 are supported") } } diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 6f4199face..9f5d78db54 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -20,7 +20,7 @@ import ( autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" @@ -29,7 +29,7 @@ import ( const defaultCPUTarget int32 = 90 -func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) runtime.Object { +func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCollector) client.Object { autoscalingVersion := cfg.AutoscalingVersion() labels := Labels(otelcol, cfg.LabelsFilter()) @@ -37,7 +37,7 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al annotations := Annotations(otelcol) cpuTarget := defaultCPUTarget - var result runtime.Object + var result client.Object objectMeta := metav1.ObjectMeta{ Name: naming.HorizontalPodAutoscaler(otelcol), diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index 8f3deeacac..a33157d39c 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -22,7 +22,6 @@ import ( autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -35,7 +34,7 @@ import ( // HorizontalPodAutoscaler reconciles HorizontalPodAutoscalers if autoscale is true and replicas is nil. func HorizontalPodAutoscalers(ctx context.Context, params Params) error { - desired := []runtime.Object{} + desired := []client.Object{} // check if autoscale mode is on, e.g MaxReplicas is not nil if params.Instance.Spec.MaxReplicas != nil { @@ -55,7 +54,7 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { return nil } -func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { +func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []client.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() var existing client.Object if autoscalingVersion == config.AutoscalingVersionV2Beta2 { @@ -74,7 +73,7 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect nns := types.NamespacedName{Namespace: desired.GetNamespace(), Name: desired.GetName()} err := params.Client.Get(ctx, nns, existing) if k8serrors.IsNotFound(err) { - if err := params.Client.Create(ctx, obj.(client.Object)); err != nil { + if err := params.Client.Create(ctx, obj); err != nil { return fmt.Errorf("failed to create: %w", err) } params.Log.V(2).Info("created", "hpa.name", desired.GetName(), "hpa.namespace", desired.GetNamespace()) @@ -131,7 +130,7 @@ func setAutoscalerSpec(params Params, autoscalingVersion string, updated client. } } -func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []runtime.Object) error { +func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected []client.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() opts := []client.ListOption{ diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 98f5fca467..6e4c2e9378 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -26,10 +26,10 @@ import ( autoscalingv2beta2 "k8s.io/api/autoscaling/v2beta2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" @@ -46,7 +46,7 @@ func TestExpectedHPA(t *testing.T) { expectedHPA := collector.HorizontalPodAutoscaler(params.Config, logger, params.Instance) t.Run("should create HPA", func(t *testing.T) { - err = expectedHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) + err = expectedHorizontalPodAutoscalers(context.Background(), params, []client.Object{expectedHPA}) assert.NoError(t, err) exists, err := populateObjectIfExists(t, &autoscalingv2beta2.HorizontalPodAutoscaler{}, types.NamespacedName{Namespace: "default", Name: "test-collector"}) @@ -65,7 +65,7 @@ func TestExpectedHPA(t *testing.T) { if autoscalingVersion == config.AutoscalingVersionV2Beta2 { updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) assert.NoError(t, err) actual := autoscalingv2beta2.HorizontalPodAutoscaler{} @@ -78,7 +78,7 @@ func TestExpectedHPA(t *testing.T) { } else { updatedAutoscaler := *updatedHPA.(*autoscalingv2.HorizontalPodAutoscaler) createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) - err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []runtime.Object{updatedHPA}) + err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) assert.NoError(t, err) actual := autoscalingv2.HorizontalPodAutoscaler{} @@ -92,7 +92,7 @@ func TestExpectedHPA(t *testing.T) { }) t.Run("should delete HPA", func(t *testing.T) { - err = deleteHorizontalPodAutoscalers(context.Background(), params, []runtime.Object{expectedHPA}) + err = deleteHorizontalPodAutoscalers(context.Background(), params, []client.Object{expectedHPA}) assert.NoError(t, err) actual := v1.Deployment{} From 5529f7a634c3bc4b17e77fe77147602ee4b58721 Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 17 Aug 2022 11:05:36 +0200 Subject: [PATCH 17/18] Implement autoscaling version constants as an enum Signed-off-by: Kevin Earls --- .../opentelemetrycollector_controller.go | 3 +- .../opentelemetrycollector_controller_test.go | 8 ++-- internal/config/main.go | 9 ++-- internal/config/main_test.go | 4 +- internal/config/options.go | 2 +- pkg/autodetect/main.go | 46 ++++++++++++++++--- pkg/autodetect/main_test.go | 6 +++ pkg/collector/horizontalpodautoscaler.go | 3 +- pkg/collector/horizontalpodautoscaler_test.go | 14 +++--- .../reconcile/horizontalpodautoscaler.go | 10 ++-- .../reconcile/horizontalpodautoscaler_test.go | 14 +++--- 11 files changed, 78 insertions(+), 41 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 6a59749eab..2a6fc45826 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -32,6 +32,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" "github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile" ) @@ -188,7 +189,7 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er Owns(&appsv1.DaemonSet{}). Owns(&appsv1.StatefulSet{}) - if autoscalingVersion == config.AutoscalingVersionV2 { + if autoscalingVersion == autodetect.AutoscalingVersionV2 { builder = builder.Owns(&autoscalingv2.HorizontalPodAutoscaler{}) } else { builder = builder.Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}) diff --git a/controllers/opentelemetrycollector_controller_test.go b/controllers/opentelemetrycollector_controller_test.go index 829f89c672..b9324225b1 100644 --- a/controllers/opentelemetrycollector_controller_test.go +++ b/controllers/opentelemetrycollector_controller_test.go @@ -43,8 +43,8 @@ import ( var logger = logf.Log.WithName("unit-tests") var mockAutoDetector = &mockAutoDetect{ - HPAVersionFunc: func() (string, error) { - return config.AutoscalingVersionV2Beta2, nil + HPAVersionFunc: func() (autodetect.AutoscalingVersion, error) { + return autodetect.AutoscalingVersionV2Beta2, nil }, } @@ -353,10 +353,10 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { PlatformFunc func() (platform.Platform, error) - HPAVersionFunc func() (string, error) + HPAVersionFunc func() (autodetect.AutoscalingVersion, error) } -func (m *mockAutoDetect) HPAVersion() (string, error) { +func (m *mockAutoDetect) HPAVersion() (autodetect.AutoscalingVersion, error) { return m.HPAVersionFunc() } diff --git a/internal/config/main.go b/internal/config/main.go index 5e3d3e92a8..e2eee51a22 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -30,9 +30,6 @@ const ( defaultAutoDetectFrequency = 5 * time.Second defaultCollectorConfigMapEntry = "collector.yaml" defaultTargetAllocatorConfigMapEntry = "targetallocator.yaml" - AutoscalingVersionV2 = "v2" - AutoscalingVersionV2Beta2 = "v2beta2" - DefaultAutoscalingVersion = AutoscalingVersionV2 ) // Config holds the static configuration for this operator. @@ -48,11 +45,11 @@ type Config struct { targetAllocatorConfigMapEntry string autoInstrumentationNodeJSImage string autoInstrumentationJavaImage string - autoscalingVersion string onChange []func() error labelsFilter []string platform platform.Platform autoDetectFrequency time.Duration + autoscalingVersion autodetect.AutoscalingVersion } // New constructs a new configuration based on the given options. @@ -65,7 +62,7 @@ func New(opts ...Option) Config { logger: logf.Log.WithName("config"), platform: platform.Unknown, version: version.Get(), - autoscalingVersion: DefaultAutoscalingVersion, + autoscalingVersion: autodetect.DefaultAutoscalingVersion, } for _, opt := range opts { opt(&o) @@ -174,7 +171,7 @@ func (c *Config) Platform() platform.Platform { } // AutoscalingVersion represents the preferred version of autoscaling. -func (c *Config) AutoscalingVersion() string { +func (c *Config) AutoscalingVersion() autodetect.AutoscalingVersion { return c.autoscalingVersion } diff --git a/internal/config/main_test.go b/internal/config/main_test.go index ceec3aa36e..1259aab488 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -102,8 +102,8 @@ type mockAutoDetect struct { PlatformFunc func() (platform.Platform, error) } -func (m *mockAutoDetect) HPAVersion() (string, error) { - return config.DefaultAutoscalingVersion, nil // TODO add a test? +func (m *mockAutoDetect) HPAVersion() (autodetect.AutoscalingVersion, error) { + return autodetect.DefaultAutoscalingVersion, nil // TODO add a test? } func (m *mockAutoDetect) Platform() (platform.Platform, error) { diff --git a/internal/config/options.go b/internal/config/options.go index 9bb30f94d6..a3846ce156 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -41,11 +41,11 @@ type options struct { collectorConfigMapEntry string targetAllocatorConfigMapEntry string targetAllocatorImage string - autoscalingVersion string onChange []func() error labelsFilter []string platform platform.Platform autoDetectFrequency time.Duration + autoscalingVersion autodetect.AutoscalingVersion } func WithAutoDetect(a autodetect.AutoDetect) Option { diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go index afd6dbd849..57d6748f6a 100644 --- a/pkg/autodetect/main.go +++ b/pkg/autodetect/main.go @@ -30,13 +30,23 @@ var _ AutoDetect = (*autoDetect)(nil) // AutoDetect provides an assortment of routines that auto-detect traits based on the runtime. type AutoDetect interface { Platform() (platform.Platform, error) - HPAVersion() (string, error) + HPAVersion() (AutoscalingVersion, error) } type autoDetect struct { dcl discovery.DiscoveryInterface } +type AutoscalingVersion int + +const ( + AutoscalingVersionV2 AutoscalingVersion = iota + AutoscalingVersionV2Beta2 + AutoscalingVersionUnknown +) + +const DefaultAutoscalingVersion = AutoscalingVersionV2 + // New creates a new auto-detection worker, using the given client when talking to the current cluster. func New(restConfig *rest.Config) (AutoDetect, error) { dcl, err := discovery.NewDiscoveryClientForConfig(restConfig) @@ -69,10 +79,10 @@ func (a *autoDetect) Platform() (platform.Platform, error) { return platform.Kubernetes, nil } -func (a *autoDetect) HPAVersion() (string, error) { +func (a *autoDetect) HPAVersion() (AutoscalingVersion, error) { apiList, err := a.dcl.ServerGroups() if err != nil { - return "", err + return AutoscalingVersionUnknown, err } for _, apiGroup := range apiList.Groups { @@ -84,13 +94,35 @@ func (a *autoDetect) HPAVersion() (string, error) { }) for _, version := range versions { - if version.Version == "v2" || version.Version == "v2beta2" { // Can't use the constants from internal/config/main.go as that would create an import cycle - return version.Version, nil + if version.Version == "v2" || version.Version == "v2beta2" { + return toAutoscalingVersion(version.Version), nil } } - return "", errors.New("Failed to find appropriate version of apiGroup autoscaling, only v2 and v2beta2 are supported") + return AutoscalingVersionUnknown, errors.New("Failed to find appropriate version of apiGroup autoscaling, only v2 and v2beta2 are supported") } } - return "", errors.New("Failed to find apiGroup autoscaling") + return AutoscalingVersionUnknown, errors.New("Failed to find apiGroup autoscaling") +} + +func (v AutoscalingVersion) String() string { + switch v { + case AutoscalingVersionV2: + return "v2" + case AutoscalingVersionV2Beta2: + return "v2beta2" + case AutoscalingVersionUnknown: + return "unknown" + } + return "unknown" +} + +func toAutoscalingVersion(version string) AutoscalingVersion { + switch version { + case "v2": + return AutoscalingVersionV2Beta2 + case "v2beta2": + return AutoscalingVersionV2Beta2 + } + return AutoscalingVersionUnknown } diff --git a/pkg/autodetect/main_test.go b/pkg/autodetect/main_test.go index 6ffc1b9e79..42be4eba1a 100644 --- a/pkg/autodetect/main_test.go +++ b/pkg/autodetect/main_test.go @@ -88,3 +88,9 @@ func TestUnknownPlatformOnError(t *testing.T) { assert.Error(t, err) assert.Equal(t, platform.Unknown, plt) } + +func TestAutoscalingVersionToString(t *testing.T) { + assert.Equal(t, "v2", autodetect.AutoscalingVersionV2.String()) + assert.Equal(t, "v2beta2", autodetect.AutoscalingVersionV2Beta2.String()) + assert.Equal(t, "unknown", autodetect.AutoscalingVersionUnknown.String()) +} diff --git a/pkg/collector/horizontalpodautoscaler.go b/pkg/collector/horizontalpodautoscaler.go index 9f5d78db54..0cdd3dca20 100644 --- a/pkg/collector/horizontalpodautoscaler.go +++ b/pkg/collector/horizontalpodautoscaler.go @@ -24,6 +24,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" "github.com/open-telemetry/opentelemetry-operator/pkg/naming" ) @@ -46,7 +47,7 @@ func HorizontalPodAutoscaler(cfg config.Config, logger logr.Logger, otelcol v1al Annotations: annotations, } - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { targetCPUUtilization := autoscalingv2beta2.MetricSpec{ Type: autoscalingv2beta2.ResourceMetricSourceType, Resource: &autoscalingv2beta2.ResourceMetricSource{ diff --git a/pkg/collector/horizontalpodautoscaler_test.go b/pkg/collector/horizontalpodautoscaler_test.go index 9d65b4dce7..6aaa71ca5c 100644 --- a/pkg/collector/horizontalpodautoscaler_test.go +++ b/pkg/collector/horizontalpodautoscaler_test.go @@ -33,10 +33,10 @@ import ( func TestHPA(t *testing.T) { type test struct { name string - autoscalingVersion string + autoscalingVersion autodetect.AutoscalingVersion } - v2Test := test{config.AutoscalingVersionV2, config.AutoscalingVersionV2} - v2beta2Test := test{config.AutoscalingVersionV2Beta2, config.AutoscalingVersionV2Beta2} + v2Test := test{autodetect.AutoscalingVersionV2.String(), autodetect.AutoscalingVersionV2} + v2beta2Test := test{autodetect.AutoscalingVersionV2Beta2.String(), autodetect.AutoscalingVersionV2Beta2} tests := []test{v2Test, v2beta2Test} var minReplicas int32 = 3 @@ -55,7 +55,7 @@ func TestHPA(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { mockAutoDetector := &mockAutoDetect{ - HPAVersionFunc: func() (string, error) { + HPAVersionFunc: func() (autodetect.AutoscalingVersion, error) { return test.autoscalingVersion, nil }, } @@ -64,7 +64,7 @@ func TestHPA(t *testing.T) { assert.NoError(t, err) raw := HorizontalPodAutoscaler(configuration, logger, otelcol) - if configuration.AutoscalingVersion() == config.AutoscalingVersionV2Beta2 { + if configuration.AutoscalingVersion() == autodetect.AutoscalingVersionV2Beta2 { hpa := raw.(*autoscalingv2beta2.HorizontalPodAutoscaler) // verify @@ -95,10 +95,10 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { PlatformFunc func() (platform.Platform, error) - HPAVersionFunc func() (string, error) + HPAVersionFunc func() (autodetect.AutoscalingVersion, error) } -func (m *mockAutoDetect) HPAVersion() (string, error) { +func (m *mockAutoDetect) HPAVersion() (autodetect.AutoscalingVersion, error) { return m.HPAVersionFunc() } diff --git a/pkg/collector/reconcile/horizontalpodautoscaler.go b/pkg/collector/reconcile/horizontalpodautoscaler.go index a33157d39c..302f3c4a5a 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler.go @@ -26,7 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" "github.com/open-telemetry/opentelemetry-operator/pkg/collector" ) @@ -57,7 +57,7 @@ func HorizontalPodAutoscalers(ctx context.Context, params Params) error { func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expected []client.Object) error { autoscalingVersion := params.Config.AutoscalingVersion() var existing client.Object - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { existing = &autoscalingv2beta2.HorizontalPodAutoscaler{} } else { existing = &autoscalingv2.HorizontalPodAutoscaler{} @@ -109,10 +109,10 @@ func expectedHorizontalPodAutoscalers(ctx context.Context, params Params, expect return nil } -func setAutoscalerSpec(params Params, autoscalingVersion string, updated client.Object) { +func setAutoscalerSpec(params Params, autoscalingVersion autodetect.AutoscalingVersion, updated client.Object) { one := int32(1) if params.Instance.Spec.MaxReplicas != nil { - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MaxReplicas = *params.Instance.Spec.MaxReplicas if params.Instance.Spec.MinReplicas != nil { updated.(*autoscalingv2beta2.HorizontalPodAutoscaler).Spec.MinReplicas = params.Instance.Spec.MinReplicas @@ -141,7 +141,7 @@ func deleteHorizontalPodAutoscalers(ctx context.Context, params Params, expected }), } - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { list := &autoscalingv2beta2.HorizontalPodAutoscalerList{} if err := params.Client.List(ctx, list, opts...); err != nil { return fmt.Errorf("failed to list: %w", err) diff --git a/pkg/collector/reconcile/horizontalpodautoscaler_test.go b/pkg/collector/reconcile/horizontalpodautoscaler_test.go index 6e4c2e9378..af2737c518 100644 --- a/pkg/collector/reconcile/horizontalpodautoscaler_test.go +++ b/pkg/collector/reconcile/horizontalpodautoscaler_test.go @@ -39,7 +39,7 @@ import ( ) func TestExpectedHPA(t *testing.T) { - params := paramsWithHPA(config.AutoscalingVersionV2Beta2) + params := paramsWithHPA(autodetect.AutoscalingVersionV2Beta2) err := params.Config.AutoDetect() assert.NoError(t, err) autoscalingVersion := params.Config.AutoscalingVersion() @@ -57,12 +57,12 @@ func TestExpectedHPA(t *testing.T) { t.Run("should update HPA", func(t *testing.T) { minReplicas := int32(1) maxReplicas := int32(3) - updateParms := paramsWithHPA(config.AutoscalingVersionV2Beta2) + updateParms := paramsWithHPA(autodetect.AutoscalingVersionV2Beta2) updateParms.Instance.Spec.Replicas = &minReplicas updateParms.Instance.Spec.MaxReplicas = &maxReplicas updatedHPA := collector.HorizontalPodAutoscaler(updateParms.Config, logger, updateParms.Instance) - if autoscalingVersion == config.AutoscalingVersionV2Beta2 { + if autoscalingVersion == autodetect.AutoscalingVersionV2Beta2 { updatedAutoscaler := *updatedHPA.(*autoscalingv2beta2.HorizontalPodAutoscaler) createObjectIfNotExists(t, "test-collector", &updatedAutoscaler) err := expectedHorizontalPodAutoscalers(context.Background(), updateParms, []client.Object{updatedHPA}) @@ -101,7 +101,7 @@ func TestExpectedHPA(t *testing.T) { }) } -func paramsWithHPA(autoscalingVersion string) Params { +func paramsWithHPA(autoscalingVersion autodetect.AutoscalingVersion) Params { configYAML, err := ioutil.ReadFile("../testdata/test.yaml") if err != nil { fmt.Printf("Error getting yaml file: %v", err) @@ -111,7 +111,7 @@ func paramsWithHPA(autoscalingVersion string) Params { maxReplicas := int32(5) mockAutoDetector := &mockAutoDetect{ - HPAVersionFunc: func() (string, error) { + HPAVersionFunc: func() (autodetect.AutoscalingVersion, error) { return autoscalingVersion, nil }, } @@ -159,10 +159,10 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { PlatformFunc func() (platform.Platform, error) - HPAVersionFunc func() (string, error) + HPAVersionFunc func() (autodetect.AutoscalingVersion, error) } -func (m *mockAutoDetect) HPAVersion() (string, error) { +func (m *mockAutoDetect) HPAVersion() (autodetect.AutoscalingVersion, error) { return m.HPAVersionFunc() } From a122e73105948a048193385bb98b3c16e0da6b4f Mon Sep 17 00:00:00 2001 From: Kevin Earls Date: Wed, 17 Aug 2022 15:50:52 +0200 Subject: [PATCH 18/18] remove TODO, move declaration Signed-off-by: Kevin Earls --- controllers/opentelemetrycollector_controller.go | 2 +- internal/config/main_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index 2a6fc45826..4250d677b7 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -179,7 +179,6 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er if err != nil { return err } - autoscalingVersion := r.config.AutoscalingVersion() builder := ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.OpenTelemetryCollector{}). Owns(&corev1.ConfigMap{}). @@ -189,6 +188,7 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er Owns(&appsv1.DaemonSet{}). Owns(&appsv1.StatefulSet{}) + autoscalingVersion := r.config.AutoscalingVersion() if autoscalingVersion == autodetect.AutoscalingVersionV2 { builder = builder.Owns(&autoscalingv2.HorizontalPodAutoscaler{}) } else { diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 1259aab488..fbdc6d8995 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -103,7 +103,7 @@ type mockAutoDetect struct { } func (m *mockAutoDetect) HPAVersion() (autodetect.AutoscalingVersion, error) { - return autodetect.DefaultAutoscalingVersion, nil // TODO add a test? + return autodetect.DefaultAutoscalingVersion, nil } func (m *mockAutoDetect) Platform() (platform.Platform, error) {