From 793482c7382c418076e5021c8b0d01956f809840 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Wed, 7 Aug 2024 17:06:48 +0800 Subject: [PATCH] issue 8072: restic deprecation - warning messages Signed-off-by: Lyndon-Li --- pkg/cmd/cli/install/install.go | 4 ++- pkg/cmd/server/server.go | 4 ++- pkg/podvolume/backupper.go | 22 +++++++++++--- pkg/podvolume/backupper_test.go | 54 ++++++++++++++++++++++++--------- pkg/uploader/types.go | 11 +++++-- pkg/uploader/types_test.go | 27 ++++++++++++----- 6 files changed, 91 insertions(+), 31 deletions(-) diff --git a/pkg/cmd/cli/install/install.go b/pkg/cmd/cli/install/install.go index c51cee6586..b73438e78a 100644 --- a/pkg/cmd/cli/install/install.go +++ b/pkg/cmd/cli/install/install.go @@ -364,8 +364,10 @@ func (o *Options) Validate(c *cobra.Command, args []string, f client.Factory) er return err } - if err := uploader.ValidateUploaderType(o.UploaderType); err != nil { + if msg, err := uploader.ValidateUploaderType(o.UploaderType); err != nil { return err + } else if msg != "" { + fmt.Printf("⚠️ %s\n", msg) } // If we're only installing CRDs, we can skip the rest of the validation. diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index d8938ca56f..df965ed424 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -288,8 +288,10 @@ type server struct { } func newServer(f client.Factory, config serverConfig, logger *logrus.Logger) (*server, error) { - if err := uploader.ValidateUploaderType(config.uploaderType); err != nil { + if msg, err := uploader.ValidateUploaderType(config.uploaderType); err != nil { return nil, err + } else if msg != "" { + logger.Warn(msg) } if config.clientQPS < 0.0 { diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index 8d06cb2225..5576f4e407 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -36,6 +36,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/label" "github.com/vmware-tanzu/velero/pkg/nodeagent" "github.com/vmware-tanzu/velero/pkg/repository" + "github.com/vmware-tanzu/velero/pkg/uploader" uploaderutil "github.com/vmware-tanzu/velero/pkg/uploader/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -163,10 +164,13 @@ func (b *backupper) getMatchAction(resPolicies *resourcepolicies.Policies, pvc * return nil, errors.Errorf("failed to check resource policies for empty volume") } +var funcGetRepositoryType = getRepositoryType + func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api.Pod, volumesToBackup []string, resPolicies *resourcepolicies.Policies, log logrus.FieldLogger) ([]*velerov1api.PodVolumeBackup, *PVCBackupSummary, []error) { if len(volumesToBackup) == 0 { return nil, nil, nil } + log.Infof("pod %s/%s has volumes to backup: %v", pod.Namespace, pod.Name, volumesToBackup) var ( @@ -189,6 +193,13 @@ func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api. } } + if msg, err := uploader.ValidateUploaderType(b.uploaderType); err != nil { + skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log) + return nil, pvcSummary, []error{err} + } else if msg != "" { + log.Warn(msg) + } + if err := kube.IsPodRunning(pod); err != nil { skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log) return nil, pvcSummary, nil @@ -196,18 +207,21 @@ func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api. err := nodeagent.IsRunningInNode(b.ctx, backup.Namespace, pod.Spec.NodeName, b.crClient) if err != nil { - return nil, nil, []error{err} + skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log) + return nil, pvcSummary, []error{err} } - repositoryType := getRepositoryType(b.uploaderType) + repositoryType := funcGetRepositoryType(b.uploaderType) if repositoryType == "" { err := errors.Errorf("empty repository type, uploader %s", b.uploaderType) - return nil, nil, []error{err} + skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log) + return nil, pvcSummary, []error{err} } repo, err := b.repoEnsurer.EnsureRepo(b.ctx, backup.Namespace, pod.Namespace, backup.Spec.StorageLocation, repositoryType) if err != nil { - return nil, nil, []error{err} + skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log) + return nil, pvcSummary, []error{err} } // get a single non-exclusive lock since we'll wait for all individual diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index 06cec20dee..16d4ce2862 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -309,22 +309,38 @@ func TestBackupPodVolumes(t *testing.T) { corev1api.AddToScheme(scheme) tests := []struct { - name string - bsl string - uploaderType string - volumes []string - sourcePod *corev1api.Pod - kubeClientObj []runtime.Object - ctlClientObj []runtime.Object - veleroClientObj []runtime.Object - veleroReactors []reactor - runtimeScheme *runtime.Scheme - pvbs int - errs []string + name string + bsl string + uploaderType string + volumes []string + sourcePod *corev1api.Pod + kubeClientObj []runtime.Object + ctlClientObj []runtime.Object + veleroClientObj []runtime.Object + veleroReactors []reactor + runtimeScheme *runtime.Scheme + pvbs int + mockGetRepositoryType bool + errs []string }{ { name: "empty volume list", }, + { + name: "wrong uploader type", + volumes: []string{ + "fake-volume-1", + "fake-volume-2", + }, + sourcePod: createPodObj(true, false, false, 2), + kubeClientObj: []runtime.Object{ + createNodeAgentPodObj(true), + }, + uploaderType: "fake-uploader-type", + errs: []string{ + "invalid uploader type 'fake-uploader-type', valid upload types are: 'restic', 'kopia'", + }, + }, { name: "pod is not running", volumes: []string{ @@ -348,7 +364,8 @@ func TestBackupPodVolumes(t *testing.T) { "fake-volume-1", "fake-volume-2", }, - sourcePod: createPodObj(true, false, false, 2), + sourcePod: createPodObj(true, false, false, 2), + uploaderType: "kopia", errs: []string{ "daemonset pod not found in running state in node fake-node-name", }, @@ -363,9 +380,10 @@ func TestBackupPodVolumes(t *testing.T) { kubeClientObj: []runtime.Object{ createNodeAgentPodObj(true), }, - uploaderType: "fake-uploader-type", + uploaderType: "kopia", + mockGetRepositoryType: true, errs: []string{ - "empty repository type, uploader fake-uploader-type", + "empty repository type, uploader kopia", }, }, { @@ -542,6 +560,12 @@ func TestBackupPodVolumes(t *testing.T) { require.NoError(t, err) + if test.mockGetRepositoryType { + funcGetRepositoryType = func(string) string { return "" } + } else { + funcGetRepositoryType = getRepositoryType + } + pvbs, _, errs := bp.BackupPodVolumes(backupObj, test.sourcePod, test.volumes, nil, velerotest.NewLogger()) if errs == nil { diff --git a/pkg/uploader/types.go b/pkg/uploader/types.go index 02106e266e..fcd1f47379 100644 --- a/pkg/uploader/types.go +++ b/pkg/uploader/types.go @@ -39,12 +39,17 @@ const ( // ValidateUploaderType validates if the input param is a valid uploader type. // It will return an error if it's invalid. -func ValidateUploaderType(t string) error { +func ValidateUploaderType(t string) (string, error) { t = strings.TrimSpace(t) if t != ResticType && t != KopiaType { - return fmt.Errorf("invalid uploader type '%s', valid upload types are: '%s', '%s'", t, ResticType, KopiaType) + return "", fmt.Errorf("invalid uploader type '%s', valid upload types are: '%s', '%s'", t, ResticType, KopiaType) } - return nil + + if t == ResticType { + return fmt.Sprintf("Uploader '%s' is deprecated, don't use it for new backups, otherwise, the backed up data may not be accessed by new versions of Velero", t), nil + } + + return "", nil } type SnapshotInfo struct { diff --git a/pkg/uploader/types_test.go b/pkg/uploader/types_test.go index 492051bf2e..9cf4f6edca 100644 --- a/pkg/uploader/types_test.go +++ b/pkg/uploader/types_test.go @@ -1,34 +1,47 @@ package uploader -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestValidateUploaderType(t *testing.T) { tests := []struct { name string input string - wantErr bool + wantErr string + wantMsg string }{ { "'restic' is a valid type", "restic", - false, + "", + "Uploader 'restic' is deprecated, don't use it for new backups, otherwise, the backed up data may not be accessed by new versions of Velero", }, { "' kopia ' is a valid type (space will be trimmed)", " kopia ", - false, + "", + "", }, { "'anything_else' is invalid", "anything_else", - true, + "invalid uploader type 'anything_else', valid upload types are: 'restic', 'kopia'", + "", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := ValidateUploaderType(tt.input); (err != nil) != tt.wantErr { - t.Errorf("ValidateUploaderType(), input = '%s' error = %v, wantErr %v", tt.input, err, tt.wantErr) + msg, err := ValidateUploaderType(tt.input) + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } else { + assert.NoError(t, err) } + + assert.Equal(t, tt.wantMsg, msg) }) } }