K8SPSMDB-1527: Cluster can't be ready before PBM is ready #2152
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
|
|
||
| "github.com/percona/percona-backup-mongodb/pbm/config" | ||
|
|
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
| "github.com/percona/percona-backup-mongodb/pbm/config" | ||
|
|
||
| pbmVersion "github.com/percona/percona-backup-mongodb/pbm/version" | ||
| psmdbv1 "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1" |
There was a problem hiding this comment.
[goimports-reviser] reported by reviewdog 🐶
| psmdbv1 "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1" | |
| psmdbv1 "github.com/percona/percona-server-mongodb-operator/pkg/apis/psmdb/v1" |
egegunes
force-pushed
the
K8SPSMDB-1527
branch
from
4b99c58 to
4461031
Compare
| Backup: api.BackupSpec{ | ||
| Enabled: true, | ||
| Storages: map[string]api.BackupStorageSpec{ | ||
| "s3": api.BackupStorageSpec{ |
There was a problem hiding this comment.
[gofmt] reported by reviewdog 🐶
| "s3": api.BackupStorageSpec{ | |
| "s3": { |
| Backup: api.BackupSpec{ | ||
| Enabled: true, | ||
| Storages: map[string]api.BackupStorageSpec{ | ||
| "s3": api.BackupStorageSpec{ |
There was a problem hiding this comment.
[gofmt] reported by reviewdog 🐶
| "s3": api.BackupStorageSpec{ | |
| "s3": { |
There was a problem hiding this comment.
Pull request overview
This PR ensures that a MongoDB cluster cannot reach the "ready" state until PBM (Percona Backup for MongoDB) is fully configured and ready when backup is enabled. This prevents operations from proceeding when the backup infrastructure is not yet available.
Key changes:
- Added
pbmInProgresscheck to prevent cluster from becoming ready before PBM version and configuration are established - Introduced
PBMReadycondition type to track PBM configuration state - Moved
reconcileBackupVersionfunction frombackup.gotopbm.goand removed the requirement for cluster to be ready before checking PBM version
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/controller/perconaservermongodb/status.go |
Added pbmInProgress flag that checks if backup version or config hash is empty, preventing cluster ready state until PBM is configured |
pkg/controller/perconaservermongodb/status_test.go |
Added comprehensive table-driven tests covering various PBM readiness scenarios including backup disabled, version empty, config hash empty, and fully ready states |
pkg/controller/perconaservermongodb/pbm.go |
Added checks for replset initialization and shard status before PBM config reconciliation; moved reconcileBackupVersion here and removed ready state requirement; added PBMReady condition updates |
pkg/controller/perconaservermongodb/backup.go |
Removed reconcileBackupVersion function (moved to pbm.go) and cleaned up imports |
pkg/controller/perconaservermongodb/conditions.go |
New file with helper function updateCondition to update cluster conditions and write status |
pkg/apis/psmdb/v1/psmdb_types.go |
Added ConditionTypePBMReady constant; moved AddCondition method earlier in file for better organization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| continue | ||
| } | ||
|
|
||
| if cr.Spec.Sharding.Enabled && status.AddedAsShard == nil || !*status.AddedAsShard { |
There was a problem hiding this comment.
The logical condition has incorrect operator precedence. The AND operator (&&) should be evaluated before OR (||), but the current expression may not behave as intended. The condition should be grouped as: cr.Spec.Sharding.Enabled && (status.AddedAsShard == nil || !*status.AddedAsShard). Currently, when sharding is disabled, it will still check !*status.AddedAsShard which could panic if status.AddedAsShard is nil.
| if cr.Spec.Sharding.Enabled && status.AddedAsShard == nil || !*status.AddedAsShard { | |
| if cr.Spec.Sharding.Enabled && (status.AddedAsShard == nil || !*status.AddedAsShard) { |
|
|
||
| func (r *ReconcilePerconaServerMongoDB) updateCondition(ctx context.Context, cr *psmdbv1.PerconaServerMongoDB, c psmdbv1.ClusterCondition) error { | ||
| cr.Status.AddCondition(c) | ||
| return r.writeStatus(ctx, cr) |
There was a problem hiding this comment.
The updateCondition function calls writeStatus which immediately persists the status to the API server. This is called multiple times during reconciliation (lines 166 and 191 in pbm.go), but there's also a deferred updateStatus call at the end of the Reconcile function. While the CR is passed by reference so the conditions should be preserved, calling writeStatus multiple times during a single reconciliation loop is not ideal as it creates unnecessary API calls and potential race conditions. Consider accumulating condition changes and writing them only once at the end of reconciliation, similar to how other status fields are handled.
| return r.writeStatus(ctx, cr) | |
| return nil |
commit: 4461031 |
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
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