KUBE-154: establish MongoDBSearch lifecycle identity#1362
Conversation
MCK 1.10.0 Release NotesNew Features
Bug Fixes
|
anandsyncs
added
the
skip-changelog
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Preserve label-routed Search lifecycle watches while incorporating upstream per-cluster status, healthy-cluster handling, toolchain updates, and metrics-forwarder enablement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
294c0e8 to
a8b5018
Compare
anandsyncs
changed the title
a56bea4 to
42c8208
Compare
There was a problem hiding this comment.
Pull request overview
Establishes stable MongoDBSearch lifecycle identity by stamping and protecting Search owner/name/namespace/UID/cluster/component labels on operator-managed resources, and by adding explicit “missing/matching/mismatched UID” adoption semantics for persisted Search/metrics state.
Changes:
- Introduces protected Search identity labels (including
mongodb.com/search-uidandcomponent) and a helper to reapply them after CR metadata overrides while preserving unrelated labels. - Adds UID-aware adoption/reset semantics to Search state (ConfigMap) and metrics-forwarder topology state (StateStore-backed ConfigMap), preserving legacy state when UID markers are absent.
- Extends unit test coverage for label protection on overrides and for UID-guarded state adoption/reset behavior.
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 | Extends TLS secret creation/update to stamp provided labels on operator-managed TLS Secret copies. |
| pkg/handler/labels_search.go | Adds Search UID/component label constants and ReapplyProtectedSearchLabels helper for protected identity restoration. |
| controllers/searchcontroller/search_state.go | Adds UID-guarded state read/mutate behavior; stamps identity labels + preserves existing owner refs. |
| controllers/searchcontroller/search_state_test.go | Adds table-driven tests for Search state UID adoption/reset + identity stamping. |
| controllers/searchcontroller/search_construction.go | Updates comments around StatefulSet override ordering and protected label restoration. |
| controllers/searchcontroller/mongodbsearch_reconcile_helper.go | Stamps Search identity across resources; restores protected labels after overrides; passes identity labels into TLS secret creation. |
| controllers/searchcontroller/mongodbsearch_reconcile_helper_test.go | Adds tests for override label protection and managed resource/TLS Secret identity stamping. |
| controllers/operator/state_store.go | Adds optional owner-UID marker semantics to StateStore and preserves existing owner refs on UID mismatch. |
| controllers/operator/mongodbshardedcluster_controller.go | Updates StateStore construction for new signature (no UID semantics). |
| controllers/operator/mongodbopsmanager_controller.go | Updates StateStore construction for new signature (no UID semantics). |
| controllers/operator/appdbreplicaset_controller.go | Updates StateStore construction for new signature (no UID semantics). |
| controllers/operator/mongodbsearchenvoy_controller.go | Ensures Envoy Deployment labels restore protected Search identity after deployment overrides. |
| controllers/operator/mongodbsearchenvoy_controller_test.go | Adds tests validating protected label restoration and UID/component stamping. |
| controllers/operator/mongodbsearch_metrics_controller.go | Adds UID-aware StateStore usage for topology state and restores protected labels after deployment overrides. |
| controllers/operator/mongodbsearch_metrics_controller_test.go | Adds tests validating UID adoption/reset semantics and identity labels on metrics state + deployments. |
| controllers/operator/mongodbsearch_controller.go | Switches from read-only Search state read to no-op mutate to stamp/adopt identity metadata safely. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
42c8208 to
5aa0e08
Compare
| } | ||
|
|
||
| ownerReferences := s.ownerReferences | ||
| if s.ownerUID != "" && len(s.readOwnerRefs) > 0 { |
There was a problem hiding this comment.
Stack-boundary churn (applies to the whole owner-ref layer in this PR): this readOwnerRefs merge machinery — plus the dual-owner-ref adoption in search_state.go MutateSearchState, searchResourceIdentityLabels in mongodbsearch_reconcile_helper.go, the owner-ref plumbing in pkg/tls/tls_secret.go, and their dedicated tests — is deleted wholesale by #1382 in the next layer of this stack. Since the layers land together, consider trimming this PR to the durable primitives only (the state UID data key, the mongodb.com/search-uid label, the labels param on EnsureTLSSecret, and the UID-mismatch reset) and leaving owner-ref handling untouched so #1382 removes it in one motion. Zero behavior delta at the tip of the stack, and it removes a few hundred lines of throwaway review surface from both PRs.
|
|
||
| cm := &corev1.ConfigMap{} | ||
| require.NoError(t, c.Get(ctx, stateCMName, cm)) | ||
| require.Len(t, cm.OwnerReferences, 2) |
There was a problem hiding this comment.
Test-side of the stack-boundary churn (companion to the state_store.go comment): the owner-reference assertions added here — plus the Len(2)/controller-ref asserts in the metrics TestOpenTopologyStateStore_UIDSemantics and the owner-ref asserts added to TestReconcileReplicaSet_CreatesResources/TestReconcile_IngressTLSSecretIdentity/TestEnsureX509ClientCertConfig_* — are all rewritten to assert.Empty(OwnerReferences) by #1382. That's ~40-50 test lines pinning a design that never reaches the tip of the stack. If this PR is trimmed to the durable primitives, these tests should assert the label-based end state once and never mention owner refs.
viveksinghggits
left a comment
viveksinghggits
left a comment
There was a problem hiding this comment.
looks pretty good.
| "component": metricsForwarderLabelName, | ||
| khandler.MongoDBSearchOwnerNameLabel: search.Name, | ||
| "app": search.MetricsForwarderDeploymentNameForCluster(clusterIndex), | ||
| khandler.MongoDBSearchComponentLabel: metricsForwarderLabelName, |
There was a problem hiding this comment.
naming of this const metricsForwarderLabelName is not part of this PR but it should be metricsForwarderLabelValue right? Let's change that maybe?
| dep.Labels = merge.StringToStringMap(dep.Labels, managedLB.Deployment.MetadataWrapper.Labels) | ||
| dep.Annotations = merge.StringToStringMap(dep.Annotations, managedLB.Deployment.MetadataWrapper.Annotations) | ||
| } | ||
| dep.Labels = khandler.ReapplyProtectedSearchLabels(dep.Labels, labels) |
There was a problem hiding this comment.
Should ReapplyProtectedSearchLabels not be called appendProtectedSearchlabels or something like that? Or reapply is significant?
| resourceName string | ||
| ownerLabels map[string]string | ||
| ownerReferences []metav1.OwnerReference | ||
| readOwnerRefs []metav1.OwnerReference |
There was a problem hiding this comment.
q: what does readOwnerRefs have?
| return labels | ||
| } | ||
|
|
||
| func searchResourceIdentityLabels(search *searchv1.MongoDBSearch, clusterName string) map[string]string { |
There was a problem hiding this comment.
I like the idea of UID but was just wondering if we get into any issues if we just relied on name/namespace combination to figure out if a particular search resource created the children resource.
|
|
||
| func searchResourceIdentityLabels(search *searchv1.MongoDBSearch, clusterName string) map[string]string { | ||
| labels := searchOwnerLabels(search, clusterName) | ||
| labels[khandler.MongoDBSearchOwnerUIDLabel] = string(search.UID) |
There was a problem hiding this comment.
should this also add the component: mongot label?
| func searchStateHasCurrentUID(cm *corev1.ConfigMap, search *searchv1.MongoDBSearch) bool { | ||
| recordedUID, ok := cm.Labels[khandler.MongoDBSearchOwnerUIDLabel] | ||
| return !ok || recordedUID == string(search.UID) | ||
| } |
There was a problem hiding this comment.
nit:
| func searchStateHasCurrentUID(cm *corev1.ConfigMap, search *searchv1.MongoDBSearch) bool { | |
| recordedUID, ok := cm.Labels[khandler.MongoDBSearchOwnerUIDLabel] | |
| return !ok || recordedUID == string(search.UID) | |
| } | |
| func searchStateHasUID(cm *corev1.ConfigMap, uid *type) bool { | |
| recordedUID, ok := cm.Labels[khandler.MongoDBSearchOwnerUIDLabel] | |
| return !ok || recordedUID == string(search.UID) | |
| } |
I don't think having current is important here without defining what current is.
| } | ||
| } | ||
| if !mutate(state) { | ||
|
|
There was a problem hiding this comment.
I think some small comment here would be helpful for later, to understand what's happening.
Summary
This is stack 1/4 for KUBE-154. It establishes stable MongoDBSearch resource identity and safe legacy state adoption before stack 2 atomically adds label-sweep cleanup, owner-reference removal, and lifecycle watches.
The change protects the Search owner name, namespace, UID, cluster, and component labels on operator-managed resources; reapplies those labels after user metadata overrides without removing unrelated labels; and gives Search and metrics state explicit missing/matching/mismatched UID semantics. Operator-managed ingress TLS Secret copies now carry
component=mongotconsistently for both replica-set and sharded Search topologies.Scope
component=mongotidentity to every operator-managed ingress TLS Secret copy; legacy single-cluster copies intentionally omit the cluster labelIntentionally excluded
Landing history
e0d3deb2a100ef1dc8c6f7026f910bf481f52d61—KUBE-154: establish MongoDBSearch resource identity5aa0e085809d2d414485d122a7f789a3b6e0a265—KUBE-154: adopt legacy MongoDBSearch stateBoth commits are linear on
master, include production changes and their focused tests together, carry the required co-author trailer, and pass their focused package tests independently. The state-adoption commit has the same stable patch ID as the previously accepted adoption commit.Proof of Work
Local
go test ./controllers/searchcontroller ./controllers/operator ./pkg/handler ./pkg/tlsmake test-race: 2,849 passed, 3 existing skipsmake fmt,gofmt, andgit diff --checkcleanTargeted Evergreen
6a57ea959edf8c0007894491: five unit tasks passed;lint_repoidentified one import-group formatting correction that was folded into the state-adoption commit.6a57ef4e09dd6b00078bf33c: targetedunit_tests/lint_repoconfirmation passed.Exhaustive Search matrix
Patch
6a5a08a157cb640007c484b4ran exactly 56 requested targets (55 Search plusunit_tests_golang) and 12 required build dependencies.e2e_mdb_kind_ubi_cloudqa_largee2e_mdb_kind_ubi_cloudqa_search_blocke2e_mdb_openshift_search_ubi_cloudqae2e_om80_kind_ubi_largee2e_multi_cluster_kinde2e_multi_cluster_2_clustersprivate_kind_code_snippetsunit_testsunit_tests_golange2e_operator_kind_ubi_cloudqae2e_operator_upgrade_searchThe sole failure was task
mongodb_kubernetes_e2e_operator_kind_ubi_cloudqa_e2e_operator_upgrade_search_patch_c040c80dd4767cfecdbd60a529bcf15ff558a76a_6a5a08a157cb640007c484b4_26_07_17_10_49_46, executions 0 and 1. In both executions, all Search upgrade checks passed—including post-upgrade SearchRunningand mongot version verification—buttest_database_running_after_upgradetimed out after 300 seconds waiting for AppDB membermdb-rs-2to reach Ops Manager automation-config goal state. The agent remained atWaitHasCorrectAutomationCredentialsfor automation config version 9. This repeats the prior unrelated AppDB post-upgrade convergence failure; it is not a layer-1 failure or the anticipated layer-4 Search migration gap.A configuration-audit patch,
6a5a0747b478880007d994ef, was canceled as non-evidence when generated scope showed the patch-only upgrade task was omitted. The corrected authoritative patch above generated all 68 tasks. A focused retry submission was blocked by expired Evergreen device authorization; no device approval was attempted and no retry patch was created.Focused coverage follow-up
After rebased master
c040c80dd4767cfecdbd60a529bcf15ff558a76amadee2e_search_connectivity_tool_mc_shardedexecutable in patches, exact-head patch6a5a4679d981fd00073602c7ran that task plus eight normal dependencies at accepted head5aa0e085809d2d414485d122a7f789a3b6e0a265. All 9/9 tasks passed on execution 0 with no retries.The target task was
mongodb_kubernetes_e2e_multi_cluster_2_clusters_e2e_search_connectivity_tool_mc_sharded_patch_c040c80dd4767cfecdbd60a529bcf15ff558a76a_6a5a4679d981fd00073602c7_26_07_17_15_13_06. This closes the original exhaustive matrix’s obsolete quarantine exclusion without rerunning that matrix.Temporary activation for the upgrade task was not committed; the branch is clean at
5aa0e085809d2d414485d122a7f789a3b6e0a265.Automatic broad webhook patches are not validation evidence.
The full pre-stack implementation remains preserved at
kube154-label-sweep-full-source-294c(294c0e8e3e6412ff2c76ba990fac9f0e6d649a76) for reconstruction of later stack layers.Checklist
skip-changeloglabel.skip-ci