Merge pull request #10087 from adam-jian-zhang/fix-vsc-hardcode

remove VolumeSnapshotContent from resourceMustHave

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)

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)
 		}
 	}
 

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
@@ -402,15 +403,40 @@ 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 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(),
+			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",
@@ -480,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 {
@@ -508,6 +540,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 {

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)
 }
@@ -282,12 +285,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 +296,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`)
 

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 {
@@ -184,6 +216,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)
 			}
 		})
 	}

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 {

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

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)}
 	)

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,
+	}
+}