Remove old autodetection for openshift routes, cleanup (#2398)

* remove old autodetection, cleanup

* remove unused

Author: jaronoff97

.chloggen/migrate-route-reconcilation.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# 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: removes the old way of running autodetection for openshift routes being available
+
+# One or more tracking issues related to the change
+issues: [2108]
+
+# (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/opampbridge_controller_test.go

@@ -35,14 +35,14 @@ import (
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/controllers"
+	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/autodetect"
 )
 
 var opampBridgeLogger = logf.Log.WithName("opamp-bridge-controller-unit-tests")
 var opampBridgeMockAutoDetector = &mockAutoDetect{
-	OpenShiftRoutesAvailabilityFunc: func() (autodetect.OpenShiftRoutesAvailability, error) {
-		return autodetect.OpenShiftRoutesAvailable, nil
+	OpenShiftRoutesAvailabilityFunc: func() (openshift.RoutesAvailability, error) {
+		return openshift.RoutesAvailable, nil
 	},
 }
 

controllers/opentelemetrycollector_controller.go

@@ -17,8 +17,6 @@ package controllers
 
 import (
 	"context"
-	"fmt"
-	"sync"
 
 	"github.com/go-logr/logr"
 	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
@@ -36,8 +34,6 @@ import (
 	"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"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/autodetect"
-	"github.com/open-telemetry/opentelemetry-operator/pkg/collector/reconcile"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
@@ -48,16 +44,6 @@ type OpenTelemetryCollectorReconciler struct {
 	scheme   *runtime.Scheme
 	log      logr.Logger
 	config   config.Config
-
-	tasks   []Task
-	muTasks sync.RWMutex
-}
-
-// Task represents a reconciliation task to be executed by the reconciler.
-type Task struct {
-	Do          func(context.Context, manifests.Params) error
-	Name        string
-	BailOnError bool
 }
 
 // Params is the set of options to build a new OpenTelemetryCollectorReconciler.
@@ -66,55 +52,9 @@ type Params struct {
 	Recorder record.EventRecorder
 	Scheme   *runtime.Scheme
 	Log      logr.Logger
-	Tasks    []Task
 	Config   config.Config
 }
 
-func (r *OpenTelemetryCollectorReconciler) onOpenShiftRoutesChange() error {
-	plt := r.config.OpenShiftRoutes()
-	var (
-		routesIdx = -1
-	)
-	r.muTasks.Lock()
-	for i, t := range r.tasks {
-		// search for route reconciler
-		switch t.Name {
-		case "routes":
-			routesIdx = i
-		}
-	}
-	r.muTasks.Unlock()
-
-	if err := r.addRouteTask(plt, routesIdx); err != nil {
-		return err
-	}
-
-	return r.removeRouteTask(plt, routesIdx)
-}
-
-func (r *OpenTelemetryCollectorReconciler) addRouteTask(ora autodetect.OpenShiftRoutesAvailability, routesIdx int) error {
-	r.muTasks.Lock()
-	defer r.muTasks.Unlock()
-	// if exists and openshift routes are available
-	if routesIdx == -1 && ora == autodetect.OpenShiftRoutesAvailable {
-		r.tasks = append([]Task{{reconcile.Routes, "routes", true}}, r.tasks...)
-	}
-	return nil
-}
-
-func (r *OpenTelemetryCollectorReconciler) removeRouteTask(ora autodetect.OpenShiftRoutesAvailability, routesIdx int) error {
-	r.muTasks.Lock()
-	defer r.muTasks.Unlock()
-	if len(r.tasks) < routesIdx {
-		return fmt.Errorf("can not remove route task from reconciler")
-	}
-	// if exists and openshift routes are not available
-	if routesIdx != -1 && ora == autodetect.OpenShiftRoutesNotAvailable {
-		r.tasks = append(r.tasks[:routesIdx], r.tasks[routesIdx+1:]...)
-	}
-	return nil
-}
-
 func (r *OpenTelemetryCollectorReconciler) getParams(instance v1alpha1.OpenTelemetryCollector) manifests.Params {
 	return manifests.Params{
 		Config:   r.config,
@@ -133,15 +73,8 @@ func NewReconciler(p Params) *OpenTelemetryCollectorReconciler {
 		log:      p.Log,
 		scheme:   p.Scheme,
 		config:   p.Config,
-		tasks:    p.Tasks,
 		recorder: p.Recorder,
 	}
-
-	if len(r.tasks) == 0 {
-		// TODO: put this in line with the rest of how we generate manifests
-		// https://github.com/open-telemetry/opentelemetry-operator/issues/2108
-		r.config.RegisterOpenShiftRoutesChangeCallback(r.onOpenShiftRoutesChange)
-	}
 	return r
 }
 
@@ -185,9 +118,6 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 	}
 
 	params := r.getParams(instance)
-	if err := r.RunTasks(ctx, params); err != nil {
-		return ctrl.Result{}, err
-	}
 
 	desiredObjects, buildErr := BuildCollector(params)
 	if buildErr != nil {
@@ -197,32 +127,8 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 	return collectorStatus.HandleReconcileStatus(ctx, log, params, err)
 }
 
-// RunTasks runs all the tasks associated with this reconciler.
-func (r *OpenTelemetryCollectorReconciler) RunTasks(ctx context.Context, params manifests.Params) error {
-	r.muTasks.RLock()
-	defer r.muTasks.RUnlock()
-	for _, task := range r.tasks {
-		if err := task.Do(ctx, params); err != nil {
-			// If we get an error that occurs because a pod is being terminated, then exit this loop
-			if apierrors.IsForbidden(err) && apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) {
-				r.log.V(2).Info("Exiting reconcile loop because namespace is being terminated", "namespace", params.OtelCol.Namespace)
-				return nil
-			}
-			r.log.Error(err, fmt.Sprintf("failed to reconcile %s", task.Name))
-			if task.BailOnError {
-				return err
-			}
-		}
-	}
-	return nil
-}
-
 // SetupWithManager tells the manager what our controller is interested in.
 func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	err := r.config.AutoDetect() // We need to call this, so we can get the correct autodetect version
-	if err != nil {
-		return err
-	}
 	builder := ctrl.NewControllerManagedBy(mgr).
 		For(&v1alpha1.OpenTelemetryCollector{}).
 		Owns(&corev1.ConfigMap{}).

controllers/opentelemetrycollector_controller_test.go

@@ -31,13 +31,17 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
 	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	k8sconfig "sigs.k8s.io/controller-runtime/pkg/client/config"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
 	k8sreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/controllers"
+	"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"
 	ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
@@ -530,6 +534,7 @@ func TestOpenTelemetryCollectorReconciler_Reconcile(t *testing.T) {
 				Config: config.New(
 					config.WithCollectorImage("default-collector"),
 					config.WithTargetAllocatorImage("default-ta-allocator"),
+					config.WithOpenShiftRoutesAvailability(openshift.RoutesAvailable),
 				),
 			})
 
@@ -749,6 +754,43 @@ func TestOpAMPBridgeReconciler_Reconcile(t *testing.T) {
 	}
 }
 
+func TestSkipWhenInstanceDoesNotExist(t *testing.T) {
+	// prepare
+	cfg := config.New()
+	nsn := types.NamespacedName{Name: "non-existing-my-instance", Namespace: "default"}
+	reconciler := controllers.NewReconciler(controllers.Params{
+		Client: k8sClient,
+		Log:    logger,
+		Scheme: scheme.Scheme,
+		Config: cfg,
+	})
+
+	// test
+	req := k8sreconcile.Request{
+		NamespacedName: nsn,
+	}
+	_, err := reconciler.Reconcile(context.Background(), req)
+
+	// verify
+	assert.NoError(t, err)
+}
+
+func TestRegisterWithManager(t *testing.T) {
+	t.Skip("this test requires a real cluster, otherwise the GetConfigOrDie will die")
+
+	// prepare
+	mgr, err := manager.New(k8sconfig.GetConfigOrDie(), manager.Options{})
+	require.NoError(t, err)
+
+	reconciler := controllers.NewReconciler(controllers.Params{})
+
+	// test
+	err = reconciler.SetupWithManager(mgr)
+
+	// verify
+	assert.NoError(t, err)
+}
+
 func namespacedObjectName(name string, namespace string) types.NamespacedName {
 	return types.NamespacedName{
 		Namespace: namespace,