From 7367dca836bb2dd72aa3fa6a4331b54477b4f98b Mon Sep 17 00:00:00 2001 From: Quentin Bisson Date: Wed, 7 Feb 2024 10:06:37 +0100 Subject: [PATCH] Fix VPA for latest prom-operator (#1497) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix VPA for latest prom-operator * fix forgotten verticalpodautoscaler.Config (#1506) * fix forgotten verticalpodautoscaler.Config * move a const to a more suitable place --------- Co-authored-by: Herve Nicol <12008875+hervenicol@users.noreply.github.com> * cleanup VPA test * Fix VPA for latest prom-operator (#1511) * Fix VPA for latest prom-operator * Update CHANGELOG.md --------- Co-authored-by: Hervé Nicol Co-authored-by: Herve Nicol <12008875+hervenicol@users.noreply.github.com> --- CHANGELOG.md | 4 + service/controller/clusterapi/resource.go | 8 +- .../controller/managementcluster/resource.go | 8 +- .../verticalpodautoscaler/resource.go | 88 +++++++++++++++++-- .../verticalpodautoscaler/resource_test.go | 48 ++++++---- 5 files changed, 126 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2543d4db0..83e45b0b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix VPA to support latest Prometheus-operator version (based on observability-bundle 1.2.0) as the latest version of the Prometheus CR now supports the `scale` subresource which causes issues with VPA. + ## [4.66.0] - 2024-02-06 ### Changed diff --git a/service/controller/clusterapi/resource.go b/service/controller/clusterapi/resource.go index acb2410fa..0c4a25f27 100644 --- a/service/controller/clusterapi/resource.go +++ b/service/controller/clusterapi/resource.go @@ -267,9 +267,11 @@ func New(config Config) ([]resource.Interface, error) { var verticalPodAutoScalerResource resource.Interface { c := verticalpodautoscaler.Config{ - Logger: config.Logger, - K8sClient: config.K8sClient, - VpaClient: config.VpaClient, + Logger: config.Logger, + K8sClient: config.K8sClient, + VpaClient: config.VpaClient, + Installation: config.Installation, + Provider: config.Provider, } verticalPodAutoScalerResource, err = verticalpodautoscaler.New(c) diff --git a/service/controller/managementcluster/resource.go b/service/controller/managementcluster/resource.go index 4f52bef37..6ffb1bcda 100644 --- a/service/controller/managementcluster/resource.go +++ b/service/controller/managementcluster/resource.go @@ -197,9 +197,11 @@ func newResources(config resourcesConfig) ([]resource.Interface, error) { var verticalPodAutoScalerResource resource.Interface { c := verticalpodautoscaler.Config{ - Logger: config.Logger, - K8sClient: config.K8sClient, - VpaClient: config.VpaClient, + Logger: config.Logger, + K8sClient: config.K8sClient, + VpaClient: config.VpaClient, + Installation: config.Installation, + Provider: config.Provider, } verticalPodAutoScalerResource, err = verticalpodautoscaler.New(c) diff --git a/service/controller/resource/monitoring/verticalpodautoscaler/resource.go b/service/controller/resource/monitoring/verticalpodautoscaler/resource.go index 92252b90a..42cb14df4 100644 --- a/service/controller/resource/monitoring/verticalpodautoscaler/resource.go +++ b/service/controller/resource/monitoring/verticalpodautoscaler/resource.go @@ -2,35 +2,48 @@ package verticalpodautoscaler import ( "context" + "fmt" + "github.com/blang/semver" + appsv1alpha1 "github.com/giantswarm/apiextensions-application/api/v1alpha1" "github.com/giantswarm/k8sclient/v7/pkg/k8sclient" "github.com/giantswarm/microerror" "github.com/giantswarm/micrologger" autoscaling "k8s.io/api/autoscaling/v1" v1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned" + "github.com/giantswarm/prometheus-meta-operator/v2/pkg/cluster" "github.com/giantswarm/prometheus-meta-operator/v2/service/key" ) const ( - Name = "verticalpodautoscaler" + Name = "verticalpodautoscaler" + unknownObservabilityBundleVersion = "0.0.0" ) type Config struct { K8sClient k8sclient.Interface VpaClient vpa_clientset.Interface Logger micrologger.Logger + + Installation string + Provider cluster.Provider } type Resource struct { k8sClient k8sclient.Interface vpaClient vpa_clientset.Interface logger micrologger.Logger + + installation string + provider cluster.Provider } func New(config Config) (*Resource, error) { @@ -44,10 +57,16 @@ func New(config Config) (*Resource, error) { return nil, microerror.Maskf(invalidConfigError, "%T.Logger must not be empty", config) } + if config.Installation == "" { + return nil, microerror.Maskf(invalidConfigError, "%T.Installation must not be empty", config) + } + r := &Resource{ - k8sClient: config.K8sClient, - vpaClient: config.VpaClient, - logger: config.Logger, + k8sClient: config.K8sClient, + vpaClient: config.VpaClient, + logger: config.Logger, + installation: config.Installation, + provider: config.Provider, } return r, nil @@ -103,11 +122,6 @@ func (r *Resource) getObject(ctx context.Context, v interface{}) (*vpa_types.Ver vpa := &vpa_types.VerticalPodAutoscaler{ ObjectMeta: objectMeta, Spec: vpa_types.VerticalPodAutoscalerSpec{ - TargetRef: &autoscaling.CrossVersionObjectReference{ - Kind: "StatefulSet", - Name: key.PrometheusSTSName(cluster), - APIVersion: "apps/v1", - }, UpdatePolicy: &vpa_types.PodUpdatePolicy{ UpdateMode: &updateModeAuto, }, @@ -131,9 +145,65 @@ func (r *Resource) getObject(ctx context.Context, v interface{}) (*vpa_types.Ver }, } + mcObservabilityBundleAppVersion, err := r.getManagementClusterObservabilityBundleAppVersion(ctx, cluster) + if err != nil { + return nil, microerror.Mask(err) + } + + version, err := semver.Parse(mcObservabilityBundleAppVersion) + if err != nil { + return nil, microerror.Mask(err) + } + if version.LT(semver.MustParse("1.2.0")) { + // Set target reference to Prometheus StatefulSet + vpa.Spec.TargetRef = &autoscaling.CrossVersionObjectReference{ + Kind: "StatefulSet", + Name: key.PrometheusSTSName(cluster), + APIVersion: "apps/v1", + } + } else { + // Set target reference to Prometheus CR + vpa.Spec.TargetRef = &autoscaling.CrossVersionObjectReference{ + Kind: "Prometheus", + Name: key.ClusterID(cluster), + APIVersion: "monitoring.coreos.com/v1", + } + + } + return vpa, nil } +// We get the management cluster observability bundle app version for the management cluster to know if we need to configure the VPA CR to target the StatefulSet or the Prometheus CR. +// We need to do this because VPA uses the scale subresource to scale the target object, and the Prometheus CR has a scale subresource since the observability-bundle 1.2.0. +func (r *Resource) getManagementClusterObservabilityBundleAppVersion(ctx context.Context, cluster metav1.Object) (string, error) { + var appName string + var appNamespace string + if key.IsCAPIManagementCluster(r.provider) { + appName = fmt.Sprintf("%s-observability-bundle", r.installation) + appNamespace = "org-giantswarm" + } else { + appName = "observability-bundle" + appNamespace = "giantswarm" + } + + app := &appsv1alpha1.App{} + objectKey := types.NamespacedName{Namespace: appNamespace, Name: appName} + err := r.k8sClient.CtrlClient().Get(ctx, objectKey, app) + if err != nil { + if apierrors.IsNotFound(err) { + return unknownObservabilityBundleVersion, nil + } + return "", err + } + + if app.Status.Version != "" { + return app.Status.Version, nil + } + // This avoids a race condition where the app is created for the cluster but not deployed. + return unknownObservabilityBundleVersion, nil +} + func (r *Resource) listWorkerNodes(ctx context.Context) (*v1.NodeList, error) { // Selects only worker nodes selector := "node-role.kubernetes.io/control-plane!=" diff --git a/service/controller/resource/monitoring/verticalpodautoscaler/resource_test.go b/service/controller/resource/monitoring/verticalpodautoscaler/resource_test.go index cc3637a94..7fdeebd55 100644 --- a/service/controller/resource/monitoring/verticalpodautoscaler/resource_test.go +++ b/service/controller/resource/monitoring/verticalpodautoscaler/resource_test.go @@ -6,15 +6,18 @@ import ( "path/filepath" "testing" - "github.com/giantswarm/micrologger" - + appsv1alpha1 "github.com/giantswarm/apiextensions-application/api/v1alpha1" "github.com/giantswarm/k8sclient/v7/pkg/k8sclient" k8sclientfake "github.com/giantswarm/k8sclient/v7/pkg/k8sclient/fake" + "github.com/giantswarm/micrologger" v1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime" vpa_clientsetfake "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/fake" + "k8s.io/client-go/kubernetes/scheme" + "github.com/giantswarm/prometheus-meta-operator/v2/pkg/cluster" "github.com/giantswarm/prometheus-meta-operator/v2/pkg/unittest" ) @@ -46,27 +49,24 @@ func TestVerticalPodAutoScaler(t *testing.T) { var k8sClient k8sclient.Interface { + schemeBuilder := runtime.SchemeBuilder(k8sclient.SchemeBuilder{ + apiextensionsv1.AddToScheme, + appsv1alpha1.AddToScheme, + }) + + err = schemeBuilder.AddToScheme(scheme.Scheme) + if err != nil { + t.Fatal(err) + } c := k8sclient.ClientsConfig{ Logger: logger, - SchemeBuilder: k8sclient.SchemeBuilder(v1.SchemeBuilder), + SchemeBuilder: k8sclient.SchemeBuilder(schemeBuilder), } k8sClient, err = k8sclientfake.NewClients(c, node) if err != nil { t.Fatal(err) } } - testFunc := func(v interface{}) (interface{}, error) { - c := Config{ - Logger: logger, - K8sClient: k8sClient, - VpaClient: vpa_clientsetfake.NewSimpleClientset(), - } - r, err := New(c) - if err != nil { - return nil, err - } - return r.getObject(context.Background(), v) - } for _, flavor := range unittest.ProviderFlavors { outputDir, err := filepath.Abs("./test/" + flavor) @@ -74,6 +74,24 @@ func TestVerticalPodAutoScaler(t *testing.T) { t.Fatal(err) } + testFunc := func(v interface{}) (interface{}, error) { + c := Config{ + Logger: logger, + K8sClient: k8sClient, + VpaClient: vpa_clientsetfake.NewSimpleClientset(), + Provider: cluster.Provider{ + Kind: "aws", + Flavor: flavor, + }, + Installation: "test-installation", + } + r, err := New(c) + if err != nil { + return nil, err + } + return r.getObject(context.Background(), v) + } + c := unittest.Config{ Flavor: flavor, OutputDir: outputDir,