From 7ab4bfc632517c5f5485b33296cb8bb0f54be05f Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Fri, 17 Dec 2021 09:40:24 +0800 Subject: [PATCH] Revert "Migrate backup sync controller from code-generator to kubebuilder (#4423)" This reverts commit 5aaeb3ebbee05d461957dcb80c3c4d146aa82f92. --- changelogs/unreleased/4423-jxun | 1 - pkg/cmd/server/server.go | 46 +- pkg/controller/backup_sync_controller.go | 171 ++-- pkg/controller/backup_sync_controller_test.go | 943 +++++++++--------- 4 files changed, 617 insertions(+), 544 deletions(-) delete mode 100644 changelogs/unreleased/4423-jxun diff --git a/changelogs/unreleased/4423-jxun b/changelogs/unreleased/4423-jxun deleted file mode 100644 index d2274476e..000000000 --- a/changelogs/unreleased/4423-jxun +++ /dev/null @@ -1 +0,0 @@ -Migrate backup sync controller from code-generator to kubebuilder. \ No newline at end of file diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index cd5617e7a..5a00648c4 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -589,6 +589,28 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string csiVSLister, csiVSCLister := s.getCSISnapshotListers() + backupSyncControllerRunInfo := func() controllerRunInfo { + backupSyncContoller := controller.NewBackupSyncController( + s.veleroClient.VeleroV1(), + s.mgr.GetClient(), + s.veleroClient.VeleroV1(), + s.sharedInformerFactory.Velero().V1().Backups().Lister(), + s.config.backupSyncPeriod, + s.namespace, + s.csiSnapshotClient, + s.kubeClient, + s.config.defaultBackupLocation, + newPluginManager, + backupStoreGetter, + s.logger, + ) + + return controllerRunInfo{ + controller: backupSyncContoller, + numWorkers: defaultControllerWorkers, + } + } + backupTracker := controller.NewBackupTracker() backupControllerRunInfo := func() controllerRunInfo { @@ -747,6 +769,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string } enabledControllers := map[string]func() controllerRunInfo{ + controller.BackupSync: backupSyncControllerRunInfo, controller.Backup: backupControllerRunInfo, controller.Schedule: scheduleControllerRunInfo, controller.GarbageCollection: gcControllerRunInfo, @@ -758,7 +781,6 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string enabledRuntimeControllers := make(map[string]struct{}) enabledRuntimeControllers[controller.ServerStatusRequest] = struct{}{} enabledRuntimeControllers[controller.DownloadRequest] = struct{}{} - enabledRuntimeControllers[controller.BackupSync] = struct{}{} if s.config.restoreOnly { s.logger.Info("Restore only mode - not starting the backup, schedule, delete-backup, or GC controllers") @@ -849,28 +871,6 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string } } - if _, ok := enabledRuntimeControllers[controller.BackupSync]; ok { - syncPeriod := s.config.backupSyncPeriod - if syncPeriod <= 0 { - syncPeriod = time.Minute - } - - r := controller.BackupSyncReconciler{ - Client: s.mgr.GetClient(), - PodVolumeBackupClient: s.veleroClient.VeleroV1(), - BackupLister: s.sharedInformerFactory.Velero().V1().Backups().Lister(), - BackupClient: s.veleroClient.VeleroV1(), - Namespace: s.namespace, - DefaultBackupSyncPeriod: syncPeriod, - NewPluginManager: newPluginManager, - BackupStoreGetter: backupStoreGetter, - Logger: s.logger, - } - if err := r.SetupWithManager(s.mgr); err != nil { - s.logger.Fatal(err, " unable to create controller ", "controller ", controller.BackupSync) - } - } - // TODO(2.0): presuming all controllers and resources are converted to runtime-controller // by v2.0, the block from this line and including the `s.mgr.Start() will be // deprecated, since the manager auto-starts all the caches. Until then, we need to start the diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index ea2378dd7..aeb848ac0 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -1,5 +1,5 @@ /* -Copyright The Velero Contributors. +Copyright 2020 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. @@ -20,12 +20,14 @@ import ( "context" "time" + snapshotterClientSet "github.com/kubernetes-csi/external-snapshotter/client/v4/clientset/versioned" "github.com/pkg/errors" "github.com/sirupsen/logrus" kuberrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/kubernetes" "github.com/vmware-tanzu/velero/internal/storage" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -36,49 +38,114 @@ import ( "github.com/vmware-tanzu/velero/pkg/persistence" "github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) -type BackupSyncReconciler struct { - Client client.Client - PodVolumeBackupClient velerov1client.PodVolumeBackupsGetter - BackupLister velerov1listers.BackupLister - BackupClient velerov1client.BackupsGetter - Namespace string - DefaultBackupSyncPeriod time.Duration - NewPluginManager func(logrus.FieldLogger) clientmgmt.Manager - BackupStoreGetter persistence.ObjectBackupStoreGetter - Logger logrus.FieldLogger +type backupSyncController struct { + *genericController + + backupClient velerov1client.BackupsGetter + kbClient client.Client + podVolumeBackupClient velerov1client.PodVolumeBackupsGetter + backupLister velerov1listers.BackupLister + csiSnapshotClient *snapshotterClientSet.Clientset + kubeClient kubernetes.Interface + namespace string + defaultBackupLocation string + defaultBackupSyncPeriod time.Duration + newPluginManager func(logrus.FieldLogger) clientmgmt.Manager + backupStoreGetter persistence.ObjectBackupStoreGetter } -func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - log := b.Logger.WithField("controller", BackupSync) - log.Debug("Checking for existing backup storage locations to sync into cluster.") +func NewBackupSyncController( + backupClient velerov1client.BackupsGetter, + kbClient client.Client, + podVolumeBackupClient velerov1client.PodVolumeBackupsGetter, + backupLister velerov1listers.BackupLister, + syncPeriod time.Duration, + namespace string, + csiSnapshotClient *snapshotterClientSet.Clientset, + kubeClient kubernetes.Interface, + defaultBackupLocation string, + newPluginManager func(logrus.FieldLogger) clientmgmt.Manager, + backupStoreGetter persistence.ObjectBackupStoreGetter, + logger logrus.FieldLogger, +) Interface { + if syncPeriod <= 0 { + syncPeriod = time.Minute + } + logger.Infof("Backup sync period is %v", syncPeriod) - locationList, err := storage.ListBackupStorageLocations(ctx, b.Client, b.Namespace) + c := &backupSyncController{ + genericController: newGenericController(BackupSync, logger), + backupClient: backupClient, + kbClient: kbClient, + podVolumeBackupClient: podVolumeBackupClient, + namespace: namespace, + defaultBackupLocation: defaultBackupLocation, + defaultBackupSyncPeriod: syncPeriod, + backupLister: backupLister, + csiSnapshotClient: csiSnapshotClient, + kubeClient: kubeClient, + + // use variables to refer to these functions so they can be + // replaced with fakes for testing. + newPluginManager: newPluginManager, + backupStoreGetter: backupStoreGetter, + } + + c.resyncFunc = c.run + c.resyncPeriod = 30 * time.Second + + return c +} + +// orderedBackupLocations returns a new slice with the default backup location first (if it exists), +// followed by the rest of the locations in no particular order. +func orderedBackupLocations(locationList *velerov1api.BackupStorageLocationList, defaultLocationName string) []velerov1api.BackupStorageLocation { + var result []velerov1api.BackupStorageLocation + + for i := range locationList.Items { + if locationList.Items[i].Name == defaultLocationName { + // put the default location first + result = append(result, locationList.Items[i]) + // append everything before the default + result = append(result, locationList.Items[:i]...) + // append everything after the default + result = append(result, locationList.Items[i+1:]...) + + return result + } + } + + return locationList.Items +} + +func (c *backupSyncController) run() { + c.logger.Debug("Checking for existing backup storage locations to sync into cluster") + + locationList, err := storage.ListBackupStorageLocations(context.Background(), c.kbClient, c.namespace) if err != nil { - log.WithError(err).Error("No backup storage locations found, at least one is required") - return ctrl.Result{}, err + c.logger.WithError(err).Error("No backup storage locations found, at least one is required") + return } // sync the default backup storage location first, if it exists - defaultBackupLocationName := "" for _, location := range locationList.Items { if location.Spec.Default { - defaultBackupLocationName = location.Name + c.defaultBackupLocation = location.Name break } } - locations := orderedBackupLocations(&locationList, defaultBackupLocationName) + locations := orderedBackupLocations(&locationList, c.defaultBackupLocation) - pluginManager := b.NewPluginManager(log) + pluginManager := c.newPluginManager(c.logger) defer pluginManager.CleanupClients() for _, location := range locations { - log := log.WithField("backupLocation", location.Name) + log := c.logger.WithField("backupLocation", location.Name) - syncPeriod := b.DefaultBackupSyncPeriod + syncPeriod := c.defaultBackupSyncPeriod if location.Spec.BackupSyncPeriod != nil { syncPeriod = location.Spec.BackupSyncPeriod.Duration if syncPeriod == 0 { @@ -88,7 +155,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) if syncPeriod < 0 { log.Debug("Backup sync period must be non-negative") - syncPeriod = b.DefaultBackupSyncPeriod + syncPeriod = c.defaultBackupSyncPeriod } } @@ -103,7 +170,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) log.Debug("Checking backup location for backups to sync into cluster") - backupStore, err := b.BackupStoreGetter.Get(&location, pluginManager, log) + backupStore, err := c.backupStoreGetter.Get(&location, pluginManager, log) if err != nil { log.WithError(err).Error("Error getting backup store for this location") continue @@ -119,7 +186,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) log.WithField("backupCount", len(backupStoreBackups)).Debug("Got backups from backup store") // get a list of all the backups that exist as custom resources in the cluster - clusterBackups, err := b.BackupLister.Backups(b.Namespace).List(labels.Everything()) + clusterBackups, err := c.backupLister.Backups(c.namespace).List(labels.Everything()) if err != nil { log.WithError(errors.WithStack(err)).Error("Error getting backups from cluster, proceeding with sync into cluster") } else { @@ -150,7 +217,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) continue } - backup.Namespace = b.Namespace + backup.Namespace = c.namespace backup.ResourceVersion = "" // update the StorageLocation field and label since the name of the location @@ -163,7 +230,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation) // attempt to create backup custom resource via API - backup, err = b.BackupClient.Backups(backup.Namespace).Create(ctx, backup, metav1.CreateOptions{}) + backup, err = c.backupClient.Backups(backup.Namespace).Create(context.TODO(), backup, metav1.CreateOptions{}) switch { case err != nil && kuberrs.IsAlreadyExists(err): log.Debug("Backup already exists in cluster") @@ -200,7 +267,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) podVolumeBackup.Namespace = backup.Namespace podVolumeBackup.ResourceVersion = "" - _, err = b.PodVolumeBackupClient.PodVolumeBackups(backup.Namespace).Create(ctx, podVolumeBackup, metav1.CreateOptions{}) + _, err = c.podVolumeBackupClient.PodVolumeBackups(backup.Namespace).Create(context.TODO(), podVolumeBackup, metav1.CreateOptions{}) switch { case err != nil && kuberrs.IsAlreadyExists(err): log.Debug("Pod volume backup already exists in cluster") @@ -227,7 +294,7 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) for _, snapCont := range snapConts { // TODO: Reset ResourceVersion prior to persisting VolumeSnapshotContents snapCont.ResourceVersion = "" - err := b.Client.Create(ctx, snapCont, &client.CreateOptions{}) + created, err := c.csiSnapshotClient.SnapshotV1beta1().VolumeSnapshotContents().Create(context.TODO(), snapCont, metav1.CreateOptions{}) switch { case err != nil && kuberrs.IsAlreadyExists(err): log.Debugf("volumesnapshotcontent %s already exists in cluster", snapCont.Name) @@ -236,45 +303,36 @@ func (b *BackupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) log.WithError(errors.WithStack(err)).Errorf("Error syncing volumesnapshotcontent %s into cluster", snapCont.Name) continue default: - log.Infof("Created CSI volumesnapshotcontent %s", snapCont.Name) + log.Infof("Created CSI volumesnapshotcontent %s", created.Name) } } } } - b.deleteOrphanedBackups(ctx, location.Name, backupStoreBackups, log) + c.deleteOrphanedBackups(location.Name, backupStoreBackups, log) // update the location's last-synced time field statusPatch := client.MergeFrom(location.DeepCopy()) location.Status.LastSyncedTime = &metav1.Time{Time: time.Now().UTC()} - if err := b.Client.Status().Patch(ctx, &location, statusPatch); err != nil { + if err := c.kbClient.Status().Patch(context.Background(), &location, statusPatch); err != nil { log.WithError(errors.WithStack(err)).Error("Error patching backup location's last-synced time") continue } } - - return ctrl.Result{Requeue: true, RequeueAfter: time.Second * 30}, nil -} - -func (b *BackupSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). - For(&velerov1api.Backup{}). - Complete(b) } // deleteOrphanedBackups deletes backup objects (CRDs) from Kubernetes that have the specified location // and a phase of Completed, but no corresponding backup in object storage. -func (b *BackupSyncReconciler) deleteOrphanedBackups(ctx context.Context, locationName string, backupStoreBackups sets.String, log logrus.FieldLogger) { +func (c *backupSyncController) deleteOrphanedBackups(locationName string, backupStoreBackups sets.String, log logrus.FieldLogger) { locationSelector := labels.Set(map[string]string{ velerov1api.StorageLocationLabel: label.GetValidName(locationName), }).AsSelector() - backups, err := b.BackupLister.Backups(b.Namespace).List(locationSelector) + backups, err := c.backupLister.Backups(c.namespace).List(locationSelector) if err != nil { log.WithError(errors.WithStack(err)).Error("Error listing backups from cluster") return } - if len(backups) == 0 { return } @@ -285,31 +343,10 @@ func (b *BackupSyncReconciler) deleteOrphanedBackups(ctx context.Context, locati continue } - if err := b.BackupClient.Backups(backup.Namespace).Delete(ctx, backup.Name, metav1.DeleteOptions{}); err != nil { + if err := c.backupClient.Backups(backup.Namespace).Delete(context.TODO(), backup.Name, metav1.DeleteOptions{}); err != nil { log.WithError(errors.WithStack(err)).Error("Error deleting orphaned backup from cluster") } else { log.Debug("Deleted orphaned backup from cluster") } } } - -// orderedBackupLocations returns a new slice with the default backup location first (if it exists), -// followed by the rest of the locations in no particular order. -func orderedBackupLocations(locationList *velerov1api.BackupStorageLocationList, defaultLocationName string) []velerov1api.BackupStorageLocation { - var result []velerov1api.BackupStorageLocation - - for i := range locationList.Items { - if locationList.Items[i].Name == defaultLocationName { - // put the default location first - result = append(result, locationList.Items[i]) - // append everything before the default - result = append(result, locationList.Items[:i]...) - // append everything after the default - result = append(result, locationList.Items[i+1:]...) - - return result - } - } - - return locationList.Items -} diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index 43274f70e..0b615bdb5 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -18,23 +18,17 @@ package controller import ( "context" - "fmt" - "reflect" + "testing" "time" - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" - "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" core "k8s.io/client-go/testing" - ctrl "sigs.k8s.io/controller-runtime" - ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" - velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/fake" @@ -113,279 +107,263 @@ func defaultLocationsListWithLongerLocationName(namespace string) []*velerov1api } } -func getDeleteActions(actions []core.Action) []core.Action { - var deleteActions []core.Action - for _, action := range actions { - if action.GetVerb() == "delete" { - deleteActions = append(deleteActions, action) - } - } - return deleteActions -} - -func numBackups(c *fake.Clientset, ns string) (int, error) { - existingK8SBackups, err := c.VeleroV1().Backups(ns).List(context.TODO(), metav1.ListOptions{}) - if err != nil { - return 0, err +func TestBackupSyncControllerRun(t *testing.T) { + type cloudBackupData struct { + backup *velerov1api.Backup + podVolumeBackups []*velerov1api.PodVolumeBackup } - return len(existingK8SBackups.Items), nil -} + tests := []struct { + name string + namespace string + locations []*velerov1api.BackupStorageLocation + cloudBuckets map[string][]*cloudBackupData + existingBackups []*velerov1api.Backup + existingPodVolumeBackups []*velerov1api.PodVolumeBackup + longLocationNameEnabled bool + }{ + { + name: "no cloud backups", + }, + { + name: "normal case", + namespace: "ns-1", + locations: defaultLocationsList("ns-1"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-3").Result(), + }, + }, + }, + }, + { + name: "all synced backups get created in Velero server's namespace", + namespace: "velero", + locations: defaultLocationsList("velero"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-2", "backup-3").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("velero", "backup-4").Result(), + }, + }, + }, + }, + { + name: "new backups get synced when some cloud backups already exist in the cluster", + namespace: "ns-1", + locations: defaultLocationsList("ns-1"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-3").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-4").Result(), + }, + }, + }, + existingBackups: []*velerov1api.Backup{ + // add a label to each existing backup so we can differentiate it from the cloud + // backup during verification + builder.ForBackup("ns-1", "backup-1").StorageLocation("location-1").ObjectMeta(builder.WithLabels("i-exist", "true")).Result(), + builder.ForBackup("ns-1", "backup-3").StorageLocation("location-2").ObjectMeta(builder.WithLabels("i-exist", "true")).Result(), + }, + }, + { + name: "existing backups without a StorageLocation get it filled in", + namespace: "ns-1", + locations: defaultLocationsList("ns-1"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").Result(), + }, + }, + }, + existingBackups: []*velerov1api.Backup{ + // add a label to each existing backup so we can differentiate it from the cloud + // backup during verification + builder.ForBackup("ns-1", "backup-1").ObjectMeta(builder.WithLabels("i-exist", "true")).StorageLocation("location-1").Result(), + }, + }, + { + name: "backup storage location names and labels get updated", + namespace: "ns-1", + locations: defaultLocationsList("ns-1"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").StorageLocation("foo").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "foo")).Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-3").StorageLocation("bar").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "bar")).Result(), + }, + }, + }, + }, + { + name: "backup storage location names and labels get updated with location name greater than 63 chars", + namespace: "ns-1", + locations: defaultLocationsListWithLongerLocationName("ns-1"), + longLocationNameEnabled: true, + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").StorageLocation("foo").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "foo")).Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-3").StorageLocation("bar").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "bar")).Result(), + }, + }, + }, + }, + { + name: "all synced backups and pod volume backups get created in Velero server's namespace", + namespace: "ns-1", + locations: defaultLocationsList("ns-1"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), + }, + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-2").Result(), + }, + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-3").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-4").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), + builder.ForPodVolumeBackup("ns-1", "pvb-2").Result(), + builder.ForPodVolumeBackup("ns-1", "pvb-3").Result(), + }, + }, + }, + }, + }, + { + name: "new pod volume backups get synched when some pod volume backups already exist in the cluster", + namespace: "ns-1", + locations: defaultLocationsList("ns-1"), + cloudBuckets: map[string][]*cloudBackupData{ + "bucket-1": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-1").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), + }, + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-2").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-3").Result(), + }, + }, + }, + "bucket-2": { + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-3").Result(), + }, + &cloudBackupData{ + backup: builder.ForBackup("ns-1", "backup-4").Result(), + podVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), + builder.ForPodVolumeBackup("ns-1", "pvb-5").Result(), + builder.ForPodVolumeBackup("ns-1", "pvb-6").Result(), + }, + }, + }, + }, + existingPodVolumeBackups: []*velerov1api.PodVolumeBackup{ + builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), + builder.ForPodVolumeBackup("ns-1", "pvb-2").Result(), + }, + }, + } -var _ = Describe("Backup Sync Reconciler", func() { - It("Test Backup Sync Reconciler basic function", func() { - type cloudBackupData struct { - backup *velerov1api.Backup - podVolumeBackups []*velerov1api.PodVolumeBackup - } - - tests := []struct { - name string - namespace string - locations []*velerov1api.BackupStorageLocation - cloudBuckets map[string][]*cloudBackupData - existingBackups []*velerov1api.Backup - existingPodVolumeBackups []*velerov1api.PodVolumeBackup - longLocationNameEnabled bool - }{ - { - name: "no cloud backups", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - }, - { - name: "normal case", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-3").Result(), - }, - }, - }, - }, - { - name: "all synced backups get created in Velero server's namespace", - namespace: "velero", - locations: defaultLocationsList("velero"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-2", "backup-3").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("velero", "backup-4").Result(), - }, - }, - }, - }, - { - name: "new backups get synced when some cloud backups already exist in the cluster", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-3").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-4").Result(), - }, - }, - }, - existingBackups: []*velerov1api.Backup{ - // add a label to each existing backup so we can differentiate it from the cloud - // backup during verification - builder.ForBackup("ns-1", "backup-1").StorageLocation("location-1").ObjectMeta(builder.WithLabels("i-exist", "true")).Result(), - builder.ForBackup("ns-1", "backup-3").StorageLocation("location-2").ObjectMeta(builder.WithLabels("i-exist", "true")).Result(), - }, - }, - { - name: "existing backups without a StorageLocation get it filled in", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").Result(), - }, - }, - }, - existingBackups: []*velerov1api.Backup{ - // add a label to each existing backup so we can differentiate it from the cloud - // backup during verification - builder.ForBackup("ns-1", "backup-1").ObjectMeta(builder.WithLabels("i-exist", "true")).StorageLocation("location-1").Result(), - }, - }, - { - name: "backup storage location names and labels get updated", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").StorageLocation("foo").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "foo")).Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-3").StorageLocation("bar").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "bar")).Result(), - }, - }, - }, - }, - { - name: "backup storage location names and labels get updated with location name greater than 63 chars", - namespace: "ns-1", - locations: defaultLocationsListWithLongerLocationName("ns-1"), - longLocationNameEnabled: true, - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").StorageLocation("foo").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "foo")).Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-3").StorageLocation("bar").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "bar")).Result(), - }, - }, - }, - }, - { - name: "all synced backups and pod volume backups get created in Velero server's namespace", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").Result(), - podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), - }, - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-2").Result(), - }, - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-3").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-4").Result(), - podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), - builder.ForPodVolumeBackup("ns-1", "pvb-2").Result(), - builder.ForPodVolumeBackup("ns-1", "pvb-3").Result(), - }, - }, - }, - }, - }, - { - name: "new pod volume backups get synched when some pod volume backups already exist in the cluster", - namespace: "ns-1", - locations: defaultLocationsList("ns-1"), - cloudBuckets: map[string][]*cloudBackupData{ - "bucket-1": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-1").Result(), - podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), - }, - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-2").Result(), - podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-3").Result(), - }, - }, - }, - "bucket-2": { - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-3").Result(), - }, - &cloudBackupData{ - backup: builder.ForBackup("ns-1", "backup-4").Result(), - podVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), - builder.ForPodVolumeBackup("ns-1", "pvb-5").Result(), - builder.ForPodVolumeBackup("ns-1", "pvb-6").Result(), - }, - }, - }, - }, - existingPodVolumeBackups: []*velerov1api.PodVolumeBackup{ - builder.ForPodVolumeBackup("ns-1", "pvb-1").Result(), - builder.ForPodVolumeBackup("ns-1", "pvb-2").Result(), - }, - }, - } - - for _, test := range tests { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { var ( client = fake.NewSimpleClientset() + fakeClient = velerotest.NewFakeControllerRuntimeClient(t) sharedInformers = informers.NewSharedInformerFactory(client, 0) pluginManager = &pluginmocks.Manager{} backupStores = make(map[string]*persistencemocks.BackupStore) ) + c := NewBackupSyncController( + client.VeleroV1(), + fakeClient, + client.VeleroV1(), + sharedInformers.Velero().V1().Backups().Lister(), + time.Duration(0), + test.namespace, + nil, // csiSnapshotClient + nil, // kubeClient + "", + func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, + NewFakeObjectBackupStoreGetter(backupStores), + velerotest.NewLogger(), + ).(*backupSyncController) + pluginManager.On("CleanupClients").Return(nil) - r := BackupSyncReconciler{ - Client: ctrlfake.NewClientBuilder().Build(), - BackupClient: client.VeleroV1(), - PodVolumeBackupClient: client.VeleroV1(), - BackupLister: sharedInformers.Velero().V1().Backups().Lister(), - Namespace: test.namespace, - DefaultBackupSyncPeriod: time.Second * 10, - NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, - BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores), - Logger: velerotest.NewLogger(), - } for _, location := range test.locations { - Expect(r.Client.Create(ctx, location)).ShouldNot(HaveOccurred()) + require.NoError(t, fakeClient.Create(context.Background(), location)) backupStores[location.Name] = &persistencemocks.BackupStore{} } for _, location := range test.locations { backupStore, ok := backupStores[location.Name] - Expect(ok).To(BeTrue(), "no mock backup store for location %s", location.Name) + require.True(t, ok, "no mock backup store for location %s", location.Name) var backupNames []string for _, bucket := range test.cloudBuckets[location.Spec.ObjectStorage.Bucket] { @@ -397,26 +375,21 @@ var _ = Describe("Backup Sync Reconciler", func() { } for _, existingBackup := range test.existingBackups { - Expect(sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(existingBackup)).ShouldNot(HaveOccurred()) + require.NoError(t, sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(existingBackup)) _, err := client.VeleroV1().Backups(test.namespace).Create(context.TODO(), existingBackup, metav1.CreateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + require.NoError(t, err) } for _, existingPodVolumeBackup := range test.existingPodVolumeBackups { - Expect(sharedInformers.Velero().V1().PodVolumeBackups().Informer().GetStore().Add(existingPodVolumeBackup)).ShouldNot(HaveOccurred()) + require.NoError(t, sharedInformers.Velero().V1().PodVolumeBackups().Informer().GetStore().Add(existingPodVolumeBackup)) _, err := client.VeleroV1().PodVolumeBackups(test.namespace).Create(context.TODO(), existingPodVolumeBackup, metav1.CreateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + require.NoError(t, err) } client.ClearActions() - actualResult, err := r.Reconcile(ctx, ctrl.Request{ - NamespacedName: types.NamespacedName{Namespace: "ns-1"}, - }) - - Expect(actualResult).To(BeEquivalentTo(ctrl.Result{Requeue: true, RequeueAfter: 30 * time.Second})) - Expect(err).To(BeNil()) + c.run() for bucket, backupDataSet := range test.cloudBuckets { // figure out which location this bucket is for; we need this for verification @@ -428,12 +401,12 @@ var _ = Describe("Backup Sync Reconciler", func() { break } } - Expect(location).NotTo(BeNil()) + require.NotNil(t, location) // process the cloud backups for _, cloudBackupData := range backupDataSet { obj, err := client.VeleroV1().Backups(test.namespace).Get(context.TODO(), cloudBackupData.backup.Name, metav1.GetOptions{}) - Expect(err).To(BeNil()) + require.NoError(t, err) // did this cloud backup already exist in the cluster? var existing *velerov1api.Backup @@ -452,23 +425,23 @@ var _ = Describe("Backup Sync Reconciler", func() { expected := existing.DeepCopy() expected.Spec.StorageLocation = location.Name - Expect(expected).To(BeEquivalentTo(obj)) + assert.Equal(t, expected, obj) } else { // verify that the storage location field and label are set properly - Expect(location.Name).To(BeEquivalentTo(obj.Spec.StorageLocation)) + assert.Equal(t, location.Name, obj.Spec.StorageLocation) locationName := location.Name if test.longLocationNameEnabled { locationName = label.GetValidName(locationName) } - Expect(locationName).To(BeEquivalentTo(obj.Labels[velerov1api.StorageLocationLabel])) - Expect(len(obj.Labels[velerov1api.StorageLocationLabel]) <= validation.DNS1035LabelMaxLength).To(BeTrue()) + assert.Equal(t, locationName, obj.Labels[velerov1api.StorageLocationLabel]) + assert.Equal(t, true, len(obj.Labels[velerov1api.StorageLocationLabel]) <= validation.DNS1035LabelMaxLength) } // process the cloud pod volume backups for this backup, if any for _, podVolumeBackup := range cloudBackupData.podVolumeBackups { objPodVolumeBackup, err := client.VeleroV1().PodVolumeBackups(test.namespace).Get(context.TODO(), podVolumeBackup.Name, metav1.GetOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + require.NoError(t, err) // did this cloud pod volume backup already exist in the cluster? var existingPodVolumeBackup *velerov1api.PodVolumeBackup @@ -483,160 +456,134 @@ var _ = Describe("Backup Sync Reconciler", func() { // if this cloud pod volume backup already exists in the cluster, make sure that what we get from the // client is the existing backup, not the cloud one. expected := existingPodVolumeBackup.DeepCopy() - Expect(expected).To(BeEquivalentTo(objPodVolumeBackup)) + assert.Equal(t, expected, objPodVolumeBackup) } } } } - } - }) + }) + } +} - It("Test deleting orphaned backups.", func() { - longLabelName := "the-really-long-location-name-that-is-much-more-than-63-characters" +func TestDeleteOrphanedBackups(t *testing.T) { + baseBuilder := func(name string) *builder.BackupBuilder { + return builder.ForBackup("ns-1", name).ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "default")) + } - baseBuilder := func(name string) *builder.BackupBuilder { - return builder.ForBackup("ns-1", name).ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "default")) - } + tests := []struct { + name string + cloudBackups sets.String + k8sBackups []*velerov1api.Backup + namespace string + expectedDeletes sets.String + }{ + { + name: "no overlapping backups", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + baseBuilder("backupA").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backupB").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backupC").Phase(velerov1api.BackupPhaseCompleted).Result(), + }, + expectedDeletes: sets.NewString("backupA", "backupB", "backupC"), + }, + { + name: "some overlapping backups", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + baseBuilder("backup-1").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-2").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-C").Phase(velerov1api.BackupPhaseCompleted).Result(), + }, + expectedDeletes: sets.NewString("backup-C"), + }, + { + name: "all overlapping backups", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + baseBuilder("backup-1").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-2").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-3").Phase(velerov1api.BackupPhaseCompleted).Result(), + }, + expectedDeletes: sets.NewString(), + }, + { + name: "no overlapping backups but including backups that are not complete", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + baseBuilder("backupA").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("Deleting").Phase(velerov1api.BackupPhaseDeleting).Result(), + baseBuilder("Failed").Phase(velerov1api.BackupPhaseFailed).Result(), + baseBuilder("FailedValidation").Phase(velerov1api.BackupPhaseFailedValidation).Result(), + baseBuilder("InProgress").Phase(velerov1api.BackupPhaseInProgress).Result(), + baseBuilder("New").Phase(velerov1api.BackupPhaseNew).Result(), + }, + expectedDeletes: sets.NewString("backupA"), + }, + { + name: "all overlapping backups and all backups that are not complete", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + baseBuilder("backup-1").Phase(velerov1api.BackupPhaseFailed).Result(), + baseBuilder("backup-2").Phase(velerov1api.BackupPhaseFailedValidation).Result(), + baseBuilder("backup-3").Phase(velerov1api.BackupPhaseInProgress).Result(), + }, + expectedDeletes: sets.NewString(), + }, + { + name: "no completed backups in other locations are deleted", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + baseBuilder("backup-1").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-2").Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-C").Phase(velerov1api.BackupPhaseCompleted).Result(), - tests := []struct { - name string - cloudBackups sets.String - k8sBackups []*velerov1api.Backup - namespace string - expectedDeletes sets.String - useLongBSLName bool - }{ - { - name: "no overlapping backups", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - baseBuilder("backupA").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backupB").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backupC").Phase(velerov1api.BackupPhaseCompleted).Result(), - }, - expectedDeletes: sets.NewString("backupA", "backupB", "backupC"), + baseBuilder("backup-4").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "alternate")).Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-5").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "alternate")).Phase(velerov1api.BackupPhaseCompleted).Result(), + baseBuilder("backup-6").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "alternate")).Phase(velerov1api.BackupPhaseCompleted).Result(), }, - { - name: "some overlapping backups", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - baseBuilder("backup-1").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-2").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-C").Phase(velerov1api.BackupPhaseCompleted).Result(), - }, - expectedDeletes: sets.NewString("backup-C"), - }, - { - name: "all overlapping backups", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - baseBuilder("backup-1").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-2").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-3").Phase(velerov1api.BackupPhaseCompleted).Result(), - }, - expectedDeletes: sets.NewString(), - }, - { - name: "no overlapping backups but including backups that are not complete", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - baseBuilder("backupA").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("Deleting").Phase(velerov1api.BackupPhaseDeleting).Result(), - baseBuilder("Failed").Phase(velerov1api.BackupPhaseFailed).Result(), - baseBuilder("FailedValidation").Phase(velerov1api.BackupPhaseFailedValidation).Result(), - baseBuilder("InProgress").Phase(velerov1api.BackupPhaseInProgress).Result(), - baseBuilder("New").Phase(velerov1api.BackupPhaseNew).Result(), - }, - expectedDeletes: sets.NewString("backupA"), - }, - { - name: "all overlapping backups and all backups that are not complete", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - baseBuilder("backup-1").Phase(velerov1api.BackupPhaseFailed).Result(), - baseBuilder("backup-2").Phase(velerov1api.BackupPhaseFailedValidation).Result(), - baseBuilder("backup-3").Phase(velerov1api.BackupPhaseInProgress).Result(), - }, - expectedDeletes: sets.NewString(), - }, - { - name: "no completed backups in other locations are deleted", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - baseBuilder("backup-1").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-2").Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-C").Phase(velerov1api.BackupPhaseCompleted).Result(), + expectedDeletes: sets.NewString("backup-C"), + }, + } - baseBuilder("backup-4").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "alternate")).Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-5").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "alternate")).Phase(velerov1api.BackupPhaseCompleted).Result(), - baseBuilder("backup-6").ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "alternate")).Phase(velerov1api.BackupPhaseCompleted).Result(), - }, - expectedDeletes: sets.NewString("backup-C"), - }, - { - name: "some overlapping backups", - namespace: "ns-1", - cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), - k8sBackups: []*velerov1api.Backup{ - builder.ForBackup("ns-1", "backup-1"). - ObjectMeta( - builder.WithLabels(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779"), - ). - Phase(velerov1api.BackupPhaseCompleted). - Result(), - builder.ForBackup("ns-1", "backup-2"). - ObjectMeta( - builder.WithLabels(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779"), - ). - Phase(velerov1api.BackupPhaseCompleted). - Result(), - builder.ForBackup("ns-1", "backup-C"). - ObjectMeta( - builder.WithLabels(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779"), - ). - Phase(velerov1api.BackupPhaseCompleted). - Result(), - }, - expectedDeletes: sets.NewString("backup-C"), - useLongBSLName: true, - }, - } - - for _, test := range tests { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { var ( client = fake.NewSimpleClientset() + fakeClient = velerotest.NewFakeControllerRuntimeClient(t) sharedInformers = informers.NewSharedInformerFactory(client, 0) - pluginManager = &pluginmocks.Manager{} - backupStores = make(map[string]*persistencemocks.BackupStore) ) - r := BackupSyncReconciler{ - Client: ctrlfake.NewClientBuilder().Build(), - BackupClient: client.VeleroV1(), - PodVolumeBackupClient: client.VeleroV1(), - BackupLister: sharedInformers.Velero().V1().Backups().Lister(), - Namespace: test.namespace, - DefaultBackupSyncPeriod: time.Second * 10, - NewPluginManager: func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }, - BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores), - Logger: velerotest.NewLogger(), - } + c := NewBackupSyncController( + client.VeleroV1(), + fakeClient, + client.VeleroV1(), + sharedInformers.Velero().V1().Backups().Lister(), + time.Duration(0), + test.namespace, + nil, // csiSnapshotClient + nil, // kubeClient + "", + nil, // new plugin manager func + nil, // backupStoreGetter + velerotest.NewLogger(), + ).(*backupSyncController) expectedDeleteActions := make([]core.Action, 0) for _, backup := range test.k8sBackups { // add test backup to informer - Expect(sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(backup)).ShouldNot(HaveOccurred()) + require.NoError(t, sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(backup), "Error adding backup to informer") // add test backup to client _, err := client.VeleroV1().Backups(test.namespace).Create(context.TODO(), backup, metav1.CreateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + require.NoError(t, err, "Error adding backup to clientset") // if we expect this backup to be deleted, set up the expected DeleteAction if test.expectedDeletes.Has(backup.Name) { @@ -649,49 +596,139 @@ var _ = Describe("Backup Sync Reconciler", func() { } } - bslName := "default" - if test.useLongBSLName { - bslName = longLabelName - } - r.deleteOrphanedBackups(ctx, bslName, test.cloudBackups, velerotest.NewLogger()) + c.deleteOrphanedBackups("default", test.cloudBackups, velerotest.NewLogger()) - numBackups, err := numBackups(client, r.Namespace) - Expect(err).ShouldNot(HaveOccurred()) + numBackups, err := numBackups(t, client, c.namespace) + assert.NoError(t, err) expected := len(test.k8sBackups) - len(test.expectedDeletes) - Expect(expected).To(BeEquivalentTo(numBackups)) + assert.Equal(t, expected, numBackups) - compareActions(expectedDeleteActions, getDeleteActions(client.Actions())) - } - }) -}) - -func compareActions(expected, actual []core.Action) { - Expect(len(expected)).To(BeEquivalentTo(len(actual))) - - for _, e := range expected { - found := false - for _, a := range actual { - if reflect.DeepEqual(e, a) { - found = true - break - } - } - if !found { - fmt.Printf("missing expected action %#v\n", e) - } - } - - for _, a := range actual { - found := false - for _, e := range expected { - if reflect.DeepEqual(e, a) { - found = true - break - } - } - if !found { - fmt.Printf("unexpected action %#v\n", a) - } + velerotest.CompareActions(t, expectedDeleteActions, getDeleteActions(client.Actions())) + }) } } + +func TestStorageLabelsInDeleteOrphanedBackups(t *testing.T) { + longLabelName := "the-really-long-location-name-that-is-much-more-than-63-characters" + tests := []struct { + name string + cloudBackups sets.String + k8sBackups []*velerov1api.Backup + namespace string + expectedDeletes sets.String + }{ + { + name: "some overlapping backups", + namespace: "ns-1", + cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"), + k8sBackups: []*velerov1api.Backup{ + builder.ForBackup("ns-1", "backup-1"). + ObjectMeta( + builder.WithLabels(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779"), + ). + Phase(velerov1api.BackupPhaseCompleted). + Result(), + builder.ForBackup("ns-1", "backup-2"). + ObjectMeta( + builder.WithLabels(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779"), + ). + Phase(velerov1api.BackupPhaseCompleted). + Result(), + builder.ForBackup("ns-1", "backup-C"). + ObjectMeta( + builder.WithLabels(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779"), + ). + Phase(velerov1api.BackupPhaseCompleted). + Result(), + }, + expectedDeletes: sets.NewString("backup-C"), + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var ( + client = fake.NewSimpleClientset() + fakeClient = velerotest.NewFakeControllerRuntimeClient(t) + sharedInformers = informers.NewSharedInformerFactory(client, 0) + ) + + c := NewBackupSyncController( + client.VeleroV1(), + fakeClient, + client.VeleroV1(), + sharedInformers.Velero().V1().Backups().Lister(), + time.Duration(0), + test.namespace, + nil, // csiSnapshotClient + nil, // kubeClient + "", + nil, // new plugin manager func + nil, // backupStoreGetter + velerotest.NewLogger(), + ).(*backupSyncController) + + expectedDeleteActions := make([]core.Action, 0) + + for _, backup := range test.k8sBackups { + // add test backup to informer + require.NoError(t, sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(backup), "Error adding backup to informer") + + // add test backup to client + _, err := client.VeleroV1().Backups(test.namespace).Create(context.TODO(), backup, metav1.CreateOptions{}) + require.NoError(t, err, "Error adding backup to clientset") + + // if we expect this backup to be deleted, set up the expected DeleteAction + if test.expectedDeletes.Has(backup.Name) { + actionDelete := core.NewDeleteAction( + velerov1api.SchemeGroupVersion.WithResource("backups"), + test.namespace, + backup.Name, + ) + expectedDeleteActions = append(expectedDeleteActions, actionDelete) + } + } + + c.deleteOrphanedBackups(longLabelName, test.cloudBackups, velerotest.NewLogger()) + + numBackups, err := numBackups(t, client, c.namespace) + assert.NoError(t, err) + + expected := len(test.k8sBackups) - len(test.expectedDeletes) + assert.Equal(t, expected, numBackups) + + velerotest.CompareActions(t, expectedDeleteActions, getDeleteActions(client.Actions())) + }) + } +} + +func getDeleteActions(actions []core.Action) []core.Action { + var deleteActions []core.Action + for _, action := range actions { + if action.GetVerb() == "delete" { + deleteActions = append(deleteActions, action) + } + } + return deleteActions +} + +func numBackups(t *testing.T, c *fake.Clientset, ns string) (int, error) { + t.Helper() + existingK8SBackups, err := c.VeleroV1().Backups(ns).List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return 0, err + } + + return len(existingK8SBackups.Items), nil +} + +func numPodVolumeBackups(t *testing.T, c *fake.Clientset, ns string) (int, error) { + t.Helper() + existingK8SPodvolumeBackups, err := c.VeleroV1().PodVolumeBackups(ns).List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return 0, err + } + + return len(existingK8SPodvolumeBackups.Items), nil +}