diff --git a/pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go b/pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go index ecbe1d7a1..a6f8df6e4 100644 --- a/pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go +++ b/pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go @@ -18,7 +18,6 @@ import ( v1 "k8s.io/api/core/v1" policyv1 "k8s.io/api/policy/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -209,11 +208,8 @@ func (r *BaseAPIManagerLogicReconciler) ReconcileHpa(desired *hpa.HorizontalPodA } } // Delete HPA in all other cases - err := r.DeleteResource(desired) - if err != nil && !errors.IsNotFound(err) { - return err - } - return nil + helper.TagObjectToDelete(desired) + return r.ReconcileResource(&hpa.HorizontalPodAutoscaler{}, desired, mutateFn) } func (r *BaseAPIManagerLogicReconciler) ReconcilePodMonitor(desired *monitoringv1.PodMonitor, mutateFn reconcilers.MutateFn) error { diff --git a/pkg/3scale/amp/operator/base_apimanager_logic_reconciler_test.go b/pkg/3scale/amp/operator/base_apimanager_logic_reconciler_test.go index f7d45168e..16f5bbadd 100644 --- a/pkg/3scale/amp/operator/base_apimanager_logic_reconciler_test.go +++ b/pkg/3scale/amp/operator/base_apimanager_logic_reconciler_test.go @@ -5,13 +5,17 @@ import ( "testing" appsv1alpha1 "github.com/3scale/3scale-operator/apis/apps/v1alpha1" + "github.com/3scale/3scale-operator/pkg/3scale/amp/component" "github.com/3scale/3scale-operator/pkg/reconcilers" grafanav1alpha1 "github.com/grafana-operator/grafana-operator/v4/api/integreatly/v1alpha1" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + hpav2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" fakeclientset "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" @@ -400,6 +404,216 @@ func TestBaseAPIManagerLogicReconcilerHasPodMonitors(t *testing.T) { } } +func newTestReconciler(t *testing.T, apimanager *appsv1alpha1.APIManager, existingObjs ...runtime.Object) (*BaseAPIManagerLogicReconciler, client.Client) { + t.Helper() + s := scheme.Scheme + if err := appsv1alpha1.AddToScheme(s); err != nil { + t.Fatal(err) + } + + objs := append([]runtime.Object{apimanager}, existingObjs...) + cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objs...).Build() + clientAPIReader := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(objs...).Build() + clientset := fakeclientset.NewSimpleClientset() + recorder := record.NewFakeRecorder(10000) + log := logf.Log.WithName("operator_test") + + baseReconciler := reconcilers.NewBaseReconciler(context.Background(), cl, s, clientAPIReader, log, clientset.Discovery(), recorder) + return NewBaseAPIManagerLogicReconciler(baseReconciler, apimanager), cl +} + +func TestReconcileHpa(t *testing.T) { + baseAM := func(annotations map[string]string) *appsv1alpha1.APIManager { + am := &appsv1alpha1.APIManager{ + ObjectMeta: metav1.ObjectMeta{ + Name: apimanagerName, + Namespace: namespace, + Annotations: annotations, + }, + Spec: appsv1alpha1.APIManagerSpec{ + APIManagerCommonSpec: appsv1alpha1.APIManagerCommonSpec{ + WildcardDomain: wildcardDomain, + }, + }, + } + if _, err := am.SetDefaults(); err != nil { + panic(err) + } + return am + } + + hpaExists := func(t *testing.T, cl client.Client, name string) bool { + t.Helper() + obj := &hpav2.HorizontalPodAutoscaler{} + err := cl.Get(context.Background(), types.NamespacedName{Name: name, Namespace: namespace}, obj) + if k8serrors.IsNotFound(err) { + return false + } + if err != nil { + t.Fatalf("unexpected error checking HPA %s: %v", name, err) + } + return true + } + + tests := []struct { + name string + apimanager *appsv1alpha1.APIManager + existingHPA *hpav2.HorizontalPodAutoscaler + desired *hpav2.HorizontalPodAutoscaler + assert func(t *testing.T, cl client.Client) + }{ + // Backend listener + { + name: "backend listener - HPA does not exist in cluster, disabled in config - no-op", + apimanager: baseAM(nil), + desired: component.DefaultHpa(component.BackendListenerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if hpaExists(t, cl, component.BackendListenerName) { + t.Error("HPA should not exist when disabled and was not pre-existing") + } + }, + }, + { + name: "backend listener - HPA exists in cluster, disabled in config - deletes HPA", + apimanager: baseAM(nil), + existingHPA: component.DefaultHpa(component.BackendListenerName, namespace), + desired: component.DefaultHpa(component.BackendListenerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if hpaExists(t, cl, component.BackendListenerName) { + t.Error("HPA should have been deleted when disabled") + } + }, + }, + { + name: "backend listener - HPA does not exist in cluster, enabled in config - creates HPA", + apimanager: func() *appsv1alpha1.APIManager { + am := baseAM(nil) + am.Spec.Backend.ListenerSpec.Hpa = true + return am + }(), + desired: component.DefaultHpa(component.BackendListenerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if !hpaExists(t, cl, component.BackendListenerName) { + t.Error("HPA should have been created when enabled") + } + }, + }, + { + name: "backend listener - HPA exists in cluster, enabled but with async-disable annotation - deletes HPA", + apimanager: func() *appsv1alpha1.APIManager { + am := baseAM(map[string]string{appsv1alpha1.DisableAsyncAnnotation: "true"}) + am.Spec.Backend.ListenerSpec.Hpa = true + return am + }(), + existingHPA: component.DefaultHpa(component.BackendListenerName, namespace), + desired: component.DefaultHpa(component.BackendListenerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if hpaExists(t, cl, component.BackendListenerName) { + t.Error("HPA should have been deleted when async is disabled regardless of HPA flag") + } + }, + }, + // Backend worker + { + name: "backend worker - HPA exists in cluster, disabled in config - deletes HPA", + apimanager: baseAM(nil), + existingHPA: component.DefaultHpa(component.BackendWorkerName, namespace), + desired: component.DefaultHpa(component.BackendWorkerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if hpaExists(t, cl, component.BackendWorkerName) { + t.Error("backend worker HPA should have been deleted when disabled") + } + }, + }, + { + name: "backend worker - HPA does not exist in cluster, enabled in config - creates HPA", + apimanager: func() *appsv1alpha1.APIManager { + am := baseAM(nil) + am.Spec.Backend.WorkerSpec.Hpa = true + return am + }(), + desired: component.DefaultHpa(component.BackendWorkerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if !hpaExists(t, cl, component.BackendWorkerName) { + t.Error("HPA should have been created when enabled") + } + }, + }, + { + name: "backend worker - HPA exists in cluster, enabled in config but with async-disable annotation - deletes HPA", + apimanager: func() *appsv1alpha1.APIManager { + am := baseAM(map[string]string{appsv1alpha1.DisableAsyncAnnotation: "true"}) + am.Spec.Backend.WorkerSpec.Hpa = true + return am + }(), + existingHPA: component.DefaultHpa(component.BackendWorkerName, namespace), + desired: component.DefaultHpa(component.BackendWorkerName, namespace), + assert: func(t *testing.T, cl client.Client) { + if hpaExists(t, cl, component.BackendWorkerName) { + t.Error("backend worker HPA should have been deleted when async is disabled") + } + }, + }, + // Apicast + { + name: "apicast - HPA exists in cluster, disabled in config - deletes HPA", + apimanager: baseAM(nil), + existingHPA: component.DefaultHpa(component.ApicastProductionName, namespace), + desired: component.DefaultHpa(component.ApicastProductionName, namespace), + assert: func(t *testing.T, cl client.Client) { + if hpaExists(t, cl, component.ApicastProductionName) { + t.Error("Apicast HPA should have been deleted when disabled") + } + }, + }, + { + name: "apicast - HPA does not exist in cluster, enabled in config - creates HPA", + apimanager: func() *appsv1alpha1.APIManager { + am := baseAM(nil) + am.Spec.Apicast.ProductionSpec.Hpa = true + return am + }(), + desired: component.DefaultHpa(component.ApicastProductionName, namespace), + assert: func(t *testing.T, cl client.Client) { + if !hpaExists(t, cl, component.ApicastProductionName) { + t.Error("Apicast HPA should have been created when enabled") + } + }, + }, + { + name: "apicast - HPA does not exist in cluster, enabled in config with async-disable annotation - creates HPA (annotation has no effect)", + apimanager: func() *appsv1alpha1.APIManager { + am := baseAM(map[string]string{appsv1alpha1.DisableAsyncAnnotation: "true"}) + am.Spec.Apicast.ProductionSpec.Hpa = true + return am + }(), + desired: component.DefaultHpa(component.ApicastProductionName, namespace), + assert: func(t *testing.T, cl client.Client) { + if !hpaExists(t, cl, component.ApicastProductionName) { + t.Error("Apicast HPA should be created regardless of async-disable annotation") + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var existingObjs []runtime.Object + if tt.existingHPA != nil { + existingObjs = append(existingObjs, tt.existingHPA) + } + + r, cl := newTestReconciler(t, tt.apimanager, existingObjs...) + + if err := r.ReconcileHpa(tt.desired, reconcilers.CreateOnlyMutator); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + tt.assert(t, cl) + }) + } +} + func TestBaseAPIManagerLogicReconcilerHasServiceMonitors(t *testing.T) { var ( apimanagerName = "example-apimanager"