Skip to content

Commit

Permalink
issue 8072: restic deprecation - warning messages
Browse files Browse the repository at this point in the history
Signed-off-by: Lyndon-Li <[email protected]>
  • Loading branch information
Lyndon-Li committed Aug 9, 2024
1 parent 2645948 commit fefb4b8
Show file tree
Hide file tree
Showing 9 changed files with 134 additions and 31 deletions.
1 change: 1 addition & 0 deletions changelogs/unreleased/8096-Lyndon-Li
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix issue #8072, add the warning messages for restic deprecation
4 changes: 3 additions & 1 deletion pkg/cmd/cli/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Check warning on line 367 in pkg/cmd/cli/install/install.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/cli/install/install.go#L367

Added line #L367 was not covered by tests
return err
} else if msg != "" {
fmt.Printf("⚠️ %s\n", msg)

Check warning on line 370 in pkg/cmd/cli/install/install.go

View check run for this annotation

Codecov / codecov/patch

pkg/cmd/cli/install/install.go#L369-L370

Added lines #L369 - L370 were not covered by tests
}

// If we're only installing CRDs, we can skip the rest of the validation.
Expand Down
4 changes: 3 additions & 1 deletion pkg/cmd/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions pkg/cmd/server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,13 @@ func Test_newServer(t *testing.T) {
}, logger)
assert.Error(t, err)

// invalid clientQPS Restic uploader
_, err = newServer(factory, serverConfig{
uploaderType: uploader.ResticType,
clientQPS: -1,
}, logger)
assert.Error(t, err)

// invalid clientBurst
factory.On("SetClientQPS", mock.Anything).Return()
_, err = newServer(factory, serverConfig{
Expand Down
22 changes: 18 additions & 4 deletions pkg/podvolume/backupper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 (
Expand All @@ -189,25 +193,35 @@ 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)

Check warning on line 200 in pkg/podvolume/backupper.go

View check run for this annotation

Codecov / codecov/patch

pkg/podvolume/backupper.go#L200

Added line #L200 was not covered by tests
}

if err := kube.IsPodRunning(pod); err != nil {
skipAllPodVolumes(pod, volumesToBackup, err, pvcSummary, log)
return nil, pvcSummary, nil
}

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
Expand Down
54 changes: 39 additions & 15 deletions pkg/podvolume/backupper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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",
},
Expand All @@ -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",
},
},
{
Expand Down Expand Up @@ -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 {
Expand Down
11 changes: 8 additions & 3 deletions pkg/uploader/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 backups won't be available for restore when this functionality is removed in a future version of Velero", t), nil
}

return "", nil
}

type SnapshotInfo struct {
Expand Down
27 changes: 20 additions & 7 deletions pkg/uploader/types_test.go
Original file line number Diff line number Diff line change
@@ -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 backups won't be available for restore when this functionality is removed in a future version 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)
})
}
}
35 changes: 35 additions & 0 deletions site/content/docs/main/file-system-backup.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Cons:
- It access the file system from the mounted hostpath directory, so Velero Node Agent pods need to run as root user and even under privileged mode in some environments.

**NOTE:** hostPath volumes are not supported, but the [local volume type][5] is supported.
**NOTE:** restic is under the deprecation process by following [Velero Deprecation Policy][17], for more details, see the Restic Deprecation section.

## Setup File System Backup

Expand Down Expand Up @@ -643,6 +644,39 @@ If you want to constraint the CPU/memory usage, you need to [customize the resou
During the restore, the repository may also cache data/metadata so as to reduce the network footprint and speed up the restore. The repository uses its own policy to store and clean up the cache.
For Kopia repository, the cache is stored in the node-agent pod's root file system and the cleanup is triggered for the data/metadata that are older than 10 minutes (not configurable at present). So you should prepare enough disk space, otherwise, the node-agent pod may be evicted due to running out of the ephemeral storage.

## Restic Deprecation

According to the [Velero Deprecation Policy][17], restic path is being deprecated starting from v1.15, specifically:
- For 1.15 and 1.16, if restic path is used by a backup, the backup still creates and succeeds but you will see warnings
- For 1.17 and 1.18, backups with restic path are disabled, but you are still allowed to restore from your previous restic backups
- From 1.19, both backups and restores with restic path will be disabled, you are not able to use 1.19 or higher to restore your restic backup data

For 1.15 and 1.16, you will see below warnings if `--uploader-type=restic` is used in Velero installation:
In the output of installation:
```
⚠️ Uploader 'restic' is deprecated, don't use it for new backups, otherwise the backups won't be available for restore when this functionality is removed in a future version of Velero
```
In Velero server log:
```
level=warning msg="Uploader 'restic' is deprecated, don't use it for new backups, otherwise the backups won't be available for restore when this functionality is removed in a future version of Velero
```
In the output of `velero backup describe` command for a backup with fs-backup:
```
Namespaces:
<namespace>: resource: /pods name: <pod name> message: /Uploader 'restic' is deprecated, don't use it for new backups, otherwise the backups won't be available for restore when this functionality is removed in a future version of Velero
```

And you will see below warnings you upgrade from v1.9 or lower to 1.15 or 1.16:
In Velero server log:
```
level=warning msg="Uploader 'restic' is deprecated, don't use it for new backups, otherwise the backups won't be available for restore when this functionality is removed in a future version of Velero
```
In the output of `velero backup describe` command for a backup with fs-backup:
```
Namespaces:
<namespace>: resource: /pods name: <pod name> message: /Uploader 'restic' is deprecated, don't use it for new backups, otherwise the backups won't be available for restore when this functionality is removed in a future version of Velero
```


[1]: https://github.com/restic/restic
[2]: https://github.com/kopia/kopia
Expand All @@ -660,3 +694,4 @@ For Kopia repository, the cache is stored in the node-agent pod's root file syst
[14]: https://kubernetes.io/docs/concepts/workloads/pods/pod-qos/
[15]: customize-installation.md#customize-resource-requests-and-limits
[16]: performance-guidance.md
[17]: https://github.com/vmware-tanzu/velero/blob/main/GOVERNANCE.md#deprecation-policy

0 comments on commit fefb4b8

Please sign in to comment.