K8SPSMDB-1219: Fix upgrade to v1.20.0 if multiple storages are defined #1910
Conversation
egegunes
requested review from
hors,
pooknull,
nmarukovich and
gkech
as code owners
egegunes
force-pushed
the
K8SPSMDB-1219
branch
2 times, most recently
from
5e48aa0 to
8141fad
Compare
egegunes
force-pushed
the
K8SPSMDB-1219
branch
2 times, most recently
from
bb572ee to
35bffc6
Compare
| 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") |
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
this is a very old log and it'll be removed soon, i am not sure if we should change it
| @@ -236,6 +292,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 && len(cr.Spec.Backup.Storages) == 1 { | |||
There was a problem hiding this comment.
Instead of checking if the storages are equal to 1 here, I would rather push this responsibility inside the reconcile function and do something like this
if len(cr.Spec.Backup.Storages) != 1 {
log.Info("Expected exactly one storage for PiTR in legacy version", "configured", len(storages))
return nil
}
This makes the logic inside the reconcile function that selects the 1st storage it finds much easier to follow and why it happens.
| secretName = storage.Azure.CredentialsSecret | ||
| } | ||
|
|
||
| if secretName != "" { |
There was a problem hiding this comment.
Maybe we can push this validation inside the secretExists function, both for less nesting + for having consistency in case that function is reused.
There was a problem hiding this comment.
but in this case secretExists function needs to return true (exists) if secret name is empty
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| err := pbm.GetNSetConfigLegacy(ctx, r.client, cr, storage) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yeah the reason is it generates the PBM config according to values in cr.yaml and sets it
| } | ||
|
|
||
| } else { | ||
| if len(cr.Spec.Backup.Storages) == 1 { |
There was a problem hiding this comment.
Is this check really needed? Looping the storages will return immediately if len=0.
| stgName, _, err := cr.Spec.Backup.MainStorage() | ||
| if err != nil { | ||
| // no storage found | ||
| return nil | ||
| var stgName string | ||
| if cr.CompareVersion("1.20.0") >= 0 { | ||
| stgName, _, err = cr.Spec.Backup.MainStorage() | ||
| if err != nil { | ||
| // no storage found | ||
| return nil | ||
| } | ||
|
|
||
| } else { | ||
| if len(cr.Spec.Backup.Storages) == 1 { | ||
| for name := range cr.Spec.Backup.Storages { | ||
| stgName = name | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
All this can be written to something simpler:
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
}
}
I think it is the same behaviour, right?
commit: 31f14a3 |
#1910) * K8SPSMDB-1219: Fix upgrade to v1.20.0 if multiple storages are defined * fix backups
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