KUBE-179: clean up and self-heal MongoDBSearch resources#1382
KUBE-179: clean up and self-heal MongoDBSearch resources#1382anandsyncs wants to merge 2 commits into
Conversation
7416fbe to
9e05f7b
Compare
7787462 to
79421f8
Compare
There was a problem hiding this comment.
Pull request overview
This PR advances KUBE-154 stack 2/4 by shifting MongoDBSearch-managed resource lifecycle tracking and cleanup from cross-cluster OwnerReferences to label-based identity, then adding “self-heal” and fail-closed cleanup behaviors across central/member clusters.
Changes:
- Remove OwnerReferences from Search-owned resources and rely on stable, protected “search-owner” labels for event routing and garbage-collection workflows.
- Add NotFound “confirmed absence” cleanup that double-checks with uncached readers and performs label-scoped, UID-/RV-preconditioned deletions across central and member clusters.
- Add/adjust controllers and watches (main Search, Envoy, metrics-forwarder) to route lifecycle events via label mappers and to honor deletion/disable-reconciliation short-circuits.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/tls/tls_secret.go | Stops setting owner references on operator-managed TLS secrets. |
| pkg/handler/labels_search.go | Updates label semantics/docs for Search routing and cleanup. |
| controllers/searchcontroller/search_state.go | Migrates Search state ConfigMap metadata away from OwnerReferences to labels. |
| controllers/searchcontroller/search_state_test.go | Updates state tests for label-only identity and owner-ref removal. |
| controllers/searchcontroller/search_construction.go | Removes STS owner-reference stamping and updates comments for label lifecycle. |
| controllers/searchcontroller/mongodbsearch_reconcile_helper.go | Introduces cleanup error propagation; converts identity helpers to label-only; expands stale-shard cleanup. |
| controllers/searchcontroller/mongodbsearch_reconcile_helper_test.go | Updates reconcile tests for no owner refs and adds stale-shard cleanup coverage. |
| controllers/operator/watch/config_change_handler.go | Adds optional MapFunc routing to support Search lifecycle mapping on create/update/delete. |
| controllers/operator/watch/config_change_handler_test.go | Adds coverage for delete routing and MapFunc-driven event mapping behavior. |
| controllers/operator/state_store.go | Adds reader injection (cached vs uncached) and simplifies owner-ref handling in state storage. |
| controllers/operator/mongodbsearchenvoy_controller.go | Adds deletion/disable short-circuits; converts watches to label-based mapping; removes owner refs from Envoy resources. |
| controllers/operator/mongodbsearchenvoy_controller_test.go | Updates/extends envoy tests for label-based routing, pause semantics, and watch registration. |
| controllers/operator/mongodbsearch_metrics_controller.go | Adds historical topology cleanup, uncached readers, and removes owner refs; tightens deletion/removed-cluster behavior. |
| controllers/operator/mongodbsearch_metrics_controller_test.go | Adds extensive coverage for topology persistence, deletion semantics, and removed-cluster cleanup. |
| controllers/operator/mongodbsearch_controller.go | Implements confirmed-NotFound cleanup sweep using uncached readers and label/UID/RV preconditions; rewires watches to label mapping. |
| controllers/operator/mongodbsearch_controller_test.go | Adds/updates tests for NotFound sweep lifecycle, uncached reader confirmation, and watch behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
42c8208 to
5aa0e08
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
79421f8 to
90e0eae
Compare
anandsyncs
changed the title
| op, err := controllerutil.CreateOrUpdate(ctx, kubeClient, sts, func() error { | ||
| statefulset.Apply(modifications...)(sts) | ||
| return controllerutil.SetOwnerReference(r.mdbSearch, sts, kubeClient.Scheme()) | ||
| sts.OwnerReferences = nil |
There was a problem hiding this comment.
Design decision reviewers should ratify — owner-ref removal without a finalizer: this PR removes owner references from every central-cluster managed resource (STS here, Services/ConfigMap, TLS secret, Envoy, state CMs) while the search and Envoy controllers gain no finalizer (only the metrics controller keeps one). Master relied on operator-independent k8s GC for central resources; after this PR, cleanup is entirely operator-dependent — if the operator is down when the CR is deleted (nothing holds the CR), or the sweep fails closed permanently, central resources leak until a later reconcile. Two cheap outs: keep owner refs on central-cluster writes only (harmless alongside the UID-scoped sweep, restores the GC backstop; cross-cluster refs stay removed since they're the actual bug), or add the canonical finalizer like the metrics controller. Worth an explicit decision in review rather than an implicit one.
| return reconcile.Result{}, nil | ||
| } | ||
|
|
||
| // Short-circuit: the disable-reconciliation annotation allows to |
There was a problem hiding this comment.
This annotation check is copy-pasted verbatim in all three controllers (here, mongodbsearch_metrics_controller.go, mongodbsearchenvoy_controller.go). Suggest a func (s *MongoDBSearch) IsReconciliationDisabled() bool on the API type so the semantics can't drift three ways.
| // member-cluster resource owned by this MongoDBSearch must carry. Owner refs | ||
| // don't cross cluster boundaries; labels are the only path back from a member | ||
| // resource to its central CR (for watch routing and label-based GC). | ||
| func searchOwnerLabels(search *searchv1.MongoDBSearch, clusterName string) map[string]string { |
There was a problem hiding this comment.
searchOwnerLabels, envoyLabelsForCluster (mongodbsearchenvoy_controller.go), and metricsForwarderLabelsForCluster (mongodbsearch_metrics_controller.go) build the same owner-name/namespace/uid/cluster map and differ only in app/component values. One searchManagedLabels(search, component, appName, clusterName) would remove the three-way drift risk. (Related nit: the 5-managed-kinds set is enumerated in ~4 places — watch wiring ×3 and the sweep's orderedKinds.)
|
|
||
| if mdbSearch.Status.Version == "" { | ||
| return workflow.Pending("Waiting for MongoDBSearch version to be reconciled") | ||
| func (r *MongoDBSearchMetricsForwarderReconciler) reconcileCore(ctx context.Context, mdbSearch *searchv1.MongoDBSearch, log *zap.SugaredLogger) workflow.Status { |
There was a problem hiding this comment.
reconcileCore is ~193 lines with interleaved deleting / cleaningRemovedOperator branches and finalizer removal inlined in multiple places. The read-once/write-once topology-state flow is a real improvement — but consider extracting the removed-operator-cluster path and the deletion/finalizer path into named functions so the happy path reads linearly.
| khandler.MongoDBSearchOwnerNamespaceLabel: nsName.Namespace, | ||
| } | ||
|
|
||
| clusters := []struct { |
There was a problem hiding this comment.
Nit: this 4-field anonymous struct type is written out three times (slice literal + appends). Naming it (type sweepCluster struct{ name string; listReader, parentReader client.Reader; deleteClient kubernetesClient.Client }) makes the sweep signature self-documenting.
| return errors.Join(errs, xerrors.Errorf("MongoDBSearch %s was recreated during cleanup; aborting %s deletion on cluster %q", search, kind, clusterName)) | ||
| } | ||
| log.Infof("Deleting %s %s for deleted MongoDBSearch %s on cluster %q", kind, obj.GetName(), search, clusterName) | ||
| if err := deleteClient.Delete(ctx, obj, client.Preconditions{ |
There was a problem hiding this comment.
Discussion: the UID precondition is the load-bearing guard here (delete only the exact incarnation). The resourceVersion precondition on top adds little — nothing should be mutating a resource whose owner is confirmed gone — and a benign concurrent update (e.g. kubelet status write) turns a clean delete into a 409 that fails the whole kind's sweep and requeues. Also, on the central cluster parentReader == uncachedReader, so the per-item eligibility re-read duplicates the sweep-top absence confirmation — N extra uncached GETs. Suggest: keep UID, drop RV, skip the central re-read.
| for k, v := range memberClustersMap { | ||
| clientsMap[k] = kubernetesClient.NewClient(v) | ||
| } | ||
| readerForMemberCluster := func(clusterName string) client.Reader { |
There was a problem hiding this comment.
Nit: uncachedReader/readerForMemberCluster are initialized to cached values here and only overridden with GetAPIReader() in AddMongoDBSearchController — so unit tests exercise different (cached) read semantics than production for the one field whose whole purpose is uncached live-reads. Prefer passing the API reader as a constructor argument set once.
| gone(search.MongotStatefulSetForClusterShard(0, "x-svc"), &appsv1.StatefulSet{}, "stale x-svc STS reaped despite name-collision with live x headless Service") | ||
| } | ||
|
|
||
| func TestOwnsForSweepRequiresCurrentSearchUID(t *testing.T) { |
There was a problem hiding this comment.
Redundant micro test — all three branches (current UID → sweep eligible; different UID / missing UID → preserve) are already pinned by TestCleanupStaleShardResources and, at reconciler level, by TestMongoDBSearchReconcile_NotFoundSweepsOwnedResourcesAcrossClusters. Testing a helper directly when the same behavior is observable through the reconcile path doubles the maintenance surface for zero coverage. Suggest deleting.
| } | ||
| } | ||
|
|
||
| func TestCleanupStaleShardResources_UnmanagedLB(t *testing.T) { |
There was a problem hiding this comment.
This and TestCleanupStaleShardResources_TLSDisabledDeletesCurrentUIDSecret each differ from TestCleanupStaleShardResources by exactly one input (LB mode / TLS off) — fold them in as table rows (~50 lines saved, one place to read the sweep matrix). Separately, the higher-value addition: the stale-shard sweep (including the new TLS-secret reaping) is only ever invoked directly in tests — no test reduces the shard set through a top-level Reconcile and asserts the sweep is actually wired into the reconcile path. One reconciler-level test would close that.
| } | ||
| } | ||
|
|
||
| func TestResourcesHandlerMapFuncEvents(t *testing.T) { |
There was a problem hiding this comment.
Event routing is proven at 2-3 levels: this table duplicates TestMongoDBSearchDeploymentWatchesRouteLifecycleEvents (controller-level, real registered predicates) and the delete rows of TestResourcesHandlerDelete. Suggest dropping this one and keeping TestResourcesHandlerDelete + a single create/update case as the watch-package unit; the controller-level test already proves the wiring end to end.
| assert.True(t, apierrors.IsNotFound(err), "expected cleanup of the persisted removed cluster before finalizer removal") | ||
| } | ||
|
|
||
| func TestReconcile_DeletionFailsClosedForRemovedClusterWithoutPersistedIndex(t *testing.T) { |
There was a problem hiding this comment.
Five near-duplicate reconciler tests → two tables: the three fails-closed variants (DeletionFailsClosedForRemovedClusterWithoutPersistedIndex, RemovedClusterWithoutPersistedIndexFailsClosed, RemovedPerClusterOperatorFailsClosedWithoutPersistedTopology) differ only in {deleting?, operatorClusterName, persisted-topology}; the two cleans-persisted variants (DeletionCleansPersistedRemovedClusterInPerClusterMode, RemovedPerClusterOperatorCleansPersistedResources) share the same shape. Table-driving both families saves ~90 lines and makes the removed-cluster/per-cluster-operator matrix legible at a glance.
| } | ||
| } | ||
|
|
||
| func TestMongoDBSearchReconcile_DeletionTimestampShortCircuits(t *testing.T) { |
There was a problem hiding this comment.
Two small things: (1) CachedNotFoundPreservesRecreatedSearchResources and NotFoundSweepStopsWhenSearchIsRecreated are a natural 2-row table (pre-sweep vs mid-sweep recreate). (2) A missing interaction test: the disable-reconciliation pause and the deletion path are each tested in isolation across all three controllers, but never together — does an annotation-paused CR still get its cleanup handling on delete, and in which order? One reconciler test pinning that ordering would prevent a silent behavior change.
|
Go test audit summary (companion to the inline notes). The reconciler-level core here is comprehensive and well-leveled: the NotFound-sweep matrix, the fail-closed/missing-GVK/UID-precondition cases, and the metrics deletion family are all driven through Reconcile, and the interceptor-based edge tests are legitimately unreachable through the public path. The cleanup opportunities are concentrated: (a) the owner-ref assertion churn inherited from #1362 (~40-50 lines rewritten in this PR), (b) two redundant micro tests ( |
Jira: KUBE-179
Stack
This is stack 2/4 for KUBE-154, based on #1362 (
search-kube154-label-sweep-lifecycle) at the accepted layer-1 head5aa0e085809d2d414485d122a7f789a3b6e0a265.Landing history
74af473be71058fa747a7e99d23a6674ccf8c9ff—KUBE-154: sweep deleted MongoDBSearch resources safely90e0eae8c5226ee8cb1259260aaf1d39fa675b3d—KUBE-154: route MongoDBSearch resource lifecycle eventsBoth commits are linear on the accepted layer-1 head, include their production behavior and focused tests together, and carry the Copilot co-author trailer.
Scope
NotFoundresults through uncached readers, then performs fail-closed, label-scoped cleanup across central and member clusters for Deployment, StatefulSet, Service, ConfigMap, and Secret only.Intentionally excluded
.evergreen.yml, oroperator_upgrade_search.py.Validation
go test ./controllers/operator ./controllers/searchcontroller ./pkg/handler ./pkg/tls -count=1.make fmt, repository formatter, andgit diff --checkpassed.make test-race: 2,926 tests passed, 3 skipped.6a5a282e8ee1c90007dbd20b: 67 of 68 tasks succeeded. The exact scope was 56 requested targets plus 12 dependencies, and all 68 generated tasks used execution setting 30.6a5a3c34df5cdd0007d4df78: all nine tasks succeeded (e2e_search_connectivity_tool_mc_shardedplus eight dependencies), with every generated task using execution setting 30.e2e_mdb_kind_ubi_cloudqa_largeSearch tasks; one Search-block task; one OpenShift Search task; four Ops Manager 8.0 metrics-forwarder tasks; two simulated multi-cluster tasks; five two-cluster Search tasks; one operator-upgrade Search task; seven Search snippet tasks; andunit_tests_golang.init_test_runimage/chart tasks plusbuild_om_imageson the Ops Manager 7.0 and 8.0 build variants.e2e_operator_kind_ubi_cloudqa/e2e_operator_upgrade_searchin both executions:test_database_running_after_upgradetimed out after 300 seconds whilemdb-rs-2remained at automation goal-1. This is the accepted pre-Layer-4 upgrade gap and was not retried or attributed to Layer 2.HEAD~2is exactly5aa0e085809d2d414485d122a7f789a3b6e0a265; the branch contains exactly the two commits listed above and has treee3b69354fa29f1466222ec1f138fe2315bd26455.Broad webhook patches are non-evidence. Automatic patch
6a5a1cbc50e4c1000765a00cwas canceled after its relevant connectivity tasks succeeded. Earlier targeted patch6a58d3d7ad18c500078c3722and exhaustive patch6a58dc67aa73bb00071726c2validated a superseded base/tree and are retained only as historical context.skip-ci