chore(toposerver): always delete topo PVCs with cluster - #555
Open
niconosenzo wants to merge 1 commit into
Open
Conversation
This comment has been minimized.
This comment has been minimized.
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
July 31, 2026 12:02
c280c75 to
1d455dd
Compare
This comment has been minimized.
This comment has been minimized.
Verolop
reviewed
Jul 31, 2026
Verolop
left a comment
Verolop
left a comment
Contributor
There was a problem hiding this comment.
Thanks for tackling the orphaned PVCs. So happy we're finally at this stage 🥳
I left one lifecycle blocking comment, and a couple of small ones.
| localTopo := localTopoSpec.DeepCopy() | ||
| // Merge hierarchy: Etcd -> local toposerver -> multigrescluster. | ||
| if localTopo != nil && localTopo.Etcd != nil { | ||
| localTopo.Etcd.PVCDeletionPolicy = multigresv1alpha1.MergePVCDeletionPolicy( |
Contributor
There was a problem hiding this comment.
Switching the global topo to external currently deletes every TopoServer with the cluster label, including local ones, so with this change, that would also delete their PVCs. Could we scope that cleanup to the global TopoServer first? Could be a separate PR.
Contributor
Author
There was a problem hiding this comment.
Decided to not rely on the PVCDeletionPolicy for the topo PVCs, if the multigrescluster owner object has been deleted or will be deleted, we get rid of the PVC. Unless we have a reason to keep the volumes after that event ?
Contributor
There was a problem hiding this comment.
Agreed on deleting the topo PVCs when the MultigresCluster is deleted. I saw that the selector issue I mentioned is separate and already exists today, plus it's only for one specific case, so it does not need to block this PR :D
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
July 31, 2026 16:31
1d455dd to
2138173
Compare
This comment has been minimized.
This comment has been minimized.
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
July 31, 2026 16:37
2138173 to
3ebe028
Compare
This comment has been minimized.
This comment has been minimized.
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
July 31, 2026 17:03
3ebe028 to
ee9ec8d
Compare
This comment has been minimized.
This comment has been minimized.
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
August 3, 2026 09:55
ee9ec8d to
7f4b340
Compare
This comment has been minimized.
This comment has been minimized.
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
August 3, 2026 10:36
7f4b340 to
017d1c7
Compare
This comment has been minimized.
This comment has been minimized.
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
August 3, 2026 10:41
017d1c7 to
d418f34
Compare
This comment has been minimized.
This comment has been minimized.
Gate TopoServer deletion with a finalizer so rebuildable etcd volumes are removed when the owning MultigresCluster is terminating or gone. Reconcile deletion-timestamp transitions explicitly and preserve data-plane PVCs under Retain. Signed-off-by: Nicolas Nosenzo <niconosenzo@gmail.com>
niconosenzo
force-pushed
the
cleanup-topo-volumes
branch
from
August 3, 2026 11:21
d418f34 to
bbb8bd8
Compare
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
Verolop
requested changes
Aug 3, 2026
Verolop
left a comment
Verolop
left a comment
Contributor
There was a problem hiding this comment.
Thanks for reworking this 💚
I left two lifecycle comments that need addressing before merge. One covers the case where the TopoServer is already gone before the cluster is deleted. The other covers the StatefulSet still deleting the PVC while the cluster is running.
I also left one smaller finalizer safety comment. The global/local selector issue already existed before this change, so I don’t consider that a blocker for this PR.
| "github.com/multigres/multigres-operator/pkg/util/metadata" | ||
| ) | ||
|
|
||
| // handleDeletion runs when a TopoServer is being deleted. It is gated by |
Contributor
There was a problem hiding this comment.
When a cluster switches from managed topo to external, this TopoServer is deleted while the cluster is still running. This handler keeps the PVC and removes the finalizer, which is right at that point, but if the cluster is deleted later, there is no TopoServer left to come back and remove that PVC. Kubernetes GC cannot help because the PVC has no cluster owner, and multigres-gc will not see it because it has no orphan-since label.
One way to deal with this is to make the PVC follow the MultigresCluster, not the TopoServer. Add the cluster as a non-controller owner of the PVC, so Kubernetes keeps it while that exact cluster exists and deletes it when the cluster is gone, but if you prefer explicit cleanup, put the finalizer on the MultigresCluster for the same reason.
One e2e test for this path should help too: start with managed topo and Retain, switch to external and confirm the PVC stays, then delete the cluster and confirm the PVC is removed, but this can be a separate pr later.
| // k8s can finish deleting the TopoServer. Any error above returns | ||
| // early, leaving the finalizer in place so reconciliation retries. | ||
| if slices.Contains(toposerver.Finalizers, topoServerFinalizer) { | ||
| patch := client.MergeFrom(toposerver.DeepCopy()) |
Contributor
There was a problem hiding this comment.
small safety note: this patch replaces the whole finalizer list using the object we read earlier. If another finalizer is added at the same time, this patch can accidentally remove it.
You can fetch the latest TopoServer and use Update, or use MergeFromWithOptimisticLock, and make the same change where the finalizer is added.
| localTopo := localTopoSpec.DeepCopy() | ||
| // Merge hierarchy: Etcd -> local toposerver -> multigrescluster. | ||
| if localTopo != nil && localTopo.Etcd != nil { | ||
| localTopo.Etcd.PVCDeletionPolicy = multigresv1alpha1.MergePVCDeletionPolicy( |
Contributor
There was a problem hiding this comment.
Agreed on deleting the topo PVCs when the MultigresCluster is deleted. I saw that the selector issue I mentioned is separate and already exists today, plus it's only for one specific case, so it does not need to block this PR :D
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.
Topo etcd data is coordination state rebuilt on cluster creation, so retaining its PVCs after a MultigresCluster is deleted only leaves orphaned volumes behind.
What this change introduce, gate TopoServer deletion with a finalizer so rebuildable etcd volumes are removed when the owning MultigresCluster is terminating or gone. Reconcile deletion-timestamp transitions explicitly and preserve data-plane PVCs under Retain.