From ce372282d11a28a409086f1493ca1be8e9256ad9 Mon Sep 17 00:00:00 2001 From: Jorge Turrado Ferrero Date: Fri, 16 Jun 2023 06:52:07 +0200 Subject: [PATCH] fix: HTTPScaledObject is the owner of the underlying ScaledObject (#704) * fix: HTTPScaledObject is the owner of the underlying ScaledObject Signed-off-by: Jorge Turrado * remove not needed code Signed-off-by: Jorge Turrado * update e2e tests Signed-off-by: Jorge Turrado --------- Signed-off-by: Jorge Turrado Signed-off-by: Jorge Turrado --- CHANGELOG.md | 1 + operator/controllers/http/app.go | 73 +------------------ .../http/httpscaledobject_controller.go | 36 +-------- operator/controllers/http/scaled_object.go | 12 ++- .../controllers/http/scaled_object_test.go | 19 ++++- .../ingress_in_app_namespace_test.go | 2 +- .../ingress_in_keda_namespace_test.go | 2 +- .../internal_service/internal_service_test.go | 2 +- .../multiple_hosts/multiple_hosts_test.go | 2 +- tests/checks/path_prefix/path_prefix_test.go | 2 +- tests/checks/single_host/single_host_test.go | 2 +- 11 files changed, 36 insertions(+), 117 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ad2c00..e72ecdbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ This changelog keeps track of work items that have been completed and are ready ### Fixes +- **General**: HTTPScaledObject is the owner of the underlying ScaledObject ([#703](https://github.com/kedacore/http-add-on/issues/703)) - **Routing**: Lookup host without port ([#608](https://github.com/kedacore/http-add-on/issues/608)) - **Controller**: Use kedav1alpha1.ScaledObject default values ([#607](https://github.com/kedacore/http-add-on/issues/607)) - **General**: Changes to HTTPScaledObjects now take effect ([#605](https://github.com/kedacore/http-add-on/issues/605)) diff --git a/operator/controllers/http/app.go b/operator/controllers/http/app.go index ad66708e..89ea1345 100644 --- a/operator/controllers/http/app.go +++ b/operator/controllers/http/app.go @@ -4,83 +4,14 @@ import ( "context" "github.com/go-logr/logr" - apierrs "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" "github.com/kedacore/http-add-on/operator/controllers/http/config" ) -func removeApplicationResources( - ctx context.Context, - logger logr.Logger, - cl client.Client, - httpso *v1alpha1.HTTPScaledObject, -) error { - defer SaveStatus(context.Background(), logger, cl, httpso) - // Set initial statuses - AddCondition( - httpso, - *SetMessage( - CreateCondition( - v1alpha1.Terminating, - v1.ConditionUnknown, - v1alpha1.TerminatingResources, - ), - "Received termination signal", - ), - ) - - logger = logger.WithValues( - "reconciler.appObjects", - "removeObjects", - "HTTPScaledObject.name", - httpso.Name, - "HTTPScaledObject.namespace", - httpso.Namespace, - ) - - // Delete App ScaledObject - scaledObject := &unstructured.Unstructured{} - scaledObject.SetNamespace(httpso.Namespace) - scaledObject.SetName(httpso.Name) - scaledObject.SetGroupVersionKind(schema.GroupVersionKind{ - Group: "keda.sh", - Kind: "ScaledObject", - Version: "v1alpha1", - }) - if err := cl.Delete(ctx, scaledObject); err != nil { - if apierrs.IsNotFound(err) { - logger.Info("App ScaledObject not found, moving on") - } else { - logger.Error(err, "Deleting scaledobject") - AddCondition( - httpso, - *SetMessage( - CreateCondition( - v1alpha1.Error, - v1.ConditionFalse, - v1alpha1.AppScaledObjectTerminationError, - ), - err.Error(), - ), - ) - return err - } - } - AddCondition(httpso, *CreateCondition( - v1alpha1.Terminated, - v1.ConditionTrue, - v1alpha1.AppScaledObjectTerminated, - )) - - return nil -} - -func createOrUpdateApplicationResources( +func (r *HTTPScaledObjectReconciler) createOrUpdateApplicationResources( ctx context.Context, logger logr.Logger, cl client.Client, @@ -114,7 +45,7 @@ func createOrUpdateApplicationResources( // the app deployment and the interceptor deployment. // this needs to be submitted so that KEDA will scale both the app and // interceptor - return createOrUpdateScaledObject( + return r.createOrUpdateScaledObject( ctx, cl, logger, diff --git a/operator/controllers/http/httpscaledobject_controller.go b/operator/controllers/http/httpscaledobject_controller.go index f1f6a98a..e7f72fa0 100644 --- a/operator/controllers/http/httpscaledobject_controller.go +++ b/operator/controllers/http/httpscaledobject_controller.go @@ -76,26 +76,6 @@ func (r *HTTPScaledObjectReconciler) Reconcile(ctx context.Context, req ctrl.Req } if httpso.GetDeletionTimestamp() != nil { - logger.Info("Deletion timestamp found", "httpscaledobject", *httpso) - // if it was marked deleted, delete all the related objects - // and don't schedule for another reconcile. Kubernetes - // will finalize them - // TODO: move this function call into `finalizeScaledObject` - removeErr := removeApplicationResources( - ctx, - logger, - r.Client, - httpso, - ) - if removeErr != nil { - // if we failed to remove app resources, reschedule a reconcile so we can try - // again - logger.Error(removeErr, "Removing application objects") - return ctrl.Result{ - RequeueAfter: 1000 * time.Millisecond, - }, removeErr - } - // after we've deleted app objects, we can finalize return ctrl.Result{}, finalizeScaledObject(ctx, logger, r.Client, httpso) } @@ -120,25 +100,15 @@ func (r *HTTPScaledObjectReconciler) Reconcile(ctx context.Context, req ctrl.Req ) // Create required app objects for the application defined by the CRD - if err := createOrUpdateApplicationResources( + err := r.createOrUpdateApplicationResources( ctx, logger, r.Client, r.BaseConfig, r.ExternalScalerConfig, httpso, - ); err != nil { - // if we failed to create app resources, remove what we've created and exit - logger.Error(err, "Removing app resources") - if removeErr := removeApplicationResources( - ctx, - logger, - r.Client, - httpso, - ); removeErr != nil { - logger.Error(removeErr, "Removing previously created resources") - } - + ) + if err != nil { return ctrl.Result{}, err } diff --git a/operator/controllers/http/scaled_object.go b/operator/controllers/http/scaled_object.go index cc18de6d..4d56114f 100644 --- a/operator/controllers/http/scaled_object.go +++ b/operator/controllers/http/scaled_object.go @@ -9,6 +9,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" httpv1alpha1 "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" "github.com/kedacore/http-add-on/pkg/k8s" @@ -18,7 +19,7 @@ import ( // according to the given parameters. If the create failed because the // ScaledObject already exists, attempts to patch the scaledobject. // otherwise, fails. -func createOrUpdateScaledObject( +func (r *HTTPScaledObjectReconciler) createOrUpdateScaledObject( ctx context.Context, cl client.Client, logger logr.Logger, @@ -46,6 +47,11 @@ func createOrUpdateScaledObject( httpso.Spec.CooldownPeriod, ) + // Set HTTPScaledObject instance as the owner and controller + if err := controllerutil.SetControllerReference(httpso, appScaledObject, r.Scheme); err != nil { + return err + } + logger.Info("Creating App ScaledObject", "ScaledObject", *appScaledObject) if err := cl.Create(ctx, appScaledObject); err != nil { if errors.IsAlreadyExists(err) { @@ -98,11 +104,11 @@ func createOrUpdateScaledObject( ), ) - return purgeLegacySO(ctx, cl, logger, httpso) + return r.purgeLegacySO(ctx, cl, logger, httpso) } // TODO(pedrotorres): delete this on v0.6.0 -func purgeLegacySO( +func (r *HTTPScaledObjectReconciler) purgeLegacySO( ctx context.Context, cl client.Client, logger logr.Logger, diff --git a/operator/controllers/http/scaled_object_test.go b/operator/controllers/http/scaled_object_test.go index 3a3b1e28..af1b0003 100644 --- a/operator/controllers/http/scaled_object_test.go +++ b/operator/controllers/http/scaled_object_test.go @@ -11,14 +11,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kedacore/http-add-on/operator/apis/http/v1alpha1" + "github.com/kedacore/http-add-on/operator/controllers/http/config" ) +const externalScalerHostName = "mysvc.myns.svc.cluster.local:9090" + func TestCreateOrUpdateScaledObject(t *testing.T) { r := require.New(t) - const externalScalerHostName = "mysvc.myns.svc.cluster.local:9090" - testInfra := newCommonTestInfra("testns", "testapp") - r.NoError(createOrUpdateScaledObject( + reconciller := &HTTPScaledObjectReconciler{ + Client: testInfra.cl, + Scheme: testInfra.cl.Scheme(), + ExternalScalerConfig: config.ExternalScaler{}, + BaseConfig: config.Base{}, + } + + r.NoError(reconciller.createOrUpdateScaledObject( testInfra.ctx, testInfra.cl, testInfra.logger, @@ -46,6 +54,9 @@ func TestCreateOrUpdateScaledObject(t *testing.T) { ) r.NoError(err) + // check that the app ScaledObject has the correct owner + r.Len(retSO.OwnerReferences, 1, "ScaledObject should have the owner reference") + metadata := retSO.ObjectMeta spec := retSO.Spec @@ -87,7 +98,7 @@ func TestCreateOrUpdateScaledObject(t *testing.T) { } *testInfra.httpso.Spec.Replicas.Min++ *testInfra.httpso.Spec.Replicas.Max++ - r.NoError(createOrUpdateScaledObject( + r.NoError(reconciller.createOrUpdateScaledObject( testInfra.ctx, testInfra.cl, testInfra.logger, diff --git a/tests/checks/ingress_in_app_namespace/ingress_in_app_namespace_test.go b/tests/checks/ingress_in_app_namespace/ingress_in_app_namespace_test.go index 5fa20c3e..0f5d7c6c 100644 --- a/tests/checks/ingress_in_app_namespace/ingress_in_app_namespace_test.go +++ b/tests/checks/ingress_in_app_namespace/ingress_in_app_namespace_test.go @@ -105,7 +105,7 @@ metadata: labels: app: {{.DeploymentName}} spec: - replicas: 1 + replicas: 0 selector: matchLabels: app: {{.DeploymentName}} diff --git a/tests/checks/ingress_in_keda_namespace/ingress_in_keda_namespace_test.go b/tests/checks/ingress_in_keda_namespace/ingress_in_keda_namespace_test.go index b25bbf50..0aea4ef8 100644 --- a/tests/checks/ingress_in_keda_namespace/ingress_in_keda_namespace_test.go +++ b/tests/checks/ingress_in_keda_namespace/ingress_in_keda_namespace_test.go @@ -93,7 +93,7 @@ metadata: labels: app: {{.DeploymentName}} spec: - replicas: 1 + replicas: 0 selector: matchLabels: app: {{.DeploymentName}} diff --git a/tests/checks/internal_service/internal_service_test.go b/tests/checks/internal_service/internal_service_test.go index d839594e..5142e126 100644 --- a/tests/checks/internal_service/internal_service_test.go +++ b/tests/checks/internal_service/internal_service_test.go @@ -65,7 +65,7 @@ metadata: labels: app: {{.DeploymentName}} spec: - replicas: 1 + replicas: 0 selector: matchLabels: app: {{.DeploymentName}} diff --git a/tests/checks/multiple_hosts/multiple_hosts_test.go b/tests/checks/multiple_hosts/multiple_hosts_test.go index 1d336ce9..0b6f9775 100644 --- a/tests/checks/multiple_hosts/multiple_hosts_test.go +++ b/tests/checks/multiple_hosts/multiple_hosts_test.go @@ -68,7 +68,7 @@ metadata: labels: app: {{.DeploymentName}} spec: - replicas: 1 + replicas: 0 selector: matchLabels: app: {{.DeploymentName}} diff --git a/tests/checks/path_prefix/path_prefix_test.go b/tests/checks/path_prefix/path_prefix_test.go index 55b5c09e..a18cf1d8 100644 --- a/tests/checks/path_prefix/path_prefix_test.go +++ b/tests/checks/path_prefix/path_prefix_test.go @@ -72,7 +72,7 @@ metadata: labels: app: {{.DeploymentName}} spec: - replicas: 1 + replicas: 0 selector: matchLabels: app: {{.DeploymentName}} diff --git a/tests/checks/single_host/single_host_test.go b/tests/checks/single_host/single_host_test.go index b5c099b9..50178a6f 100644 --- a/tests/checks/single_host/single_host_test.go +++ b/tests/checks/single_host/single_host_test.go @@ -65,7 +65,7 @@ metadata: labels: app: {{.DeploymentName}} spec: - replicas: 1 + replicas: 0 selector: matchLabels: app: {{.DeploymentName}}