From ef100da89b27842bd2faf0ed1ed87671987602c0 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Tue, 28 Jul 2026 08:53:18 +0800 Subject: [PATCH 1/2] remove VolumeSnapshotContents from resourceMustHave list Stop force-including VolumeSnapshotContents via resourceMustHave on every restore; CSI VolumeSnapshot/PVC RestoreItemActions now set `restore.velero.io/must-include-additional-items` so bound snapshot dependencies are restored only when their parent is restored. Fixes: #9957 Signed-off-by: Adam Zhang --- changelogs/unreleased/10087-adam-jian-zhang | 1 + pkg/restore/actions/csi/pvc_action.go | 9 +++ pkg/restore/actions/csi/pvc_action_test.go | 27 ++++++-- .../actions/csi/volumesnapshot_action.go | 21 ++++-- .../actions/csi/volumesnapshot_action_test.go | 4 ++ pkg/restore/restore.go | 1 - pkg/restore/restore_test.go | 69 +++++++++++++++++++ pkg/test/api_server.go | 3 + pkg/test/resources.go | 34 +++++++++ 9 files changed, 155 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/10087-adam-jian-zhang diff --git a/changelogs/unreleased/10087-adam-jian-zhang b/changelogs/unreleased/10087-adam-jian-zhang new file mode 100644 index 000000000..7edaa117f --- /dev/null +++ b/changelogs/unreleased/10087-adam-jian-zhang @@ -0,0 +1 @@ +Stop force-including VolumeSnapshotContents via resourceMustHave on every restore; CSI VolumeSnapshot/PVC RestoreItemActions now set restore.velero.io/must-include-additional-items so bound snapshot dependencies are restored only when their parent is restored (fixes #9957) diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index 2203682be..6026f5378 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -175,6 +175,15 @@ func (p *pvcRestoreItemAction) Execute( Name: vsName, Namespace: pvc.Namespace, }) + + // Force-restore the VolumeSnapshot even when restore resource filters + // would otherwise exclude it (mirrors backup-side must-include). + annotations := pvc.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.MustIncludeAdditionalItemRestoreAnnotation] = "true" + pvc.SetAnnotations(annotations) } } diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index ea712c027..4ad8cd636 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -402,15 +402,22 @@ func TestExecute(t *testing.T) { vs: builder.ForVolumeSnapshot("velero", vsName).ObjectMeta( builder.WithAnnotations(velerov1api.VolumeSnapshotRestoreSize, "10Gi"), ).Result(), - expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName")).Result(), + expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations( + velerov1api.VolumeSnapshotLabel, "vsName", + velerov1api.MustIncludeAdditionalItemRestoreAnnotation, "true", + )).Result(), }, { - name: "Restore from VolumeSnapshot without volume-snapshot-name annotation", - backup: builder.ForBackup("velero", "testBackup").Result(), - restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").Result(), - pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", AnnSelectedNode, "node1")).Result(), - vs: builder.ForVolumeSnapshot("velero", "testVS").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotRestoreSize, "10Gi")).Result(), - expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", AnnSelectedNode, "node1")).Result(), + name: "Restore from VolumeSnapshot without volume-snapshot-name annotation", + backup: builder.ForBackup("velero", "testBackup").Result(), + restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").Result(), + pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", AnnSelectedNode, "node1")).Result(), + vs: builder.ForVolumeSnapshot("velero", "testVS").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotRestoreSize, "10Gi")).Result(), + expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations( + velerov1api.VolumeSnapshotLabel, "vsName", + AnnSelectedNode, "node1", + velerov1api.MustIncludeAdditionalItemRestoreAnnotation, "true", + )).Result(), }, { name: "DataUploadResult cannot be found", @@ -508,6 +515,12 @@ func TestExecute(t *testing.T) { err := runtime.DefaultUnstructuredConverter.FromUnstructured(output.UpdatedItem.UnstructuredContent(), pvc) require.NoError(t, err) require.Equal(t, tc.expectedPVC.GetObjectMeta(), pvc.GetObjectMeta()) + if tc.name == "Restore from VolumeSnapshot" { + require.Equal(t, "true", pvc.GetAnnotations()[velerov1api.MustIncludeAdditionalItemRestoreAnnotation]) + require.Len(t, output.AdditionalItems, 1) + require.Equal(t, "volumesnapshots.snapshot.storage.k8s.io", output.AdditionalItems[0].GroupResource.String()) + require.Equal(t, "vsName", output.AdditionalItems[0].Name) + } if pvc.Spec.Selector != nil && pvc.Spec.Selector.MatchLabels != nil { // This is used for long name and namespace case. if len(tc.pvc.Namespace+"."+tc.pvc.Name) >= validation.DNS1035LabelMaxLength { diff --git a/pkg/restore/actions/csi/volumesnapshot_action.go b/pkg/restore/actions/csi/volumesnapshot_action.go index da5d4d281..ec0f1912b 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action.go +++ b/pkg/restore/actions/csi/volumesnapshot_action.go @@ -282,12 +282,6 @@ func (p *volumeSnapshotRestoreItemAction) Execute( vs.Namespace, vs.Name) } - vsMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&vs) - if err != nil { - p.log.Errorf("Fail to convert VS %s to unstructured", vs.Namespace+"/"+vs.Name) - return nil, errors.WithStack(err) - } - if vsFromBackup.Status == nil || vsFromBackup.Status.BoundVolumeSnapshotContentName == nil { p.log.Errorf("VS %s doesn't have bound VSC", vsFromBackup.Name) @@ -299,6 +293,21 @@ func (p *volumeSnapshotRestoreItemAction) Execute( Name: *vsFromBackup.Status.BoundVolumeSnapshotContentName, } + // Force-restore the bound VSC even when restore resource filters would + // otherwise exclude it (mirrors backup-side must-include for CSI deps). + annotations := vs.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.MustIncludeAdditionalItemRestoreAnnotation] = "true" + vs.SetAnnotations(annotations) + + vsMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&vs) + if err != nil { + p.log.Errorf("Fail to convert VS %s to unstructured", vs.Namespace+"/"+vs.Name) + return nil, errors.WithStack(err) + } + p.log.Infof(`Returning from VolumeSnapshotRestoreItemAction with VolumeSnapshotContent in additionalItems`) diff --git a/pkg/restore/actions/csi/volumesnapshot_action_test.go b/pkg/restore/actions/csi/volumesnapshot_action_test.go index de3e592c0..d1b42b91c 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action_test.go +++ b/pkg/restore/actions/csi/volumesnapshot_action_test.go @@ -184,6 +184,10 @@ func TestVSExecute(t *testing.T) { require.NoError(t, runtime.DefaultUnstructuredConverter.FromUnstructured( result.UpdatedItem.UnstructuredContent(), &vs)) require.Equal(t, test.expectedVS.Spec, vs.Spec) + require.Equal(t, "true", vs.GetAnnotations()[velerov1api.MustIncludeAdditionalItemRestoreAnnotation]) + require.Len(t, result.AdditionalItems, 1) + require.Equal(t, "volumesnapshotcontents.snapshot.storage.k8s.io", result.AdditionalItems[0].GroupResource.String()) + require.Equal(t, "vscName", result.AdditionalItems[0].Name) } }) } diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 7ba9ae6fd..e7a284fb1 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -87,7 +87,6 @@ const ObjectStatusRestoreAnnotationKey = "velero.io/restore-status" var resourceMustHave = []string{ "datauploads.velero.io", - "volumesnapshotcontents.snapshot.storage.k8s.io", } type VolumeSnapshotterGetter interface { diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 9d46c3e53..935586e63 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -754,6 +754,29 @@ func TestRestoreResourceFiltering(t *testing.T) { apiResources: []*test.APIResource{test.ServiceAccounts()}, want: map[*test.APIResource][]string{test.ServiceAccounts(): {"ns-1/sa-1"}}, }, + { + // Regression for #9957: VSC must not be force-included via resourceMustHave + // when the restore only selects unrelated resource types. + name: "volumesnapshotcontents are not force-included for selective resource restores", + restore: defaultRestore().IncludedResources("storageclasses").IncludeClusterResources(true).Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("storageclasses.storage.k8s.io", + builder.ForStorageClass("sc-1").Result(), + ). + AddItems("volumesnapshotcontents.snapshot.storage.k8s.io", + builder.ForVolumeSnapshotContent("vsc-1").Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.StorageClasses(), + test.VolumeSnapshotContents(), + }, + want: map[*test.APIResource][]string{ + test.StorageClasses(): {"/sc-1"}, + test.VolumeSnapshotContents(): nil, + }, + }, } for _, tc := range tests { @@ -2592,6 +2615,52 @@ func TestRestoreMustIncludeAdditionalItems(t *testing.T) { test.PVCs(): nil, }) }) + + t.Run("VS must-include restores excluded VolumeSnapshotContent additional item", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.VolumeSnapshots()) + h.AddItems(t, test.VolumeSnapshotContents()) + + data := &Request{ + Log: h.log, + Restore: defaultRestore().IncludedResources("volumesnapshots.snapshot.storage.k8s.io").IncludeClusterResources(true).Result(), + Backup: defaultBackup().Result(), + BackupReader: test.NewTarWriter(t). + AddItems("volumesnapshots.snapshot.storage.k8s.io", builder.ForVolumeSnapshot("ns-1", "vs-1").Result()). + AddItems("volumesnapshotcontents.snapshot.storage.k8s.io", builder.ForVolumeSnapshotContent("vsc-1").Result()). + Done(), + } + warnings, errs := h.restorer.Restore( + data, + []riav2.RestoreItemAction{ + &pluggableAction{ + selector: velero.ResourceSelector{IncludedResources: []string{"volumesnapshots.snapshot.storage.k8s.io"}}, + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + item := input.Item.(*unstructured.Unstructured) + annotations := item.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.MustIncludeAdditionalItemRestoreAnnotation] = "true" + item.SetAnnotations(annotations) + return &velero.RestoreItemActionExecuteOutput{ + UpdatedItem: item, + AdditionalItems: []velero.ResourceIdentifier{ + {GroupResource: kuberesource.VolumeSnapshotContents, Name: "vsc-1"}, + }, + }, nil + }, + }, + }, + nil, + ) + + assertEmptyResults(t, warnings, errs) + assertAPIContents(t, h, map[*test.APIResource][]string{ + test.VolumeSnapshots(): {"ns-1/vs-1"}, + test.VolumeSnapshotContents(): {"/vsc-1"}, + }) + }) } // TestShouldRestore runs the ShouldRestore function for various permutations of diff --git a/pkg/test/api_server.go b/pkg/test/api_server.go index 63975014a..c69dc5926 100644 --- a/pkg/test/api_server.go +++ b/pkg/test/api_server.go @@ -58,6 +58,9 @@ func NewAPIServer(t *testing.T) *APIServer { {Group: "velero.io", Version: "v2alpha1", Resource: "datauploads"}: "DataUploadsList", {Group: "mygroup.io", Version: "v1", Resource: "mycustomkinds"}: "MyCustomKindList", {Group: "mygroup.io", Version: "v1", Resource: "myclustercustomkinds"}: "MyClusterCustomKindList", + {Group: "storage.k8s.io", Version: "v1", Resource: "storageclasses"}: "StorageClassList", + {Group: "snapshot.storage.k8s.io", Version: "v1", Resource: "volumesnapshots"}: "VolumeSnapshotList", + {Group: "snapshot.storage.k8s.io", Version: "v1", Resource: "volumesnapshotcontents"}: "VolumeSnapshotContentList", }) discoveryClient = &DiscoveryClient{FakeDiscovery: kubeClient.Discovery().(*discoveryfake.FakeDiscovery)} ) diff --git a/pkg/test/resources.go b/pkg/test/resources.go index fe2ad6352..975359d47 100644 --- a/pkg/test/resources.go +++ b/pkg/test/resources.go @@ -220,3 +220,37 @@ func DataUploads(items ...metav1.Object) *APIResource { Items: items, } } + +func StorageClasses(items ...metav1.Object) *APIResource { + return &APIResource{ + Group: "storage.k8s.io", + Version: "v1", + Name: "storageclasses", + ShortName: "sc", + Kind: "StorageClass", + Namespaced: false, + Items: items, + } +} + +func VolumeSnapshotContents(items ...metav1.Object) *APIResource { + return &APIResource{ + Group: "snapshot.storage.k8s.io", + Version: "v1", + Name: "volumesnapshotcontents", + Kind: "VolumeSnapshotContent", + Namespaced: false, + Items: items, + } +} + +func VolumeSnapshots(items ...metav1.Object) *APIResource { + return &APIResource{ + Group: "snapshot.storage.k8s.io", + Version: "v1", + Name: "volumesnapshots", + Kind: "VolumeSnapshot", + Namespaced: true, + Items: items, + } +} From 63cfddd18de1204c801febab72c8fa24cf3ab845 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Tue, 28 Jul 2026 16:06:04 +0800 Subject: [PATCH 2/2] add tests to cover pvc and vsc ria Signed-off-by: Adam Zhang --- pkg/restore/actions/csi/pvc_action_test.go | 27 +++++++++++++++- .../actions/csi/volumesnapshot_action.go | 3 ++ .../actions/csi/volumesnapshot_action_test.go | 32 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index 4ad8cd636..0e10144f6 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -371,6 +371,7 @@ func TestExecute(t *testing.T) { backup *velerov1api.Backup restore *velerov1api.Restore pvc *corev1api.PersistentVolumeClaim + pvcFromBackup *corev1api.PersistentVolumeClaim vs *snapshotv1api.VolumeSnapshot dataUploadResult *corev1api.ConfigMap expectedErr string @@ -407,6 +408,24 @@ func TestExecute(t *testing.T) { velerov1api.MustIncludeAdditionalItemRestoreAnnotation, "true", )).Result(), }, + { + name: "Restore from VolumeSnapshot with nil PVC annotations", + backup: builder.ForBackup("velero", "testBackup").Result(), + restore: builder.ForRestore("velero", "testRestore").ObjectMeta(builder.WithUID("restoreUID")).Backup("testBackup").Result(), + pvc: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testPVC", + Namespace: "velero", + }, + }, + pvcFromBackup: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName")).Result(), + vs: builder.ForVolumeSnapshot("velero", vsName).ObjectMeta( + builder.WithAnnotations(velerov1api.VolumeSnapshotRestoreSize, "10Gi"), + ).Result(), + expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations( + velerov1api.MustIncludeAdditionalItemRestoreAnnotation, "true", + )).Result(), + }, { name: "Restore from VolumeSnapshot without volume-snapshot-name annotation", backup: builder.ForBackup("velero", "testBackup").Result(), @@ -487,7 +506,13 @@ func TestExecute(t *testing.T) { require.NoError(t, err) input.Item = &unstructured.Unstructured{Object: pvcMap} - input.ItemFromBackup = &unstructured.Unstructured{Object: pvcMap} + if tc.pvcFromBackup != nil { + pvcFromBackupMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pvcFromBackup) + require.NoError(t, err) + input.ItemFromBackup = &unstructured.Unstructured{Object: pvcFromBackupMap} + } else { + input.ItemFromBackup = &unstructured.Unstructured{Object: pvcMap} + } input.Restore = tc.restore } if tc.preCreatePVC { diff --git a/pkg/restore/actions/csi/volumesnapshot_action.go b/pkg/restore/actions/csi/volumesnapshot_action.go index ec0f1912b..13b7cb246 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action.go +++ b/pkg/restore/actions/csi/volumesnapshot_action.go @@ -66,6 +66,9 @@ func resetVolumeSnapshotSpecForRestore(vs *snapshotv1api.VolumeSnapshot, vscName } func resetVolumeSnapshotAnnotation(vs *snapshotv1api.VolumeSnapshot) { + if vs.ObjectMeta.Annotations == nil { + vs.ObjectMeta.Annotations = make(map[string]string) + } vs.ObjectMeta.Annotations[velerov1api.VSCDeletionPolicyAnnotation] = string(snapshotv1api.VolumeSnapshotContentRetain) } diff --git a/pkg/restore/actions/csi/volumesnapshot_action_test.go b/pkg/restore/actions/csi/volumesnapshot_action_test.go index d1b42b91c..9d72971d0 100644 --- a/pkg/restore/actions/csi/volumesnapshot_action_test.go +++ b/pkg/restore/actions/csi/volumesnapshot_action_test.go @@ -103,6 +103,26 @@ func TestResetVolumeSnapshotSpecForRestore(t *testing.T) { } } +func TestResetVolumeSnapshotAnnotation(t *testing.T) { + t.Run("should set deletion policy annotation when annotations is nil", func(t *testing.T) { + vs := snapshotv1api.VolumeSnapshot{} + resetVolumeSnapshotAnnotation(&vs) + assert.NotNil(t, vs.ObjectMeta.Annotations) + assert.Equal(t, string(snapshotv1api.VolumeSnapshotContentRetain), vs.ObjectMeta.Annotations[velerov1api.VSCDeletionPolicyAnnotation]) + }) + + t.Run("should preserve existing annotations and set deletion policy annotation", func(t *testing.T) { + vs := snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"foo": "bar"}, + }, + } + resetVolumeSnapshotAnnotation(&vs) + assert.Equal(t, "bar", vs.ObjectMeta.Annotations["foo"]) + assert.Equal(t, string(snapshotv1api.VolumeSnapshotContentRetain), vs.ObjectMeta.Annotations[velerov1api.VSCDeletionPolicyAnnotation]) + }) +} + func TestVSExecute(t *testing.T) { newVscName := util.GenerateSha256FromRestoreUIDAndVsName("restoreUID", "vsName") tests := []struct { @@ -145,6 +165,18 @@ func TestVSExecute(t *testing.T) { expectErr: false, expectedVS: builder.ForVolumeSnapshot("ns", "test").SourceVolumeSnapshotContentName(newVscName).Result(), }, + { + name: "Normal case with nil VS annotations, VSC should be created", + vs: builder.ForVolumeSnapshot("ns", "vsName"). + SourceVolumeSnapshotContentName(newVscName). + VolumeSnapshotClass("vscClass"). + Status(). + BoundVolumeSnapshotContentName("vscName"). + Result(), + restore: builder.ForRestore("velero", "restore").ObjectMeta(builder.WithUID("restoreUID")).Result(), + expectErr: false, + expectedVS: builder.ForVolumeSnapshot("ns", "test").SourceVolumeSnapshotContentName(newVscName).Result(), + }, } for _, test := range tests {