Repository navigation
fix(components): attach the CMS cleanup finalizer before any side effect - #1226
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the deletion protection and finalizer logic in xmc-bridge. It replaces the legacy finalizers.xmc.dev/deletion-protection finalizer with xmc.dev/cms-cleanup, delegating deletion protection enforcement to an admission policy. The controller now ensures the cleanup finalizer is attached before executing any CMS or Helm side effects, and uses the controller-runtime client instead of a dynamic client for status and finalizer updates. Feedback suggests using /metadata/resourceVersion instead of /metadata/finalizers in the JSON patch test operation to provide a more robust and standard optimistic concurrency control mechanism, preventing potential failures when finalizers are empty or absent.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors finalizer management in xmc-bridge by replacing the legacy deletion-protection finalizer with a new cms-cleanup finalizer. It also transitions the controller and status applier to use the controller-runtime client instead of a dynamic client, replacing the optimistic concurrency update loop with JSON patches. The review feedback suggests optimizing finalizer operations by checking for their presence or absence before making API calls, and recommends testing /metadata/resourceVersion instead of /metadata/finalizers to avoid fragile JSON patch test operations when finalizers are empty.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the deletion and finalizer lifecycle in xmc-bridge. It replaces the legacy finalizers.xmc.dev/deletion-protection finalizer with xmc.dev/cms-cleanup, ensuring it is attached to a Component before any CMS or Helm side effects occur. Deletion protection enforcement is decoupled from the bridge reconciler and delegated to the component-deletion-protection admission policy. Additionally, finalizer updates are refactored to use safe JSON patches via AddCleanupFinalizer and ReleaseCleanupFinalizer instead of the previous optimistic concurrency loop, and ComponentController is updated to use a unified client.Client. There are no review comments to address, and I have no further feedback to provide.
d477d03 to
f1f1f06
Compare
743063e to
75f9364
Compare
75f9364 to
f4fd439
Compare
2de933a to
3bcb131
Compare
179b0b6 to
80a8449
Compare
80a8449 to
70810e3
Compare
The bridge now attaches its finalizer as the first step of every pass on a live Component, before it touches CMS, so a Component that is created and deleted in quick succession can no longer leak a CMS record. The finalizer is attached with a JSON patch (test + add) and the pass continues with the patched object. - Rename the finalizer to xmc.dev/cms-cleanup. It now means "CMS cleanup pending" for every Component, regardless of spec.deletionProtection. - Make the deletion ValidatingAdmissionPolicy the sole deletion veto: remove the DeletionBlocked condition, and always run CMS cleanup on deletion. - Use a single controller-runtime client in the controller in place of the separate dynamic client. Add AddCleanupFinalizer, ReleaseCleanupFinalizer, and HasCleanupFinalizer, and remove SyncComponentFinalizers, AddFinalizer, HasFinalizer, and the DeletionProtection reconcile input. - Share one set of test fakes across the bridge tests. Add envtests for the create/delete race and for deleting protected and unprotected Components. - Document how to manually replace the legacy finalizer on existing Components. No migration code is added. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Treating every 422 from the finalizer patch as a stale cache and requeueing after a fixed second hid persistent validation rejections in an Info-logged loop. Return the error instead so the Component rate limiter backs off and the failure keeps surfacing. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
AddCleanupFinalizer returns the item unchanged when it already carries the cleanup finalizer or its legacy name, and ReleaseCleanupFinalizer returns early when neither is present, so neither issues a redundant write. Bug: 548436300 TAG=agy CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
70810e3 to
d513f91
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @andrzejfudro on commit 70810e3.
Summary
The bridge attaches its finalizer as the first step of every pass on a live Component, before any CMS or Helm side effect, so a Component created and deleted in quick succession can no longer leak a CMS record. The finalizer becomes cleanup-only and is renamed
xmc.dev/cms-cleanup.Motivation & Context
A fast create→delete could register intent in CMS before the finalizer existed, leaking the record. Using the finalizer as a deletion veto also hung namespace deletion for protected Components.
Key Changes
test+add); the pass continues with the patched object. A rejected patch (for example a stale cache) returns an error and is retried with backoff.xmc.dev/cms-cleanup, meaning "CMS cleanup pending" for every Component regardless ofspec.deletionProtection.DeletionBlockedis removed and CMS cleanup always runs on deletion (honoringOrphan).AddCleanupFinalizer,ReleaseCleanupFinalizer,HasCleanupFinalizer);SyncComponentFinalizersand theDeletionProtectionreconcile input are removed.finalizers.xmc.dev/deletion-protectionfrom earlier bridge versions counts as the cleanup finalizer (CMS cleanup runs on deletion, and releasing removes both names). Docs explain this; no migration is needed.Stack Position
Assumptions
finalizers.xmc.dev/deletion-protectionfinalizer need no manual step: the bridge treats it as the cleanup finalizer and removes it on deletion. Such a CR keeps the legacy name instead of gettingxmc.dev/cms-cleanup, unless it is stripped manually (optional).DeletionBlockedcondition is acceptable — the Component API isv1alpha1and unlaunched — clients reading it must switch to the admission denial.main, andgit range-diffshows no behavioral change — if wrong, the checkpoint must be re-run before merge.Verification & Testing
bazel test //pkg/components/bridge:bridge_test //cmd/xmc_bridge:xmc_bridge_test //test/e2e:components_lifecycle_test//test/e2e:components_lifecycle_testpassed with the whole stack applied.tools/analyze_diff_size.py): 148 prod / 595 test / 16 doc lines.Live Deployment & Manual Testing
enable_components: true, Spanner-backed CMS), upgraded with the dev-environment upgrade script.5/5,16/16pods Ready) before and after the checkpoint.main.git range-diffagainst the current branches shows only rebase-induced import path updates (internal/logging) and the removal of unrelatedMODULE.bazel.lockchurn from feat(components): send RequestSource from bridge and record status.componentGeneration #1237; no behavioral change.clickhouse,redpandaandingestion-gateway, the bridge attached and keptxmc.dev/cms-cleanupon the next pass.mt-cp1-race)kubectl applyandkubectl deleteattached the finalizer before any CMS write, executed CMS deletion and released the finalizer; the Spanner row endedState=5, ActiveName=NULLwith no leak.mt-cp1-fin)kubectl deletewithdeletionProtection: truewas denied by the deletion ValidatingAdmissionPolicy. WithdeletionProtection: false, delete soft-deleted the CMS row (State=5) and released the finalizer.Related Issues / Bugs
Bug: 548436300
TAG=agy
CONV=d833b0e6-4821-48f6-8576-776d53f55a5c
Stack created with GitHub Stacks CLI • Give Feedback 💬