CLOUDP-353180: Refactor replica set controller state handling, and use helper pattern #544

Julien-Ben · 2025-10-21T11:20:40Z

Summary

This PR refactors the ReplicaSet controller to use the helper pattern and separate state reading/writing, matching what we already do in the ShardedCluster and OpsManager controllers.

This is a no-op refactor: no behavior changes, just reorganizing code to prepare for multi-cluster support.

What Changed

Added ReplicaSetReconcilerHelper

New helper struct that holds state for a single reconcile run
Main reconcile logic moved from the controller to the helper
Helper gets created fresh for each reconcile
This is the pattern we used in our other reconcilers

Separated State Operations

readState() - reads deployment state from annotations
writeState() - writes deployment state back to annotations ; write vault annotations only on successful reconciliation. In the same way it is done in the sharded controller. (note that vault is not supported for multi replica set at this point)
initialize() - loads state during helper creation
This is done to clearly show where we handle data persisted on the cluster, as opposed to reading and writing in multiple places during the reconciliation. This is important for multi-cluster support

Helper's updateStatus() Override

Writes deployment state after every status update
Keeps state consistent even when we return early
Same pattern as ShardedCluster controller

Added ReplicaSetDeploymentState

Holds the state we persist between reconciles
Just has LastAchievedSpec and status member count for now
The status is still read during the reconciliation loop, because of the scaler. Refactoring the scaler is also something that will be done as part of the epic

Not in This PR

These will (potentially) come in follow-up PRs, in the main feature branch for multi cluster support:

StateStore pattern (like ShardedCluster/OpsManager use)
Use ConfigMap for state persistence
State migration logic

Proof of work

Existing tests pass without changes

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 added changelog file?
- use skip-changelog label if not needed
- refer to Changelog files and Release Notes section in CONTRIBUTING.md

github-actions · 2025-10-21T11:21:47Z

⚠️ (this preview might not be accurate if the PR is not rebased on current master branch)

MCK 1.6.0 Release Notes

New Features

MongoDBCommunity: Added support to configure custom cluster domain via newly introduced spec.clusterDomain resource field. If spec.clusterDomain is not set, environment variable CLUSTER_DOMAIN is used as cluster domain. If the environment variable CLUSTER_DOMAIN is also not set, operator falls back to cluster.local as default cluster domain.
Helm Chart: Introduced two new helm fields operator.podSecurityContext and operator.securityContext that can be used to configure securityContext for Operator deployment through Helm Chart.

Bug Fixes

Fixed parsing of the customEnvVars Helm value when values contain = characters.
ReplicaSet: Blocked disabling TLS and changing member count simultaneously. These operations must now be applied separately to prevent configuration inconsistencies.

Other Changes

kubectl-mongodb plugin: cosign, the signing tool that is used to sign kubectl-mongodb plugin binaries, has been updated to version 3.0.2. With this change, released binaries will be bundled with .bundle files containing both signature and certificate information. For more information on how to verify signatures using new cosign version please refer to -> https://github.com/sigstore/cosign/blob/v3.0.2/doc/cosign_verify-blob.md

controllers/operator/mongodbreplicaset_controller.go

m1kola · 2025-10-27T10:18:04Z

controllers/operator/mongodbreplicaset_controller.go

+	if includeVaultAnnotations && vault.IsVaultSecretBackend() {
+		secrets := r.resource.GetSecretsMountedIntoDBPod()
+		vaultMap := make(map[string]string)
+		for _, s := range secrets {
+			path := fmt.Sprintf("%s/%s/%s", r.reconciler.VaultClient.DatabaseSecretMetadataPath(), r.resource.Namespace, s)
+			vaultMap = merge.StringToStringMap(vaultMap, r.reconciler.VaultClient.GetSecretAnnotation(path))
+		}
+		path := fmt.Sprintf("%s/%s/%s", r.reconciler.VaultClient.OperatorScretMetadataPath(), r.resource.Namespace, r.resource.Spec.Credentials)
+		vaultMap = merge.StringToStringMap(vaultMap, r.reconciler.VaultClient.GetSecretAnnotation(path))
+		for k, val := range vaultMap {
+			annotationsToAdd[k] = val
+		}
+	}


This is in writeState method. Is vault integration related to deployment state?

We were storing it in annotations on successful reconciliations previously so I treated it as state:

mongodb-kubernetes/controllers/operator/mongodbreplicaset_controller.go

Line 269 in 582d248

if vault.IsVaultSecretBackend() {

@Julien-Ben I don't know a lot about Vault integration, but it seems unrelated to the deployment state to me. I also see that a similar piece of code in the sharded controller is is part of ShardedClusterReconcileHelper.Reconcile method:

mongodb-kubernetes/controllers/operator/mongodbshardedcluster_controller.go

Lines 928 to 940 in 582d248

if vault.IsVaultSecretBackend() {

secrets := sc.GetSecretsMountedIntoDBPod()

vaultMap := make(map[string]string)

for _, s := range secrets {

path := fmt.Sprintf("%s/%s/%s", r.commonController.VaultClient.DatabaseSecretMetadataPath(), sc.Namespace, s)

vaultMap = merge.StringToStringMap(vaultMap, r.commonController.VaultClient.GetSecretAnnotation(path))

}

path := fmt.Sprintf("%s/%s/%s", r.commonController.VaultClient.OperatorScretMetadataPath(), sc.Namespace, sc.Spec.Credentials)

vaultMap = merge.StringToStringMap(vaultMap, r.commonController.VaultClient.GetSecretAnnotation(path))

for k, val := range vaultMap {

annotationsToAdd[k] = val

}

}

Should we follow the same pattern?

I separated them, you were right.
Vault just needs to read annotations, and they are not state as we don't rely on them ourselves. They should be written only upon successful reconcile

a777b2f

I also changed how we write to lastAchievedSpec
It is actually an annotation we should change only when we achieve Running phase, not on every reconcile.

controllers/operator/mongodbreplicaset_controller.go

…ult annotations

Some unit tests for the annotations

Julien-Ben · 2025-10-29T15:55:08Z

controllers/operator/mongodbreplicaset_controller_test.go

 	testPVCFinishedResizing(t, ctx, memberClient, p, reconciledResource, statefulSet, logger)
 }

+// ===== Test for state and vault annotations handling in replicaset controller =====


@m1kola FYI after your review I added these unit tests

Resolved conflicts: - Removed TLS lock members logic (now blocked by validation per CLOUDP-349087) - Kept helper pattern refactoring with ReplicaSetReconcilerHelper - Removed test for deleted updateOmDeploymentDisableTLSConfiguration function

Julien-Ben · 2025-10-29T16:17:48Z

api/v1/mdb/mongodb_types.go

 }

 // GetLastAdditionalMongodConfigByType returns the last successfully achieved AdditionalMongodConfigType for the given component.
 func (m *MongoDB) GetLastAdditionalMongodConfigByType(configType AdditionalMongodConfigType) (*AdditionalMongodConfig, error) {


This function is now used by the standalone controller only.
The reason we don't want it any more is because it acceeds the spec with m.GetLastSpec()

We now do it only once per reconcile

m1kola

Looks good. The only concern is an unnecessary write because of two annotations.SetAnnotations calls.

m1kola · 2025-10-30T10:24:36Z

controllers/operator/mongodbreplicaset_controller.go

-		vaultMap = merge.StringToStringMap(vaultMap, r.VaultClient.GetSecretAnnotation(path))
-		for k, val := range vaultMap {
-			annotationsToAdd[k] = val
+	if err := r.writeVaultAnnotations(ctx); err != nil {


Both writeState and writeVaultAnnotations call annotations.SetAnnotations which gets the object and patches it.

If we can reduce write operations to the API server - that would be great.

m1kola · 2025-10-30T10:33:10Z

controllers/operator/mongodbreplicaset_controller.go

+	// Read current member count from Status once at initialization. This provides a stable view throughout
+	// reconciliation and prepares for eventually storing this in ConfigMap state instead of ephemeral status.
+	memberCountBefore := r.resource.Status.Members
+
+	return &ReplicaSetDeploymentState{
+		LastAchievedSpec:         lastAchievedSpec,
+		LastReconcileMemberCount: memberCountBefore,
+	}, nil


nit: Old name in the var memberCountBefore. Alternatively we can just get rid of it:

Suggested change

// Read current member count from Status once at initialization. This provides a stable view throughout

// reconciliation and prepares for eventually storing this in ConfigMap state instead of ephemeral status.

memberCountBefore := r.resource.Status.Members

return &ReplicaSetDeploymentState{

LastAchievedSpec: lastAchievedSpec,

LastReconcileMemberCount: memberCountBefore,

}, nil

return &ReplicaSetDeploymentState{

LastAchievedSpec: lastAchievedSpec,

// Read current member count from Status once at initialization. This provides a stable view throughout

// reconciliation and prepares for eventually storing this in ConfigMap state instead of ephemeral status.

LastReconcileMemberCount: r.resource.Status.Members,

}, nil

m1kola · 2025-10-30T10:38:21Z

controllers/operator/mongodbreplicaset_controller.go

+type ReplicaSetDeploymentState struct {
+	LastAchievedSpec         *mdbv1.MongoDbSpec `json:"lastAchievedSpec"`
+	LastReconcileMemberCount int                `json:"memberCountBefore"`
+}


Nit: does it need to be exportable?

Julien-Ben added 5 commits October 21, 2025 11:06

Refactor state reading

90387b0

Refactor state writing

bf0ead1

Refactor to helper pattern

ab6e334

Moved all methods to helper

a3fae90

Use helper pattern for reconcileHostnameOverrideConfigMap

e714e04

Julien-Ben added the skip-changelog Use this label in Pull Request to not require new changelog entry file label Oct 21, 2025

Julien-Ben added 7 commits October 21, 2025 13:25

Lint

430cb51

Rename variables for consistency

894ef00

typo

4bb174c

Rename r := h.reconciler to reconciler

0ae98c8

Rename helper receivers to r

95f0686

Remove TODOs

08ffa3e

Refactor onDelete

c99c221

Julien-Ben mentioned this pull request Oct 21, 2025

WIP Helper pattern #537

Closed

3 tasks

Julien-Ben added 2 commits October 21, 2025 15:33

Move comments

5e39e4a

Update vault annotations only when reconciliation is successful

279a7ac

Julien-Ben changed the title ~~Refactor replica set controller state handling, and use helper pattern~~ CLOUDP-353180: Refactor replica set controller state handling, and use helper pattern Oct 22, 2025

Julien-Ben added 2 commits October 22, 2025 14:46

Handle PVC Resize in a separate method

5f5cebd

Merge branch 'master' into jben/refactor-state-and-helper-pattern

e88439b

Julien-Ben marked this pull request as ready for review October 22, 2025 12:48

Julien-Ben requested a review from a team as a code owner October 22, 2025 12:48

Julien-Ben requested review from lsierant, lucian-tosa, m1kola and viveksinghggits October 22, 2025 12:48

Add status member count to the state

b831d72

m1kola reviewed Oct 27, 2025

View reviewed changes

Julien-Ben added 2 commits October 28, 2025 10:26

Uncapitalize errors

ac8fb33

Write lastAchievedSpec only on successful reconciliation, separate va…

a777b2f

…ult annotations

Julien-Ben added 2 commits October 28, 2025 16:32

Rename MemberCountBefore

d5bdd18

Some unit tests for the annotations

bbb173b

Some unit tests for the annotations

Julien-Ben force-pushed the jben/refactor-state-and-helper-pattern branch from 8366804 to bbb173b Compare October 29, 2025 15:53

Julien-Ben commented Oct 29, 2025

View reviewed changes

Julien-Ben force-pushed the jben/refactor-state-and-helper-pattern branch from 641056f to 6ce9706 Compare October 29, 2025 16:16

Julien-Ben commented Oct 29, 2025

View reviewed changes

lucian-tosa approved these changes Oct 29, 2025

View reviewed changes

lint

2b0d2be

m1kola reviewed Oct 30, 2025

View reviewed changes

	if vault.IsVaultSecretBackend() {
	secrets := sc.GetSecretsMountedIntoDBPod()
	vaultMap := make(map[string]string)
	for _, s := range secrets {
	path := fmt.Sprintf("%s/%s/%s", r.commonController.VaultClient.DatabaseSecretMetadataPath(), sc.Namespace, s)
	vaultMap = merge.StringToStringMap(vaultMap, r.commonController.VaultClient.GetSecretAnnotation(path))
	}
	path := fmt.Sprintf("%s/%s/%s", r.commonController.VaultClient.OperatorScretMetadataPath(), sc.Namespace, sc.Spec.Credentials)
	vaultMap = merge.StringToStringMap(vaultMap, r.commonController.VaultClient.GetSecretAnnotation(path))
	for k, val := range vaultMap {
	annotationsToAdd[k] = val
	}
	}

CLOUDP-353180: Refactor replica set controller state handling, and use helper pattern #544

Are you sure you want to change the base?

CLOUDP-353180: Refactor replica set controller state handling, and use helper pattern #544

Conversation

Julien-Ben commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Changed

Added ReplicaSetReconcilerHelper

Separated State Operations

Helper's updateStatus() Override

Added ReplicaSetDeploymentState

Not in This PR

Proof of work

Checklist

Uh oh!

github-actions bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MCK 1.6.0 Release Notes

New Features

Bug Fixes

Other Changes

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1kola 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Julien-Ben commented Oct 21, 2025 •

edited

Loading

github-actions bot commented Oct 21, 2025 •

edited

Loading