diff --git a/changelogs/unreleased/10475-chlins b/changelogs/unreleased/10475-chlins new file mode 100644 index 000000000..19ecba1ff --- /dev/null +++ b/changelogs/unreleased/10475-chlins @@ -0,0 +1 @@ +Add in-place restore pre-flight check: PVC must be bound to the backed-up PV diff --git a/design/volume-data-inplace-restore/volume-data-inplace-restore.md b/design/volume-data-inplace-restore/volume-data-inplace-restore.md index 4977c09d1..d6022084d 100644 --- a/design/volume-data-inplace-restore/volume-data-inplace-restore.md +++ b/design/volume-data-inplace-restore/volume-data-inplace-restore.md @@ -245,7 +245,11 @@ The check runs on both restore paths before any side effect on the existing PVC/ This check is a fail-fast validation, not an atomic guarantee; the `pvc-protection` finalizer remains the actual safety gate for PVC deletion. A residual `VolumeAttachment` check (e.g. a `Failed` Pod imposed by the control plane after a non-graceful node shutdown, where the node never unmounted the volume) may be added as a future enhancement. #### 2. PVC is Bound to the Original PV -Velero checks whether the existing PVC in the cluster is still bound to the same PersistentVolume (PV) it was bound to at the time of the backup. If the PVC is bound to a different PV, performing an in-place restore (especially an incremental one that relies on Changed Block Tracking) may be unsafe or result in unpredictable behavior. If this check fails, Velero will log an error and skip the in-place restore for that volume. +Velero checks whether the existing PVC in the cluster is still bound to the same PersistentVolume (PV) it was bound to at the time of the backup (compared by PV name against the backed-up PVC's `spec.volumeName`). If the PVC is bound to a different PV, performing an in-place restore (especially an incremental one that relies on Changed Block Tracking) may be unsafe or result in unpredictable behavior. If this check fails, Velero will log an error and skip the in-place restore for that volume. + +The PV comparison is skipped when the PVC is restored into a mapped namespace, where the target PVC is necessarily bound to a different PV (see [Namespace Mapping](#namespace-mapping) for the cross-namespace clone-and-restore workflow). The PVC must still be bound in all cases. + +The check runs on both restore paths: in the PVC CSI RIA (using the backed-up PVC's volume name), and before creating the `PodVolumeRestore` on the file system path (using the PVC-to-PV mapping recorded in the backup's volume info). #### 3. Volume Size Validation diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index 92fab63ed..f32cc254e 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -333,7 +333,8 @@ func createPVObj(index int, withHostPath bool) *corev1api.PersistentVolume { } func createPVCObj(index int) *corev1api.PersistentVolumeClaim { - pvcObj := builder.ForPersistentVolumeClaim("fake-ns", fmt.Sprintf("fake-pvc-%d", index)).VolumeName(fmt.Sprintf("fake-pv-%d", index)).Result() + pvcObj := builder.ForPersistentVolumeClaim("fake-ns", fmt.Sprintf("fake-pvc-%d", index)).VolumeName(fmt.Sprintf("fake-pv-%d", index)). + Phase(corev1api.ClaimBound).Result() return pvcObj } diff --git a/pkg/podvolume/restorer.go b/pkg/podvolume/restorer.go index e135ba860..488ce7331 100644 --- a/pkg/podvolume/restorer.go +++ b/pkg/podvolume/restorer.go @@ -49,6 +49,9 @@ type RestoreData struct { Pod *corev1api.Pod PodVolumeBackups []*velerov1api.PodVolumeBackup SourceNamespace, BackupLocation string + // BackupVolumeInfos is the backup's volume info keyed by PV name, used by + // the in-place restore pre-flight checks. + BackupVolumeInfos map[string]volume.BackupVolumeInfo } // Restorer can execute pod volume restores of volumes in a pod. @@ -186,6 +189,10 @@ func (r *restorer) RestorePodVolumes(data RestoreData, tracker *volume.RestoreVo // to write into, and they cannot write to it themselves until this // restore's PodVolumeRestores complete. if data.Restore.IsVolumeDataInplaceRestore() && pvc != nil { + if err := inplace.CheckPVCBoundToBackedUpPV(pvc, backedUpPVName(data.BackupVolumeInfos, data.SourceNamespace, pvc.Name), data.SourceNamespace); err != nil { + errs = append(errs, err) + continue + } if err := inplace.CheckPVCNotInUse(r.ctx, r.crClient, pvc, data.Restore.UID); err != nil { errs = append(errs, err) continue @@ -317,6 +324,17 @@ func newPodVolumeRestore(restore *velerov1api.Restore, pod *corev1api.Pod, backu return pvr } +// backedUpPVName returns the name of the PV the given source-namespace PVC was +// bound to at backup time, or "" if unknown. +func backedUpPVName(infos map[string]volume.BackupVolumeInfo, pvcNamespace, pvcName string) string { + for pvName, info := range infos { + if info.PVCNamespace == pvcNamespace && info.PVCName == pvcName { + return pvName + } + } + return "" +} + func getVolumesRepositoryType(volumes map[string]volumeBackupInfo) (string, error) { if len(volumes) == 0 { return "", errors.New("empty volume list") diff --git a/pkg/podvolume/restorer_test.go b/pkg/podvolume/restorer_test.go index e75f2f42b..3d39d0a05 100644 --- a/pkg/podvolume/restorer_test.go +++ b/pkg/podvolume/restorer_test.go @@ -197,6 +197,7 @@ func TestRestorePodVolumes(t *testing.T) { pvbs []*velerov1api.PodVolumeBackup restoredPod *corev1api.Pod sourceNamespace string + volumeInfos map[string]volume.BackupVolumeInfo inplace bool errs []expectError }{ @@ -413,6 +414,31 @@ func TestRestorePodVolumes(t *testing.T) { }, }, }, + { + name: "in-place restore blocked when the PVC is bound to a different PV than at backup time", + pvbs: []*velerov1api.PodVolumeBackup{ + createPVBObj(true, true, 1, "kopia"), + }, + inplace: true, + kubeClientObj: []runtime.Object{ + createNodeAgentDaemonset(), + createPVCObj(1), + }, + ctlClientObj: []runtime.Object{ + createBackupRepoObj(), + }, + restoredPod: createPodObj(true, true, true, 1), + sourceNamespace: "fake-ns", + bsl: "fake-bsl", + volumeInfos: map[string]volume.BackupVolumeInfo{"some-other-pv": {PVCNamespace: "fake-ns", PVCName: "fake-pvc-1"}}, + runtimeScheme: scheme, + errs: []expectError{ + { + err: "in-place restore pre-flight check failed", + prefixOnly: true, + }, + }, + }, { name: "in-place restore proceeds when the PVC is only used by the gated restored pod", pvbs: []*velerov1api.PodVolumeBackup{ @@ -481,11 +507,12 @@ func TestRestorePodVolumes(t *testing.T) { }() errs := rs.RestorePodVolumes(RestoreData{ - Restore: restoreObj, - Pod: test.restoredPod, - PodVolumeBackups: test.pvbs, - SourceNamespace: test.sourceNamespace, - BackupLocation: test.bsl, + Restore: restoreObj, + Pod: test.restoredPod, + PodVolumeBackups: test.pvbs, + SourceNamespace: test.sourceNamespace, + BackupLocation: test.bsl, + BackupVolumeInfos: test.volumeInfos, }, volume.NewRestoreVolInfoTracker(restoreObj, logrus.New(), fakeCRClient)) if errs == nil { diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index 5bf8f45d1..f71d0965a 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -234,11 +234,10 @@ func (p *pvcRestoreItemAction) executeWithDataMove(logger *logrus.Entry, input * var volumeSnapshot *snapshotv1api.VolumeSnapshot restoreType := input.Restore.Spec.ExistingVolumeDataPolicy if pvcExists { - if existingPVC.Status.Phase != corev1api.ClaimBound { - return nil, errors.New("ExistingVolumeDataPolicy is in-place restore, but the existing PVC is not bound.") - } - // Pre-flight checks must pass before any side effect on the existing PVC/PV. + if err := inplace.CheckPVCBoundToBackedUpPV(existingPVC, pvcFromBackup.Spec.VolumeName, pvcFromBackup.Namespace); err != nil { + return nil, errors.WithStack(err) + } if err := inplace.CheckPVCNotInUse(ctx, p.crClient, existingPVC, input.Restore.UID); err != nil { return nil, errors.WithStack(err) } diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index 15cc4ca4d..b8e4912f6 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -742,8 +742,8 @@ func TestExecuteInplaceRestore(t *testing.T) { } // TestExecuteInplaceRestorePreflight verifies the RIA fails the item without -// side effects when the pre-flight check fails. The in-use semantics are -// covered by the pkg/restore/inplace unit tests. +// side effects when a pre-flight check fails. The check semantics themselves +// are covered by the pkg/restore/inplace unit tests. func TestExecuteInplaceRestorePreflight(t *testing.T) { newPodUsingPVC := func(phase corev1api.PodPhase) *corev1api.Pod { pod := builder.ForPod("velero", "consumer-pod"). @@ -754,17 +754,25 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { } tests := []struct { - name string - pod *corev1api.Pod - expectBlock bool + name string + pod *corev1api.Pod + backedUpPVName string + expectBlock string }{ { - name: "no pod, restore proceeds", + name: "checks pass, restore proceeds", + backedUpPVName: "testPV", }, { - name: "active pod blocks the restore", - pod: newPodUsingPVC(corev1api.PodRunning), - expectBlock: true, + name: "active pod blocks the restore", + pod: newPodUsingPVC(corev1api.PodRunning), + backedUpPVName: "testPV", + expectBlock: "consumer-pod", + }, + { + name: "PVC bound to a different PV blocks the restore", + backedUpPVName: "backupPV", + expectBlock: "was bound to PV backupPV at backup time", }, } @@ -778,6 +786,7 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { restore := builder.ForRestore("velero", "testRestore").Backup("testBackup"). ObjectMeta(builder.WithUID("uid")).ExistingVolumeDataPolicy("full").Result() pvcFromBackup := builder.ForPersistentVolumeClaim("velero", "testPVC"). + VolumeName(tc.backedUpPVName). ObjectMeta(builder.WithAnnotations( velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.DataUploadNameAnnotation, "velero/testDU", @@ -817,10 +826,10 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { dataDownloadList := new(velerov2alpha1.DataDownloadList) require.NoError(t, pvcRIA.crClient.List(t.Context(), dataDownloadList, &crclient.ListOptions{})) - if tc.expectBlock { + if tc.expectBlock != "" { require.Error(t, err) require.Contains(t, err.Error(), "pre-flight check failed") - require.Contains(t, err.Error(), "consumer-pod") + require.Contains(t, err.Error(), tc.expectBlock) // No side effects: PVC untouched with the original volumeName, // PV reclaim policy not patched, no DataDownload created. require.NoError(t, getErr) diff --git a/pkg/restore/inplace/preflight.go b/pkg/restore/inplace/preflight.go index c1a4fa3a4..2ad76931d 100644 --- a/pkg/restore/inplace/preflight.go +++ b/pkg/restore/inplace/preflight.go @@ -129,3 +129,23 @@ func gatedByThisRestore(pod *corev1api.Pod, restoreUID types.UID) bool { // is still closed. return true } + +// CheckPVCBoundToBackedUpPV verifies the existing PVC is still bound to the +// same PV it was bound to at backup time. An in-place restore onto a +// different volume is unsafe: an incremental (CBT) restore computes deltas +// against a different volume lineage, and even a full restore would patch and +// write into a volume unrelated to the backup. The PV comparison is skipped +// when the PVC is restored into a different namespace, where it is necessarily +// bound to a different PV (the documented cross-namespace clone-and-restore +// workflow), and when the backed-up PV name is unknown. +func CheckPVCBoundToBackedUpPV(existingPVC *corev1api.PersistentVolumeClaim, backedUpPVName, sourceNamespace string) error { + if existingPVC.Status.Phase != corev1api.ClaimBound { + return errors.Errorf("in-place restore pre-flight check failed, skipping volume data restore: PVC %s/%s is not bound (phase %s)", + existingPVC.Namespace, existingPVC.Name, existingPVC.Status.Phase) + } + if existingPVC.Namespace != sourceNamespace || backedUpPVName == "" || existingPVC.Spec.VolumeName == backedUpPVName { + return nil + } + return errors.Errorf("in-place restore pre-flight check failed, skipping volume data restore: PVC %s/%s is bound to PV %s, but was bound to PV %s at backup time", + existingPVC.Namespace, existingPVC.Name, existingPVC.Spec.VolumeName, backedUpPVName) +} diff --git a/pkg/restore/inplace/preflight_test.go b/pkg/restore/inplace/preflight_test.go index 916b786f0..094a70273 100644 --- a/pkg/restore/inplace/preflight_test.go +++ b/pkg/restore/inplace/preflight_test.go @@ -200,3 +200,66 @@ func TestCheckPVCNotInUse(t *testing.T) { }) } } + +func TestCheckPVCBoundToBackedUpPV(t *testing.T) { + pvc := func(namespace, pvName string, phase corev1api.PersistentVolumeClaimPhase) *corev1api.PersistentVolumeClaim { + return &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-1", Namespace: namespace}, + Spec: corev1api.PersistentVolumeClaimSpec{VolumeName: pvName}, + Status: corev1api.PersistentVolumeClaimStatus{Phase: phase}, + } + } + + tests := []struct { + name string + existingPVC *corev1api.PersistentVolumeClaim + backedUpPVName string + expectError string + }{ + { + name: "bound to the backed-up PV, check passes", + existingPVC: pvc("default", "pv-1", corev1api.ClaimBound), + backedUpPVName: "pv-1", + }, + { + name: "bound to a different PV, check fails", + existingPVC: pvc("default", "pv-other", corev1api.ClaimBound), + backedUpPVName: "pv-1", + expectError: "is bound to PV pv-other, but was bound to PV pv-1 at backup time", + }, + { + name: "PVC not bound, check fails", + existingPVC: pvc("default", "", corev1api.ClaimPending), + backedUpPVName: "pv-1", + expectError: "is not bound (phase Pending)", + }, + { + name: "different PV in a different namespace, check passes", + existingPVC: pvc("mapped-ns", "pv-other", corev1api.ClaimBound), + backedUpPVName: "pv-1", + }, + { + name: "backed-up PV name unknown, check passes", + existingPVC: pvc("default", "pv-other", corev1api.ClaimBound), + backedUpPVName: "", + }, + { + name: "different namespace but PVC not bound, check still fails", + existingPVC: pvc("mapped-ns", "", corev1api.ClaimLost), + backedUpPVName: "pv-1", + expectError: "is not bound (phase Lost)", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := CheckPVCBoundToBackedUpPV(tc.existingPVC, tc.backedUpPVName, "default") + if tc.expectError == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectError) + }) + } +} diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index aec181a97..247c8abb3 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -2287,11 +2287,12 @@ func restorePodVolumeBackups(ctx *restoreContext, createdObj *unstructured.Unstr } data := podvolume.RestoreData{ - Restore: ctx.restore, - Pod: pod, - PodVolumeBackups: ctx.podVolumeBackups, - SourceNamespace: originalNamespace, - BackupLocation: ctx.backup.Spec.StorageLocation, + Restore: ctx.restore, + Pod: pod, + PodVolumeBackups: ctx.podVolumeBackups, + SourceNamespace: originalNamespace, + BackupLocation: ctx.backup.Spec.StorageLocation, + BackupVolumeInfos: ctx.backupVolumeInfoMap, } if errs := ctx.podVolumeRestorer.RestorePodVolumes(data, ctx.restoreVolumeInfoTracker); errs != nil { ctx.log.WithError(kubeerrs.NewAggregate(errs)).Error("unable to successfully complete pod volume restores of pod's volumes")