Skip VS and VSC not created by backup.

Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>

Author: blackpiglet

changelogs/unreleased/8990-blackpiglet

@@ -0,0 +1 @@
+Skip VS and VSC not created by backup.

pkg/backup/actions/csi/pvc_action.go

@@ -285,7 +285,6 @@ func (p *pvcBackupItemAction) Execute(
 			vs,
 			p.crClient,
 			p.log,
-			true,
 			backup.Spec.CSISnapshotTimeout.Duration,
 		)
 		if err != nil {

pkg/backup/actions/csi/volumesnapshot_action.go

@@ -96,30 +96,13 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 		},
 	}
 
-	// determine if we are backing up a VolumeSnapshot that was created by velero while
-	// performing backup of a CSI backed PVC.
-	// For VolumeSnapshots that were created during the backup of a CSI backed PVC,
-	// we will wait for the VolumeSnapshotContents to be available.
-	// For VolumeSnapshots created outside of velero, we expect the VolumeSnapshotContent
-	// to be available prior to backing up the VolumeSnapshot. In case of a failure,
-	// backup should be re-attempted after the CSI driver has reconciled the VolumeSnapshot.
-	// existence of the velerov1api.BackupNameLabel indicates that the VolumeSnapshot was
-	// created while backing up a CSI backed PVC.
-
-	// We want to await reconciliation of only those VolumeSnapshots created during the
-	// ongoing backup.  For this we will wait only if the backup label exists on the
-	// VolumeSnapshot object and the backup name is the same as that of the value of the
-	// backup label.
-	backupOngoing := vs.Labels[velerov1api.BackupNameLabel] == label.GetValidName(backup.Name)
-
 	p.log.Infof("Getting VolumesnapshotContent for Volumesnapshot %s/%s",
 		vs.Namespace, vs.Name)
 
 	vsc, err := csi.WaitUntilVSCHandleIsReady(
 		vs,
 		p.crClient,
 		p.log,
-		backupOngoing,
 		backup.Spec.CSISnapshotTimeout.Duration,
 	)
 	if err != nil {
@@ -171,42 +154,40 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 			}
 		}
 
-		if backupOngoing {
-			p.log.Infof("Patching VolumeSnapshotContent %s with velero BackupNameLabel",
-				vsc.Name)
-			// If we created the VolumeSnapshotContent object during this ongoing backup,
-			// we would have created it with a DeletionPolicy of Retain.
-			// But, we want to retain these VolumeSnapshotContent ONLY for the lifetime
-			// of the backup. To that effect, during velero backup
-			// deletion, we will update the DeletionPolicy of the VolumeSnapshotContent
-			// and then delete the VolumeSnapshot object which will cascade delete the
-			// VolumeSnapshotContent and the associated snapshot in the storage
-			// provider (handled by the CSI driver and the CSI common controller).
-			// However, in the event that the VolumeSnapshot object is deleted outside
-			// of the backup deletion process, it is possible that the dynamically created
-			// VolumeSnapshotContent object will be left as an orphaned and non-discoverable
-			// resource in the cluster as well as in the storage provider. To avoid piling
-			// up of such orphaned resources, we will want to discover and delete the
-			// dynamically created VolumeSnapshotContent. We do that by adding
-			// the "velero.io/backup-name" label on the VolumeSnapshotContent.
-			// Further, we want to add this label only on VolumeSnapshotContents that
-			// were created during an ongoing velero backup.
-			originVSC := vsc.DeepCopy()
-			kubeutil.AddLabels(
-				&vsc.ObjectMeta,
-				map[string]string{
-					velerov1api.BackupNameLabel: label.GetValidName(backup.Name),
-				},
-			)
-
-			if vscPatchError := p.crClient.Patch(
-				context.TODO(),
-				vsc,
-				crclient.MergeFrom(originVSC),
-			); vscPatchError != nil {
-				p.log.Warnf("Failed to patch VolumeSnapshotContent %s: %v",
-					vsc.Name, vscPatchError)
-			}
+		p.log.Infof("Patching VolumeSnapshotContent %s with velero BackupNameLabel",
+			vsc.Name)
+		// If we created the VolumeSnapshotContent object during this ongoing backup,
+		// we would have created it with a DeletionPolicy of Retain.
+		// But, we want to retain these VolumeSnapshotContent ONLY for the lifetime
+		// of the backup. To that effect, during velero backup
+		// deletion, we will update the DeletionPolicy of the VolumeSnapshotContent
+		// and then delete the VolumeSnapshot object which will cascade delete the
+		// VolumeSnapshotContent and the associated snapshot in the storage
+		// provider (handled by the CSI driver and the CSI common controller).
+		// However, in the event that the VolumeSnapshot object is deleted outside
+		// of the backup deletion process, it is possible that the dynamically created
+		// VolumeSnapshotContent object will be left as an orphaned and non-discoverable
+		// resource in the cluster as well as in the storage provider. To avoid piling
+		// up of such orphaned resources, we will want to discover and delete the
+		// dynamically created VolumeSnapshotContent. We do that by adding
+		// the "velero.io/backup-name" label on the VolumeSnapshotContent.
+		// Further, we want to add this label only on VolumeSnapshotContents that
+		// were created during an ongoing velero backup.
+		originVSC := vsc.DeepCopy()
+		kubeutil.AddLabels(
+			&vsc.ObjectMeta,
+			map[string]string{
+				velerov1api.BackupNameLabel: label.GetValidName(backup.Name),
+			},
+		)
+
+		if vscPatchError := p.crClient.Patch(
+			context.TODO(),
+			vsc,
+			crclient.MergeFrom(originVSC),
+		); vscPatchError != nil {
+			p.log.Warnf("Failed to patch VolumeSnapshotContent %s: %v",
+				vsc.Name, vscPatchError)
 		}
 	}
 
@@ -244,20 +225,18 @@ func (p *volumeSnapshotBackupItemAction) Execute(
 	var itemToUpdate []velero.ResourceIdentifier
 
 	// Only return Async operation for VSC created for this backup.
-	if backupOngoing {
-		// The operationID is of the form <namespace>/<volumesnapshot-name>/<started-time>
-		operationID = vs.Namespace + "/" + vs.Name + "/" + time.Now().Format(time.RFC3339)
-		itemToUpdate = []velero.ResourceIdentifier{
-			{
-				GroupResource: kuberesource.VolumeSnapshots,
-				Namespace:     vs.Namespace,
-				Name:          vs.Name,
-			},
-			{
-				GroupResource: kuberesource.VolumeSnapshotContents,
-				Name:          vsc.Name,
-			},
-		}
+	// The operationID is of the form <namespace>/<volumesnapshot-name>/<started-time>
+	operationID = vs.Namespace + "/" + vs.Name + "/" + time.Now().Format(time.RFC3339)
+	itemToUpdate = []velero.ResourceIdentifier{
+		{
+			GroupResource: kuberesource.VolumeSnapshots,
+			Namespace:     vs.Namespace,
+			Name:          vs.Name,
+		},
+		{
+			GroupResource: kuberesource.VolumeSnapshotContents,
+			Name:          vsc.Name,
+		},
 	}
 
 	return &unstructured.Unstructured{Object: vsMap},

pkg/backup/actions/csi/volumesnapshot_action_test.go

@@ -49,23 +49,6 @@ func TestVSExecute(t *testing.T) {
 		expectedAdditionalItems []velero.ResourceIdentifier
 		expectedItemToUpdate    []velero.ResourceIdentifier
 	}{
-		{
-			name: "VS not created by backup, has no status. Backup is finalizing",
-			backup: builder.ForBackup("velero", "backup").
-				Phase(velerov1api.BackupPhaseFinalizing).Result(),
-			vs: builder.ForVolumeSnapshot("velero", "vs").
-				VolumeSnapshotClass("class").Result(),
-			expectedErr: "",
-		},
-		{
-			name: "VS is not created by the backup, associated VSC not exists",
-			backup: builder.ForBackup("velero", "backup").
-				Phase(velerov1api.BackupPhaseInProgress).Result(),
-			vs: builder.ForVolumeSnapshot("velero", "vs").
-				VolumeSnapshotClass("class").Status().
-				BoundVolumeSnapshotContentName("vsc").Result(),
-			expectedErr: `error getting volume snapshot content from API: volumesnapshotcontents.snapshot.storage.k8s.io "vsc" not found`,
-		},
 		{
 			name: "Normal case",
 			backup: builder.ForBackup("velero", "backup").

pkg/controller/backup_controller.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"slices"
 	"time"
 
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v7/apis/volumesnapshot/v1"
@@ -31,6 +32,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/utils/clock"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -63,6 +65,20 @@ const (
 	backupResyncPeriod = time.Minute
 )
 
+var nonBackupNamespaceScopedResources = []string{
+	// CSI VolumeSnapshot and VolumeSnapshotContent are intermediate resources.
+	// Velero only handle the VS and VSC created during backup,
+	// not during resource collecting.
+	"volumesnapshots.snapshot.storage.k8s.io",
+}
+
+var nonBackupClusterScopedResources = []string{
+	// CSI VolumeSnapshot and VolumeSnapshotContent are intermediate resources.
+	// Velero only handle the VS and VSC created during backup,
+	// not during resource collecting.
+	"volumesnapshotcontents.snapshot.storage.k8s.io",
+}
+
 type backupReconciler struct {
 	ctx                         context.Context
 	logger                      logrus.FieldLogger
@@ -481,19 +497,51 @@ func (b *backupReconciler) prepareBackupRequest(backup *velerov1api.Backup, logg
 		request.Status.ValidationErrors = append(request.Status.ValidationErrors, validatedError)
 	}
 
-	// validate the included/excluded resources
-	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedResources, request.Spec.ExcludedResources) {
-		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded resource lists: %v", err))
-	}
-
-	// validate the cluster-scoped included/excluded resources
-	for _, err := range collections.ValidateScopedIncludesExcludes(request.Spec.IncludedClusterScopedResources, request.Spec.ExcludedClusterScopedResources) {
-		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid cluster-scoped included/excluded resource lists: %s", err))
-	}
+	if collections.UseOldResourceFilters(request.Spec) {
+		// validate the included/excluded resources
+		ieErr := collections.ValidateIncludesExcludes(request.Spec.IncludedResources, request.Spec.ExcludedResources)
+		if len(ieErr) > 0 {
+			for _, err := range ieErr {
+				request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded resource lists: %v", err))
+			}
+		} else {
+			request.Spec.IncludedResources, request.Spec.ExcludedResources =
+				modifyResourceIncludeExclude(
+					request.Spec.IncludedResources,
+					request.Spec.ExcludedResources,
+					append(nonBackupNamespaceScopedResources, nonBackupClusterScopedResources...),
+				)
+		}
+	} else {
+		// validate the cluster-scoped included/excluded resources
+		clusterErr := collections.ValidateScopedIncludesExcludes(request.Spec.IncludedClusterScopedResources, request.Spec.ExcludedClusterScopedResources)
+		if len(clusterErr) > 0 {
+			for _, err := range clusterErr {
+				request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid cluster-scoped included/excluded resource lists: %s", err))
+			}
+		} else {
+			request.Spec.IncludedClusterScopedResources, request.Spec.ExcludedClusterScopedResources =
+				modifyResourceIncludeExclude(
+					request.Spec.IncludedClusterScopedResources,
+					request.Spec.ExcludedClusterScopedResources,
+					nonBackupClusterScopedResources,
+				)
+		}
 
-	// validate the namespace-scoped included/excluded resources
-	for _, err := range collections.ValidateScopedIncludesExcludes(request.Spec.IncludedNamespaceScopedResources, request.Spec.ExcludedNamespaceScopedResources) {
-		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid namespace-scoped included/excluded resource lists: %s", err))
+		// validate the namespace-scoped included/excluded resources
+		namespaceErr := collections.ValidateScopedIncludesExcludes(request.Spec.IncludedNamespaceScopedResources, request.Spec.ExcludedNamespaceScopedResources)
+		if len(namespaceErr) > 0 {
+			for _, err := range namespaceErr {
+				request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid namespace-scoped included/excluded resource lists: %s", err))
+			}
+		} else {
+			request.Spec.IncludedNamespaceScopedResources, request.Spec.ExcludedNamespaceScopedResources =
+				modifyResourceIncludeExclude(
+					request.Spec.IncludedNamespaceScopedResources,
+					request.Spec.ExcludedNamespaceScopedResources,
+					nonBackupNamespaceScopedResources,
+				)
+		}
 	}
 
 	// validate the included/excluded namespaces
@@ -932,3 +980,25 @@ func oldAndNewFilterParametersUsedTogether(backupSpec velerov1api.BackupSpec) bo
 
 	return haveOldResourceFilterParameters && haveNewResourceFilterParameters
 }
+
+func modifyResourceIncludeExclude(include, exclude, addedExclude []string) (modifiedInclude, modifiedExclude []string) {
+	modifiedInclude = include
+	modifiedExclude = exclude
+
+	excludeStrSet := sets.NewString(exclude...)
+	for _, ex := range addedExclude {
+		if !excludeStrSet.Has(ex) {
+			modifiedExclude = append(modifiedExclude, ex)
+		}
+	}
+
+	for _, exElem := range modifiedExclude {
+		for inIndex, inElem := range modifiedInclude {
+			if inElem == exElem {
+				modifiedInclude = slices.Delete(modifiedInclude, inIndex, inIndex+1)
+			}
+		}
+	}
+
+	return modifiedInclude, modifiedExclude
+}