Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simultaneously support versions v2 and v2beta2 of Autoscaling #1014

Merged
merged 18 commits into from
Aug 18, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 28 additions & 28 deletions apis/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 17 additions & 5 deletions controllers/opentelemetrycollector_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import (

"github.com/go-logr/logr"
appsv1 "k8s.io/api/apps/v1"
autoscalingv1 "k8s.io/api/autoscaling/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"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -173,14 +174,25 @@ 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() // We need to call this so we can get the correct autodetect version
if err != nil {
return err
}
autoscalingVersion := r.config.AutoscalingVersion()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this closer to the place where it is used - line 192

builder := ctrl.NewControllerManagedBy(mgr).
For(&v1alpha1.OpenTelemetryCollector{}).
Owns(&corev1.ConfigMap{}).
Owns(&corev1.ServiceAccount{}).
Owns(&corev1.Service{}).
Owns(&appsv1.Deployment{}).
Owns(&autoscalingv1.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)
}
29 changes: 27 additions & 2 deletions controllers/opentelemetrycollector_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,21 @@ 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
},
}

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,
Expand Down Expand Up @@ -129,7 +136,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,
Expand Down Expand Up @@ -341,3 +348,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
}
19 changes: 19 additions & 0 deletions internal/config/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ const (
defaultAutoDetectFrequency = 5 * time.Second
defaultCollectorConfigMapEntry = "collector.yaml"
defaultTargetAllocatorConfigMapEntry = "targetallocator.yaml"
AutoscalingVersionV2 = "v2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: define these as enum (a golang type)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavolloffay Can you point me to an example of what you want here? Googling "golang enums" returns a bunch of disparate answers

AutoscalingVersionV2Beta2 = "v2beta2"
DefaultAutoscalingVersion = AutoscalingVersionV2
)

// Config holds the static configuration for this operator.
Expand All @@ -45,6 +48,7 @@ type Config struct {
targetAllocatorConfigMapEntry string
autoInstrumentationNodeJSImage string
autoInstrumentationJavaImage string
autoscalingVersion string
onChange []func() error
labelsFilter []string
platform platform.Platform
Expand All @@ -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)
Expand All @@ -81,6 +86,7 @@ func New(opts ...Option) Config {
autoInstrumentationPythonImage: o.autoInstrumentationPythonImage,
autoInstrumentationDotNetImage: o.autoInstrumentationDotNetImage,
labelsFilter: o.labelsFilter,
autoscalingVersion: o.autoscalingVersion,
}
}

Expand Down Expand Up @@ -132,6 +138,14 @@ func (c *Config) AutoDetect() error {
}
}

hpaVersion, err := c.autoDetect.HPAVersion()
if err != nil {
return err
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for else branch

c.autoscalingVersion = hpaVersion
c.logger.Info("In Autodetect, Set HPA version to [", c.autoscalingVersion, "] from [", hpaVersion, "]")
}

return nil
}

Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions internal/config/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions internal/config/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type options struct {
collectorConfigMapEntry string
targetAllocatorConfigMapEntry string
targetAllocatorImage string
autoscalingVersion string
onChange []func() error
labelsFilter []string
platform platform.Platform
Expand Down
23 changes: 23 additions & 0 deletions pkg/autodetect/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
package autodetect

import (
"errors"

"k8s.io/client-go/discovery"
"k8s.io/client-go/rest"

Expand All @@ -27,6 +29,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 {
Expand Down Expand Up @@ -64,3 +67,23 @@ 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" { // Can't use the constants from internal/config/main.go as that would create an import cycle
return version.Version, nil
}
}
return "v2beta2", nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning this just like this does not seem right. It should be returned only if that version is present in the cluster.

}
}

return "", errors.New("Failed to find apiGroup autoscaling")
}
Loading