Fall back to full restore rather than fail if fail to get the volume ID (#10465)

Fall back to full restore rather than fail if fail to get the volum
e ID

Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>

pkg/exposer/generic_restore.go

@@ -255,27 +255,11 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap
 	// Get volumeID before creating the restore pod because the existingPV may be deleted when creating the PVC if the volume policy is different
 	var volumeID string
 	if param.CSI != nil && param.CSI.Snapshot != nil {
-		vs := &snapshotv1api.VolumeSnapshot{}
-		if err := e.ctrlClient.Get(ctx, client.ObjectKey{
-			Namespace: param.CSI.Snapshot.VolumeSnapshotNamespace,
-			Name:      param.CSI.Snapshot.VolumeSnapshot,
-		}, vs); err != nil {
-			return errors.Wrapf(err, "error to get volume snapshot %s/%s", param.CSI.Snapshot.VolumeSnapshotNamespace, param.CSI.Snapshot.VolumeSnapshot)
-		}
-
-		var vsc *snapshotv1api.VolumeSnapshotContent
-		vsc, err = csi.GetVSCForVS(ctx, vs, e.ctrlClient)
+		volumeID, err = e.getVolumeID(ctx, param.CSI.Snapshot, param.TargetPVName)
 		if err != nil {
-			return errors.Wrapf(err, "error to get volume snapshot content for volume snapshot %s/%s", vs.Namespace, vs.Name)
+			// only log the error. Without the volume ID, exposer will fallback to full restore.
+			curLog.Errorf("failed to get volume ID from snapshot %s/%s, err: %v", param.CSI.Snapshot.VolumeSnapshotNamespace, param.CSI.Snapshot.VolumeSnapshot, err)
 		}
-
-		var cbtInfo csi.CBTInfo
-		cbtInfo, err = csi.GetCBTInfo(ctx, e.kubeClient, e.log, vs, vsc, param.TargetPVName)
-		if err != nil {
-			return errors.Wrap(err, "error to get CBT info")
-		}
-		curLog.Debugf("CBT info: %+v", cbtInfo)
-		volumeID = cbtInfo.VolumeID
 	}
 
 	curLog.Info("Creating restore PVC")
@@ -1082,3 +1066,25 @@ func (e *genericRestoreExposer) validateSelectedNode(ctx context.Context, node s
 
 	return true
 }
+
+func (e *genericRestoreExposer) getVolumeID(ctx context.Context, snapshot *velerov2alpha1api.CSISnapshotSpec, targetPVName string) (string, error) {
+	vs := &snapshotv1api.VolumeSnapshot{}
+	if err := e.ctrlClient.Get(ctx, client.ObjectKey{
+		Namespace: snapshot.VolumeSnapshotNamespace,
+		Name:      snapshot.VolumeSnapshot,
+	}, vs); err != nil {
+		return "", errors.Wrapf(err, "error to get volume snapshot %s/%s", snapshot.VolumeSnapshotNamespace, snapshot.VolumeSnapshot)
+	}
+
+	vsc, err := csi.GetVSCForVS(ctx, vs, e.ctrlClient)
+	if err != nil {
+		return "", errors.Wrapf(err, "error to get volume snapshot content for volume snapshot %s/%s", vs.Namespace, vs.Name)
+	}
+
+	var cbtInfo csi.CBTInfo
+	cbtInfo, err = csi.GetCBTInfo(ctx, e.kubeClient, e.log, vs, vsc, targetPVName)
+	if err != nil {
+		return "", errors.Wrap(err, "error to get CBT info")
+	}
+	return cbtInfo.VolumeID, nil
+}

pkg/exposer/generic_restore_test.go

@@ -21,6 +21,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/errors"
+	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1api "k8s.io/api/apps/v1"
@@ -33,8 +34,10 @@ import (
 	clientTesting "k8s.io/client-go/testing"
 
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
+	"github.com/vmware-tanzu/velero/pkg/util"
 	"github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
@@ -716,6 +719,336 @@ func TestRestoreExpose_SecretCopy(t *testing.T) {
 	})
 }
 
+func TestGetVolumeID(t *testing.T) {
+	vscName := "fake-vsc"
+	snapshotHandle := "fake-snapshot-handle"
+
+	tests := []struct {
+		name          string
+		snapshot      *velerov2alpha1api.CSISnapshotSpec
+		targetPVName  string
+		ctrlClientObj []runtime.Object
+		kubeClientObj []runtime.Object
+		expectedID    string
+		expectedErr   string
+	}{
+		{
+			name: "VS not found in ctrlClient",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "non-existent-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			expectedErr: "error to get volume snapshot fake-ns/non-existent-vs",
+		},
+		{
+			name: "GetVSCForVS error - VS has no bound VSC",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "fake-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			ctrlClientObj: []runtime.Object{
+				&snapshotv1api.VolumeSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "fake-ns",
+						Name:      "fake-vs",
+					},
+					Status: nil,
+				},
+			},
+			expectedErr: "error to get volume snapshot content for volume snapshot fake-ns/fake-vs: invalid snapshot info in volume snapshot fake-vs",
+		},
+		{
+			name: "GetVSCForVS error - VSC not found in ctrlClient",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "fake-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			ctrlClientObj: []runtime.Object{
+				&snapshotv1api.VolumeSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "fake-ns",
+						Name:      "fake-vs",
+					},
+					Status: &snapshotv1api.VolumeSnapshotStatus{
+						BoundVolumeSnapshotContentName: &vscName,
+					},
+				},
+			},
+			expectedErr: "error to get volume snapshot content for volume snapshot fake-ns/fake-vs: error getting volume snapshot content from API",
+		},
+		{
+			name: "GetCBTInfo error - target PV not found",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "fake-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			targetPVName: "missing-pv",
+			ctrlClientObj: []runtime.Object{
+				&snapshotv1api.VolumeSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "fake-ns",
+						Name:      "fake-vs",
+					},
+					Status: &snapshotv1api.VolumeSnapshotStatus{
+						BoundVolumeSnapshotContentName: &vscName,
+					},
+				},
+				&snapshotv1api.VolumeSnapshotContent{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: vscName,
+					},
+					Status: &snapshotv1api.VolumeSnapshotContentStatus{
+						SnapshotHandle: &snapshotHandle,
+					},
+				},
+			},
+			expectedErr: "error to get CBT info: failed to get pv missing-pv",
+		},
+		{
+			name: "GetCBTInfo error - empty volumeID on PV",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "fake-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			targetPVName: "fake-pv",
+			ctrlClientObj: []runtime.Object{
+				&snapshotv1api.VolumeSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "fake-ns",
+						Name:      "fake-vs",
+					},
+					Status: &snapshotv1api.VolumeSnapshotStatus{
+						BoundVolumeSnapshotContentName: &vscName,
+					},
+				},
+				&snapshotv1api.VolumeSnapshotContent{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: vscName,
+					},
+					Status: &snapshotv1api.VolumeSnapshotContentStatus{
+						SnapshotHandle: &snapshotHandle,
+					},
+				},
+			},
+			kubeClientObj: []runtime.Object{
+				&corev1api.PersistentVolume{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "fake-pv",
+					},
+				},
+			},
+			expectedErr: "error to get CBT info: volumeID must not be empty for CBT",
+		},
+		{
+			name: "success with VKS annotations",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "fake-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			ctrlClientObj: []runtime.Object{
+				&snapshotv1api.VolumeSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "fake-ns",
+						Name:      "fake-vs",
+						Annotations: map[string]string{
+							util.VSphereCNSChangeIDAnno: "c-1",
+							util.VSphereCNSSnapshotAnno: "vol-vks+snap-1",
+						},
+					},
+					Status: &snapshotv1api.VolumeSnapshotStatus{
+						BoundVolumeSnapshotContentName: &vscName,
+					},
+				},
+				&snapshotv1api.VolumeSnapshotContent{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: vscName,
+					},
+				},
+			},
+			expectedID: "vol-vks",
+		},
+		{
+			name: "success with PV CSI volume handle",
+			snapshot: &velerov2alpha1api.CSISnapshotSpec{
+				VolumeSnapshot:          "fake-vs",
+				VolumeSnapshotNamespace: "fake-ns",
+			},
+			targetPVName: "fake-pv",
+			ctrlClientObj: []runtime.Object{
+				&snapshotv1api.VolumeSnapshot{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "fake-ns",
+						Name:      "fake-vs",
+					},
+					Status: &snapshotv1api.VolumeSnapshotStatus{
+						BoundVolumeSnapshotContentName: &vscName,
+					},
+				},
+				&snapshotv1api.VolumeSnapshotContent{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: vscName,
+					},
+					Status: &snapshotv1api.VolumeSnapshotContentStatus{
+						SnapshotHandle: &snapshotHandle,
+					},
+				},
+			},
+			kubeClientObj: []runtime.Object{
+				&corev1api.PersistentVolume{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "fake-pv",
+					},
+					Spec: corev1api.PersistentVolumeSpec{
+						PersistentVolumeSource: corev1api.PersistentVolumeSource{
+							CSI: &corev1api.CSIPersistentVolumeSource{
+								VolumeHandle: "csi-vol-789",
+							},
+						},
+					},
+				},
+			},
+			expectedID: "csi-vol-789",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...)
+			fakeCtrlClient := velerotest.NewFakeControllerRuntimeClient(t, test.ctrlClientObj...)
+
+			exposer := genericRestoreExposer{
+				kubeClient: fakeKubeClient,
+				ctrlClient: fakeCtrlClient,
+				log:        velerotest.NewLogger(),
+			}
+
+			volID, err := exposer.getVolumeID(t.Context(), test.snapshot, test.targetPVName)
+			if test.expectedErr != "" {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), test.expectedErr)
+				assert.Empty(t, volID)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, test.expectedID, volID)
+			}
+		})
+	}
+}
+
+func TestRestoreExpose_CSISnapshot(t *testing.T) {
+	scName := "fake-sc"
+	restore := &velerov1.Restore{
+		TypeMeta:   metav1.TypeMeta{APIVersion: velerov1.SchemeGroupVersion.String(), Kind: "Restore"},
+		ObjectMeta: metav1.ObjectMeta{Namespace: velerov1.DefaultNamespace, Name: "fake-restore", UID: "fake-uid"},
+	}
+	ownerObject := corev1api.ObjectReference{
+		Kind:       restore.Kind,
+		Namespace:  restore.Namespace,
+		Name:       restore.Name,
+		UID:        restore.UID,
+		APIVersion: restore.APIVersion,
+	}
+	targetPVCObj := &corev1api.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "fake-ns", Name: "fake-target-pvc"},
+		Spec:       corev1api.PersistentVolumeClaimSpec{StorageClassName: &scName},
+	}
+	storageClass := &storagev1api.StorageClass{ObjectMeta: metav1.ObjectMeta{Name: "fake-sc"}}
+	daemonSet := &appsv1api.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "velero", Name: "node-agent"},
+		TypeMeta:   metav1.TypeMeta{Kind: "DaemonSet", APIVersion: appsv1api.SchemeGroupVersion.String()},
+		Spec: appsv1api.DaemonSetSpec{
+			Template: corev1api.PodTemplateSpec{
+				Spec: corev1api.PodSpec{Containers: []corev1api.Container{{Image: "fake-image"}}},
+			},
+		},
+	}
+
+	vscName := "fake-vsc"
+
+	t.Run("getVolumeID fails - falls back to full restore and creates pod without volume ID", func(t *testing.T) {
+		fakeKubeClient := fake.NewSimpleClientset(targetPVCObj, storageClass, daemonSet)
+		fakeCtrlClient := velerotest.NewFakeControllerRuntimeClient(t)
+		exposer := genericRestoreExposer{
+			kubeClient: fakeKubeClient,
+			ctrlClient: fakeCtrlClient,
+			log:        velerotest.NewLogger(),
+		}
+
+		err := exposer.Expose(t.Context(), ownerObject, GenericRestoreExposeParam{
+			TargetPVCName:    "fake-target-pvc",
+			TargetNamespace:  "fake-ns",
+			HostingPodLabels: map[string]string{},
+			Resources:        corev1api.ResourceRequirements{},
+			ExposeTimeout:    time.Millisecond,
+			CSI: &GenericRestoreExposeCSI{
+				Snapshot: &velerov2alpha1api.CSISnapshotSpec{
+					VolumeSnapshot:          "non-existent-vs",
+					VolumeSnapshotNamespace: "fake-ns",
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		pod, err := fakeKubeClient.CoreV1().Pods(ownerObject.Namespace).Get(t.Context(), ownerObject.Name, metav1.GetOptions{})
+		require.NoError(t, err)
+		require.Len(t, pod.Spec.Containers, 1)
+		for _, arg := range pod.Spec.Containers[0].Args {
+			assert.NotContains(t, arg, "--volume-id=")
+			assert.NotContains(t, arg, "--vs-namespace=")
+		}
+	})
+
+	t.Run("getVolumeID succeeds - passes volume ID to restore pod", func(t *testing.T) {
+		fakeKubeClient := fake.NewSimpleClientset(targetPVCObj, storageClass, daemonSet)
+		fakeCtrlClient := velerotest.NewFakeControllerRuntimeClient(t,
+			&snapshotv1api.VolumeSnapshot{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "fake-ns",
+					Name:      "fake-vs",
+					Annotations: map[string]string{
+						util.VSphereCNSChangeIDAnno: "c-1",
+						util.VSphereCNSSnapshotAnno: "vol-123+snap-1",
+					},
+				},
+				Status: &snapshotv1api.VolumeSnapshotStatus{
+					BoundVolumeSnapshotContentName: &vscName,
+				},
+			},
+			&snapshotv1api.VolumeSnapshotContent{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: vscName,
+				},
+			},
+		)
+		exposer := genericRestoreExposer{
+			kubeClient: fakeKubeClient,
+			ctrlClient: fakeCtrlClient,
+			log:        velerotest.NewLogger(),
+		}
+
+		err := exposer.Expose(t.Context(), ownerObject, GenericRestoreExposeParam{
+			TargetPVCName:    "fake-target-pvc",
+			TargetNamespace:  "fake-ns",
+			HostingPodLabels: map[string]string{},
+			Resources:        corev1api.ResourceRequirements{},
+			ExposeTimeout:    time.Millisecond,
+			CSI: &GenericRestoreExposeCSI{
+				Snapshot: &velerov2alpha1api.CSISnapshotSpec{
+					VolumeSnapshot:          "fake-vs",
+					VolumeSnapshotNamespace: "fake-ns",
+				},
+			},
+		})
+		require.NoError(t, err)
+
+		pod, err := fakeKubeClient.CoreV1().Pods(ownerObject.Namespace).Get(t.Context(), ownerObject.Name, metav1.GetOptions{})
+		require.NoError(t, err)
+		require.Len(t, pod.Spec.Containers, 1)
+		assert.Contains(t, pod.Spec.Containers[0].Args, "--volume-id=vol-123")
+		assert.Contains(t, pod.Spec.Containers[0].Args, "--vs-namespace=fake-ns")
+	})
+}
+
 func TestRebindVolume(t *testing.T) {
 	restore := &velerov1.Restore{
 		TypeMeta: metav1.TypeMeta{

pkg/uploader/block/snapshot.go

@@ -225,6 +225,9 @@ func Restore(ctx context.Context, blkUp Uploader, rep udmrepo.BackupRepo, snapsh
 		} else if snapshot.Tags[uploader.CBTVolumeIDTag] == "" {
 			log.Warnf("No VolumeID tag from snapshot %s, fallback to full restore", snapshotID)
 			incremental = false
+		} else if cbtSource.VolumeID == "" {
+			log.Warnf("No VolumeID in cbt source %v, fallback to full restore", cbtSource)
+			incremental = false
 		} else if snapshot.Tags[uploader.CBTVolumeIDTag] != cbtSource.VolumeID {
 			log.Warnf("VolumeID %s from snapshot %s is not expected as %s, fallback to full restore", snapshot.Tags[uploader.CBTVolumeIDTag], snapshotID, cbtSource.VolumeID)
 			incremental = false

pkg/uploader/block/snapshot_test.go

@@ -766,6 +766,27 @@ func TestRestore(t *testing.T) {
 			},
 			expectedSize: 4096,
 		},
+		{
+			name:        "incremental restore fallback - empty cbtSource VolumeID",
+			incremental: true,
+			cbtSource:   cbtservice.SourceInfo{Snapshot: "snap-cbt", VolumeID: ""},
+			setupMocks: func(blkup *mockUploader, repo *udmrepomocks.BackupRepo) {
+				snapWithTags := udmrepo.Snapshot{
+					Tags: map[string]string{
+						uploader.CBTChangeIDTag: "cid-1",
+						uploader.CBTVolumeIDTag: "vol-1",
+					},
+				}
+				repo.On("GetSnapshot", mock.Anything, udmrepo.ID("snap-001")).Return(snapWithTags, nil)
+				blkup.On("Restore", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
+					Return(int64(4096), int64(4096), nil)
+			},
+			setupOpenDev: func(t *testing.T) *os.File {
+				t.Helper()
+				return tempFile(t, "")
+			},
+			expectedSize: 4096,
+		},
 		{
 			name:        "incremental restore fallback - VolumeID mismatch",
 			incremental: true,