From 0325889ac4ff7c05be3cfb311a0fc92b55166537 Mon Sep 17 00:00:00 2001 From: Frederik Ring Date: Sat, 4 Nov 2023 12:19:44 +0100 Subject: [PATCH] Pruning method logs nonsensical configuration values (#301) * Pruning method logs nonsensical configuration values * Adjust test assertion about log output --- cmd/backup/script.go | 18 +++++++++--------- internal/storage/azure/azure.go | 10 ++++------ internal/storage/dropbox/dropbox.go | 8 +++----- internal/storage/local/local.go | 8 +++----- internal/storage/s3/s3.go | 8 +++----- internal/storage/ssh/ssh.go | 8 +++----- internal/storage/storage.go | 13 +++++++++---- internal/storage/webdav/webdav.go | 9 +++------ test/dropbox/run.sh | 2 +- 9 files changed, 38 insertions(+), 46 deletions(-) diff --git a/cmd/backup/script.go b/cmd/backup/script.go index c370acb..29d8d73 100644 --- a/cmd/backup/script.go +++ b/cmd/backup/script.go @@ -172,11 +172,11 @@ func newScript() (*script, error) { CACert: s.c.AwsEndpointCACert.Cert, PartSize: s.c.AwsPartSize, } - if s3Backend, err := s3.NewStorageBackend(s3Config, logFunc); err != nil { + s3Backend, err := s3.NewStorageBackend(s3Config, logFunc) + if err != nil { return nil, fmt.Errorf("newScript: error creating s3 storage backend: %w", err) - } else { - s.storages = append(s.storages, s3Backend) } + s.storages = append(s.storages, s3Backend) } if s.c.WebdavUrl != "" { @@ -187,11 +187,11 @@ func newScript() (*script, error) { Password: s.c.WebdavPassword, RemotePath: s.c.WebdavPath, } - if webdavBackend, err := webdav.NewStorageBackend(webDavConfig, logFunc); err != nil { + webdavBackend, err := webdav.NewStorageBackend(webDavConfig, logFunc) + if err != nil { return nil, fmt.Errorf("newScript: error creating webdav storage backend: %w", err) - } else { - s.storages = append(s.storages, webdavBackend) } + s.storages = append(s.storages, webdavBackend) } if s.c.SSHHostName != "" { @@ -204,11 +204,11 @@ func newScript() (*script, error) { IdentityPassphrase: s.c.SSHIdentityPassphrase, RemotePath: s.c.SSHRemotePath, } - if sshBackend, err := ssh.NewStorageBackend(sshConfig, logFunc); err != nil { + sshBackend, err := ssh.NewStorageBackend(sshConfig, logFunc) + if err != nil { return nil, fmt.Errorf("newScript: error creating ssh storage backend: %w", err) - } else { - s.storages = append(s.storages, sshBackend) } + s.storages = append(s.storages, sshBackend) } if _, err := os.Stat(s.c.BackupArchive); !os.IsNotExist(err) { diff --git a/internal/storage/azure/azure.go b/internal/storage/azure/azure.go index f3078d6..00fe3ac 100644 --- a/internal/storage/azure/azure.go +++ b/internal/storage/azure/azure.go @@ -127,12 +127,12 @@ func (b *azureBlobStorage) Prune(deadline time.Time, pruningPrefix string) (*sto } } - stats := storage.PruneStats{ + stats := &storage.PruneStats{ Total: totalCount, Pruned: uint(len(matches)), } - if err := b.DoPrune(b.Name(), len(matches), int(totalCount), func() error { + pruneErr := b.DoPrune(b.Name(), len(matches), int(totalCount), deadline, func() error { wg := sync.WaitGroup{} wg.Add(len(matches)) var errs []error @@ -152,9 +152,7 @@ func (b *azureBlobStorage) Prune(deadline time.Time, pruningPrefix string) (*sto return errors.Join(errs...) } return nil - }); err != nil { - return &stats, err - } + }) - return &stats, nil + return stats, pruneErr } diff --git a/internal/storage/dropbox/dropbox.go b/internal/storage/dropbox/dropbox.go index 038160d..c6b5b17 100644 --- a/internal/storage/dropbox/dropbox.go +++ b/internal/storage/dropbox/dropbox.go @@ -245,16 +245,14 @@ func (b *dropboxStorage) Prune(deadline time.Time, pruningPrefix string) (*stora Pruned: uint(len(matches)), } - if err := b.DoPrune(b.Name(), len(matches), lenCandidates, func() error { + pruneErr := b.DoPrune(b.Name(), len(matches), lenCandidates, deadline, func() error { for _, match := range matches { if _, err := b.client.DeleteV2(files.NewDeleteArg(filepath.Join(b.DestinationPath, match.Name))); err != nil { return fmt.Errorf("(*dropboxStorage).Prune: Error removing file from Dropbox storage: %w", err) } } return nil - }); err != nil { - return stats, err - } + }) - return stats, nil + return stats, pruneErr } diff --git a/internal/storage/local/local.go b/internal/storage/local/local.go index 0fdf251..d85f4a6 100644 --- a/internal/storage/local/local.go +++ b/internal/storage/local/local.go @@ -116,7 +116,7 @@ func (b *localStorage) Prune(deadline time.Time, pruningPrefix string) (*storage Pruned: uint(len(matches)), } - if err := b.DoPrune(b.Name(), len(matches), len(candidates), func() error { + pruneErr := b.DoPrune(b.Name(), len(matches), len(candidates), deadline, func() error { var removeErrors []error for _, match := range matches { if err := os.Remove(match); err != nil { @@ -131,11 +131,9 @@ func (b *localStorage) Prune(deadline time.Time, pruningPrefix string) (*storage ) } return nil - }); err != nil { - return stats, err - } + }) - return stats, nil + return stats, pruneErr } // copy creates a copy of the file located at `dst` at `src`. diff --git a/internal/storage/s3/s3.go b/internal/storage/s3/s3.go index fac6fb6..0578b8c 100644 --- a/internal/storage/s3/s3.go +++ b/internal/storage/s3/s3.go @@ -167,7 +167,7 @@ func (b *s3Storage) Prune(deadline time.Time, pruningPrefix string) (*storage.Pr Pruned: uint(len(matches)), } - if err := b.DoPrune(b.Name(), len(matches), lenCandidates, func() error { + pruneErr := b.DoPrune(b.Name(), len(matches), lenCandidates, deadline, func() error { objectsCh := make(chan minio.ObjectInfo) go func() { for _, match := range matches { @@ -186,9 +186,7 @@ func (b *s3Storage) Prune(deadline time.Time, pruningPrefix string) (*storage.Pr return errors.Join(removeErrors...) } return nil - }); err != nil { - return stats, err - } + }) - return stats, nil + return stats, pruneErr } diff --git a/internal/storage/ssh/ssh.go b/internal/storage/ssh/ssh.go index 21a1e10..09525ca 100644 --- a/internal/storage/ssh/ssh.go +++ b/internal/storage/ssh/ssh.go @@ -174,16 +174,14 @@ func (b *sshStorage) Prune(deadline time.Time, pruningPrefix string) (*storage.P Pruned: uint(len(matches)), } - if err := b.DoPrune(b.Name(), len(matches), len(candidates), func() error { + pruneErr := b.DoPrune(b.Name(), len(matches), len(candidates), deadline, func() error { for _, match := range matches { if err := b.sftpClient.Remove(filepath.Join(b.DestinationPath, match)); err != nil { return fmt.Errorf("(*sshStorage).Prune: error removing file: %w", err) } } return nil - }); err != nil { - return stats, err - } + }) - return stats, nil + return stats, pruneErr } diff --git a/internal/storage/storage.go b/internal/storage/storage.go index 14d95dc..8da7f1f 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -4,6 +4,7 @@ package storage import ( + "fmt" "time" ) @@ -17,7 +18,6 @@ type Backend interface { // StorageBackend is a generic type of storage. Everything here are common properties of all storage types. type StorageBackend struct { DestinationPath string - RetentionDays int Log Log } @@ -39,16 +39,21 @@ type PruneStats struct { // DoPrune holds general control flow that applies to any kind of storage. // Callers can pass in a thunk that performs the actual deletion of files. -func (b *StorageBackend) DoPrune(context string, lenMatches, lenCandidates int, doRemoveFiles func() error) error { +func (b *StorageBackend) DoPrune(context string, lenMatches, lenCandidates int, deadline time.Time, doRemoveFiles func() error) error { if lenMatches != 0 && lenMatches != lenCandidates { if err := doRemoveFiles(); err != nil { return err } + + formattedDeadline, err := deadline.Local().MarshalText() + if err != nil { + return fmt.Errorf("(*StorageBackend).DoPrune: error marshaling deadline: %w", err) + } b.Log(LogLevelInfo, context, - "Pruned %d out of %d backups as their age exceeded the configured retention period of %d days.", + "Pruned %d out of %d backups as they were older than the given deadline of %s.", lenMatches, lenCandidates, - b.RetentionDays, + string(formattedDeadline), ) } else if lenMatches != 0 && lenMatches == lenCandidates { b.Log(LogLevelWarning, context, "The current configuration would delete all %d existing backups.", lenMatches) diff --git a/internal/storage/webdav/webdav.go b/internal/storage/webdav/webdav.go index ed5039e..c1484ac 100644 --- a/internal/storage/webdav/webdav.go +++ b/internal/storage/webdav/webdav.go @@ -108,16 +108,13 @@ func (b *webDavStorage) Prune(deadline time.Time, pruningPrefix string) (*storag Pruned: uint(len(matches)), } - if err := b.DoPrune(b.Name(), len(matches), lenCandidates, func() error { + pruneErr := b.DoPrune(b.Name(), len(matches), lenCandidates, deadline, func() error { for _, match := range matches { if err := b.client.Remove(filepath.Join(b.DestinationPath, match.Name())); err != nil { return fmt.Errorf("(*webDavStorage).Prune: error removing file: %w", err) } } return nil - }); err != nil { - return stats, err - } - - return stats, nil + }) + return stats, pruneErr } diff --git a/test/dropbox/run.sh b/test/dropbox/run.sh index 069b26b..8b9136f 100755 --- a/test/dropbox/run.sh +++ b/test/dropbox/run.sh @@ -50,7 +50,7 @@ info "Create second backup and prune" logs=$(docker compose exec -T backup backup) echo "$logs" -if echo "$logs" | grep -q "Pruned 1 out of 2 backups as their age exceeded the configured retention period"; then +if echo "$logs" | grep -q "Pruned 1 out of 2 backups as they were older"; then pass "Old remote backup has been pruned, new one is still present." elif echo "$logs" | grep -q "ERROR"; then fail "Pruning failed, errors reported: $logs"