Merge pull request #7111 from kaovilai/backup-patch-status-unittest-r…

…elease-1.12

release-1.12: Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize

changelogs/unreleased/7111-kaovilai

@@ -0,0 +1 @@
+Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize

pkg/backup/request.go

@@ -20,8 +20,6 @@ import (
 	"fmt"
 	"sort"
 
-	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
-
 	"github.com/vmware-tanzu/velero/internal/hook"
 	"github.com/vmware-tanzu/velero/internal/resourcepolicies"
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -51,7 +49,6 @@ type Request struct {
 	VolumeSnapshots           []*volume.Snapshot
 	PodVolumeBackups          []*velerov1api.PodVolumeBackup
 	BackedUpItems             map[itemKey]struct{}
-	CSISnapshots              []snapshotv1api.VolumeSnapshot
 	itemOperationsList        *[]*itemoperation.BackupOperation
 	ResPolicies               *resourcepolicies.Policies
 	SkippedPVTracker          *skipPVTracker

pkg/backup/snapshots.go

@@ -0,0 +1,68 @@
+package backup
+
+import (
+	"context"
+
+	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
+	snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1"
+	"github.com/sirupsen/logrus"
+	"k8s.io/apimachinery/pkg/util/sets"
+	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
+
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/features"
+	"github.com/vmware-tanzu/velero/pkg/label"
+	"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) {
+	if boolptr.IsSetToTrue(backup.Spec.SnapshotMoveData) {
+		backupLog.Info("backup SnapshotMoveData is set to true, skip VolumeSnapshot resource persistence.")
+	} else if features.IsEnabled(velerov1api.CSIFeatureFlag) {
+		selector := label.NewSelectorForBackup(backup.Name)
+		vscList := &snapshotv1api.VolumeSnapshotContentList{}
+
+		if volumeSnapshotLister != nil {
+			tmpVSs, err := volumeSnapshotLister.List(label.NewSelectorForBackup(backup.Name))
+			if err != nil {
+				backupLog.Error(err)
+			}
+			for _, vs := range tmpVSs {
+				volumeSnapshots = append(volumeSnapshots, *vs)
+			}
+		}
+
+		err := client.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector})
+		if err != nil {
+			backupLog.Error(err)
+		}
+		if len(vscList.Items) >= 0 {
+			volumeSnapshotContents = vscList.Items
+		}
+
+		vsClassSet := sets.NewString()
+		for index := range volumeSnapshotContents {
+			// persist the volumesnapshotclasses referenced by vsc
+			if volumeSnapshotContents[index].Spec.VolumeSnapshotClassName != nil && !vsClassSet.Has(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName) {
+				vsClass := &snapshotv1api.VolumeSnapshotClass{}
+				if err := client.Get(context.TODO(), kbclient.ObjectKey{Name: *volumeSnapshotContents[index].Spec.VolumeSnapshotClassName}, vsClass); err != nil {
+					backupLog.Error(err)
+				} else {
+					vsClassSet.Insert(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName)
+					volumeSnapshotClasses = append(volumeSnapshotClasses, *vsClass)
+				}
+			}
+		}
+		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/builder/backup_builder.go

@@ -299,3 +299,9 @@ func (b *BackupBuilder) DataMover(name string) *BackupBuilder {
 	b.object.Spec.DataMover = name
 	return b
 }
+
+// WithStatus sets the Backup's status.
+func (b *BackupBuilder) WithStatus(status velerov1api.BackupStatus) *BackupBuilder {
+	b.object.Status = status
+	return b
+}

pkg/cmd/server/server.go

@@ -761,7 +761,6 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string
 			backupStoreGetter,
 			s.config.formatFlag.Parse(),
 			s.csiSnapshotLister,
-			s.csiSnapshotClient,
 			s.credentialFileStore,
 			s.config.maxConcurrentK8SConnections,
 			s.config.defaultSnapshotMoveData,
@@ -825,6 +824,7 @@ 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

@@ -34,7 +34,6 @@ 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"
@@ -112,7 +111,6 @@ func NewBackupReconciler(
 	backupStoreGetter persistence.ObjectBackupStoreGetter,
 	formatFlag logging.Format,
 	volumeSnapshotLister snapshotv1listers.VolumeSnapshotLister,
-	volumeSnapshotClient snapshotterClientSet.Interface,
 	credentialStore credentials.FileStore,
 	maxConcurrentK8SConnections int,
 	defaultSnapshotMoveData bool,
@@ -138,7 +136,6 @@ func NewBackupReconciler(
 		backupStoreGetter:           backupStoreGetter,
 		formatFlag:                  formatFlag,
 		volumeSnapshotLister:        volumeSnapshotLister,
-		volumeSnapshotClient:        volumeSnapshotClient,
 		credentialFileStore:         credentialStore,
 		maxConcurrentK8SConnections: maxConcurrentK8SConnections,
 		defaultSnapshotMoveData:     defaultSnapshotMoveData,
@@ -656,65 +653,15 @@ func (b *backupReconciler) runBackup(backup *pkgbackup.Request) error {
 		fatalErrs = append(fatalErrs, err)
 	}
 
-	// Empty slices here so that they can be passed in to the persistBackup call later, regardless of whether or not CSI's enabled.
-	// This way, we only make the Lister call if the feature flag's on.
-	var volumeSnapshots []snapshotv1api.VolumeSnapshot
-	var volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent
-	var 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) {
-		selector := label.NewSelectorForBackup(backup.Name)
-		vscList := &snapshotv1api.VolumeSnapshotContentList{}
-
-		if b.volumeSnapshotLister != nil {
-			tmpVSs, err := b.volumeSnapshotLister.List(label.NewSelectorForBackup(backup.Name))
-			if err != nil {
-				backupLog.Error(err)
-			}
-			for _, vs := range tmpVSs {
-				volumeSnapshots = append(volumeSnapshots, *vs)
-			}
-		}
-
-		backup.CSISnapshots = volumeSnapshots
-
-		err = b.kbClient.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector})
-		if err != nil {
-			backupLog.Error(err)
-		}
-		if len(vscList.Items) >= 0 {
-			volumeSnapshotContents = vscList.Items
-		}
-
-		vsClassSet := sets.NewString()
-		for index := range volumeSnapshotContents {
-			// persist the volumesnapshotclasses referenced by vsc
-			if volumeSnapshotContents[index].Spec.VolumeSnapshotClassName != nil && !vsClassSet.Has(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName) {
-				vsClass := &snapshotv1api.VolumeSnapshotClass{}
-				if err := b.kbClient.Get(context.TODO(), kbclient.ObjectKey{Name: *volumeSnapshotContents[index].Spec.VolumeSnapshotClassName}, vsClass); err != nil {
-					backupLog.Error(err)
-				} else {
-					vsClassSet.Insert(*volumeSnapshotContents[index].Spec.VolumeSnapshotClassName)
-					volumeSnapshotClasses = append(volumeSnapshotClasses, *vsClass)
-				}
-			}
-		}
-	}
-
+	// native snapshots phase will either be failed or completed right away
+	// https://github.com/vmware-tanzu/velero/blob/de3ea52f0cc478e99efa7b9524c7f353514261a4/pkg/backup/item_backupper.go#L632-L639
 	backup.Status.VolumeSnapshotsAttempted = len(backup.VolumeSnapshots)
 	for _, snap := range backup.VolumeSnapshots {
 		if snap.Status.Phase == volume.SnapshotPhaseCompleted {
 			backup.Status.VolumeSnapshotsCompleted++
 		}
 	}
-
-	backup.Status.CSIVolumeSnapshotsAttempted = len(backup.CSISnapshots)
-	for _, vs := range backup.CSISnapshots {
-		if vs.Status != nil && boolptr.IsSetToTrue(vs.Status.ReadyToUse) {
-			backup.Status.CSIVolumeSnapshotsCompleted++
-		}
-	}
+	volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses := pkgbackup.UpdateBackupCSISnapshotsStatus(b.kbClient, b.volumeSnapshotLister, backup.Backup, backupLog)
 
 	// Iterate over backup item operations and update progress.
 	// Any errors on operations at this point should be added to backup errors.

pkg/controller/backup_controller_test.go

@@ -21,11 +21,13 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"reflect"
 	"sort"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
 	snapshotfake "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned/fake"
 	snapshotinformers "github.com/kubernetes-csi/external-snapshotter/client/v4/informers/externalversions"
@@ -43,6 +45,10 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
+
+	fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	pkgbackup "github.com/vmware-tanzu/velero/pkg/backup"
 	"github.com/vmware-tanzu/velero/pkg/builder"
@@ -1665,3 +1671,63 @@ func Test_getLastSuccessBySchedule(t *testing.T) {
 		})
 	}
 }
+
+// Unit tests to make sure that the backup's status is updated correctly during reconcile.
+// To clear up confusion whether status can be updated with Patch alone without status writer and not kbClient.Status().Patch()
+func TestPatchResourceWorksWithStatus(t *testing.T) {
+	type args struct {
+		original *velerov1api.Backup
+		updated  *velerov1api.Backup
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "patch backup status",
+			args: args{
+				original: defaultBackup().SnapshotMoveData(false).Result(),
+				updated: defaultBackup().SnapshotMoveData(false).WithStatus(velerov1api.BackupStatus{
+					CSIVolumeSnapshotsCompleted: 1,
+				}).Result(),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			scheme := runtime.NewScheme()
+			error := velerov1api.AddToScheme(scheme)
+			if error != nil {
+				t.Errorf("PatchResource() error = %v", error)
+			}
+			fakeClient := fakeClient.NewClientBuilder().WithScheme(scheme).WithObjects(tt.args.original).Build()
+			fromCluster := &velerov1api.Backup{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      tt.args.original.Name,
+					Namespace: tt.args.original.Namespace,
+				},
+			}
+			// check original exists
+			if err := fakeClient.Get(context.Background(), kbclient.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil {
+				t.Errorf("PatchResource() error = %v", err)
+			}
+			// ignore resourceVersion
+			tt.args.updated.ResourceVersion = fromCluster.ResourceVersion
+			tt.args.original.ResourceVersion = fromCluster.ResourceVersion
+			if err := kubeutil.PatchResource(tt.args.original, tt.args.updated, fakeClient); (err != nil) != tt.wantErr {
+				t.Errorf("PatchResource() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			// check updated exists
+			if err := fakeClient.Get(context.Background(), kbclient.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil {
+				t.Errorf("PatchResource() error = %v", err)
+			}
+
+			// check fromCluster is equal to updated
+			if !reflect.DeepEqual(fromCluster, tt.args.updated) {
+				t.Error(cmp.Diff(fromCluster, tt.args.updated))
+			}
+		})
+
+	}
+}

pkg/controller/backup_finalizer_controller.go

@@ -29,6 +29,8 @@ 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/metrics"
@@ -40,19 +42,21 @@ import (
 
 // backupFinalizerReconciler reconciles a Backup object
 type backupFinalizerReconciler struct {
-	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
+	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
 }
 
 // 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,
@@ -187,6 +191,7 @@ func (r *backupFinalizerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	backup.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()}
 	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 {

pkg/controller/backup_finalizer_controller_test.go

@@ -23,6 +23,7 @@ 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"
@@ -43,12 +44,14 @@ 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, fakeClock *testclocks.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
+func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeVolumeSnapshotLister snapshotv1listers.VolumeSnapshotLister, fakeClock *testclocks.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
 	backupper := new(fakeBackupper)
 	return NewBackupFinalizerReconciler(
 		fakeClient,
+		fakeVolumeSnapshotLister,
 		fakeClock,
 		backupper,
 		func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
@@ -160,7 +163,10 @@ func TestBackupFinalizerReconcile(t *testing.T) {
 			}
 
 			fakeClient := velerotest.NewFakeControllerRuntimeClient(t, initObjs...)
-			reconciler, backupper := mockBackupFinalizerReconciler(fakeClient, fakeClock)
+
+			fakeVolumeSnapshotLister := velerotestmocks.NewVolumeSnapshotLister(t)
+
+			reconciler, backupper := mockBackupFinalizerReconciler(fakeClient, fakeVolumeSnapshotLister, 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)

pkg/controller/backup_operations_controller.go

@@ -275,6 +275,8 @@ func (c *backupOperationsReconciler) updateBackupAndOperationsJSON(
 	return nil
 }
 
+// check progress of backupItemOperations
+// return: inProgressOperations, changes, completedCount, failedCount, errs
 func getBackupItemOperationProgress(
 	backup *velerov1api.Backup,
 	pluginManager clientmgmt.Manager,