-
Notifications
You must be signed in to change notification settings - Fork 21
CLOUDP-347497: Single cluster Replica Set Controller Refactoring #486
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
base: master
Are you sure you want to change the base?
Conversation
MCK 1.5.0 Release NotesNew Features
Bug Fixes
|
This reverts commit 073032b.
Builder functions Test works
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.
Early review: I had a quick-ish look at the controllers and couldn't spot anything obviously wrong with it. I didn't review the tests at all.
Looks good so far.
} | ||
} | ||
|
||
publishAutomationConfigFirst, err := r.publishAutomationConfigFirstMultiCluster(ctx, &mrs, log) |
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.
Brought it closer to where it's actually used
log.Debugw("Marked replica set members as non-voting", "replica set with members", rsMembers) | ||
} | ||
|
||
// TODO practice shows that automation agents can get stuck on setting db to "disabled" also it seems that this process |
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.
This comment was old (<2022)
// Reconcile reads that state of the cluster for a MongoDbReplicaSet object and makes changes based on the state read | ||
// and what is in the MongoDbReplicaSet.Spec | ||
func (r *ReconcileMongoDbReplicaSet) Reconcile(ctx context.Context, request reconcile.Request) (res reconcile.Result, e error) { | ||
// === 1. Initial Checks and setup |
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 added these as helpers for myself at the beginning of the refactoring, to keep track of where I was in the hundreds of lines of the reconcile loop
I'm okay to remove them now if they feel like noise more than useful comments
CLOUDP-347497 - Single cluster Replica Set Controller Refactoring
Why this refactoring
The single-cluster RS controller was mixing two concerns:
This worked fine for single-cluster, but it's a problem when you think about multi-cluster:
So we need to separate these layers properly.
Main changes
1. Broke down the huge Reconcile() method
Before: ~300 lines of inline logic in Reconcile()
Now:
This makes it way easier to understand what's happening and matches the multi-cluster controller structure.
2. Removed StatefulSet dependency from OM operations
Created new helper functions that work directly with MongoDB resources instead of StatefulSets:
CreateMongodProcessesFromMongoDB()
- was using StatefulSet beforeBuildFromMongoDBWithReplicas()
- sameWaitForRsAgentsToRegisterByResource()
- sameThese mirror the existing
...FromStatefulSet
functions but take MongoDB resources instead.Why it matters: The OM layer now only cares about the MongoDB resource definition, not how it's deployed in K8s. This makes the code work the same way for both single-cluster and multi-cluster.
3. Added publishAutomationConfigFirstRS checks
Dedicated function for RS instead of using the shared one. Does not rely on a statefulset object.
Important for review
The ideal way to review this PR is to compare the new structure to the
mongodbmultireplicaset_controller.go
. The aim of the refactoring is to get the single cluster controller closer to it.Look at:
reconcileMemberResources()
in both controllers - similar structure nowupdateOmDeploymentRs()
- no more StatefulSet dependencyom/process
andom/replicaset
- mirror existing patternsBug found along the way
The logic to handle scale up + disable TLS at the same time doesn't actually work properly and should be blocked by validation (see draft PR #490 for more details).
Tests added
Added tests for the new helper functions:
TestCreateMongodProcessesFromMongoDB
- basic scenarios, scaling, custom domains, TLS, additional configTestBuildFromMongoDBWithReplicas
- integration test checking ReplicaSet structure and member options propagationTestPublishAutomationConfigFirstRS
- automation config publish logic with various TLS/auth scenarios