[release-1.18] Fix VolumeGroupSnapshot restore failure with Ceph RBD CSI driver (#9687)

* Fix VolumeGroupSnapshot restore failure with Ceph RBD CSI driver (#9516)

* Fix VolumeGroupSnapshot restore on Ceph RBD

This PR fixes two related issues affecting CSI snapshot restore on Ceph RBD:

1. VolumeGroupSnapshot restore fails because Ceph RBD populates
   volumeGroupSnapshotHandle on pre-provisioned VSCs, but Velero doesn't
   create the required VGSC during restore.

2. CSI snapshot restore fails because VolumeSnapshotClassName is removed
   from restored VSCs, preventing the CSI controller from getting
   credentials for snapshot verification.

Changes:
- Capture volumeGroupSnapshotHandle during backup as VS annotation
- Create stub VGSC during restore with matching handle in status
- Look up VolumeSnapshotClass by driver and set on restored VSC

Fixes #9512
Fixes #9515

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Add changelog for VGS restore fix

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Fix gofmt import order

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Add changelog for VGS restore fix

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Fix import alias corev1 to corev1api per lint config

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Fix: Add snapshot handles to existing stub VGSC and add unit tests

When multiple VolumeSnapshots from the same VolumeGroupSnapshot are
restored, they share the same VolumeGroupSnapshotHandle but have
different individual snapshot handles. This commit:

1. Fixes incomplete logic where existing VGSC wasn't updated with
   new snapshot handles (addresses review feedback)

2. Fixes race condition where Create returning AlreadyExists would
   skip adding the snapshot handle

3. Adds comprehensive unit tests for ensureStubVGSCExists (5 cases)
   and addSnapshotHandleToVGSC (4 cases) functions

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Clean up stub VolumeGroupSnapshotContents during restore finalization

Add cleanup logic for stub VGSCs created during VolumeGroupSnapshot restore.
The stub VGSCs are temporary objects needed to satisfy CSI controller
validation during VSC reconciliation. Once all related VSCs become
ReadyToUse, the stub VGSCs are no longer needed and should be removed.

The cleanup runs in the restore finalizer controller's execute() phase.
Before deleting each VGSC, it polls until all related VolumeSnapshotContents
(correlated by snapshot handle) are ReadyToUse, with a timeout fallback.
Deletion failures and CRD-not-installed scenarios are treated as warnings
rather than errors to avoid failing the restore.

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Fix lint: remove unused nolint directive and simplify cleanupStubVGSC return

The cleanupStubVGSC function only produces warnings (not errors), so
simplify its return signature. Also remove the now-unused nolint:unparam
directive on execute() since warnings are no longer always nil.

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

---------

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

* Rename changelog file to match cherry-pick PR number

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

---------

Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>

pkg/controller/restore_finalizer_controller.go

@@ -22,6 +22,8 @@ import (
 	"sync"
 	"time"
 
+	volumegroupsnapshotv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1beta1"
+	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	corev1api "k8s.io/api/core/v1"
@@ -43,6 +45,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
 	"github.com/vmware-tanzu/velero/pkg/util/results"
 )
@@ -291,13 +294,16 @@ type finalizerContext struct {
 	resourceTimeout          time.Duration
 }
 
-func (ctx *finalizerContext) execute() (results.Result, results.Result) { //nolint:unparam //temporarily ignore the lint report: result 0 is always nil (unparam)
+func (ctx *finalizerContext) execute() (results.Result, results.Result) {
 	warnings, errs := results.Result{}, results.Result{}
 
 	// implement finalization tasks
 	pdpErrs := ctx.patchDynamicPVWithVolumeInfo()
 	errs.Merge(&pdpErrs)
 
+	vgscWarnings := ctx.cleanupStubVGSC()
+	warnings.Merge(&vgscWarnings)
+
 	rehErrs := ctx.WaitRestoreExecHook()
 	errs.Merge(&rehErrs)
 
@@ -443,6 +449,93 @@ func (ctx *finalizerContext) patchDynamicPVWithVolumeInfo() (errs results.Result
 	return errs
 }
 
+// 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.
+// Before deleting each VGSC, it waits for all related VolumeSnapshotContents
+// to become ReadyToUse, since the CSI controller needs the VGSC during VSC reconciliation.
+func (ctx *finalizerContext) cleanupStubVGSC() (warnings results.Result) {
+	ctx.logger.Info("cleaning up stub VolumeGroupSnapshotContents")
+
+	vgscList := &volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentList{}
+	err := ctx.crClient.List(
+		context.Background(),
+		vgscList,
+		client.MatchingLabels{velerov1api.RestoreNameLabel: ctx.restore.Name},
+	)
+	if err != nil {
+		// If the CRD is not installed, listing will fail. This is expected
+		// on clusters without VolumeGroupSnapshot support, so treat as warning.
+		ctx.logger.WithError(err).Warn("failed to list stub VolumeGroupSnapshotContents, skipping cleanup")
+		warnings.Add("cluster", errors.Wrap(err, "failed to list stub VolumeGroupSnapshotContents"))
+		return warnings
+	}
+
+	if len(vgscList.Items) == 0 {
+		ctx.logger.Info("no stub VolumeGroupSnapshotContents to clean up")
+		return warnings
+	}
+
+	for i := range vgscList.Items {
+		vgsc := &vgscList.Items[i]
+		log := ctx.logger.WithField("vgsc", vgsc.Name)
+
+		// Collect the snapshot handles associated with this VGSC
+		snapshotHandles := map[string]bool{}
+		if vgsc.Spec.Source.GroupSnapshotHandles != nil {
+			for _, h := range vgsc.Spec.Source.GroupSnapshotHandles.VolumeSnapshotHandles {
+				snapshotHandles[h] = true
+			}
+		}
+
+		if len(snapshotHandles) > 0 {
+			// Wait for related VSCs to become ReadyToUse before deleting the VGSC
+			log.Infof("waiting for %d related VolumeSnapshotContents to become ReadyToUse", len(snapshotHandles))
+			err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, ctx.resourceTimeout, true, func(context.Context) (bool, error) {
+				vscList := &snapshotv1api.VolumeSnapshotContentList{}
+				if err := ctx.crClient.List(context.Background(), vscList, client.MatchingLabels{velerov1api.RestoreNameLabel: ctx.restore.Name}); err != nil {
+					log.WithError(err).Warn("failed to list VolumeSnapshotContents")
+					return false, nil
+				}
+
+				for j := range vscList.Items {
+					vsc := &vscList.Items[j]
+					if vsc.Spec.Source.SnapshotHandle == nil {
+						continue
+					}
+					if !snapshotHandles[*vsc.Spec.Source.SnapshotHandle] {
+						continue
+					}
+					// This VSC is related to our VGSC
+					if vsc.Status == nil || !boolptr.IsSetToTrue(vsc.Status.ReadyToUse) {
+						log.Debugf("VolumeSnapshotContent %s not yet ReadyToUse", vsc.Name)
+						return false, nil
+					}
+				}
+				return true, nil
+			})
+			if err != nil {
+				log.WithError(err).Warn("timed out waiting for related VolumeSnapshotContents to become ReadyToUse, proceeding with VGSC deletion")
+				warnings.Add("cluster", errors.Wrapf(err, "timed out waiting for VSCs related to VGSC %s", vgsc.Name))
+			}
+		}
+
+		log.Info("deleting stub VolumeGroupSnapshotContent")
+		if err := ctx.crClient.Delete(context.Background(), vgsc); err != nil {
+			if apierrors.IsNotFound(err) {
+				log.Info("stub VolumeGroupSnapshotContent already deleted")
+				continue
+			}
+			log.WithError(err).Warn("failed to delete stub VolumeGroupSnapshotContent")
+			warnings.Add("cluster", errors.Wrapf(err, "failed to delete stub VolumeGroupSnapshotContent %s", vgsc.Name))
+		} else {
+			log.Info("deleted stub VolumeGroupSnapshotContent")
+		}
+	}
+
+	return warnings
+}
+
 func needPatch(newPV *corev1api.PersistentVolume, pvInfo *volume.PVInfo) bool {
 	if newPV.Spec.PersistentVolumeReclaimPolicy != corev1api.PersistentVolumeReclaimPolicy(pvInfo.ReclaimPolicy) {
 		return true

pkg/controller/restore_finalizer_controller_test.go

@@ -22,6 +22,8 @@ import (
 	"testing"
 	"time"
 
+	volumegroupsnapshotv1beta1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1beta1"
+	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -45,6 +47,7 @@ import (
 	pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	pkgUtilKubeMocks "github.com/vmware-tanzu/velero/pkg/util/kube/mocks"
 	"github.com/vmware-tanzu/velero/pkg/util/results"
 )
@@ -739,3 +742,253 @@ func TestRestoreOperationList(t *testing.T) {
 		})
 	}
 }
+
+func TestCleanupStubVGSC(t *testing.T) {
+	snapshotHandle1 := "snap-handle-1"
+	snapshotHandle2 := "snap-handle-2"
+
+	tests := []struct {
+		name              string
+		restore           *velerov1api.Restore
+		existingVGSCs     []*volumegroupsnapshotv1beta1.VolumeGroupSnapshotContent
+		existingVSCs      []*snapshotv1api.VolumeSnapshotContent
+		expectedRemaining int
+		expectedWarnings  bool
+	}{
+		{
+			name:              "no stub VGSCs to clean up",
+			restore:           builder.ForRestore(velerov1api.DefaultNamespace, "restore-1").Result(),
+			existingVGSCs:     nil,
+			expectedRemaining: 0,
+			expectedWarnings:  false,
+		},
+		{
+			name:    "single stub VGSC deleted after VSCs are ready",
+			restore: builder.ForRestore(velerov1api.DefaultNamespace, "restore-1").Result(),
+			existingVGSCs: []*volumegroupsnapshotv1beta1.VolumeGroupSnapshotContent{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vgsc-stub-1",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSpec{
+						Driver: "rbd.csi.ceph.com",
+						Source: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSource{
+							GroupSnapshotHandles: &volumegroupsnapshotv1beta1.GroupSnapshotHandles{
+								VolumeGroupSnapshotHandle: "vgs-handle-1",
+								VolumeSnapshotHandles:     []string{snapshotHandle1},
+							},
+						},
+					},
+				},
+			},
+			existingVSCs: []*snapshotv1api.VolumeSnapshotContent{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vsc-1",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: snapshotv1api.VolumeSnapshotContentSpec{
+						Driver:         "rbd.csi.ceph.com",
+						DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain,
+						Source: snapshotv1api.VolumeSnapshotContentSource{
+							SnapshotHandle: &snapshotHandle1,
+						},
+						VolumeSnapshotRef: corev1api.ObjectReference{
+							Name:      "vs-1",
+							Namespace: "ns-1",
+						},
+					},
+					Status: &snapshotv1api.VolumeSnapshotContentStatus{
+						ReadyToUse: boolptr.True(),
+					},
+				},
+			},
+			expectedRemaining: 0,
+			expectedWarnings:  false,
+		},
+		{
+			name:    "multiple stub VGSCs deleted",
+			restore: builder.ForRestore(velerov1api.DefaultNamespace, "restore-1").Result(),
+			existingVGSCs: []*volumegroupsnapshotv1beta1.VolumeGroupSnapshotContent{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vgsc-stub-1",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSpec{
+						Driver: "rbd.csi.ceph.com",
+						Source: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSource{
+							GroupSnapshotHandles: &volumegroupsnapshotv1beta1.GroupSnapshotHandles{
+								VolumeGroupSnapshotHandle: "vgs-handle-1",
+								VolumeSnapshotHandles:     []string{snapshotHandle1},
+							},
+						},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vgsc-stub-2",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSpec{
+						Driver: "rbd.csi.ceph.com",
+						Source: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSource{
+							GroupSnapshotHandles: &volumegroupsnapshotv1beta1.GroupSnapshotHandles{
+								VolumeGroupSnapshotHandle: "vgs-handle-2",
+								VolumeSnapshotHandles:     []string{snapshotHandle2},
+							},
+						},
+					},
+				},
+			},
+			existingVSCs: []*snapshotv1api.VolumeSnapshotContent{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vsc-1",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: snapshotv1api.VolumeSnapshotContentSpec{
+						Driver:         "rbd.csi.ceph.com",
+						DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain,
+						Source: snapshotv1api.VolumeSnapshotContentSource{
+							SnapshotHandle: &snapshotHandle1,
+						},
+						VolumeSnapshotRef: corev1api.ObjectReference{
+							Name:      "vs-1",
+							Namespace: "ns-1",
+						},
+					},
+					Status: &snapshotv1api.VolumeSnapshotContentStatus{
+						ReadyToUse: boolptr.True(),
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vsc-2",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: snapshotv1api.VolumeSnapshotContentSpec{
+						Driver:         "rbd.csi.ceph.com",
+						DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain,
+						Source: snapshotv1api.VolumeSnapshotContentSource{
+							SnapshotHandle: &snapshotHandle2,
+						},
+						VolumeSnapshotRef: corev1api.ObjectReference{
+							Name:      "vs-2",
+							Namespace: "ns-1",
+						},
+					},
+					Status: &snapshotv1api.VolumeSnapshotContentStatus{
+						ReadyToUse: boolptr.True(),
+					},
+				},
+			},
+			expectedRemaining: 0,
+			expectedWarnings:  false,
+		},
+		{
+			name:    "VGSCs from different restore are not deleted",
+			restore: builder.ForRestore(velerov1api.DefaultNamespace, "restore-1").Result(),
+			existingVGSCs: []*volumegroupsnapshotv1beta1.VolumeGroupSnapshotContent{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vgsc-stub-mine",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSpec{
+						Driver: "rbd.csi.ceph.com",
+						Source: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSource{},
+					},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vgsc-stub-other",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-2",
+						},
+					},
+					Spec: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSpec{
+						Driver: "rbd.csi.ceph.com",
+						Source: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSource{},
+					},
+				},
+			},
+			expectedRemaining: 1,
+			expectedWarnings:  false,
+		},
+		{
+			name:    "VGSC deleted even when no snapshot handles in spec",
+			restore: builder.ForRestore(velerov1api.DefaultNamespace, "restore-1").Result(),
+			existingVGSCs: []*volumegroupsnapshotv1beta1.VolumeGroupSnapshotContent{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "vgsc-stub-empty",
+						Labels: map[string]string{
+							velerov1api.RestoreNameLabel: "restore-1",
+						},
+					},
+					Spec: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSpec{
+						Driver: "rbd.csi.ceph.com",
+						Source: volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentSource{},
+					},
+				},
+			},
+			expectedRemaining: 0,
+			expectedWarnings:  false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			fakeClient := velerotest.NewFakeControllerRuntimeClientBuilder(t).Build()
+			logger := velerotest.NewLogger()
+
+			ctx := &finalizerContext{
+				logger:          logger,
+				crClient:        fakeClient,
+				restore:         tc.restore,
+				resourceTimeout: 10 * time.Second,
+			}
+
+			for _, vgsc := range tc.existingVGSCs {
+				require.NoError(t, fakeClient.Create(t.Context(), vgsc))
+			}
+			for _, vsc := range tc.existingVSCs {
+				require.NoError(t, fakeClient.Create(t.Context(), vsc))
+			}
+
+			warnings := ctx.cleanupStubVGSC()
+
+			if tc.expectedWarnings {
+				assert.False(t, warnings.IsEmpty())
+			} else {
+				assert.True(t, warnings.IsEmpty(), "expected no warnings")
+			}
+
+			remainingList := &volumegroupsnapshotv1beta1.VolumeGroupSnapshotContentList{}
+			require.NoError(t, fakeClient.List(t.Context(), remainingList))
+			assert.Len(t, remainingList.Items, tc.expectedRemaining)
+
+			// Verify remaining VGSCs don't belong to this restore
+			for _, remaining := range remainingList.Items {
+				assert.NotEqual(t, tc.restore.Name, remaining.Labels[velerov1api.RestoreNameLabel],
+					"VGSC %s should have been deleted", remaining.Name)
+			}
+		})
+	}
+}