From dd1def9d332e186ecf04ce30ed739fdd5a0f5234 Mon Sep 17 00:00:00 2001 From: Daniil Basin Date: Tue, 7 Apr 2026 09:12:51 +0500 Subject: [PATCH 1/3] Use strict minimal structure to parse last applied configuration JSON Signed-off-by: Daniil Basin --- changelogs/unreleased/9653-BassinD | 1 + pkg/restore/actions/service_action.go | 21 +++++++-------- pkg/restore/actions/service_action_test.go | 30 ++++++++++++++++++++++ 3 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/9653-BassinD diff --git a/changelogs/unreleased/9653-BassinD b/changelogs/unreleased/9653-BassinD new file mode 100644 index 000000000..d4eefe02a --- /dev/null +++ b/changelogs/unreleased/9653-BassinD @@ -0,0 +1 @@ +Fix service restore with null healthCheckNodePort in last-applied-configuration label diff --git a/pkg/restore/actions/service_action.go b/pkg/restore/actions/service_action.go index 9de75228e..34ba85e93 100644 --- a/pkg/restore/actions/service_action.go +++ b/pkg/restore/actions/service_action.go @@ -94,21 +94,18 @@ func deleteHealthCheckNodePort(service *corev1api.Service) error { // Search HealthCheckNodePort from server's last-applied-configuration // annotation(HealthCheckNodePort is specified by `kubectl apply` command) - lastAppliedConfig, ok := service.Annotations[annotationLastAppliedConfig] - if ok { - appliedServiceUnstructured := new(map[string]any) - if err := json.Unmarshal([]byte(lastAppliedConfig), appliedServiceUnstructured); err != nil { + if lastAppliedConfig, ok := service.Annotations[annotationLastAppliedConfig]; ok { + var appliedConfig struct { + Spec struct { + HealthCheckNodePort *int32 `json:"healthCheckNodePort"` + } `json:"spec"` + } + + if err := json.Unmarshal([]byte(lastAppliedConfig), &appliedConfig); err != nil { return errors.WithStack(err) } - healthCheckNodePort, exist, err := unstructured.NestedFloat64(*appliedServiceUnstructured, "spec", "healthCheckNodePort") - if err != nil { - return errors.WithStack(err) - } - - // Found healthCheckNodePort in lastAppliedConfig annotation, - // and the value is not 0. No need to delete, return. - if exist && healthCheckNodePort != 0 { + if appliedConfig.Spec.HealthCheckNodePort != nil && *appliedConfig.Spec.HealthCheckNodePort != 0 { return nil } } diff --git a/pkg/restore/actions/service_action_test.go b/pkg/restore/actions/service_action_test.go index 42c0d290d..f9a01d5d4 100644 --- a/pkg/restore/actions/service_action_test.go +++ b/pkg/restore/actions/service_action_test.go @@ -644,6 +644,36 @@ func TestServiceActionExecute(t *testing.T) { }, }, }, + { + name: "If PreserveNodePorts is false and HealthCheckNodePort is null in last-applied-configuration, it should not crash and the port should be cleared.", + obj: corev1api.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Annotations: map[string]string{ + "kubectl.kubernetes.io/last-applied-configuration": `{"spec":{"healthCheckNodePort":null}}`, + }, + }, + Spec: corev1api.ServiceSpec{ + HealthCheckNodePort: 8080, + ExternalTrafficPolicy: corev1api.ServiceExternalTrafficPolicyTypeLocal, + Type: corev1api.ServiceTypeLoadBalancer, + }, + }, + restore: builder.ForRestore(api.DefaultNamespace, "").PreserveNodePorts(false).Result(), + expectedRes: corev1api.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Annotations: map[string]string{ + "kubectl.kubernetes.io/last-applied-configuration": `{"spec":{"healthCheckNodePort":null}}`, + }, + }, + Spec: corev1api.ServiceSpec{ + HealthCheckNodePort: 0, + ExternalTrafficPolicy: corev1api.ServiceExternalTrafficPolicyTypeLocal, + Type: corev1api.ServiceTypeLoadBalancer, + }, + }, + }, } for _, test := range tests { From fca4d405b16814d7c831ad24f99c7274280504b9 Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Tue, 7 Apr 2026 16:51:13 +0800 Subject: [PATCH 2/3] remove restic for uploader Signed-off-by: Lyndon-Li --- changelogs/unreleased/9677-Lyndon-Li‎‎ | 1 + pkg/cmd/server/server_test.go | 4 +- .../backup_deletion_controller_test.go | 6 +- .../pod_volume_restore_controller_legacy.go | 2 +- pkg/podvolume/backupper.go | 2 +- pkg/podvolume/backupper_test.go | 2 +- pkg/podvolume/restorer_test.go | 18 - pkg/podvolume/util.go | 35 +- pkg/uploader/provider/kopia_test.go | 4 + pkg/uploader/provider/provider.go | 2 +- pkg/uploader/provider/provider_test.go | 2 +- pkg/uploader/provider/restic.go | 269 ---------- pkg/uploader/provider/restic_test.go | 464 ------------------ pkg/uploader/types.go | 1 - 14 files changed, 26 insertions(+), 786 deletions(-) create mode 100644 changelogs/unreleased/9677-Lyndon-Li‎‎ delete mode 100644 pkg/uploader/provider/restic.go delete mode 100644 pkg/uploader/provider/restic_test.go diff --git a/changelogs/unreleased/9677-Lyndon-Li‎‎ b/changelogs/unreleased/9677-Lyndon-Li‎‎ new file mode 100644 index 000000000..f722008e9 --- /dev/null +++ b/changelogs/unreleased/9677-Lyndon-Li‎‎ @@ -0,0 +1 @@ +Fix issue #9469, remove restic for uploader \ No newline at end of file diff --git a/pkg/cmd/server/server_test.go b/pkg/cmd/server/server_test.go index 1ea9d0022..c602f7c9e 100644 --- a/pkg/cmd/server/server_test.go +++ b/pkg/cmd/server/server_test.go @@ -204,9 +204,9 @@ func Test_newServer(t *testing.T) { }, logger) require.Error(t, err) - // invalid clientQPS Restic uploader + // invalid clientQPS Kopia uploader _, err = newServer(factory, &config.Config{ - UploaderType: uploader.ResticType, + UploaderType: uploader.KopiaType, ClientQPS: -1, }, logger) require.Error(t, err) diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index ab3687438..58d9b0420 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -821,12 +821,12 @@ func TestGetSnapshotsInBackup(t *testing.T) { { VolumeNamespace: "ns-1", SnapshotID: "snap-3", - RepositoryType: "restic", + RepositoryType: "kopia", }, { VolumeNamespace: "ns-1", SnapshotID: "snap-4", - RepositoryType: "restic", + RepositoryType: "kopia", }, }, }, @@ -876,7 +876,7 @@ func TestGetSnapshotsInBackup(t *testing.T) { { VolumeNamespace: "ns-1", SnapshotID: "snap-3", - RepositoryType: "restic", + RepositoryType: "kopia", }, }, }, diff --git a/pkg/controller/pod_volume_restore_controller_legacy.go b/pkg/controller/pod_volume_restore_controller_legacy.go index 731b70db9..9ddececf5 100644 --- a/pkg/controller/pod_volume_restore_controller_legacy.go +++ b/pkg/controller/pod_volume_restore_controller_legacy.go @@ -360,5 +360,5 @@ func (c *PodVolumeRestoreReconcilerLegacy) closeDataPath(ctx context.Context, pv } func IsLegacyPVR(pvr *velerov1api.PodVolumeRestore) bool { - return pvr.Spec.UploaderType == uploader.ResticType + return pvr.Spec.UploaderType == "restic" } diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index 1747f1b33..1dc88a9e5 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -272,7 +272,7 @@ func (b *backupper) BackupPodVolumes(backup *velerov1api.Backup, pod *corev1api. return nil, pvcSummary, []error{err} } - repositoryType := funcGetRepositoryType(b.uploaderType) + repositoryType := funcGetRepositoryType() if repositoryType == "" { err := errors.Errorf("empty repository type, uploader %s", b.uploaderType) skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log) diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index 846f65796..f7686978a 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -580,7 +580,7 @@ func TestBackupPodVolumes(t *testing.T) { require.NoError(t, err) if test.mockGetRepositoryType { - funcGetRepositoryType = func(string) string { return "" } + funcGetRepositoryType = func() string { return "" } } else { funcGetRepositoryType = getRepositoryType } diff --git a/pkg/podvolume/restorer_test.go b/pkg/podvolume/restorer_test.go index 36a1fc034..e10146578 100644 --- a/pkg/podvolume/restorer_test.go +++ b/pkg/podvolume/restorer_test.go @@ -204,24 +204,6 @@ func TestRestorePodVolumes(t *testing.T) { }, }, }, - { - name: "get repository type fail", - pvbs: []*velerov1api.PodVolumeBackup{ - createPVBObj(true, true, 1, "restic"), - createPVBObj(true, true, 2, "kopia"), - }, - kubeClientObj: []runtime.Object{ - createNodeAgentDaemonset(), - }, - restoredPod: createPodObj(false, false, false, 2), - sourceNamespace: "fake-ns", - errs: []expectError{ - { - err: "multiple repository type in one backup", - prefixOnly: true, - }, - }, - }, { name: "ensure repo fail", pvbs: []*velerov1api.PodVolumeBackup{ diff --git a/pkg/podvolume/util.go b/pkg/podvolume/util.go index 1864e9615..9bf6f81ca 100644 --- a/pkg/podvolume/util.go +++ b/pkg/podvolume/util.go @@ -62,12 +62,12 @@ func GetVolumeBackupsForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, pod // GetPvbRepositoryType returns the repositoryType according to the PVB information func GetPvbRepositoryType(pvb *velerov1api.PodVolumeBackup) string { - return getRepositoryType(pvb.Spec.UploaderType) + return getRepositoryType() } // GetPvrRepositoryType returns the repositoryType according to the PVR information func GetPvrRepositoryType(pvr *velerov1api.PodVolumeRestore) string { - return getRepositoryType(pvr.Spec.UploaderType) + return getRepositoryType() } // getVolumeBackupInfoForPod returns a map, of volume name -> VolumeBackupInfo, @@ -97,7 +97,7 @@ func getVolumeBackupInfoForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, snapshotID: pvb.Status.SnapshotID, snapshotSize: pvb.Status.Progress.TotalBytes, uploaderType: getUploaderTypeOrDefault(pvb.Spec.UploaderType), - repositoryType: getRepositoryType(pvb.Spec.UploaderType), + repositoryType: getRepositoryType(), } } @@ -111,7 +111,7 @@ func getVolumeBackupInfoForPod(podVolumeBackups []*velerov1api.PodVolumeBackup, } for k, v := range fromAnnntation { - volumes[k] = volumeBackupInfo{v, 0, uploader.ResticType, velerov1api.BackupRepositoryTypeRestic} + volumes[k] = volumeBackupInfo{v, 0, uploader.KopiaType, velerov1api.BackupRepositoryTypeKopia} } return volumes @@ -135,7 +135,7 @@ func GetSnapshotIdentifier(podVolumeBackups *velerov1api.PodVolumeBackupList) ma VolumeNamespace: item.Spec.Pod.Namespace, BackupStorageLocation: item.Spec.BackupStorageLocation, SnapshotID: item.Status.SnapshotID, - RepositoryType: getRepositoryType(item.Spec.UploaderType), + RepositoryType: getRepositoryType(), UploaderType: item.Spec.UploaderType, Source: item.Status.Path, RepoIdentifier: item.Spec.RepoIdentifier, @@ -164,27 +164,14 @@ func getUploaderTypeOrDefault(uploaderType string) string { if uploaderType != "" { return uploaderType } - return uploader.ResticType + return uploader.KopiaType } -// getRepositoryType returns the hardcode repositoryType for different backup methods - Restic or Kopia,uploaderType -// indicates the method. -// For Restic backup method, it is always hardcode to BackupRepositoryTypeRestic, never changed. -// For Kopia backup method, this means we hardcode repositoryType as BackupRepositoryTypeKopia for Unified Repo, -// at present (Kopia backup method is using Unified Repo). However, it doesn't mean we could deduce repositoryType -// from uploaderType for Unified Repo. -// TODO: post v1.10, refactor this function for Kopia backup method. In future, when we have multiple implementations of -// Unified Repo (besides Kopia), we will add the repositoryType to BSL, because by then, we are not able to hardcode -// the repositoryType to BackupRepositoryTypeKopia for Unified Repo. -func getRepositoryType(uploaderType string) string { - switch uploaderType { - case "", uploader.ResticType: - return velerov1api.BackupRepositoryTypeRestic - case uploader.KopiaType: - return velerov1api.BackupRepositoryTypeKopia - default: - return "" - } +// getRepositoryType returns the hardcode repositoryType +// TODO: In future, when we have multiple implementations of Unified Repo (besides Kopia), we will add the repositoryType to BSL, +// because by then, we are not able to hardcode the repositoryType to BackupRepositoryTypeKopia for Unified Repo. +func getRepositoryType() string { + return velerov1api.BackupRepositoryTypeKopia } func isPVBMatchPod(pvb *velerov1api.PodVolumeBackup, podName string, namespace string) bool { diff --git a/pkg/uploader/provider/kopia_test.go b/pkg/uploader/provider/kopia_test.go index 74eaa67f7..734bdb176 100644 --- a/pkg/uploader/provider/kopia_test.go +++ b/pkg/uploader/provider/kopia_test.go @@ -294,6 +294,10 @@ func TestGetPassword(t *testing.T) { } } +type MockCredentialGetter struct { + mock.Mock +} + func (m *MockCredentialGetter) GetCredentials() (string, error) { args := m.Called() return args.String(0), args.Error(1) diff --git a/pkg/uploader/provider/provider.go b/pkg/uploader/provider/provider.go index fe1dd3091..95a34b1a0 100644 --- a/pkg/uploader/provider/provider.go +++ b/pkg/uploader/provider/provider.go @@ -87,6 +87,6 @@ func NewUploaderProvider( if uploaderType == uploader.KopiaType { return NewKopiaUploaderProvider(requesterType, ctx, credGetter, backupRepo, log) } else { - return NewResticUploaderProvider(repoIdentifier, bsl, credGetter, repoKeySelector, log) + return nil, errors.Errorf("unsupported uploader type %v", uploaderType) } } diff --git a/pkg/uploader/provider/provider_test.go b/pkg/uploader/provider/provider_test.go index 199091e32..8f447725b 100644 --- a/pkg/uploader/provider/provider_test.go +++ b/pkg/uploader/provider/provider_test.go @@ -75,7 +75,7 @@ func TestNewUploaderProvider(t *testing.T) { UploaderType: "restic", RequestorType: "requester", needFromFile: true, - ExpectedError: "", + ExpectedError: "unsupported uploader type restic", }, } diff --git a/pkg/uploader/provider/restic.go b/pkg/uploader/provider/restic.go deleted file mode 100644 index 93b907be9..000000000 --- a/pkg/uploader/provider/restic.go +++ /dev/null @@ -1,269 +0,0 @@ -/* -Copyright The Velero Contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package provider - -import ( - "context" - "fmt" - "os" - "strings" - - "github.com/pkg/errors" - "github.com/sirupsen/logrus" - corev1api "k8s.io/api/core/v1" - - "github.com/vmware-tanzu/velero/internal/credentials" - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" - "github.com/vmware-tanzu/velero/pkg/restic" - "github.com/vmware-tanzu/velero/pkg/uploader" - uploaderutil "github.com/vmware-tanzu/velero/pkg/uploader/util" - "github.com/vmware-tanzu/velero/pkg/util/filesystem" -) - -// resticBackupCMDFunc and resticRestoreCMDFunc are mainly used to make testing more convenient -var resticBackupCMDFunc = restic.BackupCommand -var resticBackupFunc = restic.RunBackup -var resticGetSnapshotFunc = restic.GetSnapshotCommand -var resticGetSnapshotIDFunc = restic.GetSnapshotID -var resticRestoreCMDFunc = restic.RestoreCommand -var resticTempCACertFileFunc = restic.TempCACertFile -var resticCmdEnvFunc = restic.CmdEnv - -type resticProvider struct { - repoIdentifier string - credentialsFile string - caCertFile string - cmdEnv []string - extraFlags []string - bsl *velerov1api.BackupStorageLocation - log logrus.FieldLogger -} - -func NewResticUploaderProvider( - repoIdentifier string, - bsl *velerov1api.BackupStorageLocation, - credGetter *credentials.CredentialGetter, - repoKeySelector *corev1api.SecretKeySelector, - log logrus.FieldLogger, -) (Provider, error) { - provider := resticProvider{ - repoIdentifier: repoIdentifier, - bsl: bsl, - log: log, - } - - var err error - provider.credentialsFile, err = credGetter.FromFile.Path(repoKeySelector) - if err != nil { - return nil, errors.Wrap(err, "error creating temp restic credentials file") - } - - // if there's a caCert on the ObjectStorage, write it to disk so that it can be passed to restic - if bsl.Spec.ObjectStorage != nil { - var caCertData []byte - - // Try CACertRef first (new method), then fall back to CACert (deprecated) - if bsl.Spec.ObjectStorage.CACertRef != nil { - caCertString, err := credGetter.FromSecret.Get(bsl.Spec.ObjectStorage.CACertRef) - if err != nil { - return nil, errors.Wrap(err, "error getting CA certificate from secret") - } - caCertData = []byte(caCertString) - } else if bsl.Spec.ObjectStorage.CACert != nil { - caCertData = bsl.Spec.ObjectStorage.CACert - } - - if caCertData != nil { - provider.caCertFile, err = resticTempCACertFileFunc(caCertData, bsl.Name, filesystem.NewFileSystem()) - if err != nil { - return nil, errors.Wrap(err, "error create temp cert file") - } - } - } - - provider.cmdEnv, err = resticCmdEnvFunc(bsl, credGetter.FromFile) - if err != nil { - return nil, errors.Wrap(err, "error generating repository cmnd env") - } - - // #4820: restrieve insecureSkipTLSVerify from BSL configuration for - // AWS plugin. If nothing is return, that means insecureSkipTLSVerify - // is not enable for Restic command. - skipTLSRet := restic.GetInsecureSkipTLSVerifyFromBSL(bsl, log) - if len(skipTLSRet) > 0 { - provider.extraFlags = append(provider.extraFlags, skipTLSRet) - } - - return &provider, nil -} - -func (rp *resticProvider) Close(ctx context.Context) error { - _, err := os.Stat(rp.credentialsFile) - if err == nil { - return os.Remove(rp.credentialsFile) - } else if !os.IsNotExist(err) { - return errors.Errorf("failed to get file %s info with error %v", rp.credentialsFile, err) - } - - _, err = os.Stat(rp.caCertFile) - if err == nil { - return os.Remove(rp.caCertFile) - } else if !os.IsNotExist(err) { - return errors.Errorf("failed to get file %s info with error %v", rp.caCertFile, err) - } - return nil -} - -// RunBackup runs a `backup` command and watches the output to provide -// progress updates to the caller and return snapshotID, isEmptySnapshot, error -func (rp *resticProvider) RunBackup( - ctx context.Context, - path string, - realSource string, - tags map[string]string, - forceFull bool, - parentSnapshot string, - volMode uploader.PersistentVolumeMode, - uploaderCfg map[string]string, - updater uploader.ProgressUpdater) (string, bool, int64, int64, error) { - if updater == nil { - return "", false, 0, 0, errors.New("Need to initial backup progress updater first") - } - - if path == "" { - return "", false, 0, 0, errors.New("path is empty") - } - - if realSource != "" { - return "", false, 0, 0, errors.New("real source is not empty, this is not supported by restic uploader") - } - - if volMode == uploader.PersistentVolumeBlock { - return "", false, 0, 0, errors.New("unable to support block mode") - } - - log := rp.log.WithFields(logrus.Fields{ - "path": path, - "parentSnapshot": parentSnapshot, - }) - - if len(uploaderCfg) > 0 { - parallelFilesUpload, err := uploaderutil.GetParallelFilesUpload(uploaderCfg) - if err != nil { - return "", false, 0, 0, errors.Wrap(err, "failed to get uploader config") - } - if parallelFilesUpload > 0 { - log.Warnf("ParallelFilesUpload is set to %d, but restic does not support parallel file uploads. Ignoring.", parallelFilesUpload) - } - } - - backupCmd := resticBackupCMDFunc(rp.repoIdentifier, rp.credentialsFile, path, tags) - backupCmd.Env = rp.cmdEnv - backupCmd.CACertFile = rp.caCertFile - if len(rp.extraFlags) != 0 { - backupCmd.ExtraFlags = append(backupCmd.ExtraFlags, rp.extraFlags...) - } - - if parentSnapshot != "" { - backupCmd.ExtraFlags = append(backupCmd.ExtraFlags, fmt.Sprintf("--parent=%s", parentSnapshot)) - } - - summary, stderrBuf, err := resticBackupFunc(backupCmd, log, updater) - if err != nil { - if strings.Contains(stderrBuf, "snapshot is empty") { - log.Debugf("Restic backup got empty dir with %s path", path) - return "", true, 0, 0, nil - } - return "", false, 0, 0, errors.WithStack(fmt.Errorf("error running restic backup command %s with error: %v stderr: %v", backupCmd.String(), err, stderrBuf)) - } - // GetSnapshotID - snapshotIDCmd := resticGetSnapshotFunc(rp.repoIdentifier, rp.credentialsFile, tags) - snapshotIDCmd.Env = rp.cmdEnv - snapshotIDCmd.CACertFile = rp.caCertFile - if len(rp.extraFlags) != 0 { - snapshotIDCmd.ExtraFlags = append(snapshotIDCmd.ExtraFlags, rp.extraFlags...) - } - snapshotID, err := resticGetSnapshotIDFunc(snapshotIDCmd) - if err != nil { - return "", false, 0, 0, errors.WithStack(fmt.Errorf("error getting snapshot id with error: %v", err)) - } - log.Infof("Run command=%s, stdout=%s, stderr=%s", backupCmd.String(), summary, stderrBuf) - return snapshotID, false, 0, 0, nil -} - -// RunRestore runs a `restore` command and monitors the volume size to -// provide progress updates to the caller. -func (rp *resticProvider) RunRestore( - ctx context.Context, - snapshotID string, - volumePath string, - volMode uploader.PersistentVolumeMode, - uploaderCfg map[string]string, - updater uploader.ProgressUpdater) (int64, error) { - if updater == nil { - return 0, errors.New("Need to initial backup progress updater first") - } - log := rp.log.WithFields(logrus.Fields{ - "snapshotID": snapshotID, - "volumePath": volumePath, - }) - - if volMode == uploader.PersistentVolumeBlock { - return 0, errors.New("unable to support block mode") - } - - restoreCmd := resticRestoreCMDFunc(rp.repoIdentifier, rp.credentialsFile, snapshotID, volumePath) - restoreCmd.Env = rp.cmdEnv - restoreCmd.CACertFile = rp.caCertFile - if len(rp.extraFlags) != 0 { - restoreCmd.ExtraFlags = append(restoreCmd.ExtraFlags, rp.extraFlags...) - } - - extraFlags, err := rp.parseRestoreExtraFlags(uploaderCfg) - if err != nil { - return 0, errors.Wrap(err, "failed to parse uploader config") - } else if len(extraFlags) != 0 { - restoreCmd.ExtraFlags = append(restoreCmd.ExtraFlags, extraFlags...) - } - - stdout, stderr, err := restic.RunRestore(restoreCmd, log, updater) - - log.Infof("Run command=%v, stdout=%s, stderr=%s", restoreCmd, stdout, stderr) - return 0, err -} - -func (rp *resticProvider) parseRestoreExtraFlags(uploaderCfg map[string]string) ([]string, error) { - extraFlags := []string{} - if len(uploaderCfg) == 0 { - return extraFlags, nil - } - - writeSparseFiles, err := uploaderutil.GetWriteSparseFiles(uploaderCfg) - if err != nil { - return extraFlags, errors.Wrap(err, "failed to get uploader config") - } - - if writeSparseFiles { - extraFlags = append(extraFlags, "--sparse") - } - - if restoreConcurrency, err := uploaderutil.GetRestoreConcurrency(uploaderCfg); err == nil && restoreConcurrency > 0 { - return extraFlags, errors.New("restic does not support parallel restore") - } - - return extraFlags, nil -} diff --git a/pkg/uploader/provider/restic_test.go b/pkg/uploader/provider/restic_test.go deleted file mode 100644 index 24eb11e04..000000000 --- a/pkg/uploader/provider/restic_test.go +++ /dev/null @@ -1,464 +0,0 @@ -/* -Copyright The Velero Contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package provider - -import ( - "errors" - "os" - "reflect" - "strings" - "testing" - - "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" - corev1api "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - - "github.com/vmware-tanzu/velero/internal/credentials" - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" - "github.com/vmware-tanzu/velero/pkg/builder" - "github.com/vmware-tanzu/velero/pkg/restic" - "github.com/vmware-tanzu/velero/pkg/uploader" - "github.com/vmware-tanzu/velero/pkg/util" - "github.com/vmware-tanzu/velero/pkg/util/filesystem" -) - -func TestResticRunBackup(t *testing.T) { - testCases := []struct { - name string - nilUpdater bool - parentSnapshot string - rp *resticProvider - volMode uploader.PersistentVolumeMode - hookBackupFunc func(string, string, string, map[string]string) *restic.Command - hookResticBackupFunc func(*restic.Command, logrus.FieldLogger, uploader.ProgressUpdater) (string, string, error) - hookResticGetSnapshotFunc func(string, string, map[string]string) *restic.Command - hookResticGetSnapshotIDFunc func(*restic.Command) (string, error) - errorHandleFunc func(err error) bool - }{ - { - name: "nil uploader", - rp: &resticProvider{log: logrus.New()}, - nilUpdater: true, - hookBackupFunc: func(repoIdentifier string, passwordFile string, path string, tags map[string]string) *restic.Command { - return &restic.Command{Command: "date"} - }, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "Need to initial backup progress updater first") - }, - }, - { - name: "wrong restic execute command", - rp: &resticProvider{log: logrus.New()}, - hookBackupFunc: func(repoIdentifier string, passwordFile string, path string, tags map[string]string) *restic.Command { - return &restic.Command{Command: "date"} - }, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "error running") - }, - }, { - name: "has parent snapshot", - rp: &resticProvider{log: logrus.New()}, - parentSnapshot: "parentSnapshot", - hookBackupFunc: func(repoIdentifier string, passwordFile string, path string, tags map[string]string) *restic.Command { - return &restic.Command{Command: "date"} - }, - hookResticBackupFunc: func(*restic.Command, logrus.FieldLogger, uploader.ProgressUpdater) (string, string, error) { - return "", "", nil - }, - - hookResticGetSnapshotIDFunc: func(*restic.Command) (string, error) { return "test-snapshot-id", nil }, - errorHandleFunc: func(err error) bool { - return err == nil - }, - }, - { - name: "has extra flags", - rp: &resticProvider{log: logrus.New(), extraFlags: []string{"testFlags"}}, - hookBackupFunc: func(string, string, string, map[string]string) *restic.Command { - return &restic.Command{Command: "date"} - }, - hookResticBackupFunc: func(*restic.Command, logrus.FieldLogger, uploader.ProgressUpdater) (string, string, error) { - return "", "", nil - }, - hookResticGetSnapshotIDFunc: func(*restic.Command) (string, error) { return "test-snapshot-id", nil }, - errorHandleFunc: func(err error) bool { - return err == nil - }, - }, - { - name: "failed to get snapshot id", - rp: &resticProvider{log: logrus.New(), extraFlags: []string{"testFlags"}}, - hookBackupFunc: func(string, string, string, map[string]string) *restic.Command { - return &restic.Command{Command: "date"} - }, - hookResticBackupFunc: func(*restic.Command, logrus.FieldLogger, uploader.ProgressUpdater) (string, string, error) { - return "", "", nil - }, - hookResticGetSnapshotIDFunc: func(*restic.Command) (string, error) { - return "test-snapshot-id", errors.New("failed to get snapshot id") - }, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "failed to get snapshot id") - }, - }, - { - name: "failed to use block mode", - rp: &resticProvider{log: logrus.New(), extraFlags: []string{"testFlags"}}, - volMode: uploader.PersistentVolumeBlock, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "unable to support block mode") - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - var err error - parentSnapshot := tc.parentSnapshot - if tc.hookBackupFunc != nil { - resticBackupCMDFunc = tc.hookBackupFunc - } - if tc.hookResticBackupFunc != nil { - resticBackupFunc = tc.hookResticBackupFunc - } - if tc.hookResticGetSnapshotFunc != nil { - resticGetSnapshotFunc = tc.hookResticGetSnapshotFunc - } - if tc.hookResticGetSnapshotIDFunc != nil { - resticGetSnapshotIDFunc = tc.hookResticGetSnapshotIDFunc - } - if tc.volMode == "" { - tc.volMode = uploader.PersistentVolumeFilesystem - } - if !tc.nilUpdater { - updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: tc.rp.log, Ctx: t.Context(), Cli: fake.NewClientBuilder().WithScheme(util.VeleroScheme).Build()} - _, _, _, _, err = tc.rp.RunBackup(t.Context(), "var", "", map[string]string{}, false, parentSnapshot, tc.volMode, map[string]string{}, &updater) - } else { - _, _, _, _, err = tc.rp.RunBackup(t.Context(), "var", "", map[string]string{}, false, parentSnapshot, tc.volMode, map[string]string{}, nil) - } - - tc.rp.log.Infof("test name %v error %v", tc.name, err) - require.True(t, tc.errorHandleFunc(err)) - }) - } -} - -func TestResticRunRestore(t *testing.T) { - resticRestoreCMDFunc = func(repoIdentifier, passwordFile, snapshotID, target string) *restic.Command { - return &restic.Command{Args: []string{""}} - } - testCases := []struct { - name string - rp *resticProvider - nilUpdater bool - hookResticRestoreFunc func(repoIdentifier, passwordFile, snapshotID, target string) *restic.Command - errorHandleFunc func(err error) bool - volMode uploader.PersistentVolumeMode - }{ - { - name: "wrong restic execute command", - rp: &resticProvider{log: logrus.New()}, - nilUpdater: true, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "Need to initial backup progress updater first") - }, - }, - { - name: "has extral flags", - rp: &resticProvider{log: logrus.New(), extraFlags: []string{"test-extra-flags"}}, - hookResticRestoreFunc: func(repoIdentifier, passwordFile, snapshotID, target string) *restic.Command { - return &restic.Command{Args: []string{"date"}} - }, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "error running command") - }, - }, - { - name: "wrong restic execute command", - rp: &resticProvider{log: logrus.New()}, - hookResticRestoreFunc: func(repoIdentifier, passwordFile, snapshotID, target string) *restic.Command { - return &restic.Command{Args: []string{"date"}} - }, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "error running command") - }, - }, - { - name: "error block volume mode", - rp: &resticProvider{log: logrus.New()}, - errorHandleFunc: func(err error) bool { - return strings.Contains(err.Error(), "unable to support block mode") - }, - volMode: uploader.PersistentVolumeBlock, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - if tc.volMode == "" { - tc.volMode = uploader.PersistentVolumeFilesystem - } - resticRestoreCMDFunc = tc.hookResticRestoreFunc - if tc.volMode == "" { - tc.volMode = uploader.PersistentVolumeFilesystem - } - var err error - if !tc.nilUpdater { - updater := FakeBackupProgressUpdater{PodVolumeBackup: &velerov1api.PodVolumeBackup{}, Log: tc.rp.log, Ctx: t.Context(), Cli: fake.NewClientBuilder().WithScheme(util.VeleroScheme).Build()} - _, err = tc.rp.RunRestore(t.Context(), "", "var", tc.volMode, map[string]string{}, &updater) - } else { - _, err = tc.rp.RunRestore(t.Context(), "", "var", tc.volMode, map[string]string{}, nil) - } - - tc.rp.log.Infof("test name %v error %v", tc.name, err) - require.True(t, tc.errorHandleFunc(err)) - }) - } -} - -func TestClose(t *testing.T) { - t.Run("Delete existing credentials file", func(t *testing.T) { - // Create temporary files for the credentials and caCert - credentialsFile, err := os.CreateTemp(t.TempDir(), "credentialsFile") - if err != nil { - t.Fatalf("failed to create temp file: %v", err) - } - defer os.Remove(credentialsFile.Name()) - - caCertFile, err := os.CreateTemp(t.TempDir(), "caCertFile") - if err != nil { - t.Fatalf("failed to create temp file: %v", err) - } - defer os.Remove(caCertFile.Name()) - rp := &resticProvider{ - credentialsFile: credentialsFile.Name(), - caCertFile: caCertFile.Name(), - } - // Test deleting an existing credentials file - err = rp.Close(t.Context()) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - _, err = os.Stat(rp.credentialsFile) - if !os.IsNotExist(err) { - t.Errorf("expected credentials file to be deleted, got error: %v", err) - } - }) - - t.Run("Delete existing caCert file", func(t *testing.T) { - // Create temporary files for the credentials and caCert - caCertFile, err := os.CreateTemp(t.TempDir(), "caCertFile") - if err != nil { - t.Fatalf("failed to create temp file: %v", err) - } - defer os.Remove(caCertFile.Name()) - rp := &resticProvider{ - credentialsFile: "", - caCertFile: "", - } - err = rp.Close(t.Context()) - // Test deleting an existing caCert file - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - _, err = os.Stat(rp.caCertFile) - if !os.IsNotExist(err) { - t.Errorf("expected caCert file to be deleted, got error: %v", err) - } - }) -} - -type MockCredentialGetter struct { - mock.Mock -} - -func (m *MockCredentialGetter) Path(selector *corev1api.SecretKeySelector) (string, error) { - args := m.Called(selector) - return args.Get(0).(string), args.Error(1) -} - -func TestNewResticUploaderProvider(t *testing.T) { - testCases := []struct { - name string - emptyBSL bool - mockCredFunc func(*MockCredentialGetter, *corev1api.SecretKeySelector) - resticCmdEnvFunc func(backupLocation *velerov1api.BackupStorageLocation, credentialFileStore credentials.FileStore) ([]string, error) - resticTempCACertFileFunc func(caCert []byte, bsl string, fs filesystem.Interface) (string, error) - checkFunc func(t *testing.T, provider Provider, err error) - }{ - { - name: "No error in creating temp credentials file", - mockCredFunc: func(credGetter *MockCredentialGetter, repoKeySelector *corev1api.SecretKeySelector) { - credGetter.On("Path", repoKeySelector).Return("temp-credentials", nil) - }, - checkFunc: func(t *testing.T, provider Provider, err error) { - t.Helper() - require.NoError(t, err) - assert.NotNil(t, provider) - }, - }, { - name: "Error in creating temp credentials file", - mockCredFunc: func(credGetter *MockCredentialGetter, repoKeySelector *corev1api.SecretKeySelector) { - credGetter.On("Path", repoKeySelector).Return("", errors.New("error creating temp credentials file")) - }, - checkFunc: func(t *testing.T, provider Provider, err error) { - t.Helper() - require.Error(t, err) - assert.Nil(t, provider) - }, - }, { - name: "ObjectStorage with CACert present and creating CACert file failed", - mockCredFunc: func(credGetter *MockCredentialGetter, repoKeySelector *corev1api.SecretKeySelector) { - credGetter.On("Path", repoKeySelector).Return("temp-credentials", nil) - }, - resticTempCACertFileFunc: func(caCert []byte, bsl string, fs filesystem.Interface) (string, error) { - return "", errors.New("error writing CACert file") - }, - checkFunc: func(t *testing.T, provider Provider, err error) { - t.Helper() - require.Error(t, err) - assert.Nil(t, provider) - }, - }, { - name: "Generating repository cmd failed", - mockCredFunc: func(credGetter *MockCredentialGetter, repoKeySelector *corev1api.SecretKeySelector) { - credGetter.On("Path", repoKeySelector).Return("temp-credentials", nil) - }, - resticTempCACertFileFunc: func(caCert []byte, bsl string, fs filesystem.Interface) (string, error) { - return "test-ca", nil - }, - resticCmdEnvFunc: func(backupLocation *velerov1api.BackupStorageLocation, credentialFileStore credentials.FileStore) ([]string, error) { - return nil, errors.New("error generating repository cmnd env") - }, - checkFunc: func(t *testing.T, provider Provider, err error) { - t.Helper() - require.Error(t, err) - assert.Nil(t, provider) - }, - }, { - name: "New provider with not nil bsl", - mockCredFunc: func(credGetter *MockCredentialGetter, repoKeySelector *corev1api.SecretKeySelector) { - credGetter.On("Path", repoKeySelector).Return("temp-credentials", nil) - }, - resticTempCACertFileFunc: func(caCert []byte, bsl string, fs filesystem.Interface) (string, error) { - return "test-ca", nil - }, - resticCmdEnvFunc: func(backupLocation *velerov1api.BackupStorageLocation, credentialFileStore credentials.FileStore) ([]string, error) { - return nil, nil - }, - checkFunc: func(t *testing.T, provider Provider, err error) { - t.Helper() - require.NoError(t, err) - assert.NotNil(t, provider) - }, - }, - { - name: "New provider with nil bsl", - emptyBSL: true, - mockCredFunc: func(credGetter *MockCredentialGetter, repoKeySelector *corev1api.SecretKeySelector) { - credGetter.On("Path", repoKeySelector).Return("temp-credentials", nil) - }, - resticTempCACertFileFunc: func(caCert []byte, bsl string, fs filesystem.Interface) (string, error) { - return "test-ca", nil - }, - resticCmdEnvFunc: func(backupLocation *velerov1api.BackupStorageLocation, credentialFileStore credentials.FileStore) ([]string, error) { - return nil, nil - }, - checkFunc: func(t *testing.T, provider Provider, err error) { - t.Helper() - require.NoError(t, err) - assert.NotNil(t, provider) - }, - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - repoIdentifier := "my-repo" - bsl := &velerov1api.BackupStorageLocation{} - if !tc.emptyBSL { - bsl = builder.ForBackupStorageLocation("test-ns", "test-name").CACert([]byte("my-cert")).Result() - } - credGetter := &credentials.CredentialGetter{} - repoKeySelector := &corev1api.SecretKeySelector{} - log := logrus.New() - - // Mock CredentialGetter - mockCredGetter := &MockCredentialGetter{} - credGetter.FromFile = mockCredGetter - tc.mockCredFunc(mockCredGetter, repoKeySelector) - if tc.resticCmdEnvFunc != nil { - resticCmdEnvFunc = tc.resticCmdEnvFunc - } - if tc.resticTempCACertFileFunc != nil { - resticTempCACertFileFunc = tc.resticTempCACertFileFunc - } - provider, err := NewResticUploaderProvider(repoIdentifier, bsl, credGetter, repoKeySelector, log) - tc.checkFunc(t, provider, err) - }) - } -} - -func TestParseUploaderConfig(t *testing.T) { - rp := &resticProvider{} - - testCases := []struct { - name string - uploaderConfig map[string]string - expectedFlags []string - }{ - { - name: "SparseFilesEnabled", - uploaderConfig: map[string]string{ - "WriteSparseFiles": "true", - }, - expectedFlags: []string{"--sparse"}, - }, - { - name: "SparseFilesDisabled", - uploaderConfig: map[string]string{ - "writeSparseFiles": "false", - }, - expectedFlags: []string{}, - }, - { - name: "RestoreConcorrency", - uploaderConfig: map[string]string{ - "Parallel": "5", - }, - expectedFlags: []string{}, - }, - } - - for _, testCase := range testCases { - t.Run(testCase.name, func(t *testing.T) { - result, err := rp.parseRestoreExtraFlags(testCase.uploaderConfig) - if err != nil { - t.Errorf("Test case %s failed with error: %v", testCase.name, err) - return - } - - if !reflect.DeepEqual(result, testCase.expectedFlags) { - t.Errorf("Test case %s failed. Expected: %v, Got: %v", testCase.name, testCase.expectedFlags, result) - } - }) - } -} diff --git a/pkg/uploader/types.go b/pkg/uploader/types.go index f69cbf072..52f8ca5bf 100644 --- a/pkg/uploader/types.go +++ b/pkg/uploader/types.go @@ -22,7 +22,6 @@ import ( ) const ( - ResticType = "restic" KopiaType = "kopia" SnapshotRequesterTag = "snapshot-requester" SnapshotUploaderTag = "snapshot-uploader" From e8fa708933b0ca173d319009d230a5316fce6a88 Mon Sep 17 00:00:00 2001 From: Scott Seago Date: Tue, 7 Apr 2026 13:22:38 -0400 Subject: [PATCH 3/3] Add custom action type to volume policies (#9540) * Add custom action type to volume policies Signed-off-by: Scott Seago * Update internal/resourcepolicies/resource_policies.go Co-authored-by: Tiger Kaovilai Signed-off-by: Scott Seago * added "custom" to validation list Signed-off-by: Scott Seago * responding to review comments Signed-off-by: Scott Seago --------- Signed-off-by: Scott Seago Co-authored-by: Tiger Kaovilai --- changelogs/unreleased/9540-sseago | 1 + .../resourcepolicies/resource_policies.go | 2 + .../volume_resources_validator.go | 2 +- internal/volumehelper/volume_policy_helper.go | 127 ++++++++++++++++-- pkg/backup/actions/csi/pvc_action.go | 25 ++-- pkg/backup/actions/csi/pvc_action_test.go | 12 +- pkg/backup/item_backupper.go | 2 +- .../volumehelper/volume_policy_helper.go | 46 ++++++- pkg/util/volumehelper/volume_policy_helper.go | 30 +++++ 9 files changed, 215 insertions(+), 32 deletions(-) create mode 100644 changelogs/unreleased/9540-sseago create mode 100644 pkg/util/volumehelper/volume_policy_helper.go diff --git a/changelogs/unreleased/9540-sseago b/changelogs/unreleased/9540-sseago new file mode 100644 index 000000000..3606d4f30 --- /dev/null +++ b/changelogs/unreleased/9540-sseago @@ -0,0 +1 @@ +Add custom action type to volume policies diff --git a/internal/resourcepolicies/resource_policies.go b/internal/resourcepolicies/resource_policies.go index d484cabce..6b5046e57 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/internal/resourcepolicies/resource_policies.go @@ -42,6 +42,8 @@ const ( FSBackup VolumeActionType = "fs-backup" // snapshot action can have 3 different meaning based on velero configuration and backup spec - cloud provider based snapshots, local csi snapshots and datamover snapshots Snapshot VolumeActionType = "snapshot" + // custom action is used to identify a volume that will be handled by an external plugin. Velero will not snapshot or use fs-backup if action=="custom" + Custom VolumeActionType = "custom" ) // Action defined as one action for a specific way of backup diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index b6031eec2..652c41d30 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/internal/resourcepolicies/volume_resources_validator.go @@ -90,7 +90,7 @@ func decodeStruct(r io.Reader, s any) error { func (a *Action) validate() error { // validate Type valid := false - if a.Type == Skip || a.Type == Snapshot || a.Type == FSBackup { + if a.Type == Skip || a.Type == Snapshot || a.Type == FSBackup || a.Type == Custom { valid = true } if !valid { diff --git a/internal/volumehelper/volume_policy_helper.go b/internal/volumehelper/volume_policy_helper.go index a47f7be83..339b80011 100644 --- a/internal/volumehelper/volume_policy_helper.go +++ b/internal/volumehelper/volume_policy_helper.go @@ -18,13 +18,9 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/boolptr" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" podvolumeutil "github.com/vmware-tanzu/velero/pkg/util/podvolume" + vhutil "github.com/vmware-tanzu/velero/pkg/util/volumehelper" ) -type VolumeHelper interface { - ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, error) - ShouldPerformFSBackup(volume corev1api.Volume, pod corev1api.Pod) (bool, error) -} - type volumeHelperImpl struct { volumePolicy *resourcepolicies.Policies snapshotVolumes *bool @@ -53,7 +49,7 @@ func NewVolumeHelperImpl( client crclient.Client, defaultVolumesToFSBackup bool, backupExcludePVC bool, -) VolumeHelper { +) vhutil.VolumeHelper { // Pass nil namespaces - no cache will be built, so this never fails. // This is used by plugins that don't need the cache optimization. vh, _ := NewVolumeHelperImplWithNamespaces( @@ -81,7 +77,7 @@ func NewVolumeHelperImplWithNamespaces( defaultVolumesToFSBackup bool, backupExcludePVC bool, namespaces []string, -) (VolumeHelper, error) { +) (vhutil.VolumeHelper, error) { var pvcPodCache *podvolumeutil.PVCPodCache if len(namespaces) > 0 { pvcPodCache = podvolumeutil.NewPVCPodCache() @@ -110,7 +106,7 @@ func NewVolumeHelperImplWithCache( client crclient.Client, logger logrus.FieldLogger, pvcPodCache *podvolumeutil.PVCPodCache, -) (VolumeHelper, error) { +) (vhutil.VolumeHelper, error) { resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackup(backup, client, logger) if err != nil { return nil, errors.Wrap(err, "failed to get volume policies from backup") @@ -319,6 +315,121 @@ func (v volumeHelperImpl) shouldPerformFSBackupLegacy( } } +func (v *volumeHelperImpl) ShouldPerformCustomAction(obj runtime.Unstructured, groupResource schema.GroupResource, matchParams map[string]any) (bool, error) { + // check if volume policy exists and also check if the object(pv/pvc) fits a volume policy criteria and see if the associated action is custom with the provided param values + pvc := new(corev1api.PersistentVolumeClaim) + pv := new(corev1api.PersistentVolume) + var err error + + var pvNotFoundErr error + if groupResource == kuberesource.PersistentVolumeClaims { + if err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &pvc); err != nil { + v.logger.WithError(err).Error("fail to convert unstructured into PVC") + return false, err + } + + pv, err = kubeutil.GetPVForPVC(pvc, v.client) + if err != nil { + // Any error means PV not available - save to return later if no policy matches + v.logger.Debugf("PV not found for PVC %s: %v", pvc.Namespace+"/"+pvc.Name, err) + pvNotFoundErr = err + pv = nil + } + } + + if groupResource == kuberesource.PersistentVolumes { + if err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &pv); err != nil { + v.logger.WithError(err).Error("fail to convert unstructured into PV") + return false, err + } + } + + if v.volumePolicy != nil { + vfd := resourcepolicies.NewVolumeFilterData(pv, nil, pvc) + action, err := v.volumePolicy.GetMatchAction(vfd) + if err != nil { + v.logger.WithError(err).Errorf("fail to get VolumePolicy match action for %+v", vfd) + return false, err + } + + // If there is a match action, and the action type is custom, return true + // if the provided parameters match as well, else return false. + // If there is no match action, also return false + if action != nil { + if action.Type == resourcepolicies.Custom { + for k, requiredValue := range matchParams { + if actionValue, ok := action.Parameters[k]; !ok || actionValue != requiredValue { + v.logger.Infof("Skipping custom action for %+v as value for parameter %s is %s rather than the required %s", vfd, k, actionValue, requiredValue) + return false, nil + } + } + v.logger.Infof("performing custom action for %+v", vfd) + return true, nil + } else { + v.logger.Infof("Skipping custom action for %+v as the action type is %s", vfd, action.Type) + return false, nil + } + } + } + // If resource is PVC, and PV is nil (e.g., Pending/Lost PVC with no matching policy), return the original error + // Don't error out on no PV, just return false + if groupResource == kuberesource.PersistentVolumeClaims && pv == nil && pvNotFoundErr != nil { + v.logger.WithError(pvNotFoundErr).Warnf("fail to get PV for PVC %s", pvc.Namespace+"/"+pvc.Name) + return false, nil + } + + v.logger.Infof("skipping custom action for pv %s due to no matching volume policy", pv.Name) + return false, nil +} + +// returns false if no matching action found. Returns true with the action name and Parameters map if there is a matching policy +func (v *volumeHelperImpl) GetActionParameters(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, string, map[string]any, error) { + // if volume policy exists, return action parameters. + pvc := new(corev1api.PersistentVolumeClaim) + pv := new(corev1api.PersistentVolume) + var err error + + if groupResource == kuberesource.PersistentVolumeClaims { + if err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &pvc); err != nil { + v.logger.WithError(err).Error("fail to convert unstructured into PVC") + return false, "", nil, err + } + + pv, err = kubeutil.GetPVForPVC(pvc, v.client) + if err != nil { + v.logger.WithError(err).Warnf("failed to get PV for PVC %s", pvc.Namespace+"/"+pvc.Name) + return false, "", nil, nil + } + } + + if groupResource == kuberesource.PersistentVolumes { + if err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &pv); err != nil { + v.logger.WithError(err).Error("fail to convert unstructured into PV") + return false, "", nil, err + } + } + + if v.volumePolicy != nil { + vfd := resourcepolicies.NewVolumeFilterData(pv, nil, pvc) + action, err := v.volumePolicy.GetMatchAction(vfd) + if err != nil { + v.logger.WithError(err).Errorf("fail to get VolumePolicy match action for PV %s", pv.Name) + return false, "", nil, err + } + + // If there is a match action, and the action type is custom, return true + // if the provided parameters match as well, else return false. + // If there is no match action, also return false + if action != nil { + v.logger.Infof("found matching action for pv %s, returning parameters", pv.Name) + return true, string(action.Type), action.Parameters, nil + } + } + + v.logger.Infof("no matching volume policy found for pv %s, no parameters to return", pv.Name) + return false, "", nil, nil +} + func (v *volumeHelperImpl) shouldIncludeVolumeInBackup(vol corev1api.Volume) bool { includeVolumeInBackup := true // cannot backup hostpath volumes as they are not mounted into /var/lib/kubelet/pods diff --git a/pkg/backup/actions/csi/pvc_action.go b/pkg/backup/actions/csi/pvc_action.go index 8e2e77316..ac5f71a98 100644 --- a/pkg/backup/actions/csi/pvc_action.go +++ b/pkg/backup/actions/csi/pvc_action.go @@ -44,7 +44,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" - internalvolumehelper "github.com/vmware-tanzu/velero/internal/volumehelper" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" velerov2alpha1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1" veleroclient "github.com/vmware-tanzu/velero/pkg/client" @@ -59,6 +58,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/util/csi" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" podvolumeutil "github.com/vmware-tanzu/velero/pkg/util/podvolume" + vhutil "github.com/vmware-tanzu/velero/pkg/util/volumehelper" ) // TODO: Replace hardcoded VolumeSnapshot finalizer strings with constants from @@ -128,9 +128,9 @@ func (p *pvcBackupItemAction) ensurePVCPodCacheForNamespace(ctx context.Context, // getVolumeHelperWithCache creates a VolumeHelper using the pre-built PVC-to-Pod cache. // The cache should be ensured for the relevant namespace(s) before calling this. -func (p *pvcBackupItemAction) getVolumeHelperWithCache(backup *velerov1api.Backup) (internalvolumehelper.VolumeHelper, error) { +func (p *pvcBackupItemAction) getVolumeHelperWithCache(backup *velerov1api.Backup) (vhutil.VolumeHelper, error) { // Create VolumeHelper with our lazy-built cache - vh, err := internalvolumehelper.NewVolumeHelperImplWithCache( + vh, err := volumehelper.NewVolumeHelperWithCache( *backup, p.crClient, p.log, @@ -149,7 +149,7 @@ func (p *pvcBackupItemAction) getVolumeHelperWithCache(backup *velerov1api.Backu // Since plugin instances are unique per backup (created via newPluginManager and // cleaned up via CleanupClients at backup completion), we can safely cache this. // See issue #9179 and PR #9226 for details. -func (p *pvcBackupItemAction) getOrCreateVolumeHelper(backup *velerov1api.Backup) (internalvolumehelper.VolumeHelper, error) { +func (p *pvcBackupItemAction) getOrCreateVolumeHelper(backup *velerov1api.Backup) (vhutil.VolumeHelper, error) { // Initialize the PVC-to-Pod cache if needed if p.pvcPodCache == nil { p.pvcPodCache = podvolumeutil.NewPVCPodCache() @@ -322,13 +322,9 @@ func (p *pvcBackupItemAction) Execute( return nil, nil, "", nil, err } - shouldSnapshot, err := volumehelper.ShouldPerformSnapshotWithVolumeHelper( + shouldSnapshot, err := vh.ShouldPerformSnapshot( item, kuberesource.PersistentVolumeClaims, - *backup, - p.crClient, - p.log, - vh, ) if err != nil { return nil, nil, "", nil, err @@ -708,7 +704,7 @@ func (p *pvcBackupItemAction) getVolumeSnapshotReference( } // Filter PVCs by volume policy - filteredPVCs, err := p.filterPVCsByVolumePolicy(groupedPVCs, backup, vh) + filteredPVCs, err := p.filterPVCsByVolumePolicy(groupedPVCs, vh) if err != nil { return nil, errors.Wrapf(err, "failed to filter PVCs by volume policy for VolumeGroupSnapshot group %q", group) } @@ -844,8 +840,7 @@ func (p *pvcBackupItemAction) listGroupedPVCs(ctx context.Context, namespace, la func (p *pvcBackupItemAction) filterPVCsByVolumePolicy( pvcs []corev1api.PersistentVolumeClaim, - backup *velerov1api.Backup, - vh internalvolumehelper.VolumeHelper, + vh vhutil.VolumeHelper, ) ([]corev1api.PersistentVolumeClaim, error) { var filteredPVCs []corev1api.PersistentVolumeClaim @@ -859,13 +854,9 @@ func (p *pvcBackupItemAction) filterPVCsByVolumePolicy( // Check if this PVC should be snapshotted according to volume policies // Uses the cached VolumeHelper for better performance with many PVCs/pods - shouldSnapshot, err := volumehelper.ShouldPerformSnapshotWithVolumeHelper( + shouldSnapshot, err := vh.ShouldPerformSnapshot( unstructuredPVC, kuberesource.PersistentVolumeClaims, - *backup, - p.crClient, - p.log, - vh, ) if err != nil { return nil, errors.Wrapf(err, "failed to check volume policy for PVC %s/%s", pvc.Namespace, pvc.Name) diff --git a/pkg/backup/actions/csi/pvc_action_test.go b/pkg/backup/actions/csi/pvc_action_test.go index b94d63701..efcb0b0ab 100644 --- a/pkg/backup/actions/csi/pvc_action_test.go +++ b/pkg/backup/actions/csi/pvc_action_test.go @@ -842,9 +842,13 @@ volumePolicies: crClient: client, } - // Pass nil for VolumeHelper in tests - it will fall back to creating a new one per call - // This is the expected behavior for testing and third-party plugins - result, err := action.filterPVCsByVolumePolicy(tt.pvcs, backup, nil) + // Create a VolumeHelper using the same method the plugin would use + vh, err := action.getOrCreateVolumeHelper(backup) + require.NoError(t, err) + require.NotNil(t, vh) + + // Test with the pre-created VolumeHelper + result, err := action.filterPVCsByVolumePolicy(tt.pvcs, vh) if tt.expectError { require.Error(t, err) } else { @@ -959,7 +963,7 @@ volumePolicies: require.NotNil(t, vh) // Test with the pre-created VolumeHelper (non-nil path) - result, err := action.filterPVCsByVolumePolicy(pvcs, backup, vh) + result, err := action.filterPVCsByVolumePolicy(pvcs, vh) require.NoError(t, err) // Should filter out the NFS PVC, leaving only the CSI PVC diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index b50f4e119..2ca266e91 100644 --- a/pkg/backup/item_backupper.go +++ b/pkg/backup/item_backupper.go @@ -40,7 +40,6 @@ import ( "github.com/vmware-tanzu/velero/internal/hook" "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/internal/volume" - "github.com/vmware-tanzu/velero/internal/volumehelper" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/archive" "github.com/vmware-tanzu/velero/pkg/client" @@ -54,6 +53,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/podvolume" "github.com/vmware-tanzu/velero/pkg/util/boolptr" csiutil "github.com/vmware-tanzu/velero/pkg/util/csi" + "github.com/vmware-tanzu/velero/pkg/util/volumehelper" ) const ( diff --git a/pkg/plugin/utils/volumehelper/volume_policy_helper.go b/pkg/plugin/utils/volumehelper/volume_policy_helper.go index a19f8d7c8..843c23b06 100644 --- a/pkg/plugin/utils/volumehelper/volume_policy_helper.go +++ b/pkg/plugin/utils/volumehelper/volume_policy_helper.go @@ -26,6 +26,8 @@ import ( "github.com/vmware-tanzu/velero/internal/volumehelper" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/util/boolptr" + podvolumeutil "github.com/vmware-tanzu/velero/pkg/util/podvolume" + vhutil "github.com/vmware-tanzu/velero/pkg/util/volumehelper" ) // ShouldPerformSnapshotWithBackup is used for third-party plugins. @@ -66,7 +68,7 @@ func ShouldPerformSnapshotWithVolumeHelper( backup velerov1api.Backup, crClient crclient.Client, logger logrus.FieldLogger, - vh volumehelper.VolumeHelper, + vh vhutil.VolumeHelper, ) (bool, error) { // If a VolumeHelper is provided, use it directly if vh != nil { @@ -95,3 +97,45 @@ func ShouldPerformSnapshotWithVolumeHelper( return volumeHelperImpl.ShouldPerformSnapshot(unstructured, groupResource) } + +// NewVolumeHelperWithNamespaces creates a VolumeHelper with a PVC-to-Pod cache for improved performance. +// The cache is built internally from the provided namespaces list. +// This avoids O(N*M) complexity when there are many PVCs and pods. +// See issue #9179 for details. +// Returns an error if cache building fails - callers should not proceed with backup in this case. +func NewVolumeHelperWithNamespaces( + volumePolicy *resourcepolicies.Policies, + snapshotVolumes *bool, + logger logrus.FieldLogger, + client crclient.Client, + defaultVolumesToFSBackup bool, + backupExcludePVC bool, + namespaces []string, +) (vhutil.VolumeHelper, error) { + return volumehelper.NewVolumeHelperImplWithNamespaces( + volumePolicy, + snapshotVolumes, + logger, + client, + defaultVolumesToFSBackup, + backupExcludePVC, + namespaces, + ) +} + +// NewVolumeHelperWithCache creates a VolumeHelper using an externally managed PVC-to-Pod cache. +// This is used by plugins that build the cache lazily per-namespace (following the pattern from PR #9226). +// The cache can be nil, in which case PVC-to-Pod lookups will fall back to direct API calls. +func NewVolumeHelperWithCache( + backup velerov1api.Backup, + client crclient.Client, + logger logrus.FieldLogger, + pvcPodCache *podvolumeutil.PVCPodCache, +) (vhutil.VolumeHelper, error) { + return volumehelper.NewVolumeHelperImplWithCache( + backup, + client, + logger, + pvcPodCache, + ) +} diff --git a/pkg/util/volumehelper/volume_policy_helper.go b/pkg/util/volumehelper/volume_policy_helper.go new file mode 100644 index 000000000..95f104994 --- /dev/null +++ b/pkg/util/volumehelper/volume_policy_helper.go @@ -0,0 +1,30 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package volumehelper + +import ( + corev1api "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +type VolumeHelper interface { + ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, error) + ShouldPerformFSBackup(volume corev1api.Volume, pod corev1api.Pod) (bool, error) + ShouldPerformCustomAction(obj runtime.Unstructured, groupResource schema.GroupResource, matchParams map[string]any) (bool, error) + GetActionParameters(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, string, map[string]any, error) +}