fix: HTTPScaledObject is the owner of the underlying ScaledObject (#704)

* fix: HTTPScaledObject is the owner of the underlying ScaledObject Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * remove not needed code Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> * update e2e tests Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es> --------- Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl> Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
kedacore · Jun 16, 2023 · ce37228 · ce37228
1 parent d333a0e
commit ce37228
Show file tree

Hide file tree

Showing 11 changed files with 36 additions and 117 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -65,7 +65,7 @@ metadata:
   labels:
     app: {{.DeploymentName}}
 spec:
-  replicas: 1
+  replicas: 0
   selector:
     matchLabels:
       app: {{.DeploymentName}}