[CONS-8253] Propagate DDGR finalizer error #2919
Conversation
The DDGR finalizer's deleteResource callback swallowed Datadog API errors (returned nil after logging), so the shared finalizer package removed the finalizer even when remote deletion failed. Customers with monitors referenced by composite monitors hit a 400 from the API and got silently orphaned Datadog objects. - Return the error from deleteResource so the shared Finalizer keeps the finalizer in place and requeues, matching DatadogMonitor's existing behavior. - Treat 404 from DeleteMonitor as success so an already-deleted remote monitor (e.g. removed from the Datadog UI) doesn't block finalization forever.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2919 +/- ##
==========================================
+ Coverage 40.80% 40.97% +0.16%
==========================================
Files 320 320
Lines 35204 35240 +36
==========================================
+ Hits 14366 14438 +72
+ Misses 20027 19989 -38
- Partials 811 813 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
- Test_handleFinalizer_deleteErrorPropagates: covers the finalizer closure in finalizer.go. Simulates a Datadog API error (composite- reference 400) via mockDeleteErr and asserts that HandleFinalizer returns the error AND keeps the finalizer in place. Without the CONS-8253 fix, this test would fail because the error was swallowed and the finalizer was removed. - Test_deleteMonitor: covers deleteMonitor in monitors.go with four HTTP response codes (200, 404, 400, 500). Verifies 404 is treated as success (idempotency) while 400 and 500 propagate as errors. Coverage on the changed functions: - finalizer.go deleteResource: 92.3% - monitors.go deleteMonitor: 88.9%
zhuminyi
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f2c17191
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err != nil { | ||
| logger.Error(err, "failed to finalize", "custom resource Id", fmt.Sprint(datadogID)) | ||
| return nil | ||
| return err |
There was a problem hiding this comment.
When this path now propagates handler.deleteResource errors, only the monitor handler converts a 404 into success; the dashboard, notebook, downtime, and synthetic handlers still wrap any Datadog client error. If one of those remote resources was already deleted in Datadog/UI and the user deletes the DDGR, the Datadog delete returns 404, HandleFinalizer keeps the finalizer and requeues forever, leaving the Kubernetes object stuck in Terminating. Please make all delete handlers idempotent on 404 or centralize that handling before returning the error.
Useful? React with 👍 / 👎.
The previous commit propagated errors from the finalizer's deleteResource callback. Without 404-idempotency in every handler, a remote Datadog object that had been removed out-of-band (UI deletion, manual API call, retention expiry) would now cause the Kubernetes resource to be stuck in Terminating forever — every reconcile would hit the same 404. Extend the pattern already in deleteMonitor to the other four delete functions: deleteNotebook, deleteDashboard, deleteSyntheticTest, and deleteDowntime. Each now inspects httpResponse.StatusCode == 404 and returns nil when the remote object is already gone, letting the Kubernetes finalizer clear cleanly. Add Test_delete<Type>_idempotent for each of the four handlers using httptest.Server, covering 200/404/400/500 cases. Coverage on all five DDGR delete functions is now above the 80% target.
…st.go Fold Test_deleteMonitor into the shared delete_idempotency_test.go file alongside the other four handlers so all five delete paths share one naming convention (Test_delete<Type>_idempotent) and one place to grep for the 404-idempotency contract. Delete the now-redundant monitors_test.go.
3 participants
What does this PR do?
Fixes CONS-8253:
DatadogGenericResourcemonitors referenced by composite monitors are silentlyorphaned on deletion. The k8s CR gets removed while the remote Datadog monitor remains, with no warning
to the user.
Root cause: the finalizer's
deleteResourcecallback logged the Datadog API error but returnednil, so the sharedFinalizerproceeded to remove the finalizer as if cleanup had succeeded. WhateverDatadog said — 400 ("referenced in composite monitors"), 401, 429, 5xx, network timeout — the error was
dropped and the CR was deleted.
Motivation
A composite monitor in Datadog is a monitor whose query is a boolean expression built from other
monitors' IDs — for example:
monitor(123) && monitor(456)
The composite's evaluation depends on its referenced monitors existing. Datadog therefore refuses to
delete a monitor that is still referenced by an active composite, returning:
400 Bad Request: monitor [123] is referenced in composite monitors: [456]
The customer's reproduction (CONS-8253):
Before this fix: the error was logged but discarded, the k8s finalizer was removed, the CR vanished — and
composite monitor 1 stayed in Datadog, orphaned, no longer managed by any custom resource.
After this fix: the error propagates, the finalizer stays in place, the CR remains visible in Terminating
state. The operator retries every 5s. The user's path forward is to resolve the composite dependency
first — either delete composite 2, or edit its query to remove the reference to 1 — after which the
next retry succeeds and the CR is cleaned up normally.
The net effect: the operator now refuses to let the k8s resource vanish while the remote Datadog object
it was managing is still pinned by another dependency. This matches how DatadogMonitor already behaves.
Describe how you validated your changes
All delete-error paths now match
DatadogMonitor's existing behavior:Terminating, retries every 5suntil the composite dependency is resolved.
up intentionally (404 treated as "already gone").
Terminating,retries until error clears.