fix: skip attachment resource cleanup when its policy is gone - #10294
Open
ryanchou1994 wants to merge 1 commit into
Open
ryanchou1994 wants to merge 1 commit into
ryanchou1994 wants to merge 1 commit into
Conversation
Deleting a storage policy that attachments still reference leaves those attachments undeletable and floods the log with 404 errors. AttachmentReconciler#cleanUpResources calls attachmentService.delete(attachment), whose first step is client.get(Policy.class, ...). With the policy gone that errors with ExtensionNotFoundException, which the reconciler does not handle, so DefaultController requeues the request unconditionally and forever. The finalizer is never removed either, so the attachment record stays too. A missing policy or config map is permanent, not transient: no handler can resolve the attachment any more, so there is nothing left to clean up. Treat it as nothing to do, log a warning naming the attachment and the missing extension, and let the reconciler remove the finalizer so the record can finally go. Fixes halo-dev#6380
|
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
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 type of PR is this?
What this PR does / why we need it:
When a storage policy is deleted while attachments still reference it, those attachments can no longer be deleted and the log fills up with 404 errors.
AttachmentReconciler#cleanUpResourcescallsattachmentService.delete(attachment), whose first step isclient.get(Policy.class, ...). With the policy gone that errors withExtensionNotFoundException(404). The reconciler does not handle it, soDefaultControllerlogs "aborted with an error, re-enqueuing" and requeues unconditionally, forever, because the missing policy never comes back. The finalizer is never removed either, so the attachment record stays as well.A missing policy or config map is a permanent state, not a transient failure: no handler can resolve the attachment any more, so there is nothing left to clean up. This treats that case as "nothing to do" - log a warning and let the reconciler remove the finalizer, so the attachment record can finally be deleted as the user asked.
Tests: a new
AttachmentReconcilerTestwith three cases - policy missing (finalizer removed, no exception), normal cleanup (unchanged), and any other error still propagating so the finalizer is kept and the request is retried. The first one fails on current main.Which issue(s) this PR fixes:
Fixes #6380
Special notes for your reviewer:
DefaultAttachmentService#delete, so the publicAttachmentServicecontract that plugins depend on is unchanged.onErrorResumeis scoped toExtensionNotFoundException. If a plugin handler's owndelete()throws that exception for some unrelated extension, it would also be treated as "nothing to clean up". Narrowing further would mean pre-checking the policy and its config map in the reconciler, duplicating what the service already does. Happy to switch if you prefer that shape.spotlessJavaCheckis clean. Not covered: I did not reproduce the original 404 loop end to end against a live deployment.Does this PR introduce a user-facing change?