[CONTP-1335] organize finalizers#2879
Merged
Merged
Conversation
![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: e2f8940593
ℹ️ 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".
nlchung
added
refactoring
qa/skip-qa
and removed
enhancement
🛑 Gate Violations
ℹ️ Info🎯 Code Coverage (details) 🔗 Commit SHA: 71ce75b | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
nlchung
marked this pull request as ready for review
April 13, 2026 12:45
…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) <[email protected]>
…vior 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).
…t 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.
…alizer 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.
…nrelated 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Migrates DatadogAgent, DatadogAgentInternal, DatadogMonitor, DatadogDashboard, and DatadogGenericResource to the shared
internal/controller/finalizer/package, removing ~40–60 lines of duplicated finalizer scaffolding per controller. (DatadogSLO already used it.) Each controller now exposes adeleteResource()callback wrapping its existing cleanup logic.Motivation
CONTP-1335: consolidate the six copy-pasted finalizer implementations into one.
Notes for reviewers
The six controllers weren't actually doing the same thing — three small differences had to be reconciled before they could share code:
Requeue: true(reconcile again right away). Monitor returnedRequeueAfter: 60s. SLO didn't return at all — it kept running the body of the reconcile. We standardized onRequeue: true. An earlier iteration triedRequeueAfter: 60sfor everyone and it caused 34 integration-test timeouts: the tests wait up to 10s for the controller to do real work, but with a 60s delay the controller just sat there.ResourceDeleteFunctype has a doc comment explaining that returning an error blocks deletion and retries, while returning nil lets deletion proceed.Idinstead ofID. Monitor stores it as an int. Agent and DDAI don't have one. The shared callback takesdatadogID string; callers convert whatever they have. Empty string means "nothing to delete in Datadog."NewFinalizertakes two durations:defaultRequeuePeriod(how often to check in while Kubernetes is garbage-collecting the deleted object — 60s for most controllers, 15s for Agent and DDAI) anddefaultErrRequeuePeriod(5s retry onclient.UpdateordeleteFuncfailures).Describe your test plan
HandleFinalizerunit tests cover add/no-op/delete-success/delete-failure paths.make integration-testspasses in ~15s, including the 34 specs that timed out in an earlier iteration.Minimum Agent Versions
N/A — operator-side refactor only.
Checklist
refactoringqa/skip-qalabel