operator: let layered CRs delete after their cluster is removed + opt-in GC owner ref (UX-1351)#1626
Draft
sago2k8 wants to merge 3 commits into
Draft
operator: let layered CRs delete after their cluster is removed + opt-in GC owner ref (UX-1351)#1626sago2k8 wants to merge 3 commits into
sago2k8 wants to merge 3 commits into
Conversation
A RedpandaRole (and any layered CR: User/Schema/Group/ShadowLink) that references a cluster via clusterRef would hang in Terminating when the referenced cluster was fully torn down. Deleting just the cluster CR was already handled: the lookup returns NotFound, which the finalizer cleanup path swallows. But uninstalling the v1 (vectorized) operator removes the clusters.redpanda.vectorized.io CRD, so the lookup returns a *meta.NoKindMatchError instead of NotFound; stripping the type's RBAC returns Forbidden. ignoreAllConnectionErrors didn't recognize either, so DeleteResource returned an error, the finalizer was never removed, and the CR could not be deleted. Group the "cluster can no longer be resolved" cases into isClusterGone and fold NoKindMatch and Forbidden into it. The change is a strict superset of the prior swallow set, so nothing previously surfaced is now hidden; the only effect is that finalizer cleanup also completes when the whole kind is gone, not just when the CR is absent. Tests: TestIgnoreAllConnectionErrorsClusterGone pins the classifier (NoKindMatch/Forbidden were RED before this change); TestRoleDeletionWith DeletedClusterRef covers the green boundary end-to-end in envtest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…leted Adds an opt-in path so a RedpandaRole is garbage-collected when the cluster CR it references is deleted (the CRD staying installed). Setting the annotation operator.redpanda.com/delete-with-cluster=true makes the role controller stamp an owner reference pointing at the resolved cluster. Details: - ownerReferenceForCluster builds the GC edge with the correct GVK for v1 (redpanda.vectorized.io/Cluster), v2 (cluster.redpanda.com/Redpanda), and StretchCluster refs. It returns nil for cross-namespace refs (Kubernetes GC ignores those) and uses controller=false, blockOwnerDeletion=false so it never blocks the cluster's own deletion. - The owner reference is applied as a metadata patch under a dedicated server-side-apply field manager, separate from the finalizer/status owner, so SSA cannot relinquish the finalizer. - Factory gains ClusterForObject (resolve the referenced cluster as a client.Object) and exposes GetClient on the ClientFactory interface. Wired into the Role controller only for now. The actual GC cascade is exercised by the kind acceptance suite; envtest runs no garbage collector. ownerReferenceForCluster is unit-tested directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a role-crds scenario asserting that a RedpandaRole annotated with operator.redpanda.com/delete-with-cluster=true is stamped with an owner reference pointing at its referenced cluster, the garbage-collection edge that lets the role (and thus a namespace) be deleted along with the cluster. The actual GC cascade depends on the kube controller-manager's garbage collector, which only runs in a full control plane, so this lives in the kind acceptance suite rather than envtest. The deterministic RED->GREEN for the underlying finalizer fix is covered by the unit tests. Generalizes kubernetesObjectHasClusterOwner to be variant-aware: the vectorized variant rewrites clusterRefs to the v1 Cluster, so the expected owner reference GVK is resolved per variant. The v2 path is unchanged, so the existing migration scenario is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
This PR is stale because it has been open 5 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
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
2 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.
Problem
A
RedpandaRole(and other layered CRs:User/Schema/Group/ShadowLink) that references a cluster viaclusterRefcould get stuck inClusterRefInvalidand, in the v1 teardown case, block deletion of its namespace forever.The trigger is specific to v1: deleting just the cluster CR was already handled (the lookup returns
NotFound, which the finalizer-cleanup path swallows). But uninstalling the v1 (vectorized) operator removes theclusters.redpanda.vectorized.ioCRD, so the lookup returns a*meta.NoKindMatchErrorinstead ofNotFound; stripping the type's RBAC returnsForbidden.ignoreAllConnectionErrorsrecognized neither, soDeleteResourcereturned an error, the finalizer was never removed, and the role hung inTerminating— taking the namespace with it.Changes
Fix (
c91aea6a) — group the "cluster can no longer be resolved" cases intoisClusterGoneand fold inNoKindMatch+Forbidden. Strict superset of the prior swallow set; the only effect is that finalizer cleanup also completes when the whole kind is gone. Covers all five layered CRs (sharedignoreAllConnectionErrors).Feature (
45e36107) — opt-inoperator.redpanda.com/delete-with-cluster=trueannotation makes the Role controller stamp a GC owner reference at the referenced cluster (v1/v2/StretchCluster GVK, same-namespace only,controller=false/blockOwnerDeletion=falseso it never blocks the cluster's own deletion). Applied under a dedicated SSA field manager so it can't relinquish the finalizer. Factory gainsClusterForObject+GetClient.Acceptance (
3b152776) —role-crdsscenario asserting the owner reference is stamped; generalizedkubernetesObjectHasClusterOwnerto be variant-aware (v1 in the vectorized variant, v2 elsewhere). v2 path unchanged, so the migration scenario is unaffected.Testing
TestIgnoreAllConnectionErrorsClusterGonepins the classifier —NoKindMatch/Forbiddenwere RED before the fix, green after (demonstrated both directions).TestOwnerReferenceForClustercovers the builder (v1/v2 GVK, cross-namespace, nil).TestRoleDeletionWithDeletedClusterRefcovers the CR-deleted boundary; existingTestRoleReconcilestill green after theSyncResourcerestructure.cluster/Updating_admin_ports,rack-awareness,scale-down) failed on host-level nodePort contention from running the full suite in parallel locally — not touched by this change; CI isolation should clear them.Note: the literal "namespace stuck Terminating" can't be reproduced in envtest (no namespace controller/GC; warm RESTMapper returns
NotFoundnotNoKindMatch), so the deterministic RED→GREEN lives in the unit test and the GC cascade is covered by the kind suite.Follow-ups (not in this PR)
User/Schema/Group/ShadowLink(theisClusterGonefix already covers all five).NoKindMatch→ClusterRefInvalidinhandleResourceSyncErrorsso a live role whose v1 CRD vanished shows a clean condition instead ofUnexpectedError.🤖 Generated with Claude Code