-
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
Open
Julien-Ben
wants to merge
26
commits into
master
Choose a base branch
from
jben/rs-controller-refactor-clean-rebase
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
a6b9d48
RS Controller refactor fixed merge conflicts
Julien-Ben 4196529
Fix and improve certificates handling
Julien-Ben 1eead94
Pass current agent auth mode
Julien-Ben e913144
currentAgentAuthMode in deploymentOptions
Julien-Ben 74cc89a
PrepareScaleDown without need for sts
Julien-Ben dd4756f
Pass configmap to publishAutomationConfigFirstRS directly
Julien-Ben c3ef579
Lint
Julien-Ben e41d24a
Commit TODOs
Julien-Ben c89617b
Remove TODOs
Julien-Ben eb78918
Lint
Julien-Ben 073032b
Edge case TLS disabled and rs scaled
Julien-Ben 8c4f229
Remove unused lines
Julien-Ben 691d804
Import order
Julien-Ben 31daeae
Fix comment
Julien-Ben 310030e
Run applySearchOverrides early
Julien-Ben 82467d6
Revert "Edge case TLS disabled and rs scaled"
Julien-Ben c7268d1
Revert to controller gen 0.18
Julien-Ben e302990
Fix edge case
Julien-Ben 02920f8
TestPublishAutomationConfigFirstRS
Julien-Ben 580927f
Remove old comment
Julien-Ben ea14853
TestCreateMongodProcessesFromMongoDB
Julien-Ben 1376b5d
TestBuildFromMongoDBWithReplicas
Julien-Ben 3c30758
Revert unrelated changed
Julien-Ben e2d2e33
Lint`
Julien-Ben 1c1446a
Merge branch 'master' into jben/rs-controller-refactor-clean-rebase
Julien-Ben 96e6dc0
Update doc
Julien-Ben File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,230 @@ | ||
package process | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
|
||
mdbv1 "github.com/mongodb/mongodb-kubernetes/api/v1/mdb" | ||
"github.com/mongodb/mongodb-kubernetes/pkg/util/maputil" | ||
) | ||
|
||
const ( | ||
defaultMongoDBImage = "mongodb/mongodb-enterprise-server:7.0.0" | ||
defaultFCV = "7.0" | ||
defaultNamespace = "test-namespace" | ||
) | ||
|
||
func TestCreateMongodProcessesFromMongoDB(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
mdb *mdbv1.MongoDB | ||
limit int | ||
mongoDBImage string | ||
forceEnterprise bool | ||
fcv string | ||
tlsCertPath string | ||
expectedCount int | ||
expectedHostnames []string | ||
expectedNames []string | ||
}{ | ||
{ | ||
name: "3-member replica set", | ||
mdb: baseReplicaSet("test-rs", 3), | ||
limit: 3, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 3, | ||
expectedHostnames: []string{ | ||
"test-rs-0.test-rs-svc.test-namespace.svc.cluster.local", | ||
"test-rs-1.test-rs-svc.test-namespace.svc.cluster.local", | ||
"test-rs-2.test-rs-svc.test-namespace.svc.cluster.local", | ||
}, | ||
expectedNames: []string{"test-rs-0", "test-rs-1", "test-rs-2"}, | ||
}, | ||
{ | ||
name: "Single member replica set", | ||
mdb: baseReplicaSet("single-rs", 1), | ||
limit: 1, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 1, | ||
expectedHostnames: []string{ | ||
"single-rs-0.single-rs-svc.test-namespace.svc.cluster.local", | ||
}, | ||
expectedNames: []string{"single-rs-0"}, | ||
}, | ||
{ | ||
name: "Limit less than members (scale up in progress)", | ||
mdb: baseReplicaSet("scale-up-rs", 5), | ||
limit: 3, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 3, | ||
expectedHostnames: []string{ | ||
"scale-up-rs-0.scale-up-rs-svc.test-namespace.svc.cluster.local", | ||
"scale-up-rs-1.scale-up-rs-svc.test-namespace.svc.cluster.local", | ||
"scale-up-rs-2.scale-up-rs-svc.test-namespace.svc.cluster.local", | ||
}, | ||
expectedNames: []string{"scale-up-rs-0", "scale-up-rs-1", "scale-up-rs-2"}, | ||
}, | ||
{ | ||
name: "Limit greater than members (scale down in progress)", | ||
mdb: baseReplicaSet("scale-down-rs", 3), | ||
limit: 5, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 5, | ||
expectedHostnames: []string{ | ||
"scale-down-rs-0.scale-down-rs-svc.test-namespace.svc.cluster.local", | ||
"scale-down-rs-1.scale-down-rs-svc.test-namespace.svc.cluster.local", | ||
"scale-down-rs-2.scale-down-rs-svc.test-namespace.svc.cluster.local", | ||
"scale-down-rs-3.scale-down-rs-svc.test-namespace.svc.cluster.local", | ||
"scale-down-rs-4.scale-down-rs-svc.test-namespace.svc.cluster.local", | ||
}, | ||
expectedNames: []string{"scale-down-rs-0", "scale-down-rs-1", "scale-down-rs-2", "scale-down-rs-3", "scale-down-rs-4"}, | ||
}, | ||
{ | ||
name: "Limit zero creates empty slice", | ||
mdb: baseReplicaSet("empty-rs", 3), | ||
limit: 0, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 0, | ||
expectedHostnames: []string{}, | ||
expectedNames: []string{}, | ||
}, | ||
{ | ||
name: "Custom cluster domain", | ||
mdb: func() *mdbv1.MongoDB { | ||
rs := baseReplicaSet("custom-domain-rs", 2) | ||
rs.Spec.ClusterDomain = "my-cluster.local" | ||
return rs | ||
}(), | ||
|
||
limit: 2, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 2, | ||
expectedHostnames: []string{ | ||
"custom-domain-rs-0.custom-domain-rs-svc.test-namespace.svc.my-cluster.local", | ||
"custom-domain-rs-1.custom-domain-rs-svc.test-namespace.svc.my-cluster.local", | ||
}, | ||
expectedNames: []string{"custom-domain-rs-0", "custom-domain-rs-1"}, | ||
}, | ||
{ | ||
name: "Different namespace", | ||
mdb: func() *mdbv1.MongoDB { | ||
rs := baseReplicaSet("other-ns-rs", 2) | ||
rs.Namespace = "production" | ||
return rs | ||
}(), | ||
limit: 2, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
expectedCount: 2, | ||
expectedHostnames: []string{ | ||
"other-ns-rs-0.other-ns-rs-svc.production.svc.cluster.local", | ||
"other-ns-rs-1.other-ns-rs-svc.production.svc.cluster.local", | ||
}, | ||
expectedNames: []string{"other-ns-rs-0", "other-ns-rs-1"}, | ||
}, | ||
{ | ||
name: "With TLS cert path", | ||
mdb: func() *mdbv1.MongoDB { | ||
rs := baseReplicaSet("tls-rs", 2) | ||
rs.Spec.Security = &mdbv1.Security{ | ||
TLSConfig: &mdbv1.TLSConfig{Enabled: true}, | ||
} | ||
return rs | ||
}(), | ||
limit: 2, | ||
mongoDBImage: defaultMongoDBImage, | ||
forceEnterprise: false, | ||
fcv: defaultFCV, | ||
tlsCertPath: "/path/to/cert.pem", | ||
expectedCount: 2, | ||
expectedHostnames: []string{ | ||
"tls-rs-0.tls-rs-svc.test-namespace.svc.cluster.local", | ||
"tls-rs-1.tls-rs-svc.test-namespace.svc.cluster.local", | ||
}, | ||
expectedNames: []string{"tls-rs-0", "tls-rs-1"}, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
processes := CreateMongodProcessesFromMongoDB( | ||
tt.mongoDBImage, | ||
tt.forceEnterprise, | ||
tt.mdb, | ||
tt.limit, | ||
tt.fcv, | ||
tt.tlsCertPath, | ||
) | ||
|
||
assert.Equal(t, tt.expectedCount, len(processes), "Process count mismatch") | ||
|
||
for i, process := range processes { | ||
assert.Equal(t, tt.expectedNames[i], process.Name(), "Process name mismatch at index %d", i) | ||
assert.Equal(t, tt.expectedHostnames[i], process.HostName(), "Hostname mismatch at index %d", i) | ||
assert.Equal(t, tt.fcv, process.FeatureCompatibilityVersion(), "FCV mismatch at index %d", i) | ||
|
||
if tt.tlsCertPath != "" { | ||
tlsConfig := process.TLSConfig() | ||
assert.NotNil(t, tlsConfig, "TLS config should not be nil when cert path is provided") | ||
assert.Equal(t, tt.tlsCertPath, tlsConfig["certificateKeyFile"], "TLS cert path mismatch at index %d", i) | ||
} | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestCreateMongodProcessesFromMongoDB_AdditionalConfig(t *testing.T) { | ||
config := mdbv1.NewAdditionalMongodConfig("storage.engine", "inMemory"). | ||
AddOption("replication.oplogSizeMB", 2048) | ||
|
||
mdb := mdbv1.NewReplicaSetBuilder(). | ||
SetName("config-rs"). | ||
SetNamespace(defaultNamespace). | ||
SetMembers(2). | ||
SetVersion("7.0.0"). | ||
SetFCVersion(defaultFCV). | ||
SetAdditionalConfig(config). | ||
Build() | ||
|
||
processes := CreateMongodProcessesFromMongoDB( | ||
defaultMongoDBImage, | ||
false, | ||
mdb, | ||
2, | ||
defaultFCV, | ||
"", | ||
) | ||
|
||
assert.Len(t, processes, 2) | ||
|
||
for i, process := range processes { | ||
assert.Equal(t, "inMemory", maputil.ReadMapValueAsInterface(process.Args(), "storage", "engine"), | ||
"Storage engine mismatch at index %d", i) | ||
assert.Equal(t, 2048, maputil.ReadMapValueAsInterface(process.Args(), "replication", "oplogSizeMB"), | ||
"OplogSizeMB mismatch at index %d", i) | ||
} | ||
} | ||
|
||
func baseReplicaSet(name string, members int) *mdbv1.MongoDB { | ||
return mdbv1.NewReplicaSetBuilder(). | ||
SetName(name). | ||
SetNamespace(defaultNamespace). | ||
SetMembers(members). | ||
SetVersion("7.0.0"). | ||
SetFCVersion(defaultFCV). | ||
Build() | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,16 @@ func BuildFromStatefulSetWithReplicas(mongoDBImage string, forceEnterprise bool, | |
return rsWithProcesses | ||
} | ||
|
||
// BuildFromMongoDBWithReplicas returns a replica set that can be set in the Automation Config | ||
// based on the given MongoDB resource directly without requiring a StatefulSet. | ||
func BuildFromMongoDBWithReplicas(mongoDBImage string, forceEnterprise bool, mdb *mdbv1.MongoDB, replicas int, fcv string, tlsCertPath string) om.ReplicaSetWithProcesses { | ||
members := process.CreateMongodProcessesFromMongoDB(mongoDBImage, forceEnterprise, mdb, replicas, fcv, tlsCertPath) | ||
replicaSet := om.NewReplicaSet(mdb.Name, mdb.Spec.GetMongoDBVersion()) | ||
rsWithProcesses := om.NewReplicaSetWithProcesses(replicaSet, members, mdb.Spec.GetMemberOptions()) | ||
rsWithProcesses.SetHorizons(mdb.Spec.GetHorizonConfig()) | ||
return rsWithProcesses | ||
} | ||
|
||
// PrepareScaleDownFromMap performs additional steps necessary to make sure removed members are not primary (so no | ||
// election happens and replica set is available) (see | ||
// https://jira.mongodb.org/browse/HELP-3818?focusedCommentId=1548348 for more details) | ||
|
@@ -65,30 +75,13 @@ func PrepareScaleDownFromMap(omClient om.Connection, rsMembers map[string][]stri | |
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 commentThe reason will be displayed to describe this comment to others. Learn more. This comment was old (<2022) |
||
// works correctly without explicit disabling - feel free to remove this code after some time when it is clear | ||
// that everything works correctly without disabling | ||
|
||
// Stage 2. Set disabled to true | ||
//err = omClient.ReadUpdateDeployment( | ||
// func(d om.Deployment) error { | ||
// d.DisableProcesses(allProcesses) | ||
// return nil | ||
// }, | ||
//) | ||
// | ||
//if err != nil { | ||
// return errors.New(fmt.Sprintf("Unable to set disabled to true, hosts: %v, err: %w", allProcesses, err)) | ||
//} | ||
//log.Debugw("Disabled processes", "processes", allProcesses) | ||
|
||
log.Infow("Performed some preliminary steps to support scale down", "hosts", processes) | ||
|
||
return nil | ||
} | ||
|
||
func PrepareScaleDownFromStatefulSet(omClient om.Connection, statefulSet appsv1.StatefulSet, rs *mdbv1.MongoDB, log *zap.SugaredLogger) error { | ||
_, podNames := dns.GetDnsForStatefulSetReplicasSpecified(statefulSet, rs.Spec.GetClusterDomain(), rs.Status.Members, nil) | ||
func PrepareScaleDownFromMongoDB(omClient om.Connection, rs *mdbv1.MongoDB, log *zap.SugaredLogger) error { | ||
_, podNames := dns.GetDNSNames(rs.Name, rs.ServiceName(), rs.Namespace, rs.Spec.GetClusterDomain(), rs.Status.Members, rs.Spec.DbCommonSpec.GetExternalDomain()) | ||
podNames = podNames[scale.ReplicasThisReconciliation(rs):rs.Status.Members] | ||
|
||
if len(podNames) != 1 { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 test and
TestCreateMongodProcessesFromMongoDB_AdditionalConfig
seem to be testingom.NewMongodProcess
(already has own unit tests) anddns.GetDNSNames
(doesn't have own unit tests).I think it is better to unit-test
om.NewMongodProcess
anddns.GetDNSNames
separately. Once you have unit tests for these building blocks you wouldn't need to test the integration so thoroughly.This also would (indirectly) cover
WaitForRsAgentsToRegisterByResource
and other places where these functions used.