Add MustIncludeAdditionalItemPVCs to help track BIA added PVC's PVB creation. (#10501)

* Add MustIncludeAdditionalItemPVCs structure in backup. It's used to track PVCs returned by BIA with mustIncluded annotaion and PVC is excluded from backup by global filter.
* Modfiy the volumeHelper interface to add a parameter function for ShouldPerformFSBackup.
* Modify to support fine-grained backup filters.
* Modify according to comments. Use a read-only interface to replace the parameter function.

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

pkg/backup/actions/csi/pvc_action.go

@@ -136,6 +136,7 @@ func (p *pvcBackupItemAction) getVolumeHelperWithCache(backup *velerov1api.Backu
 		p.crClient,
 		p.log,
 		p.pvcPodCache,
+		nil,
 	)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create VolumeHelper")

pkg/backup/backup.go

@@ -485,6 +485,8 @@ func (kb *kubernetesBackupper) BackupWithResolvers(
 		return err
 	}
 
+	pvcMustInclusionTracker := NewPVCMustInclusionTracker(backupRequest.MustIncludeAdditionalItemPVCs)
+
 	volumeHelperImpl, err := volumehelper.NewVolumeHelperImplWithNamespaces(
 		backupRequest.ResPolicies,
 		backupRequest.Spec.SnapshotVolumes,
@@ -493,6 +495,7 @@ func (kb *kubernetesBackupper) BackupWithResolvers(
 		boolptr.IsSetToTrue(backupRequest.Spec.DefaultVolumesToFsBackup),
 		!backupRequest.ResourceIncludesExcludes.ShouldInclude(kuberesource.PersistentVolumeClaims.String()),
 		namespaces,
+		pvcMustInclusionTracker,
 	)
 	if err != nil {
 		log.WithError(err).Error("Failed to build PVC-to-Pod cache for volume policy lookups")

pkg/backup/item_backupper.go

@@ -245,6 +245,7 @@ func (ib *itemBackupper) backupItemInternal(logger logrus.FieldLogger, obj runti
 			// where it's been backed up from another pod), since we don't need >1 backup per PVC.
 			for _, volume := range pod.Spec.Volumes {
 				shouldDoFSBackup, err := ib.volumeHelperImpl.ShouldPerformFSBackup(volume, *pod)
+
 				if err != nil {
 					backupErrs = append(backupErrs, errors.WithStack(err))
 				}
@@ -480,6 +481,26 @@ func (ib *itemBackupper) executeActions(
 		delete(u.GetAnnotations(), velerov1api.MustIncludeAdditionalItemAnnotation)
 		obj = u
 
+		// If the BIA specifies that additional items must be included, we track any PVCs returned as additional items.
+		// This tracking is necessary because the FSB (File System Backup) evaluation for a Pod
+		// happens before its PVCs are processed. By tracking these explicitly included PVCs here,
+		// the FSB logic can correctly determine that the PVC will be backed up and therefore
+		// a PodVolumeBackup should be created.
+		// We track this unconditionally when mustInclude is true, because fine-grained backup filters
+		// might exclude a PVC even if it's globally included, but mustInclude overrides those filters.
+		if mustInclude && ib.backupRequest.MustIncludeAdditionalItemPVCs != nil {
+			for _, additionalItem := range additionalItemIdentifiers {
+				if additionalItem.GroupResource == kuberesource.PersistentVolumeClaims {
+					key := itemKey{
+						resource:  additionalItem.GroupResource.String(),
+						namespace: additionalItem.Namespace,
+						name:      additionalItem.Name,
+					}
+					ib.backupRequest.MustIncludeAdditionalItemPVCs.AddItem(key)
+				}
+			}
+		}
+
 		// If async plugin started async operation, add it to the ItemOperations list
 		// ignore during finalize phase
 		if operationID != "" {

pkg/backup/pvc_must_inclusion_tracker.go

@@ -0,0 +1,50 @@
+/*
+Copyright the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package backup
+
+import (
+	"github.com/vmware-tanzu/velero/pkg/kuberesource"
+	vhutil "github.com/vmware-tanzu/velero/pkg/util/volumehelper"
+)
+
+// pvcMustInclusionTracker provides read-only checks for whether a PVC is included
+// in the backup as BIA's additionalItems through annotation
+// backup.velero.io/must-include-additional-items.
+type pvcMustInclusionTracker struct {
+	mustInclude *backedUpItemsMap
+}
+
+func NewPVCMustInclusionTracker(mustInclude *backedUpItemsMap) vhutil.PVCMustInclusionTracker {
+	return &pvcMustInclusionTracker{
+		mustInclude: mustInclude,
+	}
+}
+
+func (p *pvcMustInclusionTracker) IsPVCIncluded(namespace, pvcName string) bool {
+	pvcKey := itemKey{
+		resource:  kuberesource.PersistentVolumeClaims.String(),
+		namespace: namespace,
+		name:      pvcName,
+	}
+
+	// 1. If the PVC was explicitly forced into the backup by a BIA, it will be backed up.
+	if p.mustInclude != nil && p.mustInclude.Has(pvcKey) {
+		return true
+	}
+
+	return false
+}

pkg/backup/pvc_must_inclusion_tracker_test.go

@@ -0,0 +1,47 @@
+/*
+Copyright the Velero contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package backup
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/vmware-tanzu/velero/pkg/kuberesource"
+)
+
+func TestPVCMustInclusionTracker_IsPVCIncluded(t *testing.T) {
+	mustIncludeMap := NewBackedUpItemsMap()
+
+	tracker := NewPVCMustInclusionTracker(mustIncludeMap)
+
+	pvcKey1 := itemKey{
+		resource:  kuberesource.PersistentVolumeClaims.String(),
+		namespace: "ns-1",
+		name:      "pvc-1",
+	}
+
+	// Initially neither PVC is included
+	assert.False(t, tracker.IsPVCIncluded("ns-1", "pvc-1"))
+
+	// Add pvc-1 to mustInclude map
+	mustIncludeMap.AddItem(pvcKey1)
+	assert.True(t, tracker.IsPVCIncluded("ns-1", "pvc-1"))
+
+	// Check a PVC not in any map
+	assert.False(t, tracker.IsPVCIncluded("ns-1", "pvc-2"))
+}

pkg/backup/request.go

@@ -83,11 +83,16 @@ type Request struct {
 	VolumeSnapshots           SynchronizedVSList
 	PodVolumeBackups          []*velerov1api.PodVolumeBackup
 	BackedUpItems             *backedUpItemsMap
-	itemOperationsList        *[]*itemoperation.BackupOperation
-	ResPolicies               *resourcepolicies.Policies
-	SkippedPVTracker          *skipPVTracker
-	VolumesInformation        volume.BackupVolumesInformation
-	WorkerPool                *ItemBlockWorkerPool
+	// MustIncludeAdditionalItemPVCs keeps track of PVCs that are returned as additionalItems
+	// by a BackupItemAction plugin with the must-include annotation. This is specifically
+	// used to ensure PodVolumeBackups (FSB) are created for these PVCs even when PVCs are
+	// excluded by global or fine-grained backup resource filters.
+	MustIncludeAdditionalItemPVCs *backedUpItemsMap
+	itemOperationsList            *[]*itemoperation.BackupOperation
+	ResPolicies                   *resourcepolicies.Policies
+	SkippedPVTracker              *skipPVTracker
+	VolumesInformation            volume.BackupVolumesInformation
+	WorkerPool                    *ItemBlockWorkerPool
 
 	// ClusterScopedFilterMap holds resolved global filters for cluster-scoped resources.
 	// Key is the resolved group-resource string.

pkg/controller/backup_controller.go

@@ -395,10 +395,11 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *velerov1api.Backup, logger logrus.FieldLogger) *pkgbackup.Request {
 	request := &pkgbackup.Request{
-		Backup:           backup.DeepCopy(), // don't modify items in the cache
-		SkippedPVTracker: pkgbackup.NewSkipPVTracker(),
-		BackedUpItems:    pkgbackup.NewBackedUpItemsMap(),
-		WorkerPool:       pkgbackup.StartItemBlockWorkerPool(ctx, b.itemBlockWorkerCount, logger),
+		Backup:                        backup.DeepCopy(), // don't modify items in the cache
+		SkippedPVTracker:              pkgbackup.NewSkipPVTracker(),
+		BackedUpItems:                 pkgbackup.NewBackedUpItemsMap(),
+		MustIncludeAdditionalItemPVCs: pkgbackup.NewBackedUpItemsMap(),
+		WorkerPool:                    pkgbackup.StartItemBlockWorkerPool(ctx, b.itemBlockWorkerCount, logger),
 	}
 	request.VolumesInformation.Init()
 

pkg/controller/backup_finalizer_controller.go

@@ -158,10 +158,11 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	}
 
 	backupRequest := &pkgbackup.Request{
-		Backup:           backup,
-		StorageLocation:  location,
-		SkippedPVTracker: pkgbackup.NewSkipPVTracker(),
-		BackedUpItems:    pkgbackup.NewBackedUpItemsMap(),
+		Backup:                        backup,
+		StorageLocation:               location,
+		SkippedPVTracker:              pkgbackup.NewSkipPVTracker(),
+		BackedUpItems:                 pkgbackup.NewBackedUpItemsMap(),
+		MustIncludeAdditionalItemPVCs: pkgbackup.NewBackedUpItemsMap(),
 	}
 	var outBackupFile *os.File
 	if len(operations) > 0 {

pkg/plugin/utils/volumehelper/volume_policy_helper.go

@@ -93,6 +93,7 @@ func ShouldPerformSnapshotWithVolumeHelper(
 		crClient,
 		boolptr.IsSetToTrue(backup.Spec.DefaultVolumesToFsBackup),
 		true,
+		nil,
 	)
 
 	return volumeHelperImpl.ShouldPerformSnapshot(unstructured, groupResource)
@@ -111,6 +112,7 @@ func NewVolumeHelperWithNamespaces(
 	defaultVolumesToFSBackup bool,
 	backupExcludePVC bool,
 	namespaces []string,
+	pvcMustInclusionTracker vhutil.PVCMustInclusionTracker,
 ) (vhutil.VolumeHelper, error) {
 	return volumehelper.NewVolumeHelperImplWithNamespaces(
 		volumePolicy,
@@ -120,6 +122,7 @@ func NewVolumeHelperWithNamespaces(
 		defaultVolumesToFSBackup,
 		backupExcludePVC,
 		namespaces,
+		pvcMustInclusionTracker,
 	)
 }
 
@@ -131,11 +134,13 @@ func NewVolumeHelperWithCache(
 	client crclient.Client,
 	logger logrus.FieldLogger,
 	pvcPodCache *podvolumeutil.PVCPodCache,
+	pvcMustInclusionTracker vhutil.PVCMustInclusionTracker,
 ) (vhutil.VolumeHelper, error) {
 	return volumehelper.NewVolumeHelperImplWithCache(
 		backup,
 		client,
 		logger,
 		pvcPodCache,
+		pvcMustInclusionTracker,
 	)
 }

pkg/plugin/utils/volumehelper/volume_policy_helper_test.go

@@ -300,6 +300,7 @@ func TestShouldPerformSnapshotWithNonNilVolumeHelper(t *testing.T) {
 		false, // defaultVolumesToFSBackup
 		true,  // backupExcludePVC
 		[]string{"default"},
+		nil,
 	)
 	require.NoError(t, err)
 	require.NotNil(t, vh)

pkg/util/volumehelper/volume_policy_helper.go

@@ -22,6 +22,13 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
+// PVCMustInclusionTracker provides read-only checks for whether a PVC is included
+// in the backup as BIA's additionalItems through annotation
+// backup.velero.io/must-include-additional-items.
+type PVCMustInclusionTracker interface {
+	IsPVCIncluded(namespace, pvcName string) bool
+}
+
 type VolumeHelper interface {
 	ShouldPerformSnapshot(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, error)
 	ShouldPerformFSBackup(volume corev1api.Volume, pod corev1api.Pod) (bool, error)