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: