Skip to content

K8SPSMDB-1219: Fix upgrade to v1.20.0 if multiple storages are defined #1910

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 12, 2025
Merged

K8SPSMDB-1219: Fix upgrade to v1.20.0 if multiple storages are defined #1910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b7e36e
K8SPSMDB-1219: Fix upgrade to v1.20.0 if multiple storages are defined
egegunes May 6, 2025
31f14a3
fix backups
egegunes May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/apis/psmdb/v1/psmdb_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,13 @@ func (cr *PerconaServerMongoDB) CheckNSetDefaults(ctx context.Context, platform
}
}

if cr.CompareVersion("1.20.0") < 0 && cr.Spec.Backup.PITR.Enabled {
if len(cr.Spec.Backup.Storages) != 1 {
cr.Spec.Backup.PITR.Enabled = false
log.Info("Point-in-time recovery can be enabled only if one bucket is used in spec.backup.storages")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here we want to say storage instead of bucket. Maybe we can use the following version of the log with a little different meaning:

log.Info("disabling point-in-time recovery: requires exactly one storage in spec.backup.storages")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very old log and it'll be removed soon, i am not sure if we should change it

}
}

if cr.CompareVersion("1.20.0") >= 0 && len(cr.Spec.Backup.Storages) > 1 {
main := 0
for _, stg := range cr.Spec.Backup.Storages {
Expand Down
75 changes: 71 additions & 4 deletions pkg/controller/perconaservermongodb/pbm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/pkg/errors"
"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/x/bsonx/bsoncore"
"k8s.io/apimachinery/pkg/types"
logf "sigs.k8s.io/controller-runtime/pkg/log"

"github.com/percona/percona-backup-mongodb/pbm/config"
Expand Down Expand Up @@ -188,6 +189,56 @@ func isResyncNeeded(currentCfg *config.Config, newCfg *config.Config) bool {
return false
}

func (r *ReconcilePerconaServerMongoDB) reconcilePiTRStorageLegacy(
ctx context.Context,
pbm backup.PBM,
cr *psmdbv1.PerconaServerMongoDB,
) error {
log := logf.FromContext(ctx)

if len(cr.Spec.Backup.Storages) != 1 {
log.Info("Expected exactly one storage for PiTR in legacy version", "configured", len(cr.Spec.Backup.Storages))
return nil
}

// if PiTR is enabled user can configure only one storage
var storage psmdbv1.BackupStorageSpec
for name, stg := range cr.Spec.Backup.Storages {
storage = stg
log.Info("Configuring PBM with storage", "storage", name)
break
}

var secretName string
switch storage.Type {
case psmdbv1.BackupStorageS3:
secretName = storage.S3.CredentialsSecret
case psmdbv1.BackupStorageAzure:
secretName = storage.Azure.CredentialsSecret
}

if secretName != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can push this validation inside the secretExists function, both for less nesting + for having consistency in case that function is reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in this case secretExists function needs to return true (exists) if secret name is empty

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general secretExists should not be called at all if the storage type is S3 or Azure. Providing to the secretExists an empty name secret theoretically is a very good reason for this function to have a validation and throw an error. I understand that this may require some more changes, so we can skip it for now and revisit this when we touch it again.

exists, err := secretExists(ctx, r.client, types.NamespacedName{Name: secretName, Namespace: cr.Namespace})
if err != nil {
return errors.Wrap(err, "check storage credentials secret")
}

if !exists {
log.Error(nil, "Storage credentials secret does not exist", "secret", secretName)
return nil
}
}

err := pbm.GetNSetConfigLegacy(ctx, r.client, cr, storage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering why this function is called GetNSet... since it is essentially setting. If internally it gets, sets, appends, etc is not something we should leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the reason is it generates the PBM config according to values in cr.yaml and sets it

if err != nil {
return errors.Wrap(err, "set PBM config")
}

log.Info("Configured PBM storage")

return nil
}

func (r *ReconcilePerconaServerMongoDB) reconcilePiTRConfig(ctx context.Context, cr *psmdbv1.PerconaServerMongoDB) error {
log := logf.FromContext(ctx)

Expand All @@ -209,10 +260,17 @@ func (r *ReconcilePerconaServerMongoDB) reconcilePiTRConfig(ctx context.Context,
return nil
}

stgName, _, err := cr.Spec.Backup.MainStorage()
if err != nil {
// no storage found
return nil
var stgName string
for name := range cr.Spec.Backup.Storages {
stgName = name
break
}
if cr.CompareVersion("1.20.0") >= 0 {
stgName, _, err = cr.Spec.Backup.MainStorage()
if err != nil {
// no storage found
return nil
}
}

if cr.Spec.Backup.PITR.Enabled && !cr.Spec.Backup.PITR.OplogOnly {
Expand All @@ -236,6 +294,15 @@ func (r *ReconcilePerconaServerMongoDB) reconcilePiTRConfig(ctx context.Context,
defer pbm.Close(ctx)

if err := enablePiTRIfNeeded(ctx, pbm, cr); err != nil {
if backup.IsErrNoDocuments(err) {
if cr.CompareVersion("1.20.0") < 0 {
if err := r.reconcilePiTRStorageLegacy(ctx, pbm, cr); err != nil {
return errors.Wrap(err, "reconcile pitr storage")
}
}
return nil
}

return errors.Wrap(err, "enable pitr if needed")
}

Expand Down
23 changes: 16 additions & 7 deletions pkg/controller/perconaservermongodbbackup/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ func (b *Backup) Start(ctx context.Context, k8sclient client.Client, cluster *ap
return status, errors.Errorf("unable to get storage '%s'", cr.Spec.StorageName)
}

if cluster.CompareVersion("1.20.0") < 0 {
err := b.pbm.GetNSetConfigLegacy(ctx, k8sclient, cluster, stg)
if err != nil {
return status, errors.Wrapf(err, "set backup config with storage %s", cr.Spec.StorageName)
}
}

name := time.Now().UTC().Format(time.RFC3339)

var compLevel *int
Expand All @@ -83,16 +90,18 @@ func (b *Backup) Start(ctx context.Context, k8sclient client.Client, cluster *ap
},
}

mainStgName, _, err := b.spec.MainStorage()
if err != nil {
return status, errors.Wrap(err, "get main storage")
}
if cluster.CompareVersion("1.20.0") >= 0 {
mainStgName, _, err := b.spec.MainStorage()
if err != nil {
return status, errors.Wrap(err, "get main storage")
}

if cr.Spec.StorageName != mainStgName {
cmd.Backup.Profile = cr.Spec.StorageName
if cr.Spec.StorageName != mainStgName {
cmd.Backup.Profile = cr.Spec.StorageName
}
}

log.Info("Sending backup command", "backupCmd", cmd)
log.Info("Sending backup command", "backupCmd", cmd, "profile", cmd.Backup.Profile)

if err := b.pbm.SendCmd(ctx, cmd); err != nil {
return status, err
Expand Down
17 changes: 17 additions & 0 deletions pkg/controller/perconaservermongodbbackup/perconaservermongodbbackup_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,23 @@ func (r *ReconcilePerconaServerMongoDBBackup) deleteBackupFinalizer(ctx context.
return nil
}

if cluster.CompareVersion("1.20.0") < 0 {
err = b.pbm.DeletePITRChunks(ctx, meta.LastWriteTS)
if err != nil {
return errors.Wrap(err, "failed to delete PITR")
}
log.Info("PiTR chunks deleted", "until", meta.LastWriteTS)

err = b.pbm.DeleteBackup(ctx, cr.Status.PBMname)
if err != nil {
return errors.Wrap(err, "failed to delete backup")
}

log.Info("Backup deleted")

return nil
}

mainStgName, _, err := cluster.Spec.Backup.MainStorage()
if err != nil {
return errors.Wrap(err, "get main storage")
Expand Down
8 changes: 8 additions & 0 deletions pkg/controller/perconaservermongodbrestore/perconaservermongodbrestore_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,14 @@ func (r *ReconcilePerconaServerMongoDBRestore) resyncStorage(
return errors.Wrap(err, "get backup")
}

if cluster.CompareVersion("1.20.0") < 0 {
if err := pbmC.ResyncMainStorageAndWait(ctx); err != nil {
return errors.Wrap(err, "start config resync")
}

return nil
}

mainStgName, _, err := cluster.Spec.Backup.MainStorage()
if err != nil {
return errors.Wrap(err, "get main storage")
Expand Down
4 changes: 4 additions & 0 deletions pkg/psmdb/backup/fake/pbm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ func (p *fakePBM) GetNSetConfig(ctx context.Context, k8sclient client.Client, cl
return nil
}

func (p *fakePBM) GetNSetConfigLegacy(ctx context.Context, k8sclient client.Client, cluster *api.PerconaServerMongoDB, stg api.BackupStorageSpec) error {
return nil
}

func (p *fakePBM) SetConfigVar(ctx context.Context, key, val string) error {
return nil
}
Expand Down
21 changes: 21 additions & 0 deletions pkg/psmdb/backup/pbm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ type PBM interface {
GetProfile(ctx context.Context, name string) (*config.Config, error)
RemoveProfile(ctx context.Context, name string) error
GetNSetConfig(ctx context.Context, k8sclient client.Client, cluster *api.PerconaServerMongoDB) error
GetNSetConfigLegacy(ctx context.Context, k8sclient client.Client, cluster *api.PerconaServerMongoDB, stg api.BackupStorageSpec) error
SetConfig(ctx context.Context, cfg *config.Config) error
SetConfigVar(ctx context.Context, key, val string) error

Expand Down Expand Up @@ -522,6 +523,26 @@ func (b *pbmC) RemoveProfile(ctx context.Context, name string) error {
return config.RemoveProfile(ctx, b.Client, name)
}

// GetNSetConfigLegacy sets the PBM config with given storage
func (b *pbmC) GetNSetConfigLegacy(ctx context.Context, k8sclient client.Client, cluster *api.PerconaServerMongoDB, stg api.BackupStorageSpec) error {
log := logf.FromContext(ctx)

conf, err := GetPBMConfig(ctx, k8sclient, cluster, stg)
if err != nil {
return errors.Wrap(err, "get PBM config")
}

log.Info("Setting config", "cluster", cluster.Name, "storage", stg)

if err := config.SetConfig(ctx, b.Client, &conf); err != nil {
return errors.Wrap(err, "write config")
}

time.Sleep(11 * time.Second) // give time to init new storage

return nil
}

// GetNSetConfig sets the PBM config with main storage defined in the cluster CR
func (b *pbmC) GetNSetConfig(ctx context.Context, k8sclient client.Client, cluster *api.PerconaServerMongoDB) error {
log := logf.FromContext(ctx)
Expand Down
Loading