CLOUDP-324440 Save rs member ids #206

lucian-tosa · 2025-06-20T09:54:54Z

Summary

This PR fixes the behaviour of changing the project in a MongoDBMultiCluster resource. Multicluster resources have a different method of computing the member ids of each process in a replicaset (replicasets field in AC). Because of that, it is possible to get non-sequential ids by doing scaling operations. For example, we start from this

mongodb-0-0: id 0
mongodb-0-1: id 1
mongodb-1-0: id 2

If we now scale the first cluster up we end up with:

mongodb-0-0: id 0
mongodb-0-1: id 1
mongodb-1-0: id 2
mongodb-0-2: id 3

If we now change the project, the operator will find an empty AC and will rebuild it and the member ids in the replicaset. It will attempt to create them sequentially by setting id 2 to the member 0-2. This is not possible, and the automation agents will reject this change, leaving the deployment stuck.

In this PR we will now save the member ids we achieved in an annotation, and whenever there is an empty AC in OM, we try to read the annotation in case the deployment is being migrated to a new project.

Proof of Work

Added unit tests to check that annotations are created and updated successfully, as well as using the annotation when building the replicaset.
Also added an E2E test to ensure the resource will become ready.

Checklist

Have you linked a jira ticket and/or is the ticket in the title?
Have you checked whether your jira ticket required DOCSP changes?
Have you checked for release_note changes?

Reminder (Please remove this when merging)

Please try to Approve or Reject Changes the PR, keep PRs in review as short as possible
Our Short Guide for PRs: Link
Remember the following Communication Standards - use comment prefixes for clarity:
- blocking: Must be addressed before approval.
- follow-up: Can be addressed in a later PR or ticket.
- q: Clarifying question.
- nit: Non-blocking suggestions.
- note: Side-note, non-actionable. Example: Praise
- --> no prefix is considered a question

Copilot

Pull Request Overview

This PR fixes an issue with migrating a MongoDBMultiCluster resource to a new project by saving and restoring replica set member IDs in an annotation. It also updates test cases and refactors various functions to use the new GetReplicaSets naming convention.

Added a new constant for the replica set member IDs annotation.
Updated reconciliation logic to save and use member IDs during project changes.
Refactored tests and Om client methods for consistent API usage.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/util/constants.go	Introduces a new constant for saving RS member IDs.
docker/mongodb-kubernetes-tests/tests/multicluster/multi_cluster_scale_up_cluster.py	Updates test scenarios to account for scaled member counts and project changes.
controllers/operator/mongodbmultireplicaset_controller.go	Adds logic to retrieve and save RS member IDs from OM and updates the signature for saving the last achieved spec.
controllers/operator/mongodbmultireplicaset_controller_test.go	Adds new test cases to verify annotation persistence and proper member ID usage.
controllers/om/*	Adds new methods (e.g., MemberIds and GetReplicaSetMemberIds) and updates references to use the new GetReplicaSets API.
RELEASE_NOTES.md	Documents the bug fix related to project migration for MongoDBMultiCluster resources.

Comments suppressed due to low confidence (1)

controllers/operator/mongodbmultireplicaset_controller.go:815

[nitpick] The inner variable 'processIds' is a nested map; consider renaming it (for example to 'allMemberIds') to more clearly reflect its structure and improve code readability.

func getReplicaSetProcessIdsFromAnnotation(mrs mdbmultiv1.MongoDBMultiCluster) map[string]int {

Copilot · 2025-07-01T11:20:20Z

controllers/operator/mongodbmultireplicaset_controller.go

+	if err != nil {
+		return err
+	}
+	if string(rsMemberIdsBytes) != "null" {


[nitpick] Consider checking if rsMemberIds is nil or if the marshalled byte slice is empty instead of comparing to the string "null", as this would improve clarity and robustness if the serialization behavior changes.

Suggested change

if string(rsMemberIdsBytes) != "null" {

if rsMemberIds != nil && len(rsMemberIdsBytes) > 0 {

Good suggestion.

blocking: oh - yeah definitely please don't check for a null string. I think in other code places we have a better way of doing that

vinilage · 2025-07-01T11:24:04Z

RELEASE_NOTES.md

@@ -25,6 +25,9 @@
    * [Manage Database Users using OIDC](https://www.mongodb.com/docs/kubernetes/upcoming/manage-users/) # TODO
    * [Authentication and Authorization with OIDC/OAuth 2.0](https://www.mongodb.com/docs/manual/core/oidc/security-oidc/)

+## Bug Fixes


Can you please highlight better the introduction of OIDC? Checking the release notes, I see the new features but our main goal for this release is the OIDC, so I would suggest to add a section for OIDC. I will create a suggestion in GDocs and share with you.

Just did a proposal:
https://docs.google.com/document/d/143v_VS4graZZgGp2r63WzLcPH1tcv3OvShHtM5HPiSY/edit?usp=sharing

I would like to keep this PR focused on the bug fix. I think we can do this refactoring when we release. Wdyt?

nammn · 2025-07-01T13:42:37Z

controllers/operator/mongodbmultireplicaset_controller.go

@@ -699,6 +713,10 @@ func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Conte
 	}

 	processIds := getReplicaSetProcessIdsFromReplicaSets(mrs.Name, existingDeployment)
+	// If there is no replicaset configuration saved in OM, it might be a new project, so we check the ids saved in annotation
+	if len(processIds) == 0 {
+		processIds = getReplicaSetProcessIdsFromAnnotation(mrs)


why not use conn.GetReplicaSetMemberIds() here instead? Couldn't that save us the whole annotation work?

as discussed offline - its the migration case

@lucian-tosa Can you please explain the "migration case" that requires use of annotation?

blocking: let's add a comment here on why this can happen (the migration case) for clarity

lsierant

Generally - awesome fix! 👏
But I've left one blocking comment regarding not adding another method to the OM's connection interface. Consider the rest optional.

lsierant · 2025-07-01T13:37:05Z

controllers/om/omclient.go

@@ -273,6 +277,21 @@ func (oc *HTTPOmConnection) GetAgentAuthMode() (string, error) {
 	return ac.Auth.AutoAuthMechanism, nil
 }

+func (oc *HTTPOmConnection) GetReplicaSetMemberIds() (map[string]map[string]int, error) {


blocking: Do we need that separate method? Could we have an external function calling ReadDeployment and extracting the ids from the result?

I'd prefer to limit the interface of OMConnection to the distinct API operations and have them combined for the logic like this in a different layer above it.

With this we'd also avoic changing the mock's implementation

lsierant · 2025-07-01T13:39:43Z

controllers/operator/mongodbmultireplicaset_controller.go

+	if err != nil {
+		return err
+	}
+	if string(rsMemberIdsBytes) != "null" {


Good suggestion.

lsierant · 2025-07-01T13:41:33Z

controllers/operator/mongodbmultireplicaset_controller.go

+	processIds := make(map[string]map[string]int)
+	if processIdsStr, ok := mrs.Annotations[util.LastAchievedRsMemberIds]; ok {
+		if err := json.Unmarshal([]byte(processIdsStr), &processIds); err != nil {
+			return map[string]int{}


Did you consider making this a fatal error? If there is some garbage in the annotation, isn't it too risky to ignore its content?

if not - we should at least log this very clearly

lsierant · 2025-07-01T13:43:51Z

controllers/operator/mongodbmultireplicaset_controller_test.go

+		assert.Equal(t, mrs.GetAnnotations()[util.LastAchievedRsMemberIds], string(rsMemberIdsBytes))
+
+		// We simulate a changing the project by recreating the omConnection. The resource will keep the annotation.
+		// This part would have failed before 1.2.0.


no need to have it written, but a good practice is to manually check if this actually is true by running this unit test against master and verifying it's indeed failing here

lsierant · 2025-07-01T13:45:50Z

docker/mongodb-kubernetes-tests/tests/multicluster/multi_cluster_scale_up_cluster.py

+def new_project_configmap(namespace: str, project_name_prefix: str) -> str:
+    cm = read_configmap(namespace=namespace, name="my-project")
+    project_name = f"{project_name_prefix}-new-project"
+    return create_or_update_configmap(


again, reconfiguring the project! We need to followup on this PR...
I'm leaving the comment there to adjust it in here as well.

q: can you check whether those new projects are properly cleaned up in cloudmanager?

lsierant · 2025-07-01T13:47:45Z

docker/mongodb-kubernetes-tests/tests/multicluster/multi_cluster_scale_up_cluster.py

@@ -97,7 +131,6 @@ def test_ops_manager_has_been_updated_correctly_before_scaling():

 @pytest.mark.e2e_multi_cluster_scale_up_cluster
 def test_scale_mongodb_multi(mongodb_multi: MongoDBMulti, member_cluster_clients: List[MultiClusterClient]):
-    mongodb_multi.load()


to not need load() you must change mongodb_multi's fixture scope to "function" to load it every time it's injected into function

lsierant · 2025-07-01T13:50:04Z

docker/mongodb-kubernetes-tests/tests/multicluster/multi_cluster_scale_up_cluster.py

@@ -139,3 +159,59 @@ def test_ops_manager_has_been_updated_correctly_after_scaling():
 def test_replica_set_is_reachable(mongodb_multi: MongoDBMulti, ca_path: str):
    tester = mongodb_multi.tester()
    tester.assert_connectivity(opts=[with_tls(use_tls=True, ca_path=ca_path)])
+
+
+# From here on, the tests are for verifying that we can change the project of the MongoDBMulti resource even with


nit: I find this comment a bit unnecessary, but you don't need to remove it.
It would be better to wrap such a set of test functions into a class and name it properly, e.g. TestNonSequentialMemberIdsInReplicaSet or sth like that.

lsierant · 2025-07-01T13:51:18Z

docker/mongodb-kubernetes-tests/tests/multicluster/multi_cluster_scale_up_cluster.py

+    assert oldRsMembers == newRsMembers
+
+
+def sts_are_ready(mdb: MongoDBMulti, member_cluster_clients: List[MultiClusterClient], members: List[int]):


this looks to me like something we must already have somewhere in our tests. Or is it doing something different than just waiting for sts to be ready? If yes, then pls name it so we can see in the name why it's different.

lsierant · 2025-07-01T13:52:45Z

pkg/util/constants.go

+
+	// Annotation keys used by the operator
+	LastAchievedSpec        = "mongodb.com/v1.lastSuccessfulConfiguration"
+	LastAchievedRsMemberIds = "mongodb.com/v1.lastAchievedRsMemberIds"


@nammn to your concern about not putting the state into annotations - I agree, we should not do that. But mdbmc is still not migrated to the deployments state config map, and doing that should not be part of this PR. IMO it's fine to handle it this way, and migrate those annotations to the state cm once we decide to do so.

anandsyncs · 2025-07-01T13:34:32Z

controllers/om/mockedomclient.go

@@ -141,6 +141,22 @@ func (oc *MockedOmConnection) ConfigureProject(project *Project) {
 	oc.context.OrgID = project.OrgID
 }

+func (oc *MockedOmConnection) GetReplicaSetMemberIds() (map[string]map[string]int, error) {


Unrelated to this PR: Given that we are just copy pasting the the whole functionality, we can simply wrap the omclient with this mock client and only override what is necessary.

anandsyncs · 2025-07-01T13:53:56Z

controllers/operator/mongodbmultireplicaset_controller.go

@@ -699,6 +713,10 @@ func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Conte
 	}

 	processIds := getReplicaSetProcessIdsFromReplicaSets(mrs.Name, existingDeployment)
+	// If there is no replicaset configuration saved in OM, it might be a new project, so we check the ids saved in annotation
+	if len(processIds) == 0 {
+		processIds = getReplicaSetProcessIdsFromAnnotation(mrs)


@lucian-tosa Can you please explain the "migration case" that requires use of annotation?

nammn

awesome and simple code! Great tests and straight-forward. 2 blocking things and we are good to go

nammn · 2025-07-01T13:56:20Z

controllers/operator/mongodbmultireplicaset_controller.go

@@ -201,8 +201,14 @@ func (r *ReconcileMongoDbMultiReplicaSet) Reconcile(ctx context.Context, request
 		return r.updateStatus(ctx, &mrs, status, log)
 	}

+	// Save replicasets member ids in annotation


nit: we can remove this comment, we can find its usage via the ide

nammn · 2025-07-01T13:56:49Z

controllers/operator/mongodbmultireplicaset_controller.go

+	if err != nil {
+		return err
+	}
+	if string(rsMemberIdsBytes) != "null" {


blocking: oh - yeah definitely please don't check for a null string. I think in other code places we have a better way of doing that

nammn · 2025-07-01T13:57:34Z

controllers/operator/mongodbmultireplicaset_controller.go

@@ -699,6 +713,10 @@ func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Conte
 	}

 	processIds := getReplicaSetProcessIdsFromReplicaSets(mrs.Name, existingDeployment)
+	// If there is no replicaset configuration saved in OM, it might be a new project, so we check the ids saved in annotation
+	if len(processIds) == 0 {
+		processIds = getReplicaSetProcessIdsFromAnnotation(mrs)


blocking: let's add a comment here on why this can happen (the migration case) for clarity

nammn · 2025-07-01T13:57:58Z

controllers/operator/mongodbmultireplicaset_controller.go

+	processIds := make(map[string]map[string]int)
+	if processIdsStr, ok := mrs.Annotations[util.LastAchievedRsMemberIds]; ok {
+		if err := json.Unmarshal([]byte(processIdsStr), &processIds); err != nil {
+			return map[string]int{}


if not - we should at least log this very clearly

nammn · 2025-07-01T13:58:49Z

docker/mongodb-kubernetes-tests/tests/multicluster/multi_cluster_scale_up_cluster.py

+def new_project_configmap(namespace: str, project_name_prefix: str) -> str:
+    cm = read_configmap(namespace=namespace, name="my-project")
+    project_name = f"{project_name_prefix}-new-project"
+    return create_or_update_configmap(


q: can you check whether those new projects are properly cleaned up in cloudmanager?

nammn · 2025-07-01T13:59:08Z

pkg/util/constants.go

+
+	// Annotation keys used by the operator
+	LastAchievedSpec        = "mongodb.com/v1.lastSuccessfulConfiguration"
+	LastAchievedRsMemberIds = "mongodb.com/v1.lastAchievedRsMemberIds"


Export GetReplicaSets

ac55cad

lucian-tosa requested review from lsierant and Julien-Ben June 20, 2025 10:06

Save rs member ids in annotation

8ae690c

lucian-tosa force-pushed the CLOUDP-324440-fix-rs-ids branch from add6a32 to 8ae690c Compare June 30, 2025 15:48

lucian-tosa added 2 commits June 30, 2025 18:45

Unit test

c7f1de2

Merge branch 'master' into CLOUDP-324440-fix-rs-ids

9a587d6

lucian-tosa marked this pull request as ready for review July 1, 2025 07:07

lucian-tosa requested a review from a team as a code owner July 1, 2025 07:07

lucian-tosa requested review from nammn and removed request for Julien-Ben July 1, 2025 08:39

E2E test

1da9a93

lucian-tosa force-pushed the CLOUDP-324440-fix-rs-ids branch from f76fc3a to 1da9a93 Compare July 1, 2025 10:57

lucian-tosa added 2 commits July 1, 2025 13:01

Merge branch 'master' into CLOUDP-324440-fix-rs-ids

fc74a25

Release notes

fd44784

lucian-tosa requested a review from vinilage July 1, 2025 11:14

nammn requested a review from Copilot July 1, 2025 11:19

Copilot AI reviewed Jul 1, 2025

View reviewed changes

vinilage reviewed Jul 1, 2025

View reviewed changes

nammn reviewed Jul 1, 2025

View reviewed changes

lsierant mentioned this pull request Jul 1, 2025

Allow for more than one deployment per test under cloud-qa #173

Draft

3 tasks

lsierant requested changes Jul 1, 2025

View reviewed changes

anandsyncs reviewed Jul 1, 2025

View reviewed changes

nammn requested changes Jul 1, 2025

View reviewed changes

	if string(rsMemberIdsBytes) != "null" {
	if rsMemberIds != nil && len(rsMemberIdsBytes) > 0 {

		assert oldRsMembers == newRsMembers


		def sts_are_ready(mdb: MongoDBMulti, member_cluster_clients: List[MultiClusterClient], members: List[int]):

CLOUDP-324440 Save rs member ids #206

Are you sure you want to change the base?

CLOUDP-324440 Save rs member ids #206

Uh oh!

Conversation

lucian-tosa commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Proof of Work

Checklist

Reminder (Please remove this when merging)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsierant left a comment

Choose a reason for hiding this comment

Uh oh!

lsierant Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nammn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucian-tosa commented Jun 20, 2025 •

edited

Loading

lsierant Jul 1, 2025 •

edited

Loading