-
Notifications
You must be signed in to change notification settings - Fork 32
CLOUDP-359062 - add ownerrefs and only deleteAll in multicluster #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
MCK 1.6.1 Release NotesBug Fixes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well tested and implementation LGTM ! Please check if the failing tests are due to flakiness 🙏
vinilage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| --- | ||
| kind: fix | ||
| date: 2025-12-02 | ||
| --- | ||
|
|
||
| * **MongoDB** Adding missing ownerrefs to ensure proper resource deletion by kubernetes. | ||
| * **Single Cluster** Deleting resources created by CRD now only happens on multi-cluster deployments. Single Cluster will solely rely on ownerrefs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| clusterClient := item.Client | ||
| clusterName := item.Name | ||
| if err := r.commonController.deleteClusterResources(ctx, clusterClient, clusterName, sc, log); err != nil { | ||
| errs = multierror.Append(errs, xerrors.Errorf("failed deleting dependant resources in cluster %s: %w", clusterName, err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in om's controller we're only logging the error in this case
here though failing to delete will result in errors returned - would that impact anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from zoom: lets clarify whether we want to use sharded or om controller approach. and why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am not sure whether its worth the effort to unify both.
Sharded interface onDelete returns error, while the one from om controller does not
but yeah in the end sharded collects those errors and them summarizes them again https://github.com/mongodb/mongodb-kubernetes/blob/d43f0840f0f0fed195bfe5e4d42d229[…]0f3022f3e/controllers/operator/mongodbresource_event_handler.go
i personally don't think its worth the hassle to unify them, but open for any opiniosn
| namespace string | ||
| resourceName string | ||
| ownerLabels map[string]string | ||
| ownerReferences []metav1.OwnerReference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how the stored owner refs are actually consumed by the controller's impl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check whether its even used. only used by configmap typemeta and not in the actual store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its used here:
| SetOwnerReferences(s.ownerReferences). |
Summary
fixes: #589
This pull request introduces important improvements to how Kubernetes resources are managed and cleaned up for MongoDB and Ops Manager deployments, especially in multi-cluster versus single-cluster scenarios. The main focus is on correctly setting owner references for resources to enable automatic garbage collection by Kubernetes, and ensuring explicit resource deletion only occurs when necessary (i.e., in multi-cluster setups where owner references cannot span clusters).
Owner Reference Management & Resource Cleanup
ownerReferences) to all relevant ConfigMaps and resources for MongoDB and Ops Manager, ensuring proper automatic cleanup by Kubernetes in single-cluster deployments.appdbreplicaset_controller.go,mongodbopsmanager_controller.go,mongodbshardedcluster_controller.go,state_store.go) to consistently pass and setownerReferences.Multi-Cluster vs Single-Cluster Resource Deletion Logic
Proof of Work
Checklist
skip-changeloglabel if not needed