diff --git a/.github/workflows/pr-ci-check.yml b/.github/workflows/pr-ci-check.yml index aea136329..ba55e6ab0 100644 --- a/.github/workflows/pr-ci-check.yml +++ b/.github/workflows/pr-ci-check.yml @@ -24,7 +24,7 @@ jobs: - name: Make ci run: make ci - name: Upload test coverage - uses: codecov/codecov-action@v5 + uses: codecov/codecov-action@v6 with: token: ${{ secrets.CODECOV_TOKEN }} files: coverage.out diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 10b191630..511113264 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -45,7 +45,7 @@ jobs: - name: Test run: make test - name: Upload test coverage - uses: codecov/codecov-action@v5 + uses: codecov/codecov-action@v6 with: token: ${{ secrets.CODECOV_TOKEN }} files: coverage.out diff --git a/changelogs/unreleased/9792-subhramit b/changelogs/unreleased/9792-subhramit new file mode 100644 index 000000000..8868ed627 --- /dev/null +++ b/changelogs/unreleased/9792-subhramit @@ -0,0 +1 @@ +Restores from backups not in a completed or partially failed phase are now rejected. diff --git a/changelogs/unreleased/9896-shubham-pampattiwar b/changelogs/unreleased/9896-shubham-pampattiwar new file mode 100644 index 000000000..034507c1f --- /dev/null +++ b/changelogs/unreleased/9896-shubham-pampattiwar @@ -0,0 +1 @@ +Skip VGS cleanup when backup did not use VolumeGroupSnapshots diff --git a/hack/build-image/Dockerfile b/hack/build-image/Dockerfile index eb1d728f5..88dedde95 100644 --- a/hack/build-image/Dockerfile +++ b/hack/build-image/Dockerfile @@ -96,7 +96,9 @@ RUN ARCH=$(go env GOARCH) && \ chmod +x /usr/bin/goreleaser # get golangci-lint -RUN curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.5.0 +# Use "go install" so the download goes through GOPROXY instead of the GitHub +# release API/CDN, which has been returning intermittent/persistent HTTP 504s. +RUN go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.5.0 # install kubectl RUN curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt)/bin/linux/$(go env GOARCH)/kubectl diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 469951f27..285b08281 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -399,6 +399,17 @@ func (r *restoreReconciler) validateAndComplete(restore *api.Restore) (backupInf return backupInfo{}, nil } + // reject restores from backups that are not in a usable phase + switch info.backup.Status.Phase { + case api.BackupPhaseCompleted, api.BackupPhasePartiallyFailed: + // ok + default: + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, + fmt.Sprintf("backup %q is in phase %q and cannot be used as a restore source", + info.backup.Name, info.backup.Status.Phase)) + return backupInfo{}, nil + } + // Fill in the ScheduleName so it's easier to consume for metrics. if restore.Spec.ScheduleName == "" { restore.Spec.ScheduleName = info.backup.GetLabels()[api.ScheduleNameLabel] diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index b013ee64d..6f03a6074 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -305,7 +305,7 @@ func TestRestoreReconcile(t *testing.T) { name: "restorer throwing an error causes the restore to fail", location: defaultStorageLocation, restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), restorerError: errors.New("blarg"), expectedErr: false, expectedPhase: string(velerov1api.RestorePhaseInProgress), @@ -319,7 +319,7 @@ func TestRestoreReconcile(t *testing.T) { name: "valid restore with none existingresourcepolicy gets executed", location: defaultStorageLocation, restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingResourcePolicy("none").Result(), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), expectedErr: false, expectedPhase: string(velerov1api.RestorePhaseInProgress), expectedStartTime: ×tamp, @@ -330,7 +330,7 @@ func TestRestoreReconcile(t *testing.T) { name: "valid restore with update existingresourcepolicy gets executed", location: defaultStorageLocation, restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).ExistingResourcePolicy("update").Result(), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), expectedErr: false, expectedPhase: string(velerov1api.RestorePhaseInProgress), expectedStartTime: ×tamp, @@ -352,7 +352,7 @@ func TestRestoreReconcile(t *testing.T) { name: "valid restore gets executed", location: defaultStorageLocation, restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), expectedErr: false, expectedPhase: string(velerov1api.RestorePhaseInProgress), expectedStartTime: ×tamp, @@ -363,7 +363,7 @@ func TestRestoreReconcile(t *testing.T) { name: "valid restore gets executed and only includes pod volume backups from restore namespace", location: defaultStorageLocation, restore: NewRestore("foo", "bar2", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), podVolumeBackups: []*velerov1api.PodVolumeBackup{ builder.ForPodVolumeBackup("foo", "pvb-1").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), builder.ForPodVolumeBackup("other-ns", "pvb-2").ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, "backup-1")).Result(), @@ -444,7 +444,7 @@ func TestRestoreReconcile(t *testing.T) { expectedStartTime: ×tamp, expectedCompletedTime: ×tamp, backupStoreGetBackupContentsErr: errors.New("Couldn't download backup"), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), }, { name: "restore attached with an expected finalizer gets cleaned up successfully", @@ -473,7 +473,7 @@ func TestRestoreReconcile(t *testing.T) { name: "valid restore with empty VolumeInfos", location: defaultStorageLocation, restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), - backup: defaultBackup().StorageLocation("default").Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseCompleted).Result(), emptyVolumeInfo: true, expectedErr: false, expectedPhase: string(velerov1api.RestorePhaseInProgress), @@ -497,6 +497,44 @@ func TestRestoreReconcile(t *testing.T) { backup: defaultBackup().StorageLocation("default").Result(), expectedErr: true, }, + { + name: "restore from backup in Deleting phase fails validation", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseDeleting).Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseFailedValidation), + expectedValidationErrors: []string{`backup "backup-1" is in phase "Deleting" and cannot be used as a restore source`}, + }, + { + name: "restore from backup in InProgress phase fails validation", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseInProgress).Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseFailedValidation), + expectedValidationErrors: []string{`backup "backup-1" is in phase "InProgress" and cannot be used as a restore source`}, + }, + { + name: "restore from backup in PartiallyFailed phase succeeds", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhasePartiallyFailed).Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseInProgress), + expectedStartTime: ×tamp, + expectedCompletedTime: ×tamp, + expectedRestorerCall: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseInProgress).Result(), + }, + { + name: "restore from backup in Failed phase fails validation", + location: defaultStorageLocation, + restore: NewRestore("foo", "bar", "backup-1", "ns-1", "", velerov1api.RestorePhaseNew).Result(), + backup: defaultBackup().StorageLocation("default").Phase(velerov1api.BackupPhaseFailed).Result(), + expectedErr: false, + expectedPhase: string(velerov1api.RestorePhaseFailedValidation), + expectedValidationErrors: []string{`backup "backup-1" is in phase "Failed" and cannot be used as a restore source`}, + }, } formatFlag := logging.FormatText diff --git a/pkg/controller/restore_finalizer_controller.go b/pkg/controller/restore_finalizer_controller.go index f82216bc3..93652c0c2 100644 --- a/pkg/controller/restore_finalizer_controller.go +++ b/pkg/controller/restore_finalizer_controller.go @@ -301,8 +301,10 @@ func (ctx *finalizerContext) execute() (results.Result, results.Result) { pdpErrs := ctx.patchDynamicPVWithVolumeInfo() errs.Merge(&pdpErrs) - vgscWarnings := ctx.cleanupStubVGSC() - warnings.Merge(&vgscWarnings) + if ctx.hasVolumeGroupSnapshotHandles() { + vgscWarnings := ctx.cleanupStubVGSC() + warnings.Merge(&vgscWarnings) + } rehErrs := ctx.WaitRestoreExecHook() errs.Merge(&rehErrs) @@ -449,6 +451,15 @@ func (ctx *finalizerContext) patchDynamicPVWithVolumeInfo() (errs results.Result return errs } +func (ctx *finalizerContext) hasVolumeGroupSnapshotHandles() bool { + for _, vi := range ctx.volumeInfo { + if vi.CSISnapshotInfo != nil && vi.CSISnapshotInfo.VolumeGroupSnapshotHandle != "" { + return true + } + } + return false +} + // cleanupStubVGSC deletes stub VolumeGroupSnapshotContent objects that were // created during restore to satisfy CSI controller validation. These stubs are // labeled with velero.io/restore-name for identification. diff --git a/pkg/controller/restore_finalizer_controller_test.go b/pkg/controller/restore_finalizer_controller_test.go index 6fb5ba303..f07d2576c 100644 --- a/pkg/controller/restore_finalizer_controller_test.go +++ b/pkg/controller/restore_finalizer_controller_test.go @@ -743,6 +743,83 @@ func TestRestoreOperationList(t *testing.T) { } } +func TestHasVolumeGroupSnapshotHandles(t *testing.T) { + tests := []struct { + name string + volumeInfo []*volume.BackupVolumeInfo + expected bool + }{ + { + name: "nil volumeInfo", + volumeInfo: nil, + expected: false, + }, + { + name: "empty volumeInfo", + volumeInfo: []*volume.BackupVolumeInfo{}, + expected: false, + }, + { + name: "no CSISnapshotInfo", + volumeInfo: []*volume.BackupVolumeInfo{ + {PVCName: "pvc-1", BackupMethod: volume.NativeSnapshot}, + }, + expected: false, + }, + { + name: "CSISnapshotInfo with empty VolumeGroupSnapshotHandle", + volumeInfo: []*volume.BackupVolumeInfo{ + { + PVCName: "pvc-1", + BackupMethod: volume.CSISnapshot, + CSISnapshotInfo: &volume.CSISnapshotInfo{ + SnapshotHandle: "snap-1", + }, + }, + }, + expected: false, + }, + { + name: "one volume with VolumeGroupSnapshotHandle", + volumeInfo: []*volume.BackupVolumeInfo{ + { + PVCName: "pvc-1", + BackupMethod: volume.CSISnapshot, + CSISnapshotInfo: &volume.CSISnapshotInfo{ + SnapshotHandle: "snap-1", + VolumeGroupSnapshotHandle: "vgs-handle-1", + }, + }, + }, + expected: true, + }, + { + name: "mixed volumes only one with VolumeGroupSnapshotHandle", + volumeInfo: []*volume.BackupVolumeInfo{ + {PVCName: "pvc-1", BackupMethod: volume.NativeSnapshot}, + { + PVCName: "pvc-2", + BackupMethod: volume.CSISnapshot, + CSISnapshotInfo: &volume.CSISnapshotInfo{ + SnapshotHandle: "snap-2", + VolumeGroupSnapshotHandle: "vgs-handle-2", + }, + }, + }, + expected: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := &finalizerContext{ + volumeInfo: tc.volumeInfo, + } + assert.Equal(t, tc.expected, ctx.hasVolumeGroupSnapshotHandles()) + }) + } +} + func TestCleanupStubVGSC(t *testing.T) { snapshotHandle1 := "snap-handle-1" snapshotHandle2 := "snap-handle-2" diff --git a/site/content/docs/main/restore-reference.md b/site/content/docs/main/restore-reference.md index b14a1dac9..eec8ad965 100644 --- a/site/content/docs/main/restore-reference.md +++ b/site/content/docs/main/restore-reference.md @@ -27,7 +27,7 @@ The following is an overview of Velero's restore process that starts after you r 1. The Velero client makes a call to the Kubernetes API server to create a [`Restore`](api-types/restore.md) object. -1. The `RestoreController` notices the new Restore object and performs validation. +1. The `RestoreController` notices the new Restore object and performs validation. This includes verifying that the referenced backup is in a usable phase. Only backups in `Completed` or `PartiallyFailed` phase are accepted as restore sources. 1. The `RestoreController` fetches basic information about the backup being restored, like the [BackupStorageLocation](locations.md) (BSL). It also fetches a tarball of the cluster resources in the backup, any volumes that will be restored using File System Backup, and any volume snapshots to be restored. @@ -78,26 +78,41 @@ By default, Velero will restore resources in the following order: * VolumeSnapshotClass * VolumeSnapshotContents * VolumeSnapshots +* DataUploads * PersistentVolumes * PersistentVolumeClaims +* ClusterRoles +* Roles +* ServiceAccounts +* ClusterRoleBindings +* RoleBindings * Secrets * ConfigMaps -* ServiceAccounts * LimitRanges +* PriorityClasses * Pods * ReplicaSets +* ClusterClasses +* Endpoints +* Services +* ClusterBootstraps * Clusters * ClusterResourceSets +* Apps (apps.kappctrl.k14s.io) +* PackageInstalls -It's recommended that you use the default order for your restores. You are able to customize this order if you need to by setting the `--restore-resource-priorities` flag on the Velero server and specifying a different resource order. This customized order will apply to all future restores. You don't have to specify all resources in the `--restore-resource-priorities` flag. Velero will append resources not listed to the end of your customized list in alphabetical order. +It's recommended that you use the default order for your restores. You are able to customize this order if you need to by setting the `--restore-resource-priorities` flag on the Velero server and specifying a different resource order. This customized order will apply to all future restores. You don't have to specify all resources in the `--restore-resource-priorities` flag. The priority list contains two parts which are split by the `-` element: resources before the `-` element are restored first as high priorities, resources after the `-` element are restored last as low priorities, and any resource not in the list will be restored alphabetically between the high and low priorities. ```shell velero server \ --restore-resource-priorities=customresourcedefinitions,namespaces,storageclasses,\ volumesnapshotclass.snapshot.storage.k8s.io,volumesnapshotcontents.snapshot.storage.k8s.io,\ -volumesnapshots.snapshot.storage.k8s.io,persistentvolumes,persistentvolumeclaims,secrets,\ -configmaps,serviceaccounts,limitranges,pods,replicasets.apps,clusters.cluster.x-k8s.io,\ -clusterresourcesets.addons.cluster.x-k8s.io +volumesnapshots.snapshot.storage.k8s.io,datauploads.velero.io,persistentvolumes,\ +persistentvolumeclaims,clusterroles,roles,serviceaccounts,clusterrolebindings,rolebindings,\ +secrets,configmaps,limitranges,priorityclasses,pods,replicasets.apps,\ +clusterclasses.cluster.x-k8s.io,endpoints,services,-,clusterbootstraps.run.tanzu.vmware.com,\ +clusters.cluster.x-k8s.io,clusterresourcesets.addons.cluster.x-k8s.io,apps.kappctrl.k14s.io,\ +packageinstalls.packaging.carvel.dev ``` diff --git a/site/content/docs/v1.17/restore-reference.md b/site/content/docs/v1.17/restore-reference.md index b062175cc..92a5f538b 100644 --- a/site/content/docs/v1.17/restore-reference.md +++ b/site/content/docs/v1.17/restore-reference.md @@ -78,26 +78,41 @@ By default, Velero will restore resources in the following order: * VolumeSnapshotClass * VolumeSnapshotContents * VolumeSnapshots +* DataUploads * PersistentVolumes * PersistentVolumeClaims +* ClusterRoles +* Roles +* ServiceAccounts +* ClusterRoleBindings +* RoleBindings * Secrets * ConfigMaps -* ServiceAccounts * LimitRanges +* PriorityClasses * Pods * ReplicaSets +* ClusterClasses +* Endpoints +* Services +* ClusterBootstraps * Clusters * ClusterResourceSets +* Apps (apps.kappctrl.k14s.io) +* PackageInstalls -It's recommended that you use the default order for your restores. You are able to customize this order if you need to by setting the `--restore-resource-priorities` flag on the Velero server and specifying a different resource order. This customized order will apply to all future restores. You don't have to specify all resources in the `--restore-resource-priorities` flag. Velero will append resources not listed to the end of your customized list in alphabetical order. +It's recommended that you use the default order for your restores. You are able to customize this order if you need to by setting the `--restore-resource-priorities` flag on the Velero server and specifying a different resource order. This customized order will apply to all future restores. You don't have to specify all resources in the `--restore-resource-priorities` flag. The priority list contains two parts which are split by the `-` element: resources before the `-` element are restored first as high priorities, resources after the `-` element are restored last as low priorities, and any resource not in the list will be restored alphabetically between the high and low priorities. ```shell velero server \ --restore-resource-priorities=customresourcedefinitions,namespaces,storageclasses,\ volumesnapshotclass.snapshot.storage.k8s.io,volumesnapshotcontents.snapshot.storage.k8s.io,\ -volumesnapshots.snapshot.storage.k8s.io,persistentvolumes,persistentvolumeclaims,secrets,\ -configmaps,serviceaccounts,limitranges,pods,replicasets.apps,clusters.cluster.x-k8s.io,\ -clusterresourcesets.addons.cluster.x-k8s.io +volumesnapshots.snapshot.storage.k8s.io,datauploads.velero.io,persistentvolumes,\ +persistentvolumeclaims,clusterroles,roles,serviceaccounts,clusterrolebindings,rolebindings,\ +secrets,configmaps,limitranges,priorityclasses,pods,replicasets.apps,\ +clusterclasses.cluster.x-k8s.io,endpoints,services,-,clusterbootstraps.run.tanzu.vmware.com,\ +clusters.cluster.x-k8s.io,clusterresourcesets.addons.cluster.x-k8s.io,apps.kappctrl.k14s.io,\ +packageinstalls.packaging.carvel.dev ``` diff --git a/site/content/docs/v1.18/restore-reference.md b/site/content/docs/v1.18/restore-reference.md index b14a1dac9..2726f8a9f 100644 --- a/site/content/docs/v1.18/restore-reference.md +++ b/site/content/docs/v1.18/restore-reference.md @@ -78,26 +78,41 @@ By default, Velero will restore resources in the following order: * VolumeSnapshotClass * VolumeSnapshotContents * VolumeSnapshots +* DataUploads * PersistentVolumes * PersistentVolumeClaims +* ClusterRoles +* Roles +* ServiceAccounts +* ClusterRoleBindings +* RoleBindings * Secrets * ConfigMaps -* ServiceAccounts * LimitRanges +* PriorityClasses * Pods * ReplicaSets +* ClusterClasses +* Endpoints +* Services +* ClusterBootstraps * Clusters * ClusterResourceSets +* Apps (apps.kappctrl.k14s.io) +* PackageInstalls -It's recommended that you use the default order for your restores. You are able to customize this order if you need to by setting the `--restore-resource-priorities` flag on the Velero server and specifying a different resource order. This customized order will apply to all future restores. You don't have to specify all resources in the `--restore-resource-priorities` flag. Velero will append resources not listed to the end of your customized list in alphabetical order. +It's recommended that you use the default order for your restores. You are able to customize this order if you need to by setting the `--restore-resource-priorities` flag on the Velero server and specifying a different resource order. This customized order will apply to all future restores. You don't have to specify all resources in the `--restore-resource-priorities` flag. The priority list contains two parts which are split by the `-` element: resources before the `-` element are restored first as high priorities, resources after the `-` element are restored last as low priorities, and any resource not in the list will be restored alphabetically between the high and low priorities. ```shell velero server \ --restore-resource-priorities=customresourcedefinitions,namespaces,storageclasses,\ volumesnapshotclass.snapshot.storage.k8s.io,volumesnapshotcontents.snapshot.storage.k8s.io,\ -volumesnapshots.snapshot.storage.k8s.io,persistentvolumes,persistentvolumeclaims,secrets,\ -configmaps,serviceaccounts,limitranges,pods,replicasets.apps,clusters.cluster.x-k8s.io,\ -clusterresourcesets.addons.cluster.x-k8s.io +volumesnapshots.snapshot.storage.k8s.io,datauploads.velero.io,persistentvolumes,\ +persistentvolumeclaims,clusterroles,roles,serviceaccounts,clusterrolebindings,rolebindings,\ +secrets,configmaps,limitranges,priorityclasses,pods,replicasets.apps,\ +clusterclasses.cluster.x-k8s.io,endpoints,services,-,clusterbootstraps.run.tanzu.vmware.com,\ +clusters.cluster.x-k8s.io,clusterresourcesets.addons.cluster.x-k8s.io,apps.kappctrl.k14s.io,\ +packageinstalls.packaging.carvel.dev ```