K8SPSMDB-1455: avoid repeated delete calls#2389
Merged
Merged
Conversation
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a small Kubernetes utility helper to make resource deletions idempotent and updates the main PSMDB controller to use it, aiming to avoid repeated delete attempts when objects are already gone.
Changes:
- Added
k8s.DeleteIfExists(ctx, client, obj)helper inpkg/k8s/utils.go. - Replaced several direct
Delete(...)/IsNotFound(...)patterns in the PSMDB controller withDeleteIfExists(arbiter SFS, non-voting SFS, config SFS/service, mongos SFS).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/k8s/utils.go | Adds DeleteIfExists helper used to skip deletion when the object is absent. |
| pkg/controller/perconaservermongodb/psmdb_controller.go | Switches multiple deletion call sites to use the new helper to reduce repeated delete calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+49
to
+55
| func DeleteIfExists(ctx context.Context, c client.Client, obj client.Object) error { | ||
| if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); k8serrors.IsNotFound(err) { | ||
| return nil | ||
| } else if err != nil { | ||
| return errors.Wrapf(err, "failed to get object: %s", obj.GetName()) | ||
| } | ||
| return c.Delete(ctx, obj) |
| err = errors.Errorf("delete nonVoting statefulset %s: %v", replset.Name, err) | ||
| return err | ||
| if err := k8sutils.DeleteIfExists(ctx, r.client, psmdb.NewStatefulSet(naming.NonVotingStatefulSetName(cr, replset), cr.Namespace)); err != nil { | ||
| return errors.Wrapf(err, "failed to delete non voting statefulset: %s", naming.NonVotingStatefulSetName(cr, replset)) |
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
mayankshah1607
force-pushed
the
K8SPSMDB-1455
branch
from
f449ee5 to
af24c9f
Compare
mayankshah1607
commented
Jun 8, 2026
Comment on lines
-84
to
-92
| client, err := client.New(mgr.GetConfig(), client.Options{ | ||
| Scheme: mgr.GetScheme(), | ||
| Cache: &client.CacheOptions{ | ||
| DisableFor: []client.Object{&corev1.Node{}}, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "create client") | ||
| } |
Member
Author
There was a problem hiding this comment.
mayankshah1607
requested review from
egegunes,
gkech,
hors,
nmarukovich,
oksana-grishchenko and
pooknull
as code owners
egegunes
previously approved these changes
Jun 9, 2026
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
egegunes
previously approved these changes
Jun 9, 2026
gkech
previously approved these changes
Jun 9, 2026
Comment on lines
+49
to
+60
| func DeleteIfExists(ctx context.Context, c client.Client, obj client.Object) error { | ||
| if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); k8serrors.IsNotFound(err) { | ||
| return nil | ||
| } else if err != nil { | ||
| return errors.Wrapf(err, "failed to get %T %s/%s", obj, obj.GetNamespace(), obj.GetName()) | ||
| } | ||
|
|
||
| if err := c.Delete(ctx, obj); client.IgnoreNotFound(err) != nil { | ||
| return errors.Wrapf(err, "failed to delete %T %s/%s", obj, obj.GetNamespace(), obj.GetName()) | ||
| } | ||
| return nil | ||
| } |
| } | ||
|
|
||
| return r.deleteMongos(ctx, cr) | ||
| if err := k8sutils.DeleteIfExists(ctx, r.client, psmdb.MongosStatefulset(cr)); err != nil { |
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
12 tasks
Comment on lines
+49
to
+60
| func DeleteIfExists(ctx context.Context, c client.Client, obj client.Object) error { | ||
| if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); k8serrors.IsNotFound(err) { | ||
| return nil | ||
| } else if err != nil { | ||
| return errors.Wrapf(err, "failed to get %T %s/%s", obj, obj.GetNamespace(), obj.GetName()) | ||
| } | ||
|
|
||
| if err := c.Delete(ctx, obj); client.IgnoreNotFound(err) != nil { | ||
| return errors.Wrapf(err, "failed to delete %T %s/%s", obj, obj.GetNamespace(), obj.GetName()) | ||
| } | ||
| return nil | ||
| } |
Comment on lines
+56
to
+60
| const eventRegardingNameIndex = "regarding.name" | ||
|
|
||
| func eventRegardingNameIndexer(o client.Object) []string { | ||
| return []string{o.(*eventsv1.Event).Regarding.Name} | ||
| } |
Signed-off-by: Mayank Shah <mayank.shah@percona.com>
Comment on lines
+58
to
+60
| func eventRegardingNameIndexer(o client.Object) []string { | ||
| return []string{o.(*eventsv1.Event).Regarding.Name} | ||
| } |
Comment on lines
+49
to
+60
| func DeleteIfExists(ctx context.Context, c client.Client, obj client.Object) error { | ||
| if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); k8serrors.IsNotFound(err) { | ||
| return nil | ||
| } else if err != nil { | ||
| return errors.Wrapf(err, "failed to get %T %s/%s", obj, obj.GetNamespace(), obj.GetName()) | ||
| } | ||
|
|
||
| if err := c.Delete(ctx, obj); client.IgnoreNotFound(err) != nil { | ||
| return errors.Wrapf(err, "failed to delete %T %s/%s", obj, obj.GetNamespace(), obj.GetName()) | ||
| } | ||
| return nil | ||
| } |
Comment on lines
+77
to
+82
| // the volume resize logic lists PVC events through the cached client, | ||
| // which requires the field to be indexed | ||
| err = mgr.GetFieldIndexer().IndexField(context.TODO(), &eventsv1.Event{}, eventRegardingNameIndex, eventRegardingNameIndexer) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "index events by %s", eventRegardingNameIndex) | ||
| } |
mayankshah1607
commented
Jun 15, 2026
|
|
||
| # user should be able to restore to the previous size and make the cluster ready | ||
| patch_pvc_request "${cluster}" "3G" | ||
| wait_pvc_request_revert "${cluster}" "3G" |
Member
Author
There was a problem hiding this comment.
We changed this behaviour in #2341
We don't land in an error state anymore, we instead revert the update in case of downscaling. This test was being very flaky
mayankshah1607
requested review from
eleo007,
jvpasinatto and
valmiranogueira
as code owners
egegunes
approved these changes
Jun 16, 2026
pooknull
approved these changes
Jun 16, 2026
Collaborator
commit: 136576e |
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.
CHANGE DESCRIPTION
Problem:
Fixes #1751
The operator makes repeated redundant
Deletecalls to KubeAPICause:
A lot of the clean-up logic makes unconditional
Deletecalls on every reconcile. This causes the operator to flood the API with unneededDeletecallsSolution:
DeleteIfExistshelper that deletes an object only if it exists. The initialGetcall hits the cache so effectively we are reducing the requests to the KubeAPICHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability