Fix issue #7163.

Update CSIVolumeSnapshotsCompleted in backup's status and the metric
during backup finalize stage according to async operations content.

Signed-off-by: Xun Jiang <jxun@vmware.com>

changelogs/unreleased/7202-blackpiglet

@@ -0,0 +1,2 @@
+Update CSIVolumeSnapshotsCompleted in backup's status and the metric 
+during backup finalize stage according to async operations content.

pkg/backup/snapshots.go

@@ -15,9 +15,18 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 )
 
-// Common function to update the status of CSI snapshots
-// returns VolumeSnapshot, VolumeSnapshotContent, VolumeSnapshotClasses referenced
-func UpdateBackupCSISnapshotsStatus(client kbclient.Client, volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, backup *velerov1api.Backup, backupLog logrus.FieldLogger) (volumeSnapshots []snapshotv1api.VolumeSnapshot, volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent, volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass) {
+// GetBackupCSIResources is used to get CSI snapshot related resources.
+// Returns VolumeSnapshot, VolumeSnapshotContent, VolumeSnapshotClasses referenced
+func GetBackupCSIResources(
+	client kbclient.Client,
+	volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister,
+	backup *velerov1api.Backup,
+	backupLog logrus.FieldLogger,
+) (
+	volumeSnapshots []snapshotv1api.VolumeSnapshot,
+	volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent,
+	volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass,
+) {
 	if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) {
 		backupLog.Info("backup SnapshotMoveData is set to true, skip VolumeSnapshot resource persistence.")
 	} else if features.IsEnabled(velerov1api.CSIFeatureFlag) {
@@ -56,13 +65,7 @@ func UpdateBackupCSISnapshotsStatus(client kbclient.Client, volumeSnapshotLister
 			}
 		}
 		backup.Status.CSIVolumeSnapshotsAttempted = len(volumeSnapshots)
-		csiVolumeSnapshotsCompleted := 0
-		for _, vs := range volumeSnapshots {
-			if vs.Status != nil && boolptr.IsSetToTrue(vs.Status.ReadyToUse) {
-				csiVolumeSnapshotsCompleted++
-			}
-		}
-		backup.Status.CSIVolumeSnapshotsCompleted = csiVolumeSnapshotsCompleted
 	}
+
 	return volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses
 }

pkg/cmd/server/server.go

@@ -824,7 +824,6 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 		cmd.CheckError(err)
 		r := controller.NewBackupFinalizerReconciler(
 			s.mgr.GetClient(),
-			s.csiSnapshotLister,
 			clock.RealClock{},
 			backupper,
 			newPluginManager,

pkg/controller/backup_controller.go

@@ -661,7 +661,9 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error {
 			backup.Status.VolumeSnapshotsCompleted++
 		}
 	}
-	volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses := pkgbackup.UpdateBackupCSISnapshotsStatus(b.kbClient, b.volumeSnapshotLister, backup.Backup, backupLog)
+	volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses := pkgbackup.GetBackupCSIResources(b.kbClient, b.volumeSnapshotLister, backup.Backup, backupLog)
+	// Update CSIVolumeSnapshotsAttempted
+	backup.Status.CSIVolumeSnapshotsAttempted = len(volumeSnapshots)
 
 	// Iterate over backup item operations and update progress.
 	// Any errors on operations at this point should be added to backup errors.
@@ -760,6 +762,7 @@ func recordBackupMetrics(log logrus.FieldLogger, backup *velerov1api.Backup, bac
 		backupDurationSeconds := float64(backupDuration / time.Second)
 		serverMetrics.RegisterBackupDuration(backupScheduleName, backupDurationSeconds)
 	}
+
 	if !finalize {
 		serverMetrics.RegisterVolumeSnapshotAttempts(backupScheduleName, backup.Status.VolumeSnapshotsAttempted)
 		serverMetrics.RegisterVolumeSnapshotSuccesses(backupScheduleName, backup.Status.VolumeSnapshotsCompleted)
@@ -767,8 +770,6 @@ func recordBackupMetrics(log logrus.FieldLogger, backup *velerov1api.Backup, bac
 
 		if features.IsEnabled(velerov1api.CSIFeatureFlag) {
 			serverMetrics.RegisterCSISnapshotAttempts(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsAttempted)
-			serverMetrics.RegisterCSISnapshotSuccesses(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsCompleted)
-			serverMetrics.RegisterCSISnapshotFailures(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsAttempted-backup.Status.CSIVolumeSnapshotsCompleted)
 		}
 
 		if backup.Status.Progress != nil {
@@ -779,6 +780,9 @@ func recordBackupMetrics(log logrus.FieldLogger, backup *velerov1api.Backup, bac
 		if backup.Status.Warnings > 0 {
 			serverMetrics.RegisterBackupWarning(backupScheduleName)
 		}
+	} else if features.IsEnabled(velerov1api.CSIFeatureFlag) {
+		serverMetrics.RegisterCSISnapshotSuccesses(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsCompleted)
+		serverMetrics.RegisterCSISnapshotFailures(backupScheduleName, backup.Name, backup.Status.CSIVolumeSnapshotsAttempted-backup.Status.CSIVolumeSnapshotsCompleted)
 	}
 }
 

pkg/controller/backup_finalizer_controller.go

@@ -29,10 +29,10 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
-	snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1"
-
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	pkgbackup "github.com/vmware-tanzu/velero/pkg/backup"
+	"github.com/vmware-tanzu/velero/pkg/itemoperation"
+	"github.com/vmware-tanzu/velero/pkg/kuberesource"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
@@ -42,21 +42,19 @@ import (
 
 // backupFinalizerReconciler reconciles a Backup object
 type backupFinalizerReconciler struct {
-	client               kbclient.Client
-	volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister
-	clock                clocks.WithTickerAndDelayedExecution
-	backupper            pkgbackup.Backupper
-	newPluginManager     func(logrus.FieldLogger) clientmgmt.Manager
-	backupTracker        BackupTracker
-	metrics              *metrics.ServerMetrics
-	backupStoreGetter    persistence.ObjectBackupStoreGetter
-	log                  logrus.FieldLogger
+	client            kbclient.Client
+	clock             clocks.WithTickerAndDelayedExecution
+	backupper         pkgbackup.Backupper
+	newPluginManager  func(logrus.FieldLogger) clientmgmt.Manager
+	backupTracker     BackupTracker
+	metrics           *metrics.ServerMetrics
+	backupStoreGetter persistence.ObjectBackupStoreGetter
+	log               logrus.FieldLogger
 }
 
 // NewBackupFinalizerReconciler initializes and returns backupFinalizerReconciler struct.
 func NewBackupFinalizerReconciler(
 	client kbclient.Client,
-	volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister,
 	clock clocks.WithTickerAndDelayedExecution,
 	backupper pkgbackup.Backupper,
 	newPluginManager func(logrus.FieldLogger) clientmgmt.Manager,
@@ -188,10 +186,12 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		r.metrics.RegisterBackupPartialFailure(backupScheduleName)
 		r.metrics.RegisterBackupLastStatus(backupScheduleName, metrics.BackupLastStatusFailure)
 	}
+
 	backup.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()}
+	backup.Status.CSIVolumeSnapshotsCompleted = updateCSIVolumeSnapshotsCompleted(operations)
+
 	recordBackupMetrics(log, backup, outBackupFile, r.metrics, true)
 
-	pkgbackup.UpdateBackupCSISnapshotsStatus(r.client, r.volumeSnapshotLister, backup, log)
 	// update backup metadata in object store
 	backupJSON := new(bytes.Buffer)
 	if err := encode.To(backup, "json", backupJSON); err != nil {
@@ -215,3 +215,19 @@ func (r *backupFinalizerReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		For(&velerov1api.Backup{}).
 		Complete(r)
 }
+
+// updateCSIVolumeSnapshotsCompleted calculate the completed VS number according to
+// the backup's async operation list.
+func updateCSIVolumeSnapshotsCompleted(
+	operations []*itemoperation.BackupOperation) int {
+	completedNum := 0
+
+	for index := range operations {
+		if operations[index].Spec.ResourceIdentifier.String() == kuberesource.VolumeSnapshots.String() &&
+			operations[index].Status.Phase == itemoperation.OperationPhaseCompleted {
+			completedNum++
+		}
+	}
+
+	return completedNum
+}

pkg/controller/backup_finalizer_controller_test.go

@@ -23,7 +23,6 @@ import (
 	"testing"
 	"time"
 
-	snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -37,6 +36,7 @@ import (
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
+	"github.com/vmware-tanzu/velero/pkg/features"
 	"github.com/vmware-tanzu/velero/pkg/itemoperation"
 	"github.com/vmware-tanzu/velero/pkg/kuberesource"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
@@ -44,14 +44,12 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/plugin/framework"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
-	velerotestmocks "github.com/vmware-tanzu/velero/pkg/test/mocks"
 )
 
-func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeVolumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, fakeClock *testclocks.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
+func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeClock *testclocks.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
 	backupper := new(fakeBackupper)
 	return NewBackupFinalizerReconciler(
 		fakeClient,
-		fakeVolumeSnapshotLister,
 		fakeClock,
 		backupper,
 		func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
@@ -68,12 +66,14 @@ func TestBackupFinalizerReconcile(t *testing.T) {
 	defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result()
 
 	tests := []struct {
-		name             string
-		backup           *velerov1api.Backup
-		backupOperations []*itemoperation.BackupOperation
-		backupLocation   *velerov1api.BackupStorageLocation
-		expectError      bool
-		expectPhase      velerov1api.BackupPhase
+		name                string
+		backup              *velerov1api.Backup
+		backupOperations    []*itemoperation.BackupOperation
+		backupLocation      *velerov1api.BackupStorageLocation
+		enableCSI           bool
+		expectError         bool
+		expectPhase         velerov1api.BackupPhase
+		expectedCompletedVS int
 	}{
 		{
 			name: "Finalizing backup is completed",
@@ -147,6 +147,50 @@ func TestBackupFinalizerReconcile(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Test calculate backup.Status.BackupItemOperationsCompleted",
+			backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-3").
+				StorageLocation("default").
+				ObjectMeta(builder.WithUID("foo")).
+				StartTimestamp(fakeClock.Now()).
+				WithStatus(velerov1api.BackupStatus{
+					StartTimestamp:              &metav1Now,
+					CompletionTimestamp:         &metav1Now,
+					CSIVolumeSnapshotsAttempted: 1,
+					Phase:                       velerov1api.BackupPhaseFinalizing,
+				}).
+				Result(),
+			backupLocation:      defaultBackupLocation,
+			enableCSI:           true,
+			expectPhase:         velerov1api.BackupPhaseCompleted,
+			expectedCompletedVS: 1,
+			backupOperations: []*itemoperation.BackupOperation{
+				{
+					Spec: itemoperation.BackupOperationSpec{
+						BackupName:       "backup-3",
+						BackupUID:        "foo",
+						BackupItemAction: "foo",
+						ResourceIdentifier: velero.ResourceIdentifier{
+							GroupResource: kuberesource.VolumeSnapshots,
+							Namespace:     "ns-1",
+							Name:          "vs-1",
+						},
+						PostOperationItems: []velero.ResourceIdentifier{
+							{
+								GroupResource: kuberesource.Secrets,
+								Namespace:     "ns-1",
+								Name:          "secret-1",
+							},
+						},
+						OperationID: "operation-3",
+					},
+					Status: itemoperation.OperationStatus{
+						Phase:   itemoperation.OperationPhaseCompleted,
+						Created: &metav1Now,
+					},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -162,11 +206,14 @@ func TestBackupFinalizerReconcile(t *testing.T) {
 				initObjs = append(initObjs, test.backupLocation)
 			}
 
+			if test.enableCSI {
+				features.Enable(velerov1api.CSIFeatureFlag)
+				defer features.Enable()
+			}
+
 			fakeClient := velerotest.NewFakeControllerRuntimeClient(t, initObjs...)
 
-			fakeVolumeSnapshotLister := velerotestmocks.NewVolumeSnapshotLister(t)
-
-			reconciler, backupper := mockBackupFinalizerReconciler(fakeClient, fakeVolumeSnapshotLister, fakeClock)
+			reconciler, backupper := mockBackupFinalizerReconciler(fakeClient, fakeClock)
 			pluginManager.On("CleanupClients").Return(nil)
 			backupStore.On("GetBackupItemOperations", test.backup.Name).Return(test.backupOperations, nil)
 			backupStore.On("GetBackupContents", mock.Anything).Return(io.NopCloser(bytes.NewReader([]byte("hello world"))), nil)
@@ -186,6 +233,7 @@ func TestBackupFinalizerReconcile(t *testing.T) {
 
 			require.NoError(t, err)
 			assert.Equal(t, test.expectPhase, backupAfter.Status.Phase)
+			assert.Equal(t, test.expectedCompletedVS, backupAfter.Status.CSIVolumeSnapshotsCompleted)
 		})
 	}
 }

pkg/controller/backup_sync_controller_test.go

@@ -24,6 +24,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
+	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -33,7 +34,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	testclocks "k8s.io/utils/clock/testing"
-
 	ctrl "sigs.k8s.io/controller-runtime"
 	ctrlClient "sigs.k8s.io/controller-runtime/pkg/client"
 	ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -429,6 +429,9 @@ var _ = Describe("Backup Sync Reconciler", func() {
 					backupNames = append(backupNames, backup.backup.Name)
 					backupStore.On("GetBackupMetadata", backup.backup.Name).Return(backup.backup, nil)
 					backupStore.On("GetPodVolumeBackups", backup.backup.Name).Return(backup.podVolumeBackups, nil)
+					backupStore.On("BackupExists", "bucket-1", backup.backup.Name).Return(true, nil)
+					backupStore.On("GetCSIVolumeSnapshotClasses", backup.backup.Name).Return([]*snapshotv1api.VolumeSnapshotClass{}, nil)
+					backupStore.On("GetCSIVolumeSnapshotContents", backup.backup.Name).Return([]*snapshotv1api.VolumeSnapshotContent{}, nil)
 				}
 				backupStore.On("ListBackups").Return(backupNames, nil)
 			}