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 <androndo@gmail.com>

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"`
 

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

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)
+		}
+	})
+}

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)
+	}
+}

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
 	}

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)
+	}
+}

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
 	}

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: