percona / percona-postgresql-operator

Percona Operator for PostgreSQL
https://www.percona.com/doc/kubernetes-operator-for-postgresql/index.html
Apache License 2.0
258 stars 50 forks source link

K8SPG-462: fix recreation backup with the same name #726

Closed pooknull closed 3 months ago

pooknull commented 4 months ago

K8SPG-462 Powered by Pull Request Badge

https://perconadev.atlassian.net/browse/K8SPG-462

DESCRIPTION

Problem: Backup doesn't start if the previous one had the same name.

Cause: After the first backup, crunchy's postgrescluster will have a section .status.pgbackrest.manualBackup that contains information about the completed backup. When a new backup with the same name is created, the operator will compare the name of the new backup with the one in the .status.pgbackrest.manualBackup section: https://github.com/percona/percona-postgresql-operator/blob/e32fb98b75bf8494317feba0c04c55c359663708/internal/controller/postgrescluster/pgbackrest.go#L2242 If the name in .status.pgbackrest.manualBackup is the same as the name of the new backup, the operator will return without starting the new backup.

Solution: Delete .status.pgbackrest.manualBackup after finishing backup.

CHECKLIST

Jira

Tests

Config/Logging/Testability

JNKPercona commented 3 months ago
Test name Status
custom-extensions passed
demand-backup passed
init-deploy passed
monitoring passed
operator-self-healing passed
scaling passed
scheduled-backup passed
self-healing passed
start-from-backup passed
telemetry-transfer passed
upgrade-minor passed
users passed
We run 12 out of 12

commit: https://github.com/percona/percona-postgresql-operator/pull/726/commits/296f7799814216b9b7fc9ed4311735003b364170 image: perconalab/percona-postgresql-operator:PR-726-296f77998

pooknull commented 3 months ago

@pooknull what are the chances we can break something with removing .status.pgbackrest.manualBackup?

Also, should we check the version before removing this field?

I don't think that we can break anything. This field is only used in the reconcileManualBackup method.

This method returns if the cluster doesn't have the postgres-operator.crunchydata.com/pgbackrest-backup annotation in these lines: https://github.com/percona/percona-postgresql-operator/blob/296f7799814216b9b7fc9ed4311735003b364170/internal/controller/postgrescluster/pgbackrest.go#L2251-L2254

We delete this annotation after the backup is finished: https://github.com/percona/percona-postgresql-operator/blob/296f7799814216b9b7fc9ed4311735003b364170/percona/controller/pgbackup/controller.go#L274-L277

pooknull commented 3 months ago

It works but I think that we need to improve logging of backup deletion. Now we do not print anything in operator log when we delete a backup. kubectl delete pg-backup demand-backup-full @pooknull we can improve it via separate PR (up to you)

Let's improve it in separate PR