From e2f8940593158c3fb495b355f077974437a76d18 Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Wed, 8 Apr 2026 09:02:20 -0400 Subject: [PATCH 1/9] [CONTP-1335] organizing finalizers initial PR --- go.work.sum | 6 +- .../datadogagent/controller_reconcile_v2.go | 4 +- internal/controller/datadogagent/finalizer.go | 53 +------------ .../controller/datadogagent/finalizer_test.go | 2 +- .../controller_reconcile_v2.go | 5 +- .../datadogagentinternal/finalizer.go | 54 +------------ .../datadogagentinternal/finalizer_test.go | 2 +- .../controller/datadogdashboard/controller.go | 4 +- .../datadogdashboard/controller_test.go | 2 +- .../controller/datadogdashboard/finalizer.go | 65 +++------------- .../datadogdashboard/finalizer_test.go | 4 +- .../datadoggenericresource/controller.go | 4 +- .../datadoggenericresource/controller_test.go | 2 +- .../datadoggenericresource/finalizer.go | 68 ++++------------ .../datadoggenericresource/finalizer_test.go | 4 +- .../controller/datadogmonitor/controller.go | 4 +- .../controller/datadogmonitor/finalizer.go | 78 +++++-------------- .../datadogmonitor/finalizer_test.go | 5 +- internal/controller/finalizer/finalizer.go | 2 + .../controller/finalizer/finalizer_test.go | 2 +- 20 files changed, 90 insertions(+), 280 deletions(-) diff --git a/go.work.sum b/go.work.sum index 16c9b638fe..3b43678ebd 100644 --- a/go.work.sum +++ b/go.work.sum @@ -58,7 +58,6 @@ cloud.google.com/go/cloudtasks v1.13.0 h1:rKVSsQwh0CI68n3RalLoGuW7sOtq2eil2gVZK4 cloud.google.com/go/cloudtasks v1.13.0/go.mod h1:O1jFRGb1Vm3sN2u/tBdPiVGVTWIsrsbEs3K3N3nNlEU= cloud.google.com/go/compute v1.28.0/go.mod h1:DEqZBtYrDnD5PvjsKwb3onnhX+qjdCVM7eshj1XdjV4= cloud.google.com/go/compute/metadata v0.3.0/go.mod h1:zFmK7XCadkQkj6TtorcaGlCW1hT1fIilQDwofLpJ20k= -cloud.google.com/go/compute/metadata v0.9.0/go.mod h1:E0bWwX5wTnLPedCKqk3pJmVgCBSM6qQI1yTBdEb3C10= cloud.google.com/go/contactcenterinsights v1.14.0 h1:hZSiEb53tyULmOSBlDPhEWPEe+vQ0F2gPQjOka1E5oE= cloud.google.com/go/contactcenterinsights v1.14.0/go.mod h1:APmWYHDN4sASnUBnXs4o68t1EUfnqadA53//CzXZ1xE= cloud.google.com/go/container v1.39.0 h1:Q1oW01ENxkkG3uf1oYoTmHPdvP+yhFCIuCJ4mk2RwkQ= @@ -1001,6 +1000,7 @@ github.com/pgavlin/text v0.0.0-20240821195002-b51d0990e284/go.mod h1:fk4+YyTLi0A github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e h1:aoZm08cpOy4WuID//EZDgcC4zIxODThtZNPirFr42+A= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/sftp v1.13.6/go.mod h1:tz1ryNURKu77RL+GuCzmoJYxQczL3wLNNpPWagdg4Qk= +github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 h1:GFCKgmp0tecUJ0sJuv4pzYCqS9+RGSn52M3FUwPs+uo= github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10/go.mod h1:t/avpk3KcrXxUnYOhZhMXJlSEyie6gQbtLq5NM3loB8= github.com/posener/complete v1.1.1 h1:ccV59UEOTzVDnDUEFdT95ZzHVZ+5+158q8+SJb2QV5w= github.com/posener/complete v1.2.3 h1:NP0eAhjcjImqslEwo/1hq7gpajME0fTLTezBKDqfXqo= @@ -1142,9 +1142,11 @@ github.com/vishvananda/netns v0.0.0-20210104183010-2eb08e3e575f h1:p4VB7kIXpOQvV github.com/vishvananda/netns v0.0.0-20210104183010-2eb08e3e575f/go.mod h1:DD4vA1DwXk04H54A1oHXtwZmA0grkVMdPxx/VGLCah0= github.com/vmihailenco/bufpool v0.1.11 h1:gOq2WmBrq0i2yW5QJ16ykccQ4wH9UyEsgLm6czKAd94= github.com/vmihailenco/bufpool v0.1.11/go.mod h1:AFf/MOy3l2CFTKbxwt0mp2MwnqjNEs5H/UxrkA5jxTQ= +github.com/vmihailenco/msgpack/v4 v4.3.13 h1:A2wsiTbvp63ilDaWmsk2wjx6xZdxQOvpiNlKBGKKXKI= github.com/vmihailenco/msgpack/v4 v4.3.13/go.mod h1:gborTTJjAo/GWTqqRjrLCn9pgNN+NXzzngzBKDPIqw4= github.com/vmihailenco/msgpack/v5 v5.4.1 h1:cQriyiUvjTwOHg8QZaPihLWeRAAVoCpE00IUPn0Bjt8= github.com/vmihailenco/msgpack/v5 v5.4.1/go.mod h1:GaZTsDaehaPpQVyxrf5mtQlH+pc21PIudVV/E3rRQok= +github.com/vmihailenco/tagparser v0.1.2 h1:gnjoVuB/kljJ5wICEEOpx98oXMWPLj22G67Vbd1qPqc= github.com/vmihailenco/tagparser v0.1.2/go.mod h1:OeAg3pn3UbLjkWt+rN9oFYB6u/cQgqMEUPoW2WPyhdI= github.com/vmihailenco/tagparser/v2 v2.0.0 h1:y09buUbR+b5aycVFQs/g70pqKVZNBmxwAhO7/IwNM9g= github.com/vmihailenco/tagparser/v2 v2.0.0/go.mod h1:Wri+At7QHww0WTrCBeu4J6bNtoV6mEfg5OIWRZA9qds= @@ -1227,7 +1229,6 @@ go.opentelemetry.io/otel/metric v1.29.0/go.mod h1:auu/QWieFVWx+DmQOUMgj0F8LHWdga go.opentelemetry.io/otel/metric v1.31.0/go.mod h1:C3dEloVbLuYoX41KpmAhOqNriGbA+qqH6PQ5E5mUfnY= go.opentelemetry.io/otel/sdk v1.28.0/go.mod h1:oYj7ClPUA7Iw3m+r7GeEjz0qckQRJK2B8zjcZEfu7Pg= go.opentelemetry.io/otel/sdk/metric v1.31.0/go.mod h1:CRInTMVvNhUKgSAMbKyTMxqOBC0zgyxzW55lZzX43Y8= -go.opentelemetry.io/otel/sdk/metric v1.39.0/go.mod h1:xq9HEVH7qeX69/JnwEfp6fVq5wosJsY1mt4lLfYdVew= go.opentelemetry.io/otel/sdk/metric v1.40.0/go.mod h1:4Z2bGMf0KSK3uRjlczMOeMhKU2rhUqdWNoKcYrtcBPg= go.opentelemetry.io/otel/trace v1.28.0/go.mod h1:jPyXzNPg6da9+38HEwElrQiHlVMTnVfM3/yv2OlIHaI= go.opentelemetry.io/otel/trace v1.29.0/go.mod h1:eHl3w0sp3paPkYstJOmAimxhiFXPg+MMTlEh3nsQgWQ= @@ -1325,7 +1326,6 @@ golang.org/x/tools v0.26.0/go.mod h1:TPVVj70c7JJ3WCazhD8OdXcZg/og+b9+tH/KxylGwH0 golang.org/x/tools v0.29.0/go.mod h1:KMQVMRsVxU6nHCFXrBPhDB8XncLNLM0lIy/F14RP588= golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= golang.org/x/tools v0.37.0/go.mod h1:MBN5QPQtLMHVdvsbtarmTNukZDdgwdwlO5qGacAzF0w= -gonum.org/v1/gonum v0.16.0/go.mod h1:fef3am4MQ93R2HHpKnLk4/Tbh/s0+wqD5nfa6Pnwy4E= google.golang.org/api v0.152.0/go.mod h1:3qNJX5eOmhiWYc67jRA/3GsDw97UFb5ivv7Y2PrriAY= google.golang.org/api v0.169.0 h1:QwWPy71FgMWqJN/l6jVlFHUa29a7dcUy02I8o799nPY= google.golang.org/api v0.169.0/go.mod h1:gpNOiMA2tZ4mf5R9Iwf4rK/Dcz0fbdIgWYWVoxmsyLg= diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index 1e59d6fe7d..eb2a8ad985 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -22,6 +22,7 @@ import ( "github.com/DataDog/datadog-operator/internal/controller/datadogagent/component" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/defaults" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/feature" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/agentprofile" "github.com/DataDog/datadog-operator/pkg/condition" "github.com/DataDog/datadog-operator/pkg/controller/utils" @@ -40,7 +41,8 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh } // 2. Handle finalizer logic. - if result, err := r.handleFinalizer(reqLogger, instance, r.finalizeDadV2); utils.ShouldReturn(result, err) { + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), 0, 0) + if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogagent/finalizer.go b/internal/controller/datadogagent/finalizer.go index 44d7745a0d..dad9e3aa14 100644 --- a/internal/controller/datadogagent/finalizer.go +++ b/internal/controller/datadogagent/finalizer.go @@ -12,11 +12,10 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/object" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/store" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/agentprofile" "github.com/DataDog/datadog-operator/pkg/constants" "github.com/DataDog/datadog-operator/pkg/kubernetes" @@ -26,41 +25,10 @@ const ( datadogAgentFinalizer = "finalizer.agent.datadoghq.com" ) -type finalizerDadFunc func(reqLogger logr.Logger, dda client.Object) error - -func (r *Reconciler) handleFinalizer(reqLogger logr.Logger, dda client.Object, finalizerDad finalizerDadFunc) (reconcile.Result, error) { - // Check if the DatadogAgent instance is marked to be deleted, which is - // indicated by the deletion timestamp being set. - isDadMarkedToBeDeleted := dda.GetDeletionTimestamp() != nil - if isDadMarkedToBeDeleted { - if controllerutil.ContainsFinalizer(dda, datadogAgentFinalizer) { - // Run finalization logic for datadogAgentFinalizer. If the - // finalization logic fails, don't remove the finalizer so - // that we can retry during the next reconciliation. - if err := finalizerDad(reqLogger, dda); err != nil { - return reconcile.Result{}, err - } - - // Remove datadogAgentFinalizer. Once all finalizers have been - // removed, the object will be deleted. - controllerutil.RemoveFinalizer(dda, datadogAgentFinalizer) - err := r.client.Update(context.TODO(), dda) - if err != nil { - return reconcile.Result{}, err - } - } - return reconcile.Result{Requeue: true}, nil - } - - // Add finalizer for this CR - if !controllerutil.ContainsFinalizer(dda, datadogAgentFinalizer) { - if err := r.addFinalizer(reqLogger, dda); err != nil { - return reconcile.Result{}, err - } - return reconcile.Result{Requeue: true}, nil +func (r *Reconciler) deleteResource(reqLogger logr.Logger) finalizer.ResourceDeleteFunc { + return func(ctx context.Context, k8sObj client.Object, datadogID string) error { + return r.finalizeDadV2(reqLogger, k8sObj) } - - return reconcile.Result{}, nil } func (r *Reconciler) finalizeDadV2(reqLogger logr.Logger, obj client.Object) error { @@ -82,19 +50,6 @@ func (r *Reconciler) finalizeDadV2(reqLogger logr.Logger, obj client.Object) err return nil } -func (r *Reconciler) addFinalizer(reqLogger logr.Logger, dda client.Object) error { - reqLogger.Info("Adding Finalizer for the DatadogAgent") - controllerutil.AddFinalizer(dda, datadogAgentFinalizer) - - // Update CR - err := r.client.Update(context.TODO(), dda) - if err != nil { - reqLogger.Error(err, "Failed to update DatadogAgent with finalizer") - return err - } - return nil -} - // profilesCleanup performs the cleanups required for the profiles feature. The // only thing that we need to do is to ensure that no nodes are left with the // profile label. diff --git a/internal/controller/datadogagent/finalizer_test.go b/internal/controller/datadogagent/finalizer_test.go index 09cd59fbc4..8bd2b4e2db 100644 --- a/internal/controller/datadogagent/finalizer_test.go +++ b/internal/controller/datadogagent/finalizer_test.go @@ -138,7 +138,7 @@ func Test_handleFinalizer(t *testing.T) { reconciler := reconcilerForFinalizerTest(initialKubeObjects) - _, err := reconciler.handleFinalizer(logf.Log.WithName("Handle Finalizer V2 test"), dda, reconciler.finalizeDadV2) + err := reconciler.finalizeDadV2(logf.Log.WithName("Handle Finalizer V2 test"), dda) assert.NoError(t, err) // Check that the cluster roles associated with the Datadog Agent have been deleted diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 602529f0c3..3d28ef4844 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -18,7 +18,9 @@ import ( "github.com/DataDog/datadog-operator/internal/controller/datadogagent/common" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/defaults" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/feature" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/condition" + "github.com/DataDog/datadog-operator/pkg/constants" "github.com/DataDog/datadog-operator/pkg/controller/utils" ) @@ -34,7 +36,8 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 // } // 2. Handle finalizer logic. - if result, err := r.handleFinalizer(ctx, instance, r.finalizeDDAI); utils.ShouldReturn(result, err) { + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), 0, 0) + if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogagentinternal/finalizer.go b/internal/controller/datadogagentinternal/finalizer.go index 6a25da304a..8949d0acd9 100644 --- a/internal/controller/datadogagentinternal/finalizer.go +++ b/internal/controller/datadogagentinternal/finalizer.go @@ -12,51 +12,19 @@ import ( "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/object" "github.com/DataDog/datadog-operator/internal/controller/datadogagent/store" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/agentprofile" "github.com/DataDog/datadog-operator/pkg/constants" "github.com/DataDog/datadog-operator/pkg/kubernetes" ) -type finalizerDDAIFunc func(ctx context.Context, dda client.Object) error - -func (r *Reconciler) handleFinalizer(ctx context.Context, ddai client.Object, finalizerDDAI finalizerDDAIFunc) (reconcile.Result, error) { - // Check if the DatadogAgentInternal instance is marked to be deleted, which is - // indicated by the deletion timestamp being set. - isDDAIMarkedToBeDeleted := ddai.GetDeletionTimestamp() != nil - if isDDAIMarkedToBeDeleted { - if controllerutil.ContainsFinalizer(ddai, constants.DatadogAgentInternalFinalizer) { - // Run finalization logic for datadogAgentFinalizer. If the - // finalization logic fails, don't remove the finalizer so - // that we can retry during the next reconciliation. - if err := finalizerDDAI(ctx, ddai); err != nil { - return reconcile.Result{}, err - } - - // Remove datadogAgentFinalizer. Once all finalizers have been - // removed, the object will be deleted. - controllerutil.RemoveFinalizer(ddai, constants.DatadogAgentInternalFinalizer) - err := r.client.Update(ctx, ddai) - if err != nil { - return reconcile.Result{}, err - } - } - return reconcile.Result{Requeue: true}, nil +func (r *Reconciler) deleteResource() finalizer.ResourceDeleteFunc { + return func(ctx context.Context, k8sObj client.Object, datadogID string) error { + return r.finalizeDDAI(ctx, k8sObj) } - - // Add finalizer for this CR - if !controllerutil.ContainsFinalizer(ddai, constants.DatadogAgentInternalFinalizer) { - if err := r.addFinalizer(ctx, ddai); err != nil { - return reconcile.Result{}, err - } - return reconcile.Result{Requeue: true}, nil - } - - return reconcile.Result{}, nil } func (r *Reconciler) finalizeDDAI(ctx context.Context, obj client.Object) error { @@ -79,20 +47,6 @@ func (r *Reconciler) finalizeDDAI(ctx context.Context, obj client.Object) error return nil } -func (r *Reconciler) addFinalizer(ctx context.Context, ddai client.Object) error { - logger := ctrl.LoggerFrom(ctx) - logger.Info("Adding Finalizer for the DatadogAgentInternal") - controllerutil.AddFinalizer(ddai, constants.DatadogAgentInternalFinalizer) - - // Update CR - err := r.client.Update(ctx, ddai) - if err != nil { - logger.Error(err, "Failed to update DatadogAgentInternal with finalizer") - return err - } - return nil -} - // profilesCleanup performs the cleanups required for the profiles feature. The // only thing that we need to do is to ensure that no nodes are left with the // profile label. diff --git a/internal/controller/datadogagentinternal/finalizer_test.go b/internal/controller/datadogagentinternal/finalizer_test.go index 9b8f35b1c5..1110792d98 100644 --- a/internal/controller/datadogagentinternal/finalizer_test.go +++ b/internal/controller/datadogagentinternal/finalizer_test.go @@ -127,7 +127,7 @@ func Test_handleFinalizer(t *testing.T) { reconciler := reconcilerForFinalizerTest(initialKubeObjects) - _, err := reconciler.handleFinalizer(context.Background(), ddai, reconciler.finalizeDDAI) + err := reconciler.finalizeDDAI(context.Background(), ddai) assert.NoError(t, err) // Check that the cluster roles associated with the Datadog Agent have been deleted diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index 80f1d15cd5..79c11f4d4c 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -22,6 +22,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/internal/controller/utils" "github.com/DataDog/datadog-operator/pkg/config" ctrutils "github.com/DataDog/datadog-operator/pkg/controller/utils" @@ -111,7 +112,8 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - if result, err = r.handleFinalizer(logger, instance); ctrutils.ShouldReturn(result, err) { + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod) + if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogDashboardFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogdashboard/controller_test.go b/internal/controller/datadogdashboard/controller_test.go index f5f6d6520a..7bb535ca99 100644 --- a/internal/controller/datadogdashboard/controller_test.go +++ b/internal/controller/datadogdashboard/controller_test.go @@ -71,7 +71,7 @@ func TestReconcileDatadogDashboard_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), genericDatadogDashboard()) }, }, - wantResult: reconcile.Result{Requeue: true}, + wantResult: reconcile.Result{RequeueAfter: defaultRequeuePeriod}, wantFunc: func(c client.Client) error { db := &datadoghqv1alpha1.DatadogDashboard{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, db); err != nil { diff --git a/internal/controller/datadogdashboard/finalizer.go b/internal/controller/datadogdashboard/finalizer.go index 143a99b2db..ea3473bc2a 100644 --- a/internal/controller/datadogdashboard/finalizer.go +++ b/internal/controller/datadogdashboard/finalizer.go @@ -10,10 +10,9 @@ import ( "fmt" "github.com/go-logr/logr" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/client" - datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/controller/utils/datadog" ) @@ -21,58 +20,18 @@ const ( datadogDashboardFinalizer = "finalizer.datadoghq.com/dashboard" ) -func (r *Reconciler) handleFinalizer(logger logr.Logger, db *datadoghqv1alpha1.DatadogDashboard) (ctrl.Result, error) { - // Check if the DatadogDashboard instance is marked to be deleted, which is indicated by the deletion timestamp being set. - if db.GetDeletionTimestamp() != nil { - if controllerutil.ContainsFinalizer(db, datadogDashboardFinalizer) { - r.finalizeDatadogDashboard(logger, db) - - controllerutil.RemoveFinalizer(db, datadogDashboardFinalizer) - err := r.client.Update(context.TODO(), db) +func (r *Reconciler) deleteResource(logger logr.Logger) finalizer.ResourceDeleteFunc { + return func(ctx context.Context, k8sObj client.Object, datadogID string) error { + if datadogID != "" { + err := deleteDashboard(r.datadogAuth, r.datadogClient, datadogID) if err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err + logger.Error(err, "failed to finalize dashboard", "dashboard ID", fmt.Sprint(datadogID)) + return nil } + logger.Info("Successfully finalized DatadogDashboard", "dashboard ID", fmt.Sprint(datadogID)) } - - // Requeue until the object is properly deleted by Kubernetes - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil + event := buildEventInfo(k8sObj.GetName(), k8sObj.GetNamespace(), datadog.DeletionEvent) + r.recordEvent(k8sObj, event) + return nil } - - // Add finalizer for this resource if it doesn't already exist. - if !controllerutil.ContainsFinalizer(db, datadogDashboardFinalizer) { - if err := r.addFinalizer(logger, db); err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err - } - - return ctrl.Result{Requeue: true}, nil - } - - // Proceed in reconcile loop. - return ctrl.Result{}, nil -} - -func (r *Reconciler) finalizeDatadogDashboard(logger logr.Logger, db *datadoghqv1alpha1.DatadogDashboard) { - err := deleteDashboard(r.datadogAuth, r.datadogClient, db.Status.ID) - if err != nil { - logger.Error(err, "failed to finalize dashboard", "dashboard ID", fmt.Sprint(db.Status.ID)) - - return - } - logger.Info("Successfully finalized DatadogDashboard", "dashboard ID", fmt.Sprint(db.Status.ID)) - event := buildEventInfo(db.Name, db.Namespace, datadog.DeletionEvent) - r.recordEvent(db, event) -} - -func (r *Reconciler) addFinalizer(logger logr.Logger, db *datadoghqv1alpha1.DatadogDashboard) error { - logger.Info("Adding Finalizer for the DatadogDashboard") - - controllerutil.AddFinalizer(db, datadogDashboardFinalizer) - - err := r.client.Update(context.TODO(), db) - if err != nil { - logger.Error(err, "failed to update DatadogDashboard with finalizer", "dashboard ID", fmt.Sprint(db.Status.ID)) - return err - } - - return nil } diff --git a/internal/controller/datadogdashboard/finalizer_test.go b/internal/controller/datadogdashboard/finalizer_test.go index c5dc133453..dd8c8fcf85 100644 --- a/internal/controller/datadogdashboard/finalizer_test.go +++ b/internal/controller/datadogdashboard/finalizer_test.go @@ -18,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" ) var ( @@ -58,7 +59,8 @@ func Test_handleFinalizer(t *testing.T) { t.Run(test.name, func(t *testing.T) { reqLogger := testLogger.WithValues("test:", test.name) _ = r.client.Create(context.TODO(), test.db) - _, err := r.handleFinalizer(reqLogger, test.db) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultRequeuePeriod, defaultErrRequeuePeriod) + _, err := final.HandleFinalizer(context.TODO(), test.db, test.db.Status.ID, datadogDashboardFinalizer) assert.NoError(t, err) if test.finalizerShouldExist { assert.True(t, controllerutil.ContainsFinalizer(test.db, datadogDashboardFinalizer)) diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index 7f32ca0e6a..1f16e9e8ac 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -21,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/internal/controller/utils" "github.com/DataDog/datadog-operator/pkg/config" ctrutils "github.com/DataDog/datadog-operator/pkg/controller/utils" @@ -105,7 +106,8 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - if result, err = r.handleFinalizer(logger, instance); ctrutils.ShouldReturn(result, err) { + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod) + if result, err = final.HandleFinalizer(ctx, instance, instance.Status.Id, datadogGenericResourceFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadoggenericresource/controller_test.go b/internal/controller/datadoggenericresource/controller_test.go index 1acd84aa4f..5138a04d58 100644 --- a/internal/controller/datadoggenericresource/controller_test.go +++ b/internal/controller/datadoggenericresource/controller_test.go @@ -72,7 +72,7 @@ func TestReconcileGenericResource_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), mockGenericResource()) }, }, - wantResult: reconcile.Result{Requeue: true}, + wantResult: reconcile.Result{RequeueAfter: defaultRequeuePeriod}, wantFunc: func(c client.Client) error { obj := &datadoghqv1alpha1.DatadogGenericResource{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, obj); err != nil { diff --git a/internal/controller/datadoggenericresource/finalizer.go b/internal/controller/datadoggenericresource/finalizer.go index b8ceb719a6..cffb4160ac 100644 --- a/internal/controller/datadoggenericresource/finalizer.go +++ b/internal/controller/datadoggenericresource/finalizer.go @@ -10,10 +10,10 @@ import ( "fmt" "github.com/go-logr/logr" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/client" datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/controller/utils/datadog" ) @@ -21,58 +21,20 @@ const ( datadogGenericResourceFinalizer = "finalizer.datadoghq.com/genericresource" ) -func (r *Reconciler) handleFinalizer(logger logr.Logger, instance *datadoghqv1alpha1.DatadogGenericResource) (ctrl.Result, error) { - // Check if the DatadogGenericResource instance is marked to be deleted, which is indicated by the deletion timestamp being set. - if instance.GetDeletionTimestamp() != nil { - if controllerutil.ContainsFinalizer(instance, datadogGenericResourceFinalizer) { - r.finalizeDatadogCustomResource(logger, instance) - - controllerutil.RemoveFinalizer(instance, datadogGenericResourceFinalizer) - err := r.client.Update(context.TODO(), instance) - if err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err - } +func (r *Reconciler) deleteResource(logger logr.Logger) finalizer.ResourceDeleteFunc { + return func(ctx context.Context, k8sObj client.Object, datadogID string) error { + instance, ok := k8sObj.(*datadoghqv1alpha1.DatadogGenericResource) + if !ok { + return fmt.Errorf("unexpected object type %T", k8sObj) } - - // Requeue until the object is properly deleted by Kubernetes - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil - } - - // Add finalizer for this resource if it doesn't already exist. - if !controllerutil.ContainsFinalizer(instance, datadogGenericResourceFinalizer) { - if err := r.addFinalizer(logger, instance); err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err + err := apiDelete(r, instance) + if err != nil { + logger.Error(err, "failed to finalize", "custom resource Id", fmt.Sprint(datadogID)) + return nil } - - return ctrl.Result{Requeue: true}, nil - } - - // Proceed in reconcile loop. - return ctrl.Result{}, nil -} - -func (r *Reconciler) finalizeDatadogCustomResource(logger logr.Logger, instance *datadoghqv1alpha1.DatadogGenericResource) { - err := apiDelete(r, instance) - if err != nil { - logger.Error(err, "failed to finalize ", "custom resource Id", fmt.Sprint(instance.Status.Id)) - - return - } - logger.Info("Successfully finalized DatadogGenericResource", "custom resource Id", fmt.Sprint(instance.Status.Id)) - event := buildEventInfo(instance.Name, instance.Namespace, datadog.DeletionEvent) - r.recordEvent(instance, event) -} - -func (r *Reconciler) addFinalizer(logger logr.Logger, instance *datadoghqv1alpha1.DatadogGenericResource) error { - logger.Info("Adding Finalizer for the DatadogGenericResource") - - controllerutil.AddFinalizer(instance, datadogGenericResourceFinalizer) - - err := r.client.Update(context.TODO(), instance) - if err != nil { - logger.Error(err, "failed to update DatadogGenericResource with finalizer", "custom resource Id", fmt.Sprint(instance.Status.Id)) - return err + logger.Info("Successfully finalized DatadogGenericResource", "custom resource Id", fmt.Sprint(datadogID)) + event := buildEventInfo(k8sObj.GetName(), k8sObj.GetNamespace(), datadog.DeletionEvent) + r.recordEvent(k8sObj, event) + return nil } - - return nil } diff --git a/internal/controller/datadoggenericresource/finalizer_test.go b/internal/controller/datadoggenericresource/finalizer_test.go index 0b22cbb6f0..26b4259642 100644 --- a/internal/controller/datadoggenericresource/finalizer_test.go +++ b/internal/controller/datadoggenericresource/finalizer_test.go @@ -24,6 +24,7 @@ import ( "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" ) const ( @@ -100,7 +101,8 @@ func Test_handleFinalizer(t *testing.T) { testGcr := &datadoghqv1alpha1.DatadogGenericResource{} err := r.client.Get(context.TODO(), client.ObjectKey{Name: test.resourceName, Namespace: testNamespace}, testGcr) - _, err = r.handleFinalizer(reqLogger, testGcr) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultRequeuePeriod, defaultErrRequeuePeriod) + _, err = final.HandleFinalizer(context.TODO(), testGcr, testGcr.Status.Id, datadogGenericResourceFinalizer) assert.NoError(t, err) if test.finalizerShouldExist { diff --git a/internal/controller/datadogmonitor/controller.go b/internal/controller/datadogmonitor/controller.go index caf3722b92..d0c90de51f 100644 --- a/internal/controller/datadogmonitor/controller.go +++ b/internal/controller/datadogmonitor/controller.go @@ -29,6 +29,7 @@ import ( datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" apiutils "github.com/DataDog/datadog-operator/api/utils" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/config" ctrutils "github.com/DataDog/datadog-operator/pkg/controller/utils" "github.com/DataDog/datadog-operator/pkg/controller/utils/comparison" @@ -138,7 +139,8 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv newStatus := instance.Status.DeepCopy() - if result, err = r.handleFinalizer(logger, instance); ctrutils.ShouldReturn(result, err) { + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod) + if result, err = final.HandleFinalizer(ctx, instance, fmt.Sprint(instance.Status.ID), datadogMonitorFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogmonitor/finalizer.go b/internal/controller/datadogmonitor/finalizer.go index 2fd4081b4d..3e60a39d63 100644 --- a/internal/controller/datadogmonitor/finalizer.go +++ b/internal/controller/datadogmonitor/finalizer.go @@ -10,10 +10,10 @@ import ( "fmt" "github.com/go-logr/logr" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/client" datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" "github.com/DataDog/datadog-operator/pkg/controller/utils/datadog" ) @@ -21,68 +21,28 @@ const ( datadogMonitorFinalizer = "finalizer.monitor.datadoghq.com" ) -func (r *Reconciler) handleFinalizer(logger logr.Logger, dm *datadoghqv1alpha1.DatadogMonitor) (ctrl.Result, error) { - // Check if the DatadogMonitor instance is marked to be deleted, which is indicated by the deletion timestamp being set. - if dm.GetDeletionTimestamp() != nil { - if controllerutil.ContainsFinalizer(dm, datadogMonitorFinalizer) { - if err := r.finalizeDatadogMonitor(logger, dm); err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err - } - - controllerutil.RemoveFinalizer(dm, datadogMonitorFinalizer) - err := r.client.Update(context.TODO(), dm) - if err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err - } +func (r *Reconciler) deleteResource(logger logr.Logger) finalizer.ResourceDeleteFunc { + return func(ctx context.Context, k8sObj client.Object, datadogID string) error { + dm, ok := k8sObj.(*datadoghqv1alpha1.DatadogMonitor) + if !ok { + return fmt.Errorf("unexpected object type %T", k8sObj) } - // Requeue until the object was properly deleted by Kuberentes - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil - } - - // Add finalizer for this resource if it doesn't already exist. - if !controllerutil.ContainsFinalizer(dm, datadogMonitorFinalizer) { - if err := r.addFinalizer(logger, dm); err != nil { - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err + if r.forwarders != nil { + r.forwarders.Unregister(dm) } - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil - } - - // Proceed in reconcile loop. - return ctrl.Result{}, nil -} - -func (r *Reconciler) finalizeDatadogMonitor(logger logr.Logger, dm *datadoghqv1alpha1.DatadogMonitor) error { - if r.forwarders != nil { - r.forwarders.Unregister(dm) - } - - if dm.Status.Primary { - err := deleteMonitor(r.datadogAuth, r.datadogClient, dm.Status.ID) - if err != nil { - logger.Error(err, "failed to finalize monitor", "Monitor ID", fmt.Sprint(dm.Status.ID)) - - return err + if dm.Status.Primary { + err := deleteMonitor(r.datadogAuth, r.datadogClient, dm.Status.ID) + if err != nil { + logger.Error(err, "failed to finalize monitor", "Monitor ID", fmt.Sprint(dm.Status.ID)) + return err + } + logger.Info("Successfully finalized DatadogMonitor", "Monitor ID", fmt.Sprint(dm.Status.ID)) + event := buildEventInfo(dm.Name, dm.Namespace, datadog.DeletionEvent) + r.recordEvent(dm, event) } - logger.Info("Successfully finalized DatadogMonitor", "Monitor ID", fmt.Sprint(dm.Status.ID)) - event := buildEventInfo(dm.Name, dm.Namespace, datadog.DeletionEvent) - r.recordEvent(dm, event) - } - return nil -} - -func (r *Reconciler) addFinalizer(logger logr.Logger, dm *datadoghqv1alpha1.DatadogMonitor) error { - logger.Info("Adding Finalizer for the DatadogMonitor") - - controllerutil.AddFinalizer(dm, datadogMonitorFinalizer) - - err := r.client.Update(context.TODO(), dm) - if err != nil { - logger.Error(err, "failed to update DatadogMonitor with finalizer", "Monitor ID", fmt.Sprint(dm.Status.ID)) - return err + return nil } - - return nil } diff --git a/internal/controller/datadogmonitor/finalizer_test.go b/internal/controller/datadogmonitor/finalizer_test.go index 1471532d47..e76fd055bd 100644 --- a/internal/controller/datadogmonitor/finalizer_test.go +++ b/internal/controller/datadogmonitor/finalizer_test.go @@ -7,6 +7,7 @@ package datadogmonitor import ( "context" + "fmt" "testing" "time" @@ -20,6 +21,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" + "github.com/DataDog/datadog-operator/internal/controller/finalizer" ) var ( @@ -90,7 +92,8 @@ func Test_handleFinalizer(t *testing.T) { testMonitor := &datadoghqv1alpha1.DatadogMonitor{} _ = r.client.Get(context.TODO(), client.ObjectKey{Namespace: "foo", Name: test.objectName}, testMonitor) - _, err := r.handleFinalizer(reqLogger, testMonitor) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultRequeuePeriod, defaultErrRequeuePeriod) + _, err := final.HandleFinalizer(context.TODO(), testMonitor, fmt.Sprint(testMonitor.Status.ID), datadogMonitorFinalizer) assert.NoError(t, err) if test.finalizerShouldExist { diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index fe495dbb52..0b4c560e53 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -74,6 +74,8 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } } + // Requeue until the object is properly deleted by Kubernetes + return ctrl.Result{Requeue: true}, nil } return ctrl.Result{}, nil } diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index 59df7504f8..50bf4b4963 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -74,7 +74,7 @@ func Test_HandleFinalizer(t *testing.T) { }, }, finalizerShouldExists: false, - expectedResult: ctrl.Result{}, + expectedResult: ctrl.Result{Requeue: true}, deleterFunc: func(ctx context.Context, k8sObj client.Object, datadogID string) error { return nil }, From c0a9165d76f566f374bc67508128a17af543f5e7 Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Wed, 8 Apr 2026 09:23:24 -0400 Subject: [PATCH 2/9] reverted bug fix --- internal/controller/datadogagent/controller_reconcile_v2.go | 3 +++ .../controller/datadogagentinternal/controller_reconcile_v2.go | 3 +++ internal/controller/datadogdashboard/controller.go | 3 +++ internal/controller/datadoggenericresource/controller.go | 3 +++ internal/controller/datadogmonitor/controller.go | 3 +++ internal/controller/finalizer/finalizer.go | 2 -- internal/controller/finalizer/finalizer_test.go | 2 +- 7 files changed, 16 insertions(+), 3 deletions(-) diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index c2bf65371f..a57d144db6 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -45,6 +45,9 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } + if !instance.GetDeletionTimestamp().IsZero() { + return reconcile.Result{Requeue: true}, nil + } // 3. Set default values for GlobalConfig and Features instanceCopy := instance.DeepCopy() diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 3d28ef4844..6af579ea20 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -40,6 +40,9 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } + if !instance.GetDeletionTimestamp().IsZero() { + return reconcile.Result{Requeue: true}, nil + } // 3. Set default values for GlobalConfig and Features instanceCopy := instance.DeepCopy() diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index 79c11f4d4c..b5acdcc0d3 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -116,6 +116,9 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogDashboardFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } + if !instance.GetDeletionTimestamp().IsZero() { + return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil + } status := instance.Status.DeepCopy() statusSpecHash := instance.Status.CurrentHash diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index 1f16e9e8ac..a6c8805392 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -110,6 +110,9 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques if result, err = final.HandleFinalizer(ctx, instance, instance.Status.Id, datadogGenericResourceFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } + if !instance.GetDeletionTimestamp().IsZero() { + return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil + } status := instance.Status.DeepCopy() statusSpecHash := instance.Status.CurrentHash diff --git a/internal/controller/datadogmonitor/controller.go b/internal/controller/datadogmonitor/controller.go index 6b6f9727b7..fa06d24b79 100644 --- a/internal/controller/datadogmonitor/controller.go +++ b/internal/controller/datadogmonitor/controller.go @@ -144,6 +144,9 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv if result, err = final.HandleFinalizer(ctx, instance, fmt.Sprint(instance.Status.ID), datadogMonitorFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } + if !instance.GetDeletionTimestamp().IsZero() { + return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil + } // Validate the DatadogMonitor spec if err = datadoghqv1alpha1.IsValidDatadogMonitor(&instance.Spec); err != nil { diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index 0b4c560e53..fe495dbb52 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -74,8 +74,6 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } } - // Requeue until the object is properly deleted by Kubernetes - return ctrl.Result{Requeue: true}, nil } return ctrl.Result{}, nil } diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index 50bf4b4963..59df7504f8 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -74,7 +74,7 @@ func Test_HandleFinalizer(t *testing.T) { }, }, finalizerShouldExists: false, - expectedResult: ctrl.Result{Requeue: true}, + expectedResult: ctrl.Result{}, deleterFunc: func(ctx context.Context, k8sObj client.Object, datadogID string) error { return nil }, From a13aca18014aa45aada984841933433ca422a9ad Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Mon, 13 Apr 2026 09:19:29 -0400 Subject: [PATCH 3/9] Fix HandleFinalizer deletion-path return and remove redundant caller guards HandleFinalizer was missing a return on the deletion path, causing reconciliation to continue into create/update logic. Remove the compensating GetDeletionTimestamp checks from all 5 callers, fix SLO deleteResource closure capture, and improve finalizer test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../datadogagent/controller_reconcile_v2.go | 3 - .../controller_reconcile_v2.go | 3 - .../controller/datadogdashboard/controller.go | 3 - .../datadoggenericresource/controller.go | 3 - .../controller/datadogmonitor/controller.go | 3 - internal/controller/datadogslo/controller.go | 6 +- internal/controller/finalizer/finalizer.go | 8 ++ .../controller/finalizer/finalizer_test.go | 89 ++++++++++++++----- 8 files changed, 78 insertions(+), 40 deletions(-) diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index a57d144db6..c2bf65371f 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -45,9 +45,6 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } - if !instance.GetDeletionTimestamp().IsZero() { - return reconcile.Result{Requeue: true}, nil - } // 3. Set default values for GlobalConfig and Features instanceCopy := instance.DeepCopy() diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 6af579ea20..3d28ef4844 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -40,9 +40,6 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } - if !instance.GetDeletionTimestamp().IsZero() { - return reconcile.Result{Requeue: true}, nil - } // 3. Set default values for GlobalConfig and Features instanceCopy := instance.DeepCopy() diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index b5acdcc0d3..79c11f4d4c 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -116,9 +116,6 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogDashboardFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } - if !instance.GetDeletionTimestamp().IsZero() { - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil - } status := instance.Status.DeepCopy() statusSpecHash := instance.Status.CurrentHash diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index a6c8805392..1f16e9e8ac 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -110,9 +110,6 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques if result, err = final.HandleFinalizer(ctx, instance, instance.Status.Id, datadogGenericResourceFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } - if !instance.GetDeletionTimestamp().IsZero() { - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil - } status := instance.Status.DeepCopy() statusSpecHash := instance.Status.CurrentHash diff --git a/internal/controller/datadogmonitor/controller.go b/internal/controller/datadogmonitor/controller.go index fa06d24b79..6b6f9727b7 100644 --- a/internal/controller/datadogmonitor/controller.go +++ b/internal/controller/datadogmonitor/controller.go @@ -144,9 +144,6 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv if result, err = final.HandleFinalizer(ctx, instance, fmt.Sprint(instance.Status.ID), datadogMonitorFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } - if !instance.GetDeletionTimestamp().IsZero() { - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil - } // Validate the DatadogMonitor spec if err = datadoghqv1alpha1.IsValidDatadogMonitor(&instance.Spec); err != nil { diff --git a/internal/controller/datadogslo/controller.go b/internal/controller/datadogslo/controller.go index 43836bca31..8c28d3ce22 100644 --- a/internal/controller/datadogslo/controller.go +++ b/internal/controller/datadogslo/controller.go @@ -119,7 +119,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques final := finalizer.NewFinalizer( logger, r.client, - r.deleteResource(logger, instance), + r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod, ) @@ -317,7 +317,7 @@ func (r *Reconciler) update(logger logr.Logger, instance *v1alpha1.DatadogSLO, s return nil } -func (r *Reconciler) deleteResource(logger logr.Logger, instance *v1alpha1.DatadogSLO) finalizer.ResourceDeleteFunc { +func (r *Reconciler) deleteResource(logger logr.Logger) finalizer.ResourceDeleteFunc { return func(ctx context.Context, k8sObj client.Object, datadogID string) error { if datadogID != "" { kind := k8sObj.GetObjectKind().GroupVersionKind().Kind @@ -330,7 +330,7 @@ func (r *Reconciler) deleteResource(logger logr.Logger, instance *v1alpha1.Datad logger.Info("Successfully deleted object", "kind", kind, "ID", datadogID) } } - r.recordEvent(instance, buildEventInfo(k8sObj.GetName(), k8sObj.GetNamespace(), datadog.DeletionEvent)) + r.recordEvent(k8sObj, buildEventInfo(k8sObj.GetName(), k8sObj.GetNamespace(), datadog.DeletionEvent)) return nil } } diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index fe495dbb52..861b8b4851 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -15,6 +15,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) +// ResourceDeleteFunc performs controller-specific cleanup when a resource is being deleted. +// If it returns an error, the finalizer is NOT removed — the object stays in a terminating +// state and the controller retries on the next reconcile. If it returns nil, the finalizer +// is removed and Kubernetes proceeds with deletion. type ResourceDeleteFunc func(ctx context.Context, k8sObj client.Object, datadogID string) error type Finalizer struct { @@ -74,6 +78,10 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } } + // Requeue while the object is still being deleted. Use defaultRequeuePeriod + // to avoid hot-looping on clusters with many terminating objects. Controllers + // that pass 0 get an immediate requeue (Requeue: true with zero RequeueAfter). + return ctrl.Result{Requeue: true, RequeueAfter: f.defaultRequeuePeriod}, nil } return ctrl.Result{}, nil } diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index 59df7504f8..a69f9a57b1 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -7,6 +7,10 @@ package finalizer import ( "context" + "fmt" + "testing" + "time" + datadoghqv1alpha1 "github.com/DataDog/datadog-operator/api/datadoghq/v1alpha1" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -17,8 +21,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" - "testing" - "time" ) type testResource struct { @@ -36,6 +38,15 @@ func Test_HandleFinalizer(t *testing.T) { s.AddKnownTypes(datadoghqv1alpha1.GroupVersion, &testResource{}) finalizerName := "test_resource.finalizer" metaNow := metav1.NewTime(time.Now()) + requeuePeriod := time.Minute + errRequeuePeriod := time.Minute + + noopDelete := func(ctx context.Context, k8sObj client.Object, datadogID string) error { + return nil + } + failDelete := func(ctx context.Context, k8sObj client.Object, datadogID string) error { + return fmt.Errorf("API error") + } tests := []struct { name string @@ -46,7 +57,7 @@ func Test_HandleFinalizer(t *testing.T) { deleterFunc ResourceDeleteFunc }{ { - name: "check if object deletion timestamp is empty add finalizer if not exists", + name: "not deleting, no finalizer: adds finalizer", clientObject: testResource{ TypeMeta: metav1.TypeMeta{ Kind: "TestResource", @@ -57,12 +68,25 @@ func Test_HandleFinalizer(t *testing.T) { }, finalizerShouldExists: true, expectedResult: ctrl.Result{}, - deleterFunc: func(ctx context.Context, k8sObj client.Object, datadogID string) error { - return nil + deleterFunc: noopDelete, + }, + { + name: "not deleting, already has finalizer: no-op, proceed to reconciliation", + clientObject: testResource{ + TypeMeta: metav1.TypeMeta{ + Kind: "TestResource", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test resource", + Finalizers: []string{finalizerName}, + }, }, + finalizerShouldExists: true, + expectedResult: ctrl.Result{}, + deleterFunc: noopDelete, }, { - name: "remove finalizer when deletion timestamp is set and finalizer is exits", + name: "deleting, has finalizer, deleteFunc succeeds: removes finalizer, requeues with period", clientObject: testResource{ TypeMeta: metav1.TypeMeta{ Kind: "TestResource", @@ -74,28 +98,49 @@ func Test_HandleFinalizer(t *testing.T) { }, }, finalizerShouldExists: false, - expectedResult: ctrl.Result{}, - deleterFunc: func(ctx context.Context, k8sObj client.Object, datadogID string) error { - return nil + expectedResult: ctrl.Result{Requeue: true, RequeueAfter: requeuePeriod}, + deleterFunc: noopDelete, + }, + { + name: "deleting, has finalizer, deleteFunc fails: keeps finalizer, requeues with error period", + clientObject: testResource{ + TypeMeta: metav1.TypeMeta{ + Kind: "TestResource", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test resource", + DeletionTimestamp: &metaNow, + Finalizers: []string{finalizerName}, + }, }, + finalizerShouldExists: true, + expectedResult: ctrl.Result{Requeue: true, RequeueAfter: errRequeuePeriod}, + expectedErr: true, + deleterFunc: failDelete, }, + // Note: "deleting, no finalizer" is not testable because the fake client + // (and the real API server) reject objects with a deletionTimestamp but no + // finalizers — Kubernetes would have already garbage-collected such objects. } for _, tt := range tests { - // arrange - fakeClient := fake.NewClientBuilder().WithObjects(&tt.clientObject).Build() - finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, time.Minute, time.Minute) + t.Run(tt.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithObjects(&tt.clientObject).Build() + finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, requeuePeriod, errRequeuePeriod) - // act - res, err := finalizer.HandleFinalizer(context.TODO(), &tt.clientObject, "123", finalizerName) + res, err := finalizer.HandleFinalizer(context.TODO(), &tt.clientObject, "123", finalizerName) - // assert - assert.NoError(t, err) - assert.Equal(t, tt.expectedResult, res) - if tt.finalizerShouldExists { - assert.True(t, controllerutil.ContainsFinalizer(&tt.clientObject, finalizerName)) - } else { - assert.False(t, controllerutil.ContainsFinalizer(&tt.clientObject, finalizerName)) - } + if tt.expectedErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.expectedResult, res) + if tt.finalizerShouldExists { + assert.True(t, controllerutil.ContainsFinalizer(&tt.clientObject, finalizerName)) + } else { + assert.False(t, controllerutil.ContainsFinalizer(&tt.clientObject, finalizerName)) + } + }) } } From 81c3dd852af6f6fe78bfb7bb49c4d583f98f528f Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Thu, 16 Apr 2026 08:44:24 -0400 Subject: [PATCH 4/9] added finalizer and error requeue periods --- internal/controller/datadogagent/controller.go | 4 +++- internal/controller/datadogagent/controller_reconcile_v2.go | 2 +- internal/controller/datadogagentinternal/controller.go | 4 +++- .../datadogagentinternal/controller_reconcile_v2.go | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/controller/datadogagent/controller.go b/internal/controller/datadogagent/controller.go index 57d728a7d8..e9728bfa0a 100644 --- a/internal/controller/datadogagent/controller.go +++ b/internal/controller/datadogagent/controller.go @@ -64,7 +64,9 @@ import ( ) const ( - defaultRequeuePeriod = 15 * time.Second + defaultRequeuePeriod = 15 * time.Second + defaultFinalizerRequeuePeriod = 60 * time.Second + defaultErrRequeuePeriod = 5 * time.Second ) // ReconcilerOptions provides options read from command line diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index c2bf65371f..15f9ce7821 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -41,7 +41,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), 0, 0) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultFinalizerRequeuePeriod, defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogagentinternal/controller.go b/internal/controller/datadogagentinternal/controller.go index fc6c04206c..8170e1b329 100644 --- a/internal/controller/datadogagentinternal/controller.go +++ b/internal/controller/datadogagentinternal/controller.go @@ -58,7 +58,9 @@ import ( ) const ( - defaultRequeuePeriod = 15 * time.Second + defaultRequeuePeriod = 15 * time.Second + defaultFinalizerRequeuePeriod = 60 * time.Second + defaultErrRequeuePeriod = 5 * time.Second ) // ReconcilerOptions provides options read from command line diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 3d28ef4844..ce71753e10 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -36,7 +36,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 // } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), 0, 0) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), defaultFinalizerRequeuePeriod, defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } From 835d9937c89aa6487d99d76cd02b2d3eb3fecdc7 Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Thu, 16 Apr 2026 10:44:58 -0400 Subject: [PATCH 5/9] fix: requeue after adding finalizer to match original controller behavior HandleFinalizer was falling through after adding a finalizer instead of requeuing, causing controllers to run create/update logic on the same reconcile pass. This broke the datadogmonitor unsupported-type and datadogslo create-failure test cases. Updated SLO tests to reconcile twice (once for finalizer, once for actual logic). --- internal/controller/datadogslo/controller_test.go | 13 ++++++++++++- internal/controller/finalizer/finalizer.go | 3 +++ internal/controller/finalizer/finalizer_test.go | 4 ++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/controller/datadogslo/controller_test.go b/internal/controller/datadogslo/controller_test.go index e0bd91fc0d..962d410d27 100644 --- a/internal/controller/datadogslo/controller_test.go +++ b/internal/controller/datadogslo/controller_test.go @@ -53,6 +53,7 @@ func TestReconciler_Reconcile(t *testing.T) { name string request ctrl.Request expectedResult ctrl.Result + reconcileCount int // number of reconcile loops to run; defaults to 1 mockOn func(t *testing.T, m *mockedFields) datadogClientHandler http.HandlerFunc }{ @@ -64,6 +65,7 @@ func TestReconciler_Reconcile(t *testing.T) { Name: resourceName, }, }, + reconcileCount: 2, mockOn: func(t *testing.T, m *mockedFields) { _ = m.k8sClient.Create(context.TODO(), defaultSLO()) }, @@ -93,6 +95,7 @@ func TestReconciler_Reconcile(t *testing.T) { Name: resourceName, }, }, + reconcileCount: 2, mockOn: func(t *testing.T, m *mockedFields) { _ = m.k8sClient.Create(context.TODO(), defaultSLO()) }, @@ -109,6 +112,7 @@ func TestReconciler_Reconcile(t *testing.T) { Name: resourceName, }, }, + reconcileCount: 2, mockOn: func(t *testing.T, m *mockedFields) { slo := defaultSLO() slo.Status.ID = "SLO123" @@ -149,7 +153,14 @@ func TestReconciler_Reconcile(t *testing.T) { log: testLogger, } - res, _ := r.Reconcile(ctx, tt.request) + reconcileCount := tt.reconcileCount + if reconcileCount == 0 { + reconcileCount = 1 + } + var res ctrl.Result + for i := 0; i < reconcileCount; i++ { + res, _ = r.Reconcile(ctx, tt.request) + } assert.Equal(t, tt.expectedResult, res) }) } diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index 861b8b4851..848a6cc810 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -62,6 +62,9 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object if err != nil { return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } + // Requeue after adding the finalizer so the next reconcile works + // with the updated object from the API server. + return ctrl.Result{RequeueAfter: f.defaultRequeuePeriod}, nil } } else { f.logger.Info("Object being deleted", "kind", clientObj.GetObjectKind(), "finalizername", finalizerName) diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index a69f9a57b1..c7593fcdba 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -57,7 +57,7 @@ func Test_HandleFinalizer(t *testing.T) { deleterFunc ResourceDeleteFunc }{ { - name: "not deleting, no finalizer: adds finalizer", + name: "not deleting, no finalizer: adds finalizer and requeues", clientObject: testResource{ TypeMeta: metav1.TypeMeta{ Kind: "TestResource", @@ -67,7 +67,7 @@ func Test_HandleFinalizer(t *testing.T) { }, }, finalizerShouldExists: true, - expectedResult: ctrl.Result{}, + expectedResult: ctrl.Result{RequeueAfter: requeuePeriod}, deleterFunc: noopDelete, }, { From 4a258a83434e253bd7cf28ca44e4dc6f41942abb Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Thu, 16 Apr 2026 12:38:43 -0400 Subject: [PATCH 6/9] fix: use immediate requeue after adding finalizer for controllers that had Requeue:true The old datadogagent, datadogagentinternal, datadogdashboard, and datadoggenericresource controllers all used Requeue:true (immediate) after adding a finalizer, while datadogmonitor/datadogslo used RequeueAfter with a period. The shared HandleFinalizer now supports both: pass 0 for immediate requeue, or a duration for delayed requeue. This fixes the 34 integration test timeouts where the 60s requeue delay exceeded the 10s test timeout. --- .../controller/datadogagent/controller.go | 5 ++-- .../datadogagent/controller_reconcile_v2.go | 2 +- .../datadogagentinternal/controller.go | 5 ++-- .../controller_reconcile_v2.go | 2 +- .../controller/datadogdashboard/controller.go | 2 +- .../datadogdashboard/controller_test.go | 2 +- .../datadoggenericresource/controller.go | 2 +- .../datadoggenericresource/controller_test.go | 2 +- internal/controller/finalizer/finalizer.go | 6 +++- .../controller/finalizer/finalizer_test.go | 28 +++++++++++++++++-- 10 files changed, 41 insertions(+), 15 deletions(-) diff --git a/internal/controller/datadogagent/controller.go b/internal/controller/datadogagent/controller.go index e9728bfa0a..943985cf13 100644 --- a/internal/controller/datadogagent/controller.go +++ b/internal/controller/datadogagent/controller.go @@ -64,9 +64,8 @@ import ( ) const ( - defaultRequeuePeriod = 15 * time.Second - defaultFinalizerRequeuePeriod = 60 * time.Second - defaultErrRequeuePeriod = 5 * time.Second + defaultRequeuePeriod = 15 * time.Second + defaultErrRequeuePeriod = 5 * time.Second ) // ReconcilerOptions provides options read from command line diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index 1bc61305e2..a3933e8867 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -41,7 +41,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultFinalizerRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), 0, defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogagentinternal/controller.go b/internal/controller/datadogagentinternal/controller.go index 8170e1b329..25aebff4d4 100644 --- a/internal/controller/datadogagentinternal/controller.go +++ b/internal/controller/datadogagentinternal/controller.go @@ -58,9 +58,8 @@ import ( ) const ( - defaultRequeuePeriod = 15 * time.Second - defaultFinalizerRequeuePeriod = 60 * time.Second - defaultErrRequeuePeriod = 5 * time.Second + defaultRequeuePeriod = 15 * time.Second + defaultErrRequeuePeriod = 5 * time.Second ) // ReconcilerOptions provides options read from command line diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index c8e0eaea27..908dcc1456 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -36,7 +36,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 // } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), defaultFinalizerRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), 0, defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index 0ff6680593..b7bf59ed3e 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -112,7 +112,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), 0, defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogDashboardFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogdashboard/controller_test.go b/internal/controller/datadogdashboard/controller_test.go index 7bb535ca99..f5f6d6520a 100644 --- a/internal/controller/datadogdashboard/controller_test.go +++ b/internal/controller/datadogdashboard/controller_test.go @@ -71,7 +71,7 @@ func TestReconcileDatadogDashboard_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), genericDatadogDashboard()) }, }, - wantResult: reconcile.Result{RequeueAfter: defaultRequeuePeriod}, + wantResult: reconcile.Result{Requeue: true}, wantFunc: func(c client.Client) error { db := &datadoghqv1alpha1.DatadogDashboard{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, db); err != nil { diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index 2308f633e2..82b7d4ff90 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -106,7 +106,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), 0, defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.Id, datadogGenericResourceFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadoggenericresource/controller_test.go b/internal/controller/datadoggenericresource/controller_test.go index 5138a04d58..1acd84aa4f 100644 --- a/internal/controller/datadoggenericresource/controller_test.go +++ b/internal/controller/datadoggenericresource/controller_test.go @@ -72,7 +72,7 @@ func TestReconcileGenericResource_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), mockGenericResource()) }, }, - wantResult: reconcile.Result{RequeueAfter: defaultRequeuePeriod}, + wantResult: reconcile.Result{Requeue: true}, wantFunc: func(c client.Client) error { obj := &datadoghqv1alpha1.DatadogGenericResource{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, obj); err != nil { diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index 848a6cc810..3b3ca3eaaa 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -63,7 +63,11 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } // Requeue after adding the finalizer so the next reconcile works - // with the updated object from the API server. + // with the updated object from the API server. Controllers that + // pass 0 get an immediate requeue (Requeue: true). + if f.defaultRequeuePeriod == 0 { + return ctrl.Result{Requeue: true}, nil + } return ctrl.Result{RequeueAfter: f.defaultRequeuePeriod}, nil } } else { diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index c7593fcdba..f061df57a2 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -55,9 +55,10 @@ func Test_HandleFinalizer(t *testing.T) { expectedResult ctrl.Result expectedErr bool deleterFunc ResourceDeleteFunc + requeuePeriodOverride *time.Duration // if set, overrides requeuePeriod }{ { - name: "not deleting, no finalizer: adds finalizer and requeues", + name: "not deleting, no finalizer: adds finalizer and requeues with period", clientObject: testResource{ TypeMeta: metav1.TypeMeta{ Kind: "TestResource", @@ -70,6 +71,21 @@ func Test_HandleFinalizer(t *testing.T) { expectedResult: ctrl.Result{RequeueAfter: requeuePeriod}, deleterFunc: noopDelete, }, + { + name: "not deleting, no finalizer, zero period: adds finalizer and requeues immediately", + clientObject: testResource{ + TypeMeta: metav1.TypeMeta{ + Kind: "TestResource", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test resource 2", + }, + }, + finalizerShouldExists: true, + expectedResult: ctrl.Result{Requeue: true}, + deleterFunc: noopDelete, + requeuePeriodOverride: durationPtr(0), + }, { name: "not deleting, already has finalizer: no-op, proceed to reconciliation", clientObject: testResource{ @@ -126,7 +142,11 @@ func Test_HandleFinalizer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fakeClient := fake.NewClientBuilder().WithObjects(&tt.clientObject).Build() - finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, requeuePeriod, errRequeuePeriod) + rp := requeuePeriod + if tt.requeuePeriodOverride != nil { + rp = *tt.requeuePeriodOverride + } + finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, rp, errRequeuePeriod) res, err := finalizer.HandleFinalizer(context.TODO(), &tt.clientObject, "123", finalizerName) @@ -144,3 +164,7 @@ func Test_HandleFinalizer(t *testing.T) { }) } } + +func durationPtr(d time.Duration) *time.Duration { + return &d +} From eb302d2ae64bfd1d41129b0c6a8c0e32a31bb1aa Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Thu, 16 Apr 2026 17:25:41 -0400 Subject: [PATCH 7/9] fix: use defaultErrRequeuePeriod for all controllers after adding finalizer All migrated controllers now pass defaultErrRequeuePeriod (5s) as the requeue period when adding a finalizer, consistent with the existing datadogslo pattern. Removes the Requeue:true special case for zero period from HandleFinalizer. --- .../datadogagent/controller_reconcile_v2.go | 2 +- .../controller_reconcile_v2.go | 2 +- .../controller/datadogdashboard/controller.go | 2 +- .../datadogdashboard/controller_test.go | 2 +- .../datadoggenericresource/controller.go | 2 +- .../datadoggenericresource/controller_test.go | 2 +- internal/controller/finalizer/finalizer.go | 6 +---- .../controller/finalizer/finalizer_test.go | 26 +------------------ 8 files changed, 8 insertions(+), 36 deletions(-) diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index a3933e8867..45e8ed84aa 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -41,7 +41,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), 0, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultErrRequeuePeriod, defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 908dcc1456..227668e308 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -36,7 +36,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 // } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), 0, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), defaultErrRequeuePeriod, defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index b7bf59ed3e..a192bc0d18 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -112,7 +112,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), 0, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod, defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogDashboardFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadogdashboard/controller_test.go b/internal/controller/datadogdashboard/controller_test.go index f5f6d6520a..873db2423d 100644 --- a/internal/controller/datadogdashboard/controller_test.go +++ b/internal/controller/datadogdashboard/controller_test.go @@ -71,7 +71,7 @@ func TestReconcileDatadogDashboard_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), genericDatadogDashboard()) }, }, - wantResult: reconcile.Result{Requeue: true}, + wantResult: reconcile.Result{RequeueAfter: defaultErrRequeuePeriod}, wantFunc: func(c client.Client) error { db := &datadoghqv1alpha1.DatadogDashboard{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, db); err != nil { diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index f8cf60760e..cf0c59543d 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -109,7 +109,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), 0, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod, defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.Id, datadogGenericResourceFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } diff --git a/internal/controller/datadoggenericresource/controller_test.go b/internal/controller/datadoggenericresource/controller_test.go index 1acd84aa4f..1d07dd2411 100644 --- a/internal/controller/datadoggenericresource/controller_test.go +++ b/internal/controller/datadoggenericresource/controller_test.go @@ -72,7 +72,7 @@ func TestReconcileGenericResource_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), mockGenericResource()) }, }, - wantResult: reconcile.Result{Requeue: true}, + wantResult: reconcile.Result{RequeueAfter: defaultErrRequeuePeriod}, wantFunc: func(c client.Client) error { obj := &datadoghqv1alpha1.DatadogGenericResource{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, obj); err != nil { diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index 3b3ca3eaaa..848a6cc810 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -63,11 +63,7 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } // Requeue after adding the finalizer so the next reconcile works - // with the updated object from the API server. Controllers that - // pass 0 get an immediate requeue (Requeue: true). - if f.defaultRequeuePeriod == 0 { - return ctrl.Result{Requeue: true}, nil - } + // with the updated object from the API server. return ctrl.Result{RequeueAfter: f.defaultRequeuePeriod}, nil } } else { diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index f061df57a2..0224a36970 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -55,7 +55,6 @@ func Test_HandleFinalizer(t *testing.T) { expectedResult ctrl.Result expectedErr bool deleterFunc ResourceDeleteFunc - requeuePeriodOverride *time.Duration // if set, overrides requeuePeriod }{ { name: "not deleting, no finalizer: adds finalizer and requeues with period", @@ -71,21 +70,6 @@ func Test_HandleFinalizer(t *testing.T) { expectedResult: ctrl.Result{RequeueAfter: requeuePeriod}, deleterFunc: noopDelete, }, - { - name: "not deleting, no finalizer, zero period: adds finalizer and requeues immediately", - clientObject: testResource{ - TypeMeta: metav1.TypeMeta{ - Kind: "TestResource", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "test resource 2", - }, - }, - finalizerShouldExists: true, - expectedResult: ctrl.Result{Requeue: true}, - deleterFunc: noopDelete, - requeuePeriodOverride: durationPtr(0), - }, { name: "not deleting, already has finalizer: no-op, proceed to reconciliation", clientObject: testResource{ @@ -142,11 +126,7 @@ func Test_HandleFinalizer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fakeClient := fake.NewClientBuilder().WithObjects(&tt.clientObject).Build() - rp := requeuePeriod - if tt.requeuePeriodOverride != nil { - rp = *tt.requeuePeriodOverride - } - finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, rp, errRequeuePeriod) + finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, requeuePeriod, errRequeuePeriod) res, err := finalizer.HandleFinalizer(context.TODO(), &tt.clientObject, "123", finalizerName) @@ -164,7 +144,3 @@ func Test_HandleFinalizer(t *testing.T) { }) } } - -func durationPtr(d time.Duration) *time.Duration { - return &d -} From 80419c39b7f50aefce3ce08ac5722df80dde721d Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Thu, 16 Apr 2026 20:18:13 -0400 Subject: [PATCH 8/9] refactor: simplify HandleFinalizer to use immediate requeue, revert unrelated changes - Remove defaultRequeuePeriod param from NewFinalizer; HandleFinalizer now always uses immediate requeue (Requeue: true) after adding a finalizer or while waiting for deletion, matching the original behavior of most controllers. - Revert result.IsZero() cleanup and checkRequiredTags signature refactor as they are out of scope for this ticket. --- .../datadogagent/component_reconciler.go | 2 +- .../datadogagent/controller_reconcile_v2.go | 6 ++--- .../component_reconciler.go | 2 +- .../controller_reconcile_v2.go | 4 +-- .../datadogcsidriver/controller_test.go | 4 +-- .../controller/datadogdashboard/controller.go | 4 +-- .../datadogdashboard/controller_test.go | 2 +- .../datadogdashboard/finalizer_test.go | 2 +- .../datadoggenericresource/controller.go | 4 +-- .../datadoggenericresource/controller_test.go | 2 +- .../datadoggenericresource/finalizer_test.go | 2 +- .../controller/datadogmonitor/controller.go | 27 +++++++------------ .../datadogmonitor/controller_test.go | 5 ++-- .../datadogmonitor/finalizer_test.go | 2 +- internal/controller/datadogslo/controller.go | 3 +-- internal/controller/finalizer/finalizer.go | 15 ++++------- .../controller/finalizer/finalizer_test.go | 11 ++++---- pkg/controller/utils/shared_utils.go | 2 +- 18 files changed, 41 insertions(+), 58 deletions(-) diff --git a/internal/controller/datadogagent/component_reconciler.go b/internal/controller/datadogagent/component_reconciler.go index fce31e6a37..20efaf2201 100644 --- a/internal/controller/datadogagent/component_reconciler.go +++ b/internal/controller/datadogagent/component_reconciler.go @@ -165,7 +165,7 @@ func (r *ComponentRegistry) ReconcileComponents(ctx context.Context, params *Rec } // Merge result (preserve requeue settings) - if !res.IsZero() { + if res.Requeue || res.RequeueAfter > 0 { result = res } } diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index 45e8ed84aa..154fc3e8cd 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -41,7 +41,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *datadogh } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultErrRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", datadogAgentFinalizer); utils.ShouldReturn(result, err) { return result, err } @@ -243,7 +243,7 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger } // Always requeue - if result.IsZero() { + if !result.Requeue && result.RequeueAfter == 0 { result.RequeueAfter = defaultRequeuePeriod } return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err, now) @@ -353,7 +353,7 @@ func (r *Reconciler) profilesToApply(ctx context.Context, logger logr.Logger, no oldStatus := profile.Status profileAppliedByNode, err = agentprofile.ApplyProfile(logger, &profile, nodeList, profileAppliedByNode, now, maxUnavailable, r.options.DatadogAgentInternalEnabled) if result, e := r.updateDAPStatus(ctx, logger, &profile, &oldStatus); utils.ShouldReturn(result, e) { - logger.Info("unable to update DatadogAgentProfile status", "error", e, "requeueAfter", result.RequeueAfter, "requeueIntent", !result.IsZero()) + logger.Info("unable to update DatadogAgentProfile status", "error", e, "requeue", result.Requeue, "requeueAfter", result.RequeueAfter) } if err != nil { // profile is invalid or conflicts diff --git a/internal/controller/datadogagentinternal/component_reconciler.go b/internal/controller/datadogagentinternal/component_reconciler.go index af500326e0..d91e6b5cca 100644 --- a/internal/controller/datadogagentinternal/component_reconciler.go +++ b/internal/controller/datadogagentinternal/component_reconciler.go @@ -166,7 +166,7 @@ func (r *ComponentRegistry) ReconcileComponents(ctx context.Context, params *Rec } // Merge result (preserve requeue settings) - if !res.IsZero() { + if res.Requeue || res.RequeueAfter > 0 { result = res } } diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 227668e308..878cf786fb 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -36,7 +36,7 @@ func (r *Reconciler) internalReconcileV2(ctx context.Context, instance *v1alpha1 // } // 2. Handle finalizer logic. - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), defaultErrRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(), defaultErrRequeuePeriod) if result, err := final.HandleFinalizer(ctx, instance, "", constants.DatadogAgentInternalFinalizer); utils.ShouldReturn(result, err) { return result, err } @@ -120,7 +120,7 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, instance *v1alpha1 } // Always requeue - if result.IsZero() { + if !result.Requeue && result.RequeueAfter == 0 { result.RequeueAfter = defaultRequeuePeriod } return r.updateStatusIfNeededV2(ctx, instance, newStatus, result, err, now) diff --git a/internal/controller/datadogcsidriver/controller_test.go b/internal/controller/datadogcsidriver/controller_test.go index 456f7cbd63..039300cab8 100644 --- a/internal/controller/datadogcsidriver/controller_test.go +++ b/internal/controller/datadogcsidriver/controller_test.go @@ -79,7 +79,7 @@ func TestReconcile_CreatesResources(t *testing.T) { // First reconcile: adds finalizer result, err := r.Reconcile(ctx, instance) require.NoError(t, err) - assert.True(t, result.IsZero()) + assert.False(t, result.Requeue) // Re-fetch the instance (finalizer was added) err = c.Get(ctx, types.NamespacedName{Name: testName, Namespace: testNamespace}, instance) @@ -89,7 +89,7 @@ func TestReconcile_CreatesResources(t *testing.T) { // Second reconcile: creates CSIDriver + DaemonSet result, err = r.Reconcile(ctx, instance) require.NoError(t, err) - assert.True(t, result.IsZero()) + assert.False(t, result.Requeue) // Verify CSIDriver was created csiDriver := &storagev1.CSIDriver{} diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index a192bc0d18..42f381504d 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -112,7 +112,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogDashboardFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } @@ -184,7 +184,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques } // If reconcile was successful and uneventful, requeue with period defaultRequeuePeriod - if result.IsZero() { + if !result.Requeue && result.RequeueAfter == 0 { result.RequeueAfter = defaultRequeuePeriod } diff --git a/internal/controller/datadogdashboard/controller_test.go b/internal/controller/datadogdashboard/controller_test.go index 873db2423d..f5f6d6520a 100644 --- a/internal/controller/datadogdashboard/controller_test.go +++ b/internal/controller/datadogdashboard/controller_test.go @@ -71,7 +71,7 @@ func TestReconcileDatadogDashboard_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), genericDatadogDashboard()) }, }, - wantResult: reconcile.Result{RequeueAfter: defaultErrRequeuePeriod}, + wantResult: reconcile.Result{Requeue: true}, wantFunc: func(c client.Client) error { db := &datadoghqv1alpha1.DatadogDashboard{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, db); err != nil { diff --git a/internal/controller/datadogdashboard/finalizer_test.go b/internal/controller/datadogdashboard/finalizer_test.go index dd8c8fcf85..e962689493 100644 --- a/internal/controller/datadogdashboard/finalizer_test.go +++ b/internal/controller/datadogdashboard/finalizer_test.go @@ -59,7 +59,7 @@ func Test_handleFinalizer(t *testing.T) { t.Run(test.name, func(t *testing.T) { reqLogger := testLogger.WithValues("test:", test.name) _ = r.client.Create(context.TODO(), test.db) - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultErrRequeuePeriod) _, err := final.HandleFinalizer(context.TODO(), test.db, test.db.Status.ID, datadogDashboardFinalizer) assert.NoError(t, err) if test.finalizerShouldExist { diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index cf0c59543d..552cda6d4d 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -109,7 +109,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques return ctrl.Result{}, err } - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.Id, datadogGenericResourceFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } @@ -171,7 +171,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques } // If reconcile was successful and uneventful, requeue with period defaultRequeuePeriod - if result.IsZero() { + if !result.Requeue && result.RequeueAfter == 0 { result.RequeueAfter = defaultRequeuePeriod } diff --git a/internal/controller/datadoggenericresource/controller_test.go b/internal/controller/datadoggenericresource/controller_test.go index 1d07dd2411..1acd84aa4f 100644 --- a/internal/controller/datadoggenericresource/controller_test.go +++ b/internal/controller/datadoggenericresource/controller_test.go @@ -72,7 +72,7 @@ func TestReconcileGenericResource_Reconcile(t *testing.T) { _ = c.Create(context.TODO(), mockGenericResource()) }, }, - wantResult: reconcile.Result{RequeueAfter: defaultErrRequeuePeriod}, + wantResult: reconcile.Result{Requeue: true}, wantFunc: func(c client.Client) error { obj := &datadoghqv1alpha1.DatadogGenericResource{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, obj); err != nil { diff --git a/internal/controller/datadoggenericresource/finalizer_test.go b/internal/controller/datadoggenericresource/finalizer_test.go index 26b4259642..a388898a05 100644 --- a/internal/controller/datadoggenericresource/finalizer_test.go +++ b/internal/controller/datadoggenericresource/finalizer_test.go @@ -101,7 +101,7 @@ func Test_handleFinalizer(t *testing.T) { testGcr := &datadoghqv1alpha1.DatadogGenericResource{} err := r.client.Get(context.TODO(), client.ObjectKey{Name: test.resourceName, Namespace: testNamespace}, testGcr) - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultErrRequeuePeriod) _, err = final.HandleFinalizer(context.TODO(), testGcr, testGcr.Status.Id, datadogGenericResourceFinalizer) assert.NoError(t, err) diff --git a/internal/controller/datadogmonitor/controller.go b/internal/controller/datadogmonitor/controller.go index c99b3f3344..d3362308d9 100644 --- a/internal/controller/datadogmonitor/controller.go +++ b/internal/controller/datadogmonitor/controller.go @@ -140,7 +140,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv newStatus := instance.Status.DeepCopy() - final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(logger, r.client, r.deleteResource(logger), defaultErrRequeuePeriod) if result, err = final.HandleFinalizer(ctx, instance, fmt.Sprint(instance.Status.ID), datadogMonitorFinalizer); ctrutils.ShouldReturn(result, err) { return result, err } @@ -205,12 +205,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv logger.V(1).Info("Creating monitor in Datadog") // Make sure required tags are present if !apiutils.BoolValue(instance.Spec.ControllerOptions.DisableRequiredTags) { - var tagsUpdated bool - tagsUpdated, err = r.checkRequiredTags(logger, instance) - if err != nil { - result.RequeueAfter = defaultErrRequeuePeriod - return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) - } else if tagsUpdated { + if result, err = r.checkRequiredTags(logger, instance); err != nil || result.Requeue { return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) } } @@ -225,12 +220,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv logger.V(1).Info("Updating monitor in Datadog") // Make sure required tags are present if !apiutils.BoolValue(instance.Spec.ControllerOptions.DisableRequiredTags) { - var tagsUpdated bool - tagsUpdated, err = r.checkRequiredTags(logger, instance) - if err != nil { - result.RequeueAfter = defaultErrRequeuePeriod - return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) - } else if tagsUpdated { + if result, err = r.checkRequiredTags(logger, instance); err != nil || result.Requeue { return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) } } @@ -240,7 +230,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv } // If reconcile was successful, requeue with period defaultRequeuePeriod - if result.IsZero() { + if !result.Requeue && result.RequeueAfter == 0 { result.RequeueAfter = defaultRequeuePeriod } @@ -349,7 +339,7 @@ func (r *Reconciler) updateStatusIfNeeded(logger logr.Logger, datadogMonitor *da return result, nil } -func (r *Reconciler) checkRequiredTags(logger logr.Logger, datadogMonitor *datadoghqv1alpha1.DatadogMonitor) (bool, error) { +func (r *Reconciler) checkRequiredTags(logger logr.Logger, datadogMonitor *datadoghqv1alpha1.DatadogMonitor) (ctrl.Result, error) { tagsToAdd := []string{} var found bool tags := datadogMonitor.Spec.Tags @@ -367,14 +357,15 @@ func (r *Reconciler) checkRequiredTags(logger logr.Logger, datadogMonitor *datad if err != nil { logger.Error(err, "failed to update DatadogMonitor with required tags") - return false, err + return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err } logger.Info("Added required tags", "Monitor Namespace", datadogMonitor.Namespace, "Monitor Name", datadogMonitor.Name, "Monitor ID", datadogMonitor.Status.ID) - return true, nil + return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil } - return false, nil + // Proceed in reconcile loop without modifying result. + return ctrl.Result{}, nil } func getRequiredTags() []string { diff --git a/internal/controller/datadogmonitor/controller_test.go b/internal/controller/datadogmonitor/controller_test.go index bb405c1150..7feba401ae 100644 --- a/internal/controller/datadogmonitor/controller_test.go +++ b/internal/controller/datadogmonitor/controller_test.go @@ -76,7 +76,7 @@ func TestReconcileDatadogMonitor_Reconcile(t *testing.T) { args: args{ loadFunc: genericDatadogMonitor, }, - wantResult: reconcile.Result{RequeueAfter: defaultRequeuePeriod}, + wantResult: reconcile.Result{Requeue: true}, wantFunc: func(c client.Client) error { dm := &datadoghqv1alpha1.DatadogMonitor{} if err := c.Get(context.TODO(), types.NamespacedName{Name: resourcesName, Namespace: resourcesNamespace}, dm); err != nil { @@ -104,7 +104,7 @@ func TestReconcileDatadogMonitor_Reconcile(t *testing.T) { }, }, { - name: "DatadogMonitor exists, adds required tags before create", + name: "DatadogMonitor exists, check required tags", args: args{ request: newRequest(resourcesNamespace, resourcesName), loadFunc: genericDatadogMonitor, @@ -117,7 +117,6 @@ func TestReconcileDatadogMonitor_Reconcile(t *testing.T) { return err } assert.Contains(t, dm.Spec.Tags, "generated:kubernetes") - assert.False(t, dm.Status.Primary) return nil }, }, diff --git a/internal/controller/datadogmonitor/finalizer_test.go b/internal/controller/datadogmonitor/finalizer_test.go index e76fd055bd..cd20c55546 100644 --- a/internal/controller/datadogmonitor/finalizer_test.go +++ b/internal/controller/datadogmonitor/finalizer_test.go @@ -92,7 +92,7 @@ func Test_handleFinalizer(t *testing.T) { testMonitor := &datadoghqv1alpha1.DatadogMonitor{} _ = r.client.Get(context.TODO(), client.ObjectKey{Namespace: "foo", Name: test.objectName}, testMonitor) - final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultRequeuePeriod, defaultErrRequeuePeriod) + final := finalizer.NewFinalizer(reqLogger, r.client, r.deleteResource(reqLogger), defaultErrRequeuePeriod) _, err := final.HandleFinalizer(context.TODO(), testMonitor, fmt.Sprint(testMonitor.Status.ID), datadogMonitorFinalizer) assert.NoError(t, err) diff --git a/internal/controller/datadogslo/controller.go b/internal/controller/datadogslo/controller.go index 581342ceb4..e2608f71f9 100644 --- a/internal/controller/datadogslo/controller.go +++ b/internal/controller/datadogslo/controller.go @@ -120,7 +120,6 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques logger, r.client, r.deleteResource(logger), - defaultRequeuePeriod, defaultErrRequeuePeriod, ) if result, err = final.HandleFinalizer(ctx, instance, instance.Status.ID, datadogSLOFinalizer); ctrutils.ShouldReturn(result, err) { @@ -191,7 +190,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques } // If reconcile was successful and uneventful, requeue with period defaultRequeuePeriod - if result.IsZero() { + if !result.Requeue && result.RequeueAfter == 0 { result.RequeueAfter = defaultRequeuePeriod } diff --git a/internal/controller/finalizer/finalizer.go b/internal/controller/finalizer/finalizer.go index 848a6cc810..0c418040ea 100644 --- a/internal/controller/finalizer/finalizer.go +++ b/internal/controller/finalizer/finalizer.go @@ -26,7 +26,6 @@ type Finalizer struct { client client.Client deleteFunc ResourceDeleteFunc - defaultRequeuePeriod time.Duration defaultErrRequeuePeriod time.Duration } @@ -34,14 +33,12 @@ func NewFinalizer( logger logr.Logger, client client.Client, deleteFunc ResourceDeleteFunc, - defaultRequeuePeriod time.Duration, defaultErrRequeuePeriod time.Duration, ) *Finalizer { return &Finalizer{ logger: logger, client: client, deleteFunc: deleteFunc, - defaultRequeuePeriod: defaultRequeuePeriod, defaultErrRequeuePeriod: defaultErrRequeuePeriod, } } @@ -62,9 +59,9 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object if err != nil { return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } - // Requeue after adding the finalizer so the next reconcile works - // with the updated object from the API server. - return ctrl.Result{RequeueAfter: f.defaultRequeuePeriod}, nil + // Requeue immediately so the next reconcile works with the + // updated object from the API server. + return ctrl.Result{Requeue: true}, nil } } else { f.logger.Info("Object being deleted", "kind", clientObj.GetObjectKind(), "finalizername", finalizerName) @@ -81,10 +78,8 @@ func (f *Finalizer) HandleFinalizer(ctx context.Context, clientObj client.Object return ctrl.Result{Requeue: true, RequeueAfter: f.defaultErrRequeuePeriod}, err } } - // Requeue while the object is still being deleted. Use defaultRequeuePeriod - // to avoid hot-looping on clusters with many terminating objects. Controllers - // that pass 0 get an immediate requeue (Requeue: true with zero RequeueAfter). - return ctrl.Result{Requeue: true, RequeueAfter: f.defaultRequeuePeriod}, nil + // Requeue while the object is still being deleted. + return ctrl.Result{Requeue: true}, nil } return ctrl.Result{}, nil } diff --git a/internal/controller/finalizer/finalizer_test.go b/internal/controller/finalizer/finalizer_test.go index 0224a36970..f6bdc4c3b8 100644 --- a/internal/controller/finalizer/finalizer_test.go +++ b/internal/controller/finalizer/finalizer_test.go @@ -38,7 +38,6 @@ func Test_HandleFinalizer(t *testing.T) { s.AddKnownTypes(datadoghqv1alpha1.GroupVersion, &testResource{}) finalizerName := "test_resource.finalizer" metaNow := metav1.NewTime(time.Now()) - requeuePeriod := time.Minute errRequeuePeriod := time.Minute noopDelete := func(ctx context.Context, k8sObj client.Object, datadogID string) error { @@ -57,7 +56,7 @@ func Test_HandleFinalizer(t *testing.T) { deleterFunc ResourceDeleteFunc }{ { - name: "not deleting, no finalizer: adds finalizer and requeues with period", + name: "not deleting, no finalizer: adds finalizer and requeues immediately", clientObject: testResource{ TypeMeta: metav1.TypeMeta{ Kind: "TestResource", @@ -67,7 +66,7 @@ func Test_HandleFinalizer(t *testing.T) { }, }, finalizerShouldExists: true, - expectedResult: ctrl.Result{RequeueAfter: requeuePeriod}, + expectedResult: ctrl.Result{Requeue: true}, deleterFunc: noopDelete, }, { @@ -86,7 +85,7 @@ func Test_HandleFinalizer(t *testing.T) { deleterFunc: noopDelete, }, { - name: "deleting, has finalizer, deleteFunc succeeds: removes finalizer, requeues with period", + name: "deleting, has finalizer, deleteFunc succeeds: removes finalizer, requeues immediately", clientObject: testResource{ TypeMeta: metav1.TypeMeta{ Kind: "TestResource", @@ -98,7 +97,7 @@ func Test_HandleFinalizer(t *testing.T) { }, }, finalizerShouldExists: false, - expectedResult: ctrl.Result{Requeue: true, RequeueAfter: requeuePeriod}, + expectedResult: ctrl.Result{Requeue: true}, deleterFunc: noopDelete, }, { @@ -126,7 +125,7 @@ func Test_HandleFinalizer(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fakeClient := fake.NewClientBuilder().WithObjects(&tt.clientObject).Build() - finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, requeuePeriod, errRequeuePeriod) + finalizer := NewFinalizer(testLogger, fakeClient, tt.deleterFunc, errRequeuePeriod) res, err := finalizer.HandleFinalizer(context.TODO(), &tt.clientObject, "123", finalizerName) diff --git a/pkg/controller/utils/shared_utils.go b/pkg/controller/utils/shared_utils.go index 28250a2789..fe698de430 100644 --- a/pkg/controller/utils/shared_utils.go +++ b/pkg/controller/utils/shared_utils.go @@ -17,7 +17,7 @@ import ( // ShouldReturn returns if we should stop the reconcile loop based on result func ShouldReturn(result reconcile.Result, err error) bool { - return err != nil || !result.IsZero() + return err != nil || result.Requeue || result.RequeueAfter > 0 } // GetDatadogLeaderElectionResourceName return the nome of the Resource managing the leader election token info. From c8c832778a6e103da6f9e8d8a0e64650cdb47247 Mon Sep 17 00:00:00 2001 From: Nicole Chung Date: Thu, 16 Apr 2026 21:35:08 -0400 Subject: [PATCH 9/9] fix: restore SA1019 cleanup from #2478 that was inadvertently reverted The previous "revert unrelated changes" commit rolled main's post-#2478 state back to pre-#2478, undoing the deprecated-Requeue-field cleanup and the checkRequiredTags (bool, error) signature. Restore main's current state for those call sites so this PR is limited to the finalizer reorganization. --- .../datadogagent/component_reconciler.go | 2 +- .../datadogagent/controller_reconcile_v2.go | 4 +-- .../component_reconciler.go | 2 +- .../controller_reconcile_v2.go | 2 +- .../datadogcsidriver/controller_test.go | 4 +-- .../controller/datadogdashboard/controller.go | 2 +- .../datadoggenericresource/controller.go | 2 +- .../controller/datadogmonitor/controller.go | 25 +++++++++++++------ internal/controller/datadogslo/controller.go | 2 +- pkg/controller/utils/shared_utils.go | 2 +- 10 files changed, 28 insertions(+), 19 deletions(-) diff --git a/internal/controller/datadogagent/component_reconciler.go b/internal/controller/datadogagent/component_reconciler.go index 20efaf2201..fce31e6a37 100644 --- a/internal/controller/datadogagent/component_reconciler.go +++ b/internal/controller/datadogagent/component_reconciler.go @@ -165,7 +165,7 @@ func (r *ComponentRegistry) ReconcileComponents(ctx context.Context, params *Rec } // Merge result (preserve requeue settings) - if res.Requeue || res.RequeueAfter > 0 { + if !res.IsZero() { result = res } } diff --git a/internal/controller/datadogagent/controller_reconcile_v2.go b/internal/controller/datadogagent/controller_reconcile_v2.go index 154fc3e8cd..f11b748ee4 100644 --- a/internal/controller/datadogagent/controller_reconcile_v2.go +++ b/internal/controller/datadogagent/controller_reconcile_v2.go @@ -243,7 +243,7 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, logger logr.Logger } // Always requeue - if !result.Requeue && result.RequeueAfter == 0 { + if result.IsZero() { result.RequeueAfter = defaultRequeuePeriod } return r.updateStatusIfNeededV2(logger, instance, newStatus, result, err, now) @@ -353,7 +353,7 @@ func (r *Reconciler) profilesToApply(ctx context.Context, logger logr.Logger, no oldStatus := profile.Status profileAppliedByNode, err = agentprofile.ApplyProfile(logger, &profile, nodeList, profileAppliedByNode, now, maxUnavailable, r.options.DatadogAgentInternalEnabled) if result, e := r.updateDAPStatus(ctx, logger, &profile, &oldStatus); utils.ShouldReturn(result, e) { - logger.Info("unable to update DatadogAgentProfile status", "error", e, "requeue", result.Requeue, "requeueAfter", result.RequeueAfter) + logger.Info("unable to update DatadogAgentProfile status", "error", e, "requeueAfter", result.RequeueAfter, "requeueIntent", !result.IsZero()) } if err != nil { // profile is invalid or conflicts diff --git a/internal/controller/datadogagentinternal/component_reconciler.go b/internal/controller/datadogagentinternal/component_reconciler.go index d91e6b5cca..af500326e0 100644 --- a/internal/controller/datadogagentinternal/component_reconciler.go +++ b/internal/controller/datadogagentinternal/component_reconciler.go @@ -166,7 +166,7 @@ func (r *ComponentRegistry) ReconcileComponents(ctx context.Context, params *Rec } // Merge result (preserve requeue settings) - if res.Requeue || res.RequeueAfter > 0 { + if !res.IsZero() { result = res } } diff --git a/internal/controller/datadogagentinternal/controller_reconcile_v2.go b/internal/controller/datadogagentinternal/controller_reconcile_v2.go index 878cf786fb..2fef68db6b 100644 --- a/internal/controller/datadogagentinternal/controller_reconcile_v2.go +++ b/internal/controller/datadogagentinternal/controller_reconcile_v2.go @@ -120,7 +120,7 @@ func (r *Reconciler) reconcileInstanceV2(ctx context.Context, instance *v1alpha1 } // Always requeue - if !result.Requeue && result.RequeueAfter == 0 { + if result.IsZero() { result.RequeueAfter = defaultRequeuePeriod } return r.updateStatusIfNeededV2(ctx, instance, newStatus, result, err, now) diff --git a/internal/controller/datadogcsidriver/controller_test.go b/internal/controller/datadogcsidriver/controller_test.go index 039300cab8..456f7cbd63 100644 --- a/internal/controller/datadogcsidriver/controller_test.go +++ b/internal/controller/datadogcsidriver/controller_test.go @@ -79,7 +79,7 @@ func TestReconcile_CreatesResources(t *testing.T) { // First reconcile: adds finalizer result, err := r.Reconcile(ctx, instance) require.NoError(t, err) - assert.False(t, result.Requeue) + assert.True(t, result.IsZero()) // Re-fetch the instance (finalizer was added) err = c.Get(ctx, types.NamespacedName{Name: testName, Namespace: testNamespace}, instance) @@ -89,7 +89,7 @@ func TestReconcile_CreatesResources(t *testing.T) { // Second reconcile: creates CSIDriver + DaemonSet result, err = r.Reconcile(ctx, instance) require.NoError(t, err) - assert.False(t, result.Requeue) + assert.True(t, result.IsZero()) // Verify CSIDriver was created csiDriver := &storagev1.CSIDriver{} diff --git a/internal/controller/datadogdashboard/controller.go b/internal/controller/datadogdashboard/controller.go index 42f381504d..3927ca3477 100644 --- a/internal/controller/datadogdashboard/controller.go +++ b/internal/controller/datadogdashboard/controller.go @@ -184,7 +184,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques } // If reconcile was successful and uneventful, requeue with period defaultRequeuePeriod - if !result.Requeue && result.RequeueAfter == 0 { + if result.IsZero() { result.RequeueAfter = defaultRequeuePeriod } diff --git a/internal/controller/datadoggenericresource/controller.go b/internal/controller/datadoggenericresource/controller.go index 552cda6d4d..3af4413646 100644 --- a/internal/controller/datadoggenericresource/controller.go +++ b/internal/controller/datadoggenericresource/controller.go @@ -171,7 +171,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques } // If reconcile was successful and uneventful, requeue with period defaultRequeuePeriod - if !result.Requeue && result.RequeueAfter == 0 { + if result.IsZero() { result.RequeueAfter = defaultRequeuePeriod } diff --git a/internal/controller/datadogmonitor/controller.go b/internal/controller/datadogmonitor/controller.go index d3362308d9..3258a9ce53 100644 --- a/internal/controller/datadogmonitor/controller.go +++ b/internal/controller/datadogmonitor/controller.go @@ -205,7 +205,12 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv logger.V(1).Info("Creating monitor in Datadog") // Make sure required tags are present if !apiutils.BoolValue(instance.Spec.ControllerOptions.DisableRequiredTags) { - if result, err = r.checkRequiredTags(logger, instance); err != nil || result.Requeue { + var tagsUpdated bool + tagsUpdated, err = r.checkRequiredTags(logger, instance) + if err != nil { + result.RequeueAfter = defaultErrRequeuePeriod + return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) + } else if tagsUpdated { return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) } } @@ -220,7 +225,12 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv logger.V(1).Info("Updating monitor in Datadog") // Make sure required tags are present if !apiutils.BoolValue(instance.Spec.ControllerOptions.DisableRequiredTags) { - if result, err = r.checkRequiredTags(logger, instance); err != nil || result.Requeue { + var tagsUpdated bool + tagsUpdated, err = r.checkRequiredTags(logger, instance) + if err != nil { + result.RequeueAfter = defaultErrRequeuePeriod + return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) + } else if tagsUpdated { return r.updateStatusIfNeeded(logger, instance, now, newStatus, err, result) } } @@ -230,7 +240,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, instance *datadoghqv } // If reconcile was successful, requeue with period defaultRequeuePeriod - if !result.Requeue && result.RequeueAfter == 0 { + if result.IsZero() { result.RequeueAfter = defaultRequeuePeriod } @@ -339,7 +349,7 @@ func (r *Reconciler) updateStatusIfNeeded(logger logr.Logger, datadogMonitor *da return result, nil } -func (r *Reconciler) checkRequiredTags(logger logr.Logger, datadogMonitor *datadoghqv1alpha1.DatadogMonitor) (ctrl.Result, error) { +func (r *Reconciler) checkRequiredTags(logger logr.Logger, datadogMonitor *datadoghqv1alpha1.DatadogMonitor) (bool, error) { tagsToAdd := []string{} var found bool tags := datadogMonitor.Spec.Tags @@ -357,15 +367,14 @@ func (r *Reconciler) checkRequiredTags(logger logr.Logger, datadogMonitor *datad if err != nil { logger.Error(err, "failed to update DatadogMonitor with required tags") - return ctrl.Result{RequeueAfter: defaultErrRequeuePeriod}, err + return false, err } logger.Info("Added required tags", "Monitor Namespace", datadogMonitor.Namespace, "Monitor Name", datadogMonitor.Name, "Monitor ID", datadogMonitor.Status.ID) - return ctrl.Result{RequeueAfter: defaultRequeuePeriod}, nil + return true, nil } - // Proceed in reconcile loop without modifying result. - return ctrl.Result{}, nil + return false, nil } func getRequiredTags() []string { diff --git a/internal/controller/datadogslo/controller.go b/internal/controller/datadogslo/controller.go index e2608f71f9..4017f3176d 100644 --- a/internal/controller/datadogslo/controller.go +++ b/internal/controller/datadogslo/controller.go @@ -190,7 +190,7 @@ func (r *Reconciler) internalReconcile(ctx context.Context, req reconcile.Reques } // If reconcile was successful and uneventful, requeue with period defaultRequeuePeriod - if !result.Requeue && result.RequeueAfter == 0 { + if result.IsZero() { result.RequeueAfter = defaultRequeuePeriod } diff --git a/pkg/controller/utils/shared_utils.go b/pkg/controller/utils/shared_utils.go index fe698de430..28250a2789 100644 --- a/pkg/controller/utils/shared_utils.go +++ b/pkg/controller/utils/shared_utils.go @@ -17,7 +17,7 @@ import ( // ShouldReturn returns if we should stop the reconcile loop based on result func ShouldReturn(result reconcile.Result, err error) bool { - return err != nil || result.Requeue || result.RequeueAfter > 0 + return err != nil || !result.IsZero() } // GetDatadogLeaderElectionResourceName return the nome of the Resource managing the leader election token info.