From 5958936fd73d337e925a95f65ef4247013825f05 Mon Sep 17 00:00:00 2001 From: Joel Takvorian Date: Tue, 30 Jan 2024 14:05:59 +0100 Subject: [PATCH] Try fixing flaky test (#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 | 21 ++++++++------ ...wcollector_controller_certificates_test.go | 29 ++++++++++++------- controllers/flp/flp_controller.go | 20 +++++++------ .../monitoring/monitoring_controller.go | 20 +++++++------ pkg/manager/status/util.go | 16 ++++++++++ 5 files changed, 68 insertions(+), 38 deletions(-) create mode 100644 pkg/manager/status/util.go diff --git a/controllers/flowcollector_controller.go b/controllers/flowcollector_controller.go index 584c285cb..02fb69220 100644 --- a/controllers/flowcollector_controller.go +++ b/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) diff --git a/controllers/flowcollector_controller_certificates_test.go b/controllers/flowcollector_controller_certificates_test.go index 570c760a8..e59830399 100644 --- a/controllers/flowcollector_controller_certificates_test.go +++ b/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()) - }) }) } diff --git a/controllers/flp/flp_controller.go b/controllers/flp/flp_controller.go index 01bdc4e7a..688f1b0a3 100644 --- a/controllers/flp/flp_controller.go +++ b/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) diff --git a/controllers/monitoring/monitoring_controller.go b/controllers/monitoring/monitoring_controller.go index 35e761306..fd511e33d 100644 --- a/controllers/monitoring/monitoring_controller.go +++ b/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 diff --git a/pkg/manager/status/util.go b/pkg/manager/status/util.go new file mode 100644 index 000000000..3b788c139 --- /dev/null +++ b/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") +}