Fixed Otel Controller - Auxiliares resources deletion

.chloggen/fix-hpa-delete.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: 'bug_fix'
+
+# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
+component: operator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fixed HPA deletion
+
+# One or more tracking issues related to the change
+issues: [2568]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:

controllers/common.go

@@ -20,18 +20,29 @@ import (
 	"fmt"
 
 	"github.com/go-logr/logr"
+	routev1 "github.com/openshift/api/route/v1"
+	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
+	networkingv1 "k8s.io/api/networking/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	"github.com/open-telemetry/opentelemetry-operator/internal/api/convert"
+	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector"
+	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/manifestutils"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/opampbridge"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator"
+	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
+	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
 func isNamespaceScoped(obj client.Object) bool {
@@ -76,9 +87,71 @@ func BuildOpAMPBridge(params manifests.Params) ([]client.Object, error) {
 	return resources, nil
 }
 
+func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Context, params manifests.Params) (map[types.UID]client.Object, error) {
+	ownedObjects := map[types.UID]client.Object{}
+	otelCol, err := convert.V1Alpha1to2(params.OtelCol)
+	if err != nil {
+		return nil, err
+	}
+
+	listOps := &client.ListOptions{
+		Namespace:     otelCol.Namespace,
+		LabelSelector: labels.SelectorFromSet(manifestutils.Labels(otelCol.ObjectMeta, naming.Collector(otelCol.Name), otelCol.Spec.Image, collector.ComponentOpenTelemetryCollector, params.Config.LabelsFilter())),
+	}
+	hpaList := &autoscalingv2.HorizontalPodAutoscalerList{}
+	err = r.List(ctx, hpaList, listOps)
+	if err != nil {
+		return nil, fmt.Errorf("Error listing HorizontalPodAutoscalers: %w", err)
+	}
+	for i := range hpaList.Items {
+		ownedObjects[hpaList.Items[i].GetUID()] = &hpaList.Items[i]
+	}
+
+	if otelCol.Spec.Observability.Metrics.EnableMetrics && featuregate.PrometheusOperatorIsAvailable.IsEnabled() {
+		servicemonitorList := &monitoringv1.ServiceMonitorList{}
+		err = r.List(ctx, servicemonitorList, listOps)
+		if err != nil {
+			return nil, fmt.Errorf("Error listing ServiceMonitors: %w", err)
+		}
+		for i := range servicemonitorList.Items {
+			ownedObjects[servicemonitorList.Items[i].GetUID()] = servicemonitorList.Items[i]
+		}
+
+		podMonitorList := &monitoringv1.ServiceMonitorList{}
+		err = r.List(ctx, podMonitorList, listOps)
+		if err != nil {
+			return nil, fmt.Errorf("Error listing PodMonitors: %w", err)
+		}
+		for i := range podMonitorList.Items {
+			ownedObjects[podMonitorList.Items[i].GetUID()] = podMonitorList.Items[i]
+		}
+	}
+	ingressList := &networkingv1.IngressList{}
+	err = r.List(ctx, ingressList, listOps)
+	if err != nil {
+		return nil, fmt.Errorf("Error listing Ingresses: %w", err)
+	}
+	for i := range ingressList.Items {
+		ownedObjects[ingressList.Items[i].GetUID()] = &ingressList.Items[i]
+	}
+
+	if params.Config.OpenShiftRoutesAvailability() == openshift.RoutesAvailable {
+		routesList := &routev1.RouteList{}
+		err := r.List(ctx, routesList, listOps)
+		if err != nil {
+			return nil, fmt.Errorf("Error listing Routes: %w", err)
+		}
+		for i := range routesList.Items {
+			ownedObjects[routesList.Items[i].GetUID()] = &routesList.Items[i]
+		}
+	}
+	return ownedObjects, nil
+}
+
 // reconcileDesiredObjects runs the reconcile process using the mutateFn over the given list of objects.
-func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logger logr.Logger, owner metav1.Object, scheme *runtime.Scheme, desiredObjects ...client.Object) error {
+func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logger logr.Logger, owner metav1.Object, scheme *runtime.Scheme, desiredObjects []client.Object, ownedObjects map[types.UID]client.Object) error {
 	var errs []error
+	pruneObjects := ownedObjects
 	for _, desired := range desiredObjects {
 		l := logger.WithValues(
 			"object_name", desired.GetName(),
@@ -91,7 +164,6 @@ func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logg
 				continue
 			}
 		}
-
 		// existing is an object the controller runtime will hydrate for us
 		// we obtain the existing object by deep copying the desired object because it's the most convenient way
 		existing := desired.DeepCopyObject().(client.Object)
@@ -116,9 +188,29 @@ func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logg
 		}
 
 		l.V(1).Info(fmt.Sprintf("desired has been %s", op))
+		// This object is still managed by the operator, remove it from the list of objects to prune
+		delete(pruneObjects, existing.GetUID())
 	}
 	if len(errs) > 0 {
 		return fmt.Errorf("failed to create objects for %s: %w", owner.GetName(), errors.Join(errs...))
 	}
+	// Pruning owned objects in the cluster which are not should not be present after the reconciliation.
+	pruneErrs := []error{}
+	for _, obj := range pruneObjects {
+		l := logger.WithValues(
+			"object_name", obj.GetName(),
+			"object_kind", obj.GetObjectKind().GroupVersionKind(),
+		)
+
+		l.Info("pruning unmanaged resource")
+		err := kubeClient.Delete(ctx, obj)
+		if err != nil {
+			l.Error(err, "failed to delete resource")
+			pruneErrs = append(pruneErrs, err)
+		}
+	}
+	if len(pruneErrs) > 0 {
+		return fmt.Errorf("failed to prune objects for %s: %w", owner.GetName(), errors.Join(pruneErrs...))
+	}
 	return nil
 }

controllers/opampbridge_controller.go

@@ -103,7 +103,7 @@ func (r *OpAMPBridgeReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	if buildErr != nil {
 		return ctrl.Result{}, buildErr
 	}
-	err := reconcileDesiredObjects(ctx, r.Client, log, &params.OpAMPBridge, params.Scheme, desiredObjects...)
+	err := reconcileDesiredObjects(ctx, r.Client, log, &params.OpAMPBridge, params.Scheme, desiredObjects, nil)
 	return opampbridgeStatus.HandleReconcileStatus(ctx, log, params, err)
 }
 

controllers/opentelemetrycollector_controller.go

@@ -19,10 +19,12 @@ import (
 	"context"
 
 	"github.com/go-logr/logr"
+	routev1 "github.com/openshift/api/route/v1"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
+	networkingv1 "k8s.io/api/networking/v1"
 	policyV1 "k8s.io/api/policy/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -32,6 +34,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
+	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	collectorStatus "github.com/open-telemetry/opentelemetry-operator/internal/status/collector"
@@ -125,7 +128,13 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 	if buildErr != nil {
 		return ctrl.Result{}, buildErr
 	}
-	err := reconcileDesiredObjects(ctx, r.Client, log, &params.OtelCol, params.Scheme, desiredObjects...)
+
+	ownedObjects, err := r.findOtelOwnedObjects(ctx, params)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	err = reconcileDesiredObjects(ctx, r.Client, log, &params.OtelCol, params.Scheme, desiredObjects, ownedObjects)
 	return collectorStatus.HandleReconcileStatus(ctx, log, params, err)
 }
 
@@ -139,6 +148,7 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er
 		Owns(&appsv1.Deployment{}).
 		Owns(&appsv1.DaemonSet{}).
 		Owns(&appsv1.StatefulSet{}).
+		Owns(&networkingv1.Ingress{}).
 		Owns(&autoscalingv2.HorizontalPodAutoscaler{}).
 		Owns(&policyV1.PodDisruptionBudget{})
 
@@ -151,6 +161,9 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er
 		builder.Owns(&monitoringv1.ServiceMonitor{})
 		builder.Owns(&monitoringv1.PodMonitor{})
 	}
+	if r.config.OpenShiftRoutesAvailability() == openshift.RoutesAvailable {
+		builder.Owns(&routev1.Route{})
+	}
 
 	return builder.Complete(r)
 }

controllers/reconcile_test.go

@@ -116,6 +116,7 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 	deletedParams := paramsWithMode(v1alpha1.ModeDeployment)
 	now := metav1.NewTime(time.Now())
 	deletedParams.OtelCol.DeletionTimestamp = &now
+	deploymentWithoutHPA := paramsWithModeAndReplicas(v1alpha1.ModeDeployment, 1)
 
 	type args struct {
 		params manifests.Params
@@ -347,6 +348,45 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "hpa v2 deployment collector deletion process",
+			args: args{
+				params:  paramsWithHPA(3, 5),
+				updates: []manifests.Params{deploymentWithoutHPA},
+			},
+			want: []want{
+				{
+					result: controllerruntime.Result{},
+					checks: []check{
+						func(t *testing.T, params manifests.Params) {
+							actual := autoscalingv2.HorizontalPodAutoscaler{}
+							exists, hpaErr := populateObjectIfExists(t, &actual, namespacedObjectName(naming.HorizontalPodAutoscaler(params.OtelCol.Name), params.OtelCol.Namespace))
+							assert.NoError(t, hpaErr)
+							require.Len(t, actual.Spec.Metrics, 1)
+							assert.Equal(t, int32(90), *actual.Spec.Metrics[0].Resource.Target.AverageUtilization)
+							assert.Equal(t, int32(3), *actual.Spec.MinReplicas)
+							assert.Equal(t, int32(5), actual.Spec.MaxReplicas)
+							assert.True(t, exists)
+						},
+					},
+					wantErr:     assert.NoError,
+					validateErr: assert.NoError,
+				},
+				{
+					result: controllerruntime.Result{},
+					checks: []check{
+						func(t *testing.T, params manifests.Params) {
+							hpa := autoscalingv2.HorizontalPodAutoscaler{}
+							exists, err := populateObjectIfExists(t, &hpa, namespacedObjectName(naming.HorizontalPodAutoscaler(params.OtelCol.Name), params.OtelCol.Namespace))
+							assert.NoError(t, err)
+							assert.False(t, exists) // There should be no hpa anymore
+						},
+					},
+					wantErr:     assert.NoError,
+					validateErr: assert.NoError,
+				},
+			},
+		},
 		{
 			name: "policy v1 deployment collector",
 			args: args{

controllers/suite_test.go

@@ -26,6 +26,7 @@ import (
 	"time"
 
 	routev1 "github.com/openshift/api/route/v1"
+	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	v1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -119,6 +120,11 @@ func TestMain(m *testing.M) {
 		os.Exit(1)
 	}
 
+	if err = autoscalingv2.AddToScheme(testScheme); err != nil {
+		fmt.Printf("failed to register scheme: %v", err)
+		os.Exit(1)
+	}
+
 	if err = v1alpha1.AddToScheme(testScheme); err != nil {
 		fmt.Printf("failed to register scheme: %v", err)
 		os.Exit(1)

internal/manifests/collector/horizontalpodautoscaler.go

@@ -18,7 +18,6 @@ import (
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/api/convert"
@@ -27,7 +26,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 )
 
-func HorizontalPodAutoscaler(params manifests.Params) (client.Object, error) {
+func HorizontalPodAutoscaler(params manifests.Params) (*autoscalingv2.HorizontalPodAutoscaler, error) {
 	otelCol, err := convert.V1Alpha1to2(params.OtelCol)
 	if err != nil {
 		return nil, err
@@ -39,7 +38,7 @@ func HorizontalPodAutoscaler(params manifests.Params) (client.Object, error) {
 		return nil, err
 	}
 
-	var result client.Object
+	var result *autoscalingv2.HorizontalPodAutoscaler
 
 	objectMeta := metav1.ObjectMeta{
 		Name:        naming.HorizontalPodAutoscaler(otelCol.Name),

internal/manifests/collector/horizontalpodautoscaler_test.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -94,11 +93,9 @@ func TestHPA(t *testing.T) {
 					},
 					Log: logger,
 				}
-				raw, err := HorizontalPodAutoscaler(params)
+				hpa, err := HorizontalPodAutoscaler(params)
 				require.NoError(t, err)
 
-				hpa := raw.(*autoscalingv2.HorizontalPodAutoscaler)
-
 				// verify
 				assert.Equal(t, "my-instance-collector", hpa.Name)
 				assert.Equal(t, "my-instance-collector", hpa.Labels["app.kubernetes.io/name"])

tests/e2e-autoscale/autoscale/04-delete.yaml

@@ -0,0 +1,11 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestStep
+delete:
+  - apiVersion: opentelemetry.io/v1alpha1
+    kind: OpenTelemetryCollector
+    metadata:
+      name: simplest
+  - apiVersion: opentelemetry.io/v1alpha1
+    kind: OpenTelemetryCollector
+    metadata:
+      name: simplest-set-utilization

tests/e2e-autoscale/autoscale/04-error.yaml

@@ -0,0 +1,28 @@
+---
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: simplest-collector
+spec:
+  maxReplicas: 2
+  scaleTargetRef:
+    apiVersion: opentelemetry.io/v1alpha1
+    kind: OpenTelemetryCollector
+    name: simplest
+status:
+  currentMetrics: null
+  currentReplicas: 1
+---
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: simplest-set-utilization-collector
+spec:
+  maxReplicas: 2
+  scaleTargetRef:
+    apiVersion: opentelemetry.io/v1alpha1
+    kind: OpenTelemetryCollector
+    name: simplest-set-utilization
+status:
+  currentMetrics: null
+  currentReplicas: 1