From 523d74a5033cedb15158c5c3e3673c4e79d98fb7 Mon Sep 17 00:00:00 2001 From: Andrey Kolkov Date: Thu, 22 Oct 2026 18:41:03 +0400 Subject: [PATCH] fix(backups): retry instead of failing when an existing Backup cannot be read When Create collides with a Backup an earlier reconcile already made and the read that follows fails, the drivers failed the BackupJob, although the backup and its Backup both exist. Return a distinct error instead, which the drivers hand back to the controller so the reconcile is retried with backoff and then finds the Backup. minio.New already refuses an endpoint with a path prefix and accepts a trailing slash, so drop the duplicate check in the shared client; the CNPG probe keeps its own check because it parses the URL itself and would otherwise drop the path before minio sees it. Say in the sizeBytes API doc what the value covers, such as the base backup without the shared WAL archive for CNPG, and that strategies which cannot attribute stored bytes to one backup leave it unset. Assisted-by: LLM Signed-off-by: Andrey Kolkov --- api/backups/v1alpha1/backup_types.go | 11 ++-- internal/backupcontroller/artifact_size.go | 31 +++++++---- .../backupcontroller/artifact_size_test.go | 52 +++++++++++++++---- .../cnpg_artifact_size_test.go | 18 +++++++ .../cnpgstrategy_controller.go | 4 +- .../mariadb_artifact_size_test.go | 17 ++++++ .../mariadbstrategy_controller.go | 4 +- .../backups.cozystack.io_backups.yaml | 11 ++-- 8 files changed, 116 insertions(+), 32 deletions(-) diff --git a/api/backups/v1alpha1/backup_types.go b/api/backups/v1alpha1/backup_types.go index 483bb1fd8..541488202 100644 --- a/api/backups/v1alpha1/backup_types.go +++ b/api/backups/v1alpha1/backup_types.go @@ -37,9 +37,14 @@ type BackupArtifact struct { // For example: s3://bucket/prefix/file.tar.gz URI string `json:"uri"` - // SizeBytes is the size of the artifact in bytes, if known. Drivers - // leave it unset rather than writing 0 when they cannot measure the - // stored size, so an absent value means "unknown", not "empty". + // SizeBytes is the stored size in bytes of the files this backup owns, + // if known: the dump of a MariaDB backup, for example, or the base backup + // of a CNPG one, which leaves out the WAL archive the server's backups + // share and a point-in-time restore also reads. Strategies that cannot + // attribute stored bytes to a single backup, such as Velero's, whose + // repository deduplicates across backups, leave it unset. Drivers never + // write 0 for a size they cannot measure, so an absent value means + // "unknown", not "empty". // +optional SizeBytes int64 `json:"sizeBytes,omitempty"` diff --git a/internal/backupcontroller/artifact_size.go b/internal/backupcontroller/artifact_size.go index 182ea0b58..5addefd5a 100644 --- a/internal/backupcontroller/artifact_size.go +++ b/internal/backupcontroller/artifact_size.go @@ -6,7 +6,6 @@ import ( "crypto/x509" "errors" "fmt" - "strings" "time" "github.com/minio/minio-go/v7" @@ -14,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" backupsv1alpha1 "github.com/cozystack/cozystack/api/backups/v1alpha1" ) @@ -38,7 +38,7 @@ type s3ObjectLister interface { // s3Connection is what a driver resolves from its rendered strategy to // read the bucket with the same identity the backup was written with. type s3Connection struct { - // Endpoint is host[:port], without a scheme or a path. + // Endpoint is host[:port], without a scheme. Endpoint string Secure bool Region string @@ -61,14 +61,6 @@ type minioObjectLister struct { } func newMinioObjectLister(conn s3Connection) (s3ObjectLister, error) { - // An endpoint behind a path prefix cannot be addressed: minio-go has no - // such option, and the SigV4 signature covers the request path, so the - // prefix cannot be added below the client either. A bare trailing slash - // is not a prefix, and the operators accept it. - endpoint := strings.TrimSuffix(conn.Endpoint, "/") - if strings.Contains(endpoint, "/") { - return nil, fmt.Errorf("endpoint %q has a path, which the S3 client cannot address", conn.Endpoint) - } opts := &minio.Options{ Creds: credentials.NewStaticV4(conn.AccessKey, conn.SecretKey, conn.SessionToken), Region: conn.Region, @@ -92,7 +84,9 @@ func newMinioObjectLister(conn s3Connection) (s3ObjectLister, error) { tr.TLSClientConfig.RootCAs = pool opts.Transport = tr } - client, err := minio.New(endpoint, opts) + // minio.New refuses an endpoint with a path prefix, which it cannot + // address, and accepts a bare trailing slash. + client, err := minio.New(conn.Endpoint, opts) if err != nil { return nil, err } @@ -121,6 +115,21 @@ func collectObjects(ctx context.Context, objects <-chan minio.ObjectInfo) ([]s3O return out, nil } +// errBackupNotReadBack reports that Create collided with an existing Backup +// that could then not be read. The backup itself succeeded, so callers +// retry the reconcile instead of failing the BackupJob. +var errBackupNotReadBack = errors.New("the Backup already exists but could not be read back") + +// failBackupArtifact handles an error from creating the Backup of a +// completed backup: errBackupNotReadBack is retried through the controller's +// backoff, anything else fails the BackupJob. +func (r *BackupJobReconciler) failBackupArtifact(ctx context.Context, j *backupsv1alpha1.BackupJob, err error) (ctrl.Result, error) { + if errors.Is(err, errBackupNotReadBack) { + return ctrl.Result{}, err + } + return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err)) +} + // existingBackup returns the Backup a previous reconcile already created // for the job, so a retry skips the size probe whose result it would // discard. It reads past the cache: a retry typically follows a Create made diff --git a/internal/backupcontroller/artifact_size_test.go b/internal/backupcontroller/artifact_size_test.go index 0e66968b5..69112cd9d 100644 --- a/internal/backupcontroller/artifact_size_test.go +++ b/internal/backupcontroller/artifact_size_test.go @@ -4,6 +4,7 @@ package backupcontroller import ( "context" "errors" + "fmt" "testing" "time" @@ -86,17 +87,6 @@ func TestCollectObjects(t *testing.T) { } } -func TestNewMinioObjectLister_RejectsEndpointPath(t *testing.T) { - if _, err := newMinioObjectLister(s3Connection{Endpoint: "s3.example.org/prefix", Secure: true}); err == nil { - t.Error("an endpoint with a path was accepted") - } - for _, ok := range []string{"s3.example.org:9000", "s3.example.org/"} { - if _, err := newMinioObjectLister(s3Connection{Endpoint: ok, Secure: true}); err != nil { - t.Errorf("endpoint %q rejected: %v", ok, err) - } - } -} - // TestExistingBackup_ReadsPastTheCache pins that the retry check sees a // Backup the cache does not show yet: the previous reconcile created it // moments earlier. @@ -181,3 +171,43 @@ func withShortProbeTimeout(t *testing.T, fn func()) { t.Fatal("the probe did not give up at its timeout") } } + +// failingReads wraps a fake client whose every Get fails, like an apiserver +// that times out. +func failingReads(t *testing.T, c client.Client) client.Client { + t.Helper() + return interceptor.NewClient(c.(client.WithWatch), interceptor.Funcs{ + Get: func(context.Context, client.WithWatch, client.ObjectKey, client.Object, ...client.GetOption) error { + return errors.New("etcdserver: request timed out") + }, + }) +} + +func TestFailBackupArtifact(t *testing.T) { + newJob := func() *backupsv1alpha1.BackupJob { + return &backupsv1alpha1.BackupJob{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj"}} + } + + t.Run("a Backup that exists but cannot be read is retried", func(t *testing.T) { + j := newJob() + r := &BackupJobReconciler{Client: newMariaDBStrategyTestClient(t, j)} + _, err := r.failBackupArtifact(context.Background(), j, fmt.Errorf("%w: timeout", errBackupNotReadBack)) + if !errors.Is(err, errBackupNotReadBack) { + t.Errorf("err = %v, want it returned so the reconcile is retried", err) + } + if j.Status.Phase == backupsv1alpha1.BackupJobPhaseFailed { + t.Error("the BackupJob was failed although its Backup exists") + } + }) + + t.Run("any other error fails the BackupJob", func(t *testing.T) { + j := newJob() + r := &BackupJobReconciler{Client: newMariaDBStrategyTestClient(t, j)} + if _, err := r.failBackupArtifact(context.Background(), j, errors.New("admission webhook denied")); err != nil { + t.Fatalf("failBackupArtifact: %v", err) + } + if j.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed { + t.Errorf("phase = %q, want Failed", j.Status.Phase) + } + }) +} diff --git a/internal/backupcontroller/cnpg_artifact_size_test.go b/internal/backupcontroller/cnpg_artifact_size_test.go index 70bb2a485..f6ea74b6f 100644 --- a/internal/backupcontroller/cnpg_artifact_size_test.go +++ b/internal/backupcontroller/cnpg_artifact_size_test.go @@ -334,3 +334,21 @@ func TestCreateCNPGBackupArtifact_AlreadyExistsRefetchReadsPastTheCache(t *testi t.Fatalf("createCNPGBackupArtifact = %v, %v; want the Backup that already exists", got, err) } } + +func TestCreateCNPGBackupArtifact_UnreadableExistingBackupIsRetried(t *testing.T) { + apiGroup := backupsv1alpha1.DefaultApplicationAPIGroup + j := &backupsv1alpha1.BackupJob{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-unreadable"}, + Spec: backupsv1alpha1.BackupJobSpec{ + ApplicationRef: corev1.TypedLocalObjectReference{APIGroup: &apiGroup, Kind: postgresAppKind, Name: "pg"}, + }, + } + backup := &backupsv1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-unreadable"}} + store := newCNPGStrategyTestClient(t, backup) + r := &BackupJobReconciler{Client: staleCache(t, store), APIReader: failingReads(t, store)} + rendered := &strategyv1alpha1.CNPGTemplate{BarmanObjectStore: strategyv1alpha1.BarmanObjectStoreTemplate{DestinationPath: "s3://b/"}} + _, err := r.createCNPGBackupArtifact(context.Background(), j, &ResolvedBackupConfig{}, &cnpgtypes.Backup{ObjectMeta: metav1.ObjectMeta{Name: "cnpg-bk"}}, "postgres-pg", "srv", "postgresql", rendered, newPostgresApp("pg", "tenant")) + if !errors.Is(err, errBackupNotReadBack) { + t.Errorf("err = %v, want errBackupNotReadBack", err) + } +} diff --git a/internal/backupcontroller/cnpgstrategy_controller.go b/internal/backupcontroller/cnpgstrategy_controller.go index 712dd8713..ed994ed22 100644 --- a/internal/backupcontroller/cnpgstrategy_controller.go +++ b/internal/backupcontroller/cnpgstrategy_controller.go @@ -435,7 +435,7 @@ func (r *BackupJobReconciler) reconcileCNPG(ctx context.Context, j *backupsv1alp } artifact, err := r.createCNPGBackupArtifact(ctx, j, resolved, cnpgBackup, clusterName, serverName, attachment.flavor, rendered, app) if err != nil { - return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err)) + return r.failBackupArtifact(ctx, j, err) } now := metav1.Now() j.Status.BackupRef = &corev1.LocalObjectReference{Name: artifact.Name} @@ -877,7 +877,7 @@ func (r *BackupJobReconciler) createCNPGBackupArtifact( } existing := &backupsv1alpha1.Backup{} if getErr := r.apiReader().Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil { - return nil, getErr + return nil, fmt.Errorf("%w: %v", errBackupNotReadBack, getErr) } return existing, nil } diff --git a/internal/backupcontroller/mariadb_artifact_size_test.go b/internal/backupcontroller/mariadb_artifact_size_test.go index 91f555bbd..fd3ec30d4 100644 --- a/internal/backupcontroller/mariadb_artifact_size_test.go +++ b/internal/backupcontroller/mariadb_artifact_size_test.go @@ -359,3 +359,20 @@ func TestCreateMariaDBBackupArtifact_AlreadyExistsRefetchReadsPastTheCache(t *te t.Fatalf("createMariaDBBackupArtifact = %v, %v; want the Backup that already exists", got, err) } } + +func TestCreateMariaDBBackupArtifact_UnreadableExistingBackupIsRetried(t *testing.T) { + apps := mariadbapp.GroupName + job := &backupsv1alpha1.BackupJob{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-unreadable"}, + Spec: backupsv1alpha1.BackupJobSpec{ + ApplicationRef: corev1.TypedLocalObjectReference{Kind: "MariaDB", Name: "src", APIGroup: &apps}, + }, + } + backup := &backupsv1alpha1.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "bj-unreadable"}} + store := newMariaDBStrategyTestClient(t, job, backup) + r := &BackupJobReconciler{Client: staleCache(t, store), APIReader: failingReads(t, store), Scheme: store.Scheme()} + _, err := r.createMariaDBBackupArtifact(context.Background(), job, &ResolvedBackupConfig{}, &mariadbtypes.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "op"}}, &strategyv1alpha1.MariaDBTemplate{}) + if !errors.Is(err, errBackupNotReadBack) { + t.Errorf("err = %v, want errBackupNotReadBack", err) + } +} diff --git a/internal/backupcontroller/mariadbstrategy_controller.go b/internal/backupcontroller/mariadbstrategy_controller.go index 8db41fb7f..4c920dcaa 100644 --- a/internal/backupcontroller/mariadbstrategy_controller.go +++ b/internal/backupcontroller/mariadbstrategy_controller.go @@ -214,7 +214,7 @@ func (r *BackupJobReconciler) reconcileMariaDB(ctx context.Context, j *backupsv1 } artifact, err := r.createMariaDBBackupArtifact(ctx, j, resolved, mdbBackup, rendered) if err != nil { - return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err)) + return r.failBackupArtifact(ctx, j, err) } now := metav1.Now() j.Status.BackupRef = &corev1.LocalObjectReference{Name: artifact.Name} @@ -422,7 +422,7 @@ func (r *BackupJobReconciler) createMariaDBBackupArtifact( } existing := &backupsv1alpha1.Backup{} if getErr := r.apiReader().Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil { - return nil, getErr + return nil, fmt.Errorf("%w: %v", errBackupNotReadBack, getErr) } return existing, nil } diff --git a/packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml b/packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml index f30d3dbc7..dfa25dbc5 100644 --- a/packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml +++ b/packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml @@ -132,9 +132,14 @@ spec: type: string sizeBytes: description: |- - SizeBytes is the size of the artifact in bytes, if known. Drivers - leave it unset rather than writing 0 when they cannot measure the - stored size, so an absent value means "unknown", not "empty". + SizeBytes is the stored size in bytes of the files this backup owns, + if known: the dump of a MariaDB backup, for example, or the base backup + of a CNPG one, which leaves out the WAL archive the server's backups + share and a point-in-time restore also reads. Strategies that cannot + attribute stored bytes to a single backup, such as Velero's, whose + repository deduplicates across backups, leave it unset. Drivers never + write 0 for a size they cannot measure, so an absent value means + "unknown", not "empty". format: int64 type: integer uri: