-
Notifications
You must be signed in to change notification settings - Fork 59
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
K8SPG-714: fix panic #1032
K8SPG-714: fix panic #1032
Conversation
if !k8serrors.IsNotFound(err) { | ||
return reconcile.Result{}, errors.Wrap(err, "get PostgresCluster") | ||
} | ||
pgCluster = nil |
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.
Commenting only to confirm that I understand this fully. What we want to do with assigning this to nil is effectively to be able to remove the finalizer when RunFinalizer
is executed
if err := f(ctx, obj); err != nil {
if errors.Is(err, ErrFinalizerPending) {
return false, nil
}
return false, errors.Wrapf(err, "run finalizer %s", finalizer)
}
if controllerutil.RemoveFinalizer(obj, finalizer) {
log.Info("Removing finalizer", "name", finalizer)
if err := cl.Patch(ctx, obj, client.MergeFrom(orig)); err != nil {
return false, errors.Wrap(err, "remove finalizers")
}
}
and more specifically, when ☝🏽 these are executed, is that correct?
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.
You are correct. If we don’t set it to nil, the finalizer will fail, preventing its removal. The deleteBackupFinalizer
function checks whether pgCluster
is nil, and if so, it returns successfully:
percona-postgresql-operator/percona/controller/pgbackup/controller.go
Lines 327 to 331 in 942c3fd
func deleteBackupFinalizer(c client.Client, pg *v2.PerconaPGCluster) func(ctx context.Context, pgBackup *v2.PerconaPGBackup) error { | |
return func(ctx context.Context, pgBackup *v2.PerconaPGBackup) error { | |
if pg == nil { | |
return nil | |
} |
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.
⭐ thanks!
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.
please add comments to new code added into files in internal/
, it'll be helpful when we do upstream merges
commit: 9fc01f5 |
https://perconadev.atlassian.net/browse/K8SPG-714
DESCRIPTION
This PR fixes the panic on update to 2.6.0 operator
Also it fixes following issues:
pgbackup
status update, which caused following log message:update PGBackup status: Operation cannot be fulfilled on perconapgbackups.pgv2.percona.com \"XXX\": the object has been modified
patroni-version-check
podpatroni-version-check
pod'sterminationGracePeriod
to 5percona.com/delete-backup
not being removed after backup completionCHECKLIST
Jira
Needs Doc
) and QA (Needs QA
)?Tests
Config/Logging/Testability