K8SPSMDB-1640 configure globally read and write concerns#2406
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new CR spec option to configure MongoDB cluster-wide default read/write concerns and wires it into the controller’s setDefaultRWConcern calls, with corresponding CRD and deepcopy regeneration.
Changes:
- Introduces
spec.defaultRWConcern(readConcern/writeConcern) in the API types and regenerates deepcopy code. - Uses the new spec values when calling
SetDefaultRWConcernduring reconciliation. - Updates CRD manifests/bundles and adds a unit test for defaulting behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/perconaservermongodb/mgo.go | Adds helper to derive default RW concerns from CR spec and uses it in SetDefaultRWConcern calls. |
| pkg/controller/perconaservermongodb/mgo_test.go | Adds unit test coverage for defaultRWConcern behavior. |
| pkg/apis/psmdb/v1/psmdb_types.go | Adds DefaultRWConcern API type and spec.defaultRWConcern field. |
| pkg/apis/psmdb/v1/zz_generated.deepcopy.go | Regenerates deepcopy methods for the new API field/type. |
| config/crd/bases/psmdb.percona.com_perconaservermongodbs.yaml | Adds CRD schema for spec.defaultRWConcern. |
| deploy/crd.yaml | Propagates CRD schema update into deploy artifact. |
| deploy/bundle.yaml | Propagates CRD schema update into bundle artifact. |
| deploy/cw-bundle.yaml | Propagates CRD schema update into cloud bundle artifact. |
| e2e-tests/version-service/conf/crd.yaml | Updates version-service CRD schema snapshot for tests. |
Files not reviewed (1)
- pkg/apis/psmdb/v1/zz_generated.deepcopy.go: Generated file
Comments suppressed due to low confidence (1)
pkg/controller/perconaservermongodb/mgo.go:263
DefaultRWConcernis only applied in this non-sharded branch whenreplset.Arbiter.Enabledis true; for a standard non-sharded replica set without an arbiter, the newspec.defaultRWConcernsetting will never be applied (confirmed: these are the onlySetDefaultRWConcerncall sites in the repo). If the intent is “configure globally”, consider applyingSetDefaultRWConcernfor all non-sharded replica sets (and/or gating oncr.Spec.DefaultRWConcern != nil), or update the API/docs to reflect the limited scope.
if replset.Arbiter.Enabled && !cr.Spec.Sharding.Enabled {
readConcern, writeConcern := defaultRWConcern(cr)
err := cli.SetDefaultRWConcern(ctx, readConcern, writeConcern)
// SetDefaultRWConcern introduced in MongoDB 4.4
if err != nil && !strings.Contains(err.Error(), "CommandNotFound") {
return api.AppStateError, nil, errors.Wrap(err, "set default RW concern")
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+114
to
118
| type DefaultRWConcern struct { | ||
| // +kubebuilder:validation:Enum={local,available,majority,linearizable,snapshot} | ||
| ReadConcern string `json:"readConcern,omitempty"` | ||
| WriteConcern string `json:"writeConcern,omitempty"` | ||
| } |
Comment on lines
+111
to
+113
| // DefaultRWConcern configures the cluster-wide default read/write concern that | ||
| // the operator enforces via setDefaultRWConcern on each reconcile. Fields left | ||
| // empty fall back to "majority". |
gkech
requested review from
egegunes,
eleo007,
hors,
jvpasinatto,
mayankshah1607,
nmarukovich,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
egegunes
reviewed
Jun 15, 2026
Comment on lines
+257
to
+258
| readConcern, writeConcern := defaultRWConcern(cr) | ||
| err := cli.SetDefaultRWConcern(ctx, readConcern, writeConcern) |
Contributor
There was a problem hiding this comment.
i know it's not introduced in this PR but i don't understand why this command depends on replset.Arbiter.Enabled
Contributor
Author
There was a problem hiding this comment.
Seems that PSS replsets already get majority as MongoDB's implicit default, making the call a no-op write, while PSA topologies need the explicit push.
With the latest commit, we allow the condition to also fire when the user has set custom values.
Comment on lines
+50
to
+55
| func shouldSetDefaultRWConcern(cr *api.PerconaServerMongoDB, replset *api.ReplsetSpec) bool { | ||
| if cr.Spec.Sharding.Enabled { | ||
| return false | ||
| } | ||
| return replset.Arbiter.Enabled || cr.Spec.DefaultRWConcern != nil | ||
| } |
Comment on lines
+112
to
116
| type DefaultRWConcern struct { | ||
| // +kubebuilder:validation:Enum={local,available,majority,linearizable,snapshot} | ||
| ReadConcern string `json:"readConcern,omitempty"` | ||
| WriteConcern string `json:"writeConcern,omitempty"` | ||
| } |
egegunes
reviewed
Jun 18, 2026
Comment on lines
+51
to
+53
| if cr.Spec.Sharding.Enabled { | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
why we don't set anything in sharded clusters?
Contributor
Author
There was a problem hiding this comment.
But we do. This function is used for the non-sharded setup. Please check err = mongosSession.SetDefaultRWConcern(ctx, readConcern, writeConcern)
egegunes
approved these changes
Jun 19, 2026
egegunes
left a comment
egegunes
left a comment
Contributor
There was a problem hiding this comment.
LGTM. we can only consider adding this case to one of existing e2e tests
Comment on lines
-208
to
+231
| err = mongosSession.SetDefaultRWConcern(ctx, mongo.DefaultReadConcern, mongo.DefaultWriteConcern) | ||
| readConcern, writeConcern := defaultRWConcern(cr) | ||
| err = mongosSession.SetDefaultRWConcern(ctx, readConcern, writeConcern) |
Contributor
There was a problem hiding this comment.
So, for the writeConcern I can set custom tags that's why the field is defined as just a string with no restrictions on what's inside. However, as I understand those strings should match mongo.RSConfig.Settings.GetLastErrorModes (WriteConcernReplsetTags), so I'm wondering if we should validate the given custom string against this map at least at runtime to avoid misconfiguration.
Contributor
Author
There was a problem hiding this comment.
WriteConcernReplsetTags
MongoDB itself will reject an unknown custom tag at setDefaultRWConcern time with something like "No write concern mode named " so misconfiguration is already caught, but it surfaces as a generic wrapped error that fails the whole reconcile loop on every pass until the CR is fixed.
Collaborator
commit: 1c5b15f |
mayankshah1607
approved these changes
Jun 22, 2026
oksana-grishchenko
approved these changes
Jun 22, 2026
hors
approved these changes
Jun 22, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://perconadev.atlassian.net/browse/K8SPSMDB-1640
CHANGE DESCRIPTION
Problem:
Configure read/write concerns separately through a global cr option.
related issue: #2311
helm pr: percona/percona-helm-charts#873
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability