Try fixing flaky test (netobserv#549)

- Add more info on test failure during certificate test cleanup
- Fix an error occuring when deleting the CR: controllers would still
  try to update the CR status despite the CR being deleted

controllers/flowcollector_controller.go

@@ -91,12 +91,22 @@ func Start(ctx context.Context, mgr *manager.Manager) error {
 func (r *FlowCollectorReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) {
 	l := log.Log.WithName("legacy") // clear context (too noisy)
 	ctx = log.IntoContext(ctx, l)
+
+	// Get flowcollector & create dedicated client
+	clh, desired, err := helper.NewFlowCollectorClientHelper(ctx, r.Client)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to get FlowCollector: %w", err)
+	} else if desired == nil {
+		// Delete case
+		return ctrl.Result{}, nil
+	}
+
 	// At the moment, status workflow is to start as ready then degrade if necessary
 	// Later (when legacy controller is broken down into individual controllers), status should start as unknown and only on success finishes as ready
 	r.status.SetReady()
 	defer r.status.Commit(ctx, r.Client)
 
-	err := r.reconcile(ctx)
+	err = r.reconcile(ctx, clh, desired)
 	if err != nil {
 		l.Error(err, "FlowCollector reconcile failure")
 		// Set status failure unless it was already set
@@ -109,14 +119,7 @@ func (r *FlowCollectorReconciler) Reconcile(ctx context.Context, _ ctrl.Request)
 	return ctrl.Result{}, nil
 }
 
-func (r *FlowCollectorReconciler) reconcile(ctx context.Context) error {
-	clh, desired, err := helper.NewFlowCollectorClientHelper(ctx, r.Client)
-	if err != nil {
-		return fmt.Errorf("failed to get FlowCollector: %w", err)
-	} else if desired == nil {
-		return nil
-	}
-
+func (r *FlowCollectorReconciler) reconcile(ctx context.Context, clh *helper.Client, desired *flowslatest.FlowCollector) error {
 	ns := helper.GetNamespace(&desired.Spec)
 	previousNamespace := r.status.GetDeployedNamespace(desired)
 	loki := helper.NewLokiConfig(&desired.Spec.Loki, ns)

controllers/flowcollector_controller_certificates_test.go

@@ -1,13 +1,13 @@
 package controllers
 
 import (
+	"fmt"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
@@ -16,6 +16,7 @@ import (
 	"github.com/netobserv/network-observability-operator/controllers/constants"
 	. "github.com/netobserv/network-observability-operator/controllers/controllerstest"
 	"github.com/netobserv/network-observability-operator/controllers/flp"
+	"github.com/netobserv/network-observability-operator/pkg/manager/status"
 	"github.com/netobserv/network-observability-operator/pkg/test"
 	"github.com/netobserv/network-observability-operator/pkg/watchers"
 )
@@ -473,15 +474,28 @@ func flowCollectorCertificatesSpecs() {
 	Context("Cleanup", func() {
 		// Retrieve CR to get its UID
 		flowCR := flowslatest.FlowCollector{}
+		It("Should get CR", func() {
+			Eventually(func() error {
+				return k8sClient.Get(ctx, crKey, &flowCR)
+			}, timeout, interval).Should(Succeed())
+		})
+
 		It("Should delete CR", func() {
 			Eventually(func() error {
-				if err := k8sClient.Get(ctx, crKey, &flowCR); err != nil {
-					return err
-				}
 				return k8sClient.Delete(ctx, &flowCR)
 			}, timeout, interval).Should(Succeed())
 		})
 
+		It("Should not get CR", func() {
+			Eventually(func() error {
+				err := k8sClient.Get(ctx, crKey, &flowCR)
+				if err == nil {
+					err = fmt.Errorf("CR is still present. Status: %s", status.ConditionsToString(flowCR.Status.Conditions))
+				}
+				return err
+			}, timeout, interval).Should(MatchError(`flowcollectors.flows.netobserv.io "cluster" not found`))
+		})
+
 		It("Should be garbage collected", func() {
 			By("Expecting flowlogs-pipeline deployment to be garbage collected")
 			Eventually(func() interface{} {
@@ -504,12 +518,5 @@ func flowCollectorCertificatesSpecs() {
 				return &d
 			}, timeout, interval).Should(BeGarbageCollectedBy(&flowCR))
 		})
-
-		It("Should not get CR", func() {
-			Eventually(func() bool {
-				err := k8sClient.Get(ctx, crKey, &flowCR)
-				return errors.IsNotFound(err)
-			}, timeout, interval).Should(BeTrue())
-		})
 	})
 }

controllers/flp/flp_controller.go

@@ -84,10 +84,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
 	l := log.Log.WithName("flp") // clear context (too noisy)
 	ctx = log.IntoContext(ctx, l)
 
+	// Get flowcollector & create dedicated client
+	clh, fc, err := helper.NewFlowCollectorClientHelper(ctx, r.Client)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to get FlowCollector: %w", err)
+	} else if fc == nil {
+		// Delete case
+		return ctrl.Result{}, nil
+	}
+
 	r.status.SetUnknown()
 	defer r.status.Commit(ctx, r.Client)
 
-	err := r.reconcile(ctx)
+	err = r.reconcile(ctx, clh, fc)
 	if err != nil {
 		l.Error(err, "FLP reconcile failure")
 		// Set status failure unless it was already set
@@ -101,16 +110,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
 	return ctrl.Result{}, nil
 }
 
-func (r *Reconciler) reconcile(ctx context.Context) error {
+func (r *Reconciler) reconcile(ctx context.Context, clh *helper.Client, fc *flowslatest.FlowCollector) error {
 	log := log.FromContext(ctx)
 
-	clh, fc, err := helper.NewFlowCollectorClientHelper(ctx, r.Client)
-	if err != nil {
-		return fmt.Errorf("failed to get FlowCollector: %w", err)
-	} else if fc == nil {
-		return nil
-	}
-
 	ns := helper.GetNamespace(&fc.Spec)
 	r.currentNamespace = ns
 	previousNamespace := r.status.GetDeployedNamespace(fc)

controllers/monitoring/monitoring_controller.go

@@ -46,10 +46,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
 	l := log.Log.WithName("monitoring") // clear context (too noisy)
 	ctx = log.IntoContext(ctx, l)
 
+	// Get flowcollector & create dedicated client
+	clh, desired, err := helper.NewFlowCollectorClientHelper(ctx, r.Client)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to get FlowCollector: %w", err)
+	} else if desired == nil {
+		// Delete case
+		return ctrl.Result{}, nil
+	}
+
 	r.status.SetUnknown()
 	defer r.status.Commit(ctx, r.Client)
 
-	err := r.reconcile(ctx)
+	err = r.reconcile(ctx, clh, desired)
 	if err != nil {
 		l.Error(err, "Monitoring reconcile failure")
 		// Set status failure unless it was already set
@@ -63,14 +72,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result
 	return ctrl.Result{}, nil
 }
 
-func (r *Reconciler) reconcile(ctx context.Context) error {
-	clh, desired, err := helper.NewFlowCollectorClientHelper(ctx, r.Client)
-	if err != nil {
-		return fmt.Errorf("failed to get FlowCollector: %w", err)
-	} else if desired == nil {
-		return nil
-	}
-
+func (r *Reconciler) reconcile(ctx context.Context, clh *helper.Client, desired *flowslatest.FlowCollector) error {
 	ns := helper.GetNamespace(&desired.Spec)
 
 	// If namespace does not exist, we create it

pkg/manager/status/util.go

@@ -0,0 +1,16 @@
+package status
+
+import (
+	"fmt"
+	"strings"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func ConditionsToString(conds []metav1.Condition) string {
+	var strConds []string
+	for _, cond := range conds {
+		strConds = append(strConds, fmt.Sprintf("- %s: %s / %s / %s", cond.Type, string(cond.Status), cond.Reason, cond.Message))
+	}
+	return strings.Join(strConds, "\n")
+}