From 65939c920e119ed3614737465e6a6ab03c136308 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 1 Nov 2023 13:13:50 -0400 Subject: [PATCH 1/8] Add PatchResource unit test for backup status Signed-off-by: Tiger Kaovilai --- pkg/builder/backup_builder.go | 6 ++ pkg/controller/backup_controller_test.go | 67 +++++++++++++++++++ .../backup_operations_controller.go | 2 + 3 files changed, 75 insertions(+) diff --git a/pkg/builder/backup_builder.go b/pkg/builder/backup_builder.go index 275852bd13..b689bbcae7 100644 --- a/pkg/builder/backup_builder.go +++ b/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 +} diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index f187877330..f4a1293705 100644 --- a/pkg/controller/backup_controller_test.go +++ b/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" @@ -41,8 +43,13 @@ import ( "k8s.io/utils/clock" testclocks "k8s.io/utils/clock/testing" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" 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 +1672,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(), client.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(), client.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)) + } + }) + + } +} diff --git a/pkg/controller/backup_operations_controller.go b/pkg/controller/backup_operations_controller.go index f00e9c2056..5e9a5cfd37 100644 --- a/pkg/controller/backup_operations_controller.go +++ b/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, From 453bd93c909dfde78cd0581c810723addbd76665 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 14:28:35 -0400 Subject: [PATCH 2/8] refactor backup snapshot status updates into UpdateBackupSnapshotsStatus() and run in backup_finalizer_controller Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/7046-kaovilai | 1 + pkg/backup/request.go | 3 - pkg/backup/snapshots.go | 64 ++++++++++++++++ pkg/cmd/server/server.go | 2 +- pkg/controller/backup_controller.go | 59 +-------------- pkg/controller/backup_controller_test.go | 11 ++- pkg/controller/backup_finalizer_controller.go | 32 +++++--- .../backup_finalizer_controller_test.go | 10 ++- pkg/test/mocks.go | 20 +++++ pkg/test/mocks/VolumeSnapshotLister.go | 73 +++++++++++++++++++ 10 files changed, 198 insertions(+), 77 deletions(-) create mode 100644 changelogs/unreleased/7046-kaovilai create mode 100644 pkg/backup/snapshots.go create mode 100644 pkg/test/mocks.go create mode 100644 pkg/test/mocks/VolumeSnapshotLister.go diff --git a/changelogs/unreleased/7046-kaovilai b/changelogs/unreleased/7046-kaovilai new file mode 100644 index 0000000000..773b8d94be --- /dev/null +++ b/changelogs/unreleased/7046-kaovilai @@ -0,0 +1 @@ +Update and wait for Backup.Status.CSIVolumeSnapshotsCompleted during finalize \ No newline at end of file diff --git a/pkg/backup/request.go b/pkg/backup/request.go index e9da4bddec..44bc5578f2 100644 --- a/pkg/backup/request.go +++ b/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 diff --git a/pkg/backup/snapshots.go b/pkg/backup/snapshots.go new file mode 100644 index 0000000000..e4505003dd --- /dev/null +++ b/pkg/backup/snapshots.go @@ -0,0 +1,64 @@ +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/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) ([]snapshotv1api.VolumeSnapshot, []snapshotv1api.VolumeSnapshotContent, []snapshotv1api.VolumeSnapshotClass) { + var volumeSnapshots []snapshotv1api.VolumeSnapshot + var volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent + var volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass + 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) + for _, vs := range volumeSnapshots { + if vs.Status != nil && boolptr.IsSetToTrue(vs.Status.ReadyToUse) { + backup.Status.CSIVolumeSnapshotsCompleted++ + } + } + return volumeSnapshots, volumeSnapshotContents, volumeSnapshotClasses +} diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 64138ac6d6..306b1af2ec 100644 --- a/pkg/cmd/server/server.go +++ 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, diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index f4a4147cbe..a708a24e75 100644 --- a/pkg/controller/backup_controller.go +++ b/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. diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index f4a1293705..3776bdb204 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -43,7 +43,6 @@ import ( "k8s.io/utils/clock" testclocks "k8s.io/utils/clock/testing" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" kbclient "sigs.k8s.io/controller-runtime/pkg/client" kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -1059,7 +1058,7 @@ func TestProcessBackupCompletions(t *testing.T) { FormatVersion: "1.1.0", StartTimestamp: ×tamp, Expiration: ×tamp, - CSIVolumeSnapshotsAttempted: 0, + CSIVolumeSnapshotsAttempted: 1, CSIVolumeSnapshotsCompleted: 0, }, }, @@ -1181,7 +1180,7 @@ func TestProcessBackupCompletions(t *testing.T) { FormatVersion: "1.1.0", StartTimestamp: ×tamp, Expiration: ×tamp, - CSIVolumeSnapshotsAttempted: 0, + CSIVolumeSnapshotsAttempted: 1, CSIVolumeSnapshotsCompleted: 0, }, }, @@ -1263,7 +1262,7 @@ func TestProcessBackupCompletions(t *testing.T) { FormatVersion: "1.1.0", StartTimestamp: ×tamp, Expiration: ×tamp, - CSIVolumeSnapshotsAttempted: 0, + CSIVolumeSnapshotsAttempted: 1, CSIVolumeSnapshotsCompleted: 0, }, }, @@ -1710,7 +1709,7 @@ func TestPatchResourceWorksWithStatus(t *testing.T) { }, } // check original exists - if err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil { + if err := fakeClient.Get(context.Background(), kbclient.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil { t.Errorf("PatchResource() error = %v", err) } // ignore resourceVersion @@ -1720,7 +1719,7 @@ func TestPatchResourceWorksWithStatus(t *testing.T) { t.Errorf("PatchResource() error = %v, wantErr %v", err, tt.wantErr) } // check updated exists - if err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil { + if err := fakeClient.Get(context.Background(), kbclient.ObjectKeyFromObject(tt.args.updated), fromCluster); err != nil { t.Errorf("PatchResource() error = %v", err) } diff --git a/pkg/controller/backup_finalizer_controller.go b/pkg/controller/backup_finalizer_controller.go index be88908de2..d0d5e4ef3e 100644 --- a/pkg/controller/backup_finalizer_controller.go +++ b/pkg/controller/backup_finalizer_controller.go @@ -25,10 +25,13 @@ import ( "github.com/sirupsen/logrus" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/retry" clocks "k8s.io/utils/clock" 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 +43,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, @@ -186,7 +191,16 @@ 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) - + CSISnapshotsNotReady := errors.New("CSI snapshots not ready") + retry.OnError(retry.DefaultBackoff, func(err error) bool { + return err == CSISnapshotsNotReady + }, func() error { + pkgbackup.UpdateBackupCSISnapshotsStatus(r.client, r.volumeSnapshotLister, backup, log) + if backup.Status.CSIVolumeSnapshotsCompleted < backup.Status.CSIVolumeSnapshotsAttempted { + return CSISnapshotsNotReady + } + return nil + }) // update backup metadata in object store backupJSON := new(bytes.Buffer) if err := encode.To(backup, "json", backupJSON); err != nil { diff --git a/pkg/controller/backup_finalizer_controller_test.go b/pkg/controller/backup_finalizer_controller_test.go index 011a6561bd..f759d03187 100644 --- a/pkg/controller/backup_finalizer_controller_test.go +++ b/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) diff --git a/pkg/test/mocks.go b/pkg/test/mocks.go new file mode 100644 index 0000000000..9a86d2b705 --- /dev/null +++ b/pkg/test/mocks.go @@ -0,0 +1,20 @@ +package test + +import ( + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + snapshotv1listers "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" + "k8s.io/apimachinery/pkg/labels" +) + +// VolumeSnapshotLister helps list VolumeSnapshots. +// All objects returned here must be treated as read-only. +// +//go:generate mockery --name VolumeSnapshotLister +type VolumeSnapshotLister interface { + // List lists all VolumeSnapshots in the indexer. + // Objects returned here must be treated as read-only. + List(selector labels.Selector) (ret []*snapshotv1.VolumeSnapshot, err error) + // VolumeSnapshots returns an object that can list and get VolumeSnapshots. + VolumeSnapshots(namespace string) snapshotv1listers.VolumeSnapshotNamespaceLister + snapshotv1listers.VolumeSnapshotListerExpansion +} diff --git a/pkg/test/mocks/VolumeSnapshotLister.go b/pkg/test/mocks/VolumeSnapshotLister.go new file mode 100644 index 0000000000..21e4bcb667 --- /dev/null +++ b/pkg/test/mocks/VolumeSnapshotLister.go @@ -0,0 +1,73 @@ +// Code generated by mockery v2.35.4. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + labels "k8s.io/apimachinery/pkg/labels" + + v1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1" + + volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/listers/volumesnapshot/v1" +) + +// VolumeSnapshotLister is an autogenerated mock type for the VolumeSnapshotLister type +type VolumeSnapshotLister struct { + mock.Mock +} + +// List provides a mock function with given fields: selector +func (_m *VolumeSnapshotLister) List(selector labels.Selector) ([]*v1.VolumeSnapshot, error) { + ret := _m.Called(selector) + + var r0 []*v1.VolumeSnapshot + var r1 error + if rf, ok := ret.Get(0).(func(labels.Selector) ([]*v1.VolumeSnapshot, error)); ok { + return rf(selector) + } + if rf, ok := ret.Get(0).(func(labels.Selector) []*v1.VolumeSnapshot); ok { + r0 = rf(selector) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v1.VolumeSnapshot) + } + } + + if rf, ok := ret.Get(1).(func(labels.Selector) error); ok { + r1 = rf(selector) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// VolumeSnapshots provides a mock function with given fields: namespace +func (_m *VolumeSnapshotLister) VolumeSnapshots(namespace string) volumesnapshotv1.VolumeSnapshotNamespaceLister { + ret := _m.Called(namespace) + + var r0 volumesnapshotv1.VolumeSnapshotNamespaceLister + if rf, ok := ret.Get(0).(func(string) volumesnapshotv1.VolumeSnapshotNamespaceLister); ok { + r0 = rf(namespace) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(volumesnapshotv1.VolumeSnapshotNamespaceLister) + } + } + + return r0 +} + +// NewVolumeSnapshotLister creates a new instance of VolumeSnapshotLister. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewVolumeSnapshotLister(t interface { + mock.TestingT + Cleanup(func()) +}) *VolumeSnapshotLister { + mock := &VolumeSnapshotLister{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} From df7274590901a819983bf6f911606da19eb6608a Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 16:14:55 -0400 Subject: [PATCH 3/8] skip this if SnapshotMoveData https://github.com/vmware-tanzu/velero/pull/7046/files#r1380708644 Signed-off-by: Tiger Kaovilai --- pkg/backup/snapshots.go | 75 +++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/pkg/backup/snapshots.go b/pkg/backup/snapshots.go index e4505003dd..0df1c6ef85 100644 --- a/pkg/backup/snapshots.go +++ b/pkg/backup/snapshots.go @@ -16,49 +16,52 @@ import ( // 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) ([]snapshotv1api.VolumeSnapshot, []snapshotv1api.VolumeSnapshotContent, []snapshotv1api.VolumeSnapshotClass) { - var volumeSnapshots []snapshotv1api.VolumeSnapshot - var volumeSnapshotContents []snapshotv1api.VolumeSnapshotContent - var volumeSnapshotClasses []snapshotv1api.VolumeSnapshotClass - selector := label.NewSelectorForBackup(backup.Name) - vscList := &snapshotv1api.VolumeSnapshotContentList{} - - if volumeSnapshotLister != nil { - tmpVSs, err := volumeSnapshotLister.List(label.NewSelectorForBackup(backup.Name)) +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 { + 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) } - for _, vs := range tmpVSs { - volumeSnapshots = append(volumeSnapshots, *vs) + if len(vscList.Items) >= 0 { + volumeSnapshotContents = vscList.Items } - } - - 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) + + 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) - for _, vs := range volumeSnapshots { - if vs.Status != nil && boolptr.IsSetToTrue(vs.Status.ReadyToUse) { - backup.Status.CSIVolumeSnapshotsCompleted++ + 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 } From fc44f3b8f03bd5e1180b68cef905ae29b35a3668 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 16:16:27 -0400 Subject: [PATCH 4/8] remove waiting during finalize Signed-off-by: Tiger Kaovilai --- pkg/controller/backup_finalizer_controller.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/pkg/controller/backup_finalizer_controller.go b/pkg/controller/backup_finalizer_controller.go index d0d5e4ef3e..0a535b95f4 100644 --- a/pkg/controller/backup_finalizer_controller.go +++ b/pkg/controller/backup_finalizer_controller.go @@ -25,7 +25,6 @@ import ( "github.com/sirupsen/logrus" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/util/retry" clocks "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" kbclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -191,16 +190,8 @@ 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) - CSISnapshotsNotReady := errors.New("CSI snapshots not ready") - retry.OnError(retry.DefaultBackoff, func(err error) bool { - return err == CSISnapshotsNotReady - }, func() error { - pkgbackup.UpdateBackupCSISnapshotsStatus(r.client, r.volumeSnapshotLister, backup, log) - if backup.Status.CSIVolumeSnapshotsCompleted < backup.Status.CSIVolumeSnapshotsAttempted { - return CSISnapshotsNotReady - } - return nil - }) + + 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 { From 2d6578635dcf9e8126190687a5a88498226b56ba Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 16:20:46 -0400 Subject: [PATCH 5/8] CSIFeatureFlag enable check Signed-off-by: Tiger Kaovilai --- pkg/backup/snapshots.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/backup/snapshots.go b/pkg/backup/snapshots.go index 0df1c6ef85..189a6ace5f 100644 --- a/pkg/backup/snapshots.go +++ b/pkg/backup/snapshots.go @@ -10,6 +10,7 @@ import ( 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" ) @@ -19,7 +20,7 @@ import ( 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 { + } else if features.IsEnabled(velerov1api.CSIFeatureFlag) { selector := label.NewSelectorForBackup(backup.Name) vscList := &snapshotv1api.VolumeSnapshotContentList{} From ea25b8a79308307d08de7f530189875c093f23f0 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 16:22:30 -0400 Subject: [PATCH 6/8] update changelog to reflect removed waits Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/7046-kaovilai | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/7046-kaovilai b/changelogs/unreleased/7046-kaovilai index 773b8d94be..abf6270396 100644 --- a/changelogs/unreleased/7046-kaovilai +++ b/changelogs/unreleased/7046-kaovilai @@ -1 +1 @@ -Update and wait for Backup.Status.CSIVolumeSnapshotsCompleted during finalize \ No newline at end of file +Update Backup.Status.CSIVolumeSnapshotsCompleted during finalize \ No newline at end of file From ccfbcc5455070cdc49f25fe5ac4eb5560245223c Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 16:46:15 -0400 Subject: [PATCH 7/8] `make update` Signed-off-by: Tiger Kaovilai --- pkg/backup/snapshots.go | 6 +++--- pkg/controller/backup_finalizer_controller.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/backup/snapshots.go b/pkg/backup/snapshots.go index 189a6ace5f..a5c6597051 100644 --- a/pkg/backup/snapshots.go +++ b/pkg/backup/snapshots.go @@ -23,7 +23,7 @@ func UpdateBackupCSISnapshotsStatus(client kbclient.Client, volumeSnapshotLister } 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 { @@ -33,7 +33,7 @@ func UpdateBackupCSISnapshotsStatus(client kbclient.Client, volumeSnapshotLister volumeSnapshots = append(volumeSnapshots, *vs) } } - + err := client.List(context.Background(), vscList, &kbclient.ListOptions{LabelSelector: selector}) if err != nil { backupLog.Error(err) @@ -41,7 +41,7 @@ func UpdateBackupCSISnapshotsStatus(client kbclient.Client, volumeSnapshotLister if len(vscList.Items) >= 0 { volumeSnapshotContents = vscList.Items } - + vsClassSet := sets.NewString() for index := range volumeSnapshotContents { // persist the volumesnapshotclasses referenced by vsc diff --git a/pkg/controller/backup_finalizer_controller.go b/pkg/controller/backup_finalizer_controller.go index 0a535b95f4..eb99f6ee53 100644 --- a/pkg/controller/backup_finalizer_controller.go +++ b/pkg/controller/backup_finalizer_controller.go @@ -190,7 +190,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) From c8fd9d4d62ddc7861303ae9dd7e452acde4ab430 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 2 Nov 2023 17:06:19 -0400 Subject: [PATCH 8/8] revert test changes Signed-off-by: Tiger Kaovilai --- changelogs/unreleased/{7046-kaovilai => 7111-kaovilai} | 0 pkg/controller/backup_controller_test.go | 6 +++--- 2 files changed, 3 insertions(+), 3 deletions(-) rename changelogs/unreleased/{7046-kaovilai => 7111-kaovilai} (100%) diff --git a/changelogs/unreleased/7046-kaovilai b/changelogs/unreleased/7111-kaovilai similarity index 100% rename from changelogs/unreleased/7046-kaovilai rename to changelogs/unreleased/7111-kaovilai diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 3776bdb204..df2e22a22a 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -1058,7 +1058,7 @@ func TestProcessBackupCompletions(t *testing.T) { FormatVersion: "1.1.0", StartTimestamp: ×tamp, Expiration: ×tamp, - CSIVolumeSnapshotsAttempted: 1, + CSIVolumeSnapshotsAttempted: 0, CSIVolumeSnapshotsCompleted: 0, }, }, @@ -1180,7 +1180,7 @@ func TestProcessBackupCompletions(t *testing.T) { FormatVersion: "1.1.0", StartTimestamp: ×tamp, Expiration: ×tamp, - CSIVolumeSnapshotsAttempted: 1, + CSIVolumeSnapshotsAttempted: 0, CSIVolumeSnapshotsCompleted: 0, }, }, @@ -1262,7 +1262,7 @@ func TestProcessBackupCompletions(t *testing.T) { FormatVersion: "1.1.0", StartTimestamp: ×tamp, Expiration: ×tamp, - CSIVolumeSnapshotsAttempted: 1, + CSIVolumeSnapshotsAttempted: 0, CSIVolumeSnapshotsCompleted: 0, }, },