fix: correct setDeletionBillingTimeStamp trigger to set deletionTime#4577
Merged
fix: correct setDeletionBillingTimeStamp trigger to set deletionTime#4577
Conversation
The setDeletionBillingTimeStamp CosmosDB trigger was incorrectly setting the creationTime field instead of deletionTime when marking billing documents for deletion. This fix updates the trigger to correctly populate the deletionTime field when MarkForDeletion is called. This was introduced in the PR [#3584](#3584) The trigger is executed server-side in CosmosDB as a pre-trigger on Replace operations, ensuring atomic timestamp updates. Files updated: - pkg/deploy/generator/const.go: Fixed trigger JavaScript function - Generated deployment templates will be updated via make generate Signed-off-by: Goutham Muguluvalli Niranjan <gniranjan@microsoft.com>
gouthamMN
requested review from
bennerv,
cadenmarchese,
hawkowl,
hlipsig,
jharrington22,
kimorris27,
mociarain,
mrWinston,
rogbas,
sankur-codes,
tiguelu,
tsatam,
wanghaoran1988 and
yjst2012
as code owners
Refactored CosmosDB trigger functions in const.go to use raw string literals (backticks) instead of escaped strings for improved readability and maintainability. Signed-off-by: Goutham Muguluvalli Niranjan <gniranjan@microsoft.com>
The original code used r.instance which was fetched once at reconciliation start. If another process modified the Cluster resource during the reconciliation loop, the patch would fail with a conflict error (HTTP 409) due to stale resourceVersion. This change wraps the annotation update in retry.RetryOnConflict, which is the standard Kubernetes pattern for optimistic concurrency control. Each retry iteration fetches a fresh copy of the Cluster resource, ensuring we have the latest resourceVersion before patching. Trade-offs: - Extra API call: Each retry does a Get before Patch, adding latency but ensuring correctness - Retry limits: retry.DefaultRetry uses exponential backoff with ~5 retries; in extreme contention scenarios this could still fail - No r.instance sync: The in-memory r.instance is not updated, which is fine since it's only used for reading during reconciliation and the annotation update is terminal Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
mociarain
reviewed
Feb 2, 2026
Collaborator
mociarain
left a comment
mociarain
left a comment
There was a problem hiding this comment.
LGTM but I'd like to see a review from some of the folks who worked on the local CosmosDB recently and got deep into the triggers. I've asked them so 🤞 . Not blocking
Also the E2E test failure is a know flake. This PR should address is: #4572
Collaborator
|
LGTM |
Contributor
Author
We had many orphaned billing docs as the billing wasn't deleting them due to deletion time not set. No impacts on the pricing or charging the customers. |
Contributor
Author
|
@mociarain - I merged the changes from PR #4572. Will you or @hlipsig could approve and merge this PR if everything looks good on CI? |
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
5 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.
Which issue this PR addresses:
Fixes: https://issues.redhat.com/browse/ARO-23757
What this PR does / why we need it:
The setDeletionBillingTimeStamp CosmosDB trigger was incorrectly setting the creationTime field instead of deletionTime when marking billing documents for deletion. This fix updates the trigger to correctly populate the deletionTime field when MarkForDeletion is called.
This was introduced in the PR #3584
The trigger is executed server-side in CosmosDB as a pre-trigger on Replace operations, ensuring atomic timestamp updates.
Files updated:
Test plan for issue:
Is there any documentation that needs to be updated for this PR?