From 5aaeb3ebbee05d461957dcb80c3c4d146aa82f92 Mon Sep 17 00:00:00 2001 From: Xun Jiang/Bruce Jiang <59276555+blackpiglet@users.noreply.github.com> Date: Wed, 15 Dec 2021 09:07:20 +0800 Subject: [PATCH] Migrate backup sync controller from code-generator to kubebuilder (#4423) * Migrate backup sync controller from code-generator to kubebuilder 1. use kubebuilder's reconcile logic to replace controller's old logic. 2. use ginkgo and gomega to replace testing. Signed-off-by: Xun Jiang * Fix: modify code according to comments 1. Remove DefaultBackupLocation 2. Remove unneccessary comment line 3. Add syncPeriod default value setting logic 4. Modify ListBackupStorageLocations function's context parameter 5. Add RequeueAfter parameter in Reconcile function return value Signed-off-by: Xun Jiang * Reconcile function use context passed from parameter 1. Use context passed from parameter, instead of using Reconciler struct's context. 2. Delete Reconciler struct's context member. 3. Modify test case accordingly. Signed-off-by: Xun Jiang --- 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 | 933 +++++++++--------- 4 files changed, 539 insertions(+), 612 deletions(-) create mode 100644 changelogs/unreleased/4423-jxun diff --git a/changelogs/unreleased/4423-jxun b/changelogs/unreleased/4423-jxun new file mode 100644 index 000000000..d2274476e --- /dev/null +++ b/changelogs/unreleased/4423-jxun @@ -0,0 +1 @@ +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 5a00648c4..cd5617e7a 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -589,28 +589,6 @@ 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 { @@ -769,7 +747,6 @@ 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, @@ -781,6 +758,7 @@ 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") @@ -871,6 +849,28 @@ 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 aeb848ac0..ea2378dd7 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -1,5 +1,5 @@ /* -Copyright 2020 the Velero contributors. +Copyright The Velero Contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -20,14 +20,12 @@ 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" @@ -38,114 +36,49 @@ 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 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 +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 } -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) +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.") - 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) + locationList, err := storage.ListBackupStorageLocations(ctx, b.Client, b.Namespace) if err != nil { - c.logger.WithError(err).Error("No backup storage locations found, at least one is required") - return + log.WithError(err).Error("No backup storage locations found, at least one is required") + return ctrl.Result{}, err } // sync the default backup storage location first, if it exists + defaultBackupLocationName := "" for _, location := range locationList.Items { if location.Spec.Default { - c.defaultBackupLocation = location.Name + defaultBackupLocationName = location.Name break } } - locations := orderedBackupLocations(&locationList, c.defaultBackupLocation) + locations := orderedBackupLocations(&locationList, defaultBackupLocationName) - pluginManager := c.newPluginManager(c.logger) + pluginManager := b.NewPluginManager(log) defer pluginManager.CleanupClients() for _, location := range locations { - log := c.logger.WithField("backupLocation", location.Name) + log := log.WithField("backupLocation", location.Name) - syncPeriod := c.defaultBackupSyncPeriod + syncPeriod := b.DefaultBackupSyncPeriod if location.Spec.BackupSyncPeriod != nil { syncPeriod = location.Spec.BackupSyncPeriod.Duration if syncPeriod == 0 { @@ -155,7 +88,7 @@ func (c *backupSyncController) run() { if syncPeriod < 0 { log.Debug("Backup sync period must be non-negative") - syncPeriod = c.defaultBackupSyncPeriod + syncPeriod = b.DefaultBackupSyncPeriod } } @@ -170,7 +103,7 @@ func (c *backupSyncController) run() { log.Debug("Checking backup location for backups to sync into cluster") - backupStore, err := c.backupStoreGetter.Get(&location, pluginManager, log) + backupStore, err := b.BackupStoreGetter.Get(&location, pluginManager, log) if err != nil { log.WithError(err).Error("Error getting backup store for this location") continue @@ -186,7 +119,7 @@ func (c *backupSyncController) run() { 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 := c.backupLister.Backups(c.namespace).List(labels.Everything()) + clusterBackups, err := b.BackupLister.Backups(b.Namespace).List(labels.Everything()) if err != nil { log.WithError(errors.WithStack(err)).Error("Error getting backups from cluster, proceeding with sync into cluster") } else { @@ -217,7 +150,7 @@ func (c *backupSyncController) run() { continue } - backup.Namespace = c.namespace + backup.Namespace = b.Namespace backup.ResourceVersion = "" // update the StorageLocation field and label since the name of the location @@ -230,7 +163,7 @@ func (c *backupSyncController) run() { backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation) // attempt to create backup custom resource via API - backup, err = c.backupClient.Backups(backup.Namespace).Create(context.TODO(), backup, metav1.CreateOptions{}) + backup, err = b.BackupClient.Backups(backup.Namespace).Create(ctx, backup, metav1.CreateOptions{}) switch { case err != nil && kuberrs.IsAlreadyExists(err): log.Debug("Backup already exists in cluster") @@ -267,7 +200,7 @@ func (c *backupSyncController) run() { podVolumeBackup.Namespace = backup.Namespace podVolumeBackup.ResourceVersion = "" - _, err = c.podVolumeBackupClient.PodVolumeBackups(backup.Namespace).Create(context.TODO(), podVolumeBackup, metav1.CreateOptions{}) + _, err = b.PodVolumeBackupClient.PodVolumeBackups(backup.Namespace).Create(ctx, podVolumeBackup, metav1.CreateOptions{}) switch { case err != nil && kuberrs.IsAlreadyExists(err): log.Debug("Pod volume backup already exists in cluster") @@ -294,7 +227,7 @@ func (c *backupSyncController) run() { for _, snapCont := range snapConts { // TODO: Reset ResourceVersion prior to persisting VolumeSnapshotContents snapCont.ResourceVersion = "" - created, err := c.csiSnapshotClient.SnapshotV1beta1().VolumeSnapshotContents().Create(context.TODO(), snapCont, metav1.CreateOptions{}) + err := b.Client.Create(ctx, snapCont, &client.CreateOptions{}) switch { case err != nil && kuberrs.IsAlreadyExists(err): log.Debugf("volumesnapshotcontent %s already exists in cluster", snapCont.Name) @@ -303,36 +236,45 @@ func (c *backupSyncController) run() { log.WithError(errors.WithStack(err)).Errorf("Error syncing volumesnapshotcontent %s into cluster", snapCont.Name) continue default: - log.Infof("Created CSI volumesnapshotcontent %s", created.Name) + log.Infof("Created CSI volumesnapshotcontent %s", snapCont.Name) } } } } - c.deleteOrphanedBackups(location.Name, backupStoreBackups, log) + b.deleteOrphanedBackups(ctx, 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 := c.kbClient.Status().Patch(context.Background(), &location, statusPatch); err != nil { + if err := b.Client.Status().Patch(ctx, &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 (c *backupSyncController) deleteOrphanedBackups(locationName string, backupStoreBackups sets.String, log logrus.FieldLogger) { +func (b *BackupSyncReconciler) deleteOrphanedBackups(ctx context.Context, locationName string, backupStoreBackups sets.String, log logrus.FieldLogger) { locationSelector := labels.Set(map[string]string{ velerov1api.StorageLocationLabel: label.GetValidName(locationName), }).AsSelector() - backups, err := c.backupLister.Backups(c.namespace).List(locationSelector) + backups, err := b.BackupLister.Backups(b.Namespace).List(locationSelector) if err != nil { log.WithError(errors.WithStack(err)).Error("Error listing backups from cluster") return } + if len(backups) == 0 { return } @@ -343,10 +285,31 @@ func (c *backupSyncController) deleteOrphanedBackups(locationName string, backup continue } - if err := c.backupClient.Backups(backup.Namespace).Delete(context.TODO(), backup.Name, metav1.DeleteOptions{}); err != nil { + if err := b.BackupClient.Backups(backup.Namespace).Delete(ctx, 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 0b615bdb5..43274f70e 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -18,17 +18,23 @@ package controller import ( "context" - "testing" + "fmt" + "reflect" "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" @@ -107,263 +113,279 @@ func defaultLocationsListWithLongerLocationName(namespace string) []*velerov1api } } -func TestBackupSyncControllerRun(t *testing.T) { - type cloudBackupData struct { - backup *velerov1api.Backup - podVolumeBackups []*velerov1api.PodVolumeBackup +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 } - 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(), - }, - }, - } + return len(existingK8SBackups.Items), nil +} - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { +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 { 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 { - require.NoError(t, fakeClient.Create(context.Background(), location)) + Expect(r.Client.Create(ctx, location)).ShouldNot(HaveOccurred()) backupStores[location.Name] = &persistencemocks.BackupStore{} } for _, location := range test.locations { backupStore, ok := backupStores[location.Name] - require.True(t, ok, "no mock backup store for location %s", location.Name) + Expect(ok).To(BeTrue(), "no mock backup store for location %s", location.Name) var backupNames []string for _, bucket := range test.cloudBuckets[location.Spec.ObjectStorage.Bucket] { @@ -375,21 +397,26 @@ func TestBackupSyncControllerRun(t *testing.T) { } for _, existingBackup := range test.existingBackups { - require.NoError(t, sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(existingBackup)) + Expect(sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(existingBackup)).ShouldNot(HaveOccurred()) _, err := client.VeleroV1().Backups(test.namespace).Create(context.TODO(), existingBackup, metav1.CreateOptions{}) - require.NoError(t, err) + Expect(err).ShouldNot(HaveOccurred()) } for _, existingPodVolumeBackup := range test.existingPodVolumeBackups { - require.NoError(t, sharedInformers.Velero().V1().PodVolumeBackups().Informer().GetStore().Add(existingPodVolumeBackup)) + Expect(sharedInformers.Velero().V1().PodVolumeBackups().Informer().GetStore().Add(existingPodVolumeBackup)).ShouldNot(HaveOccurred()) _, err := client.VeleroV1().PodVolumeBackups(test.namespace).Create(context.TODO(), existingPodVolumeBackup, metav1.CreateOptions{}) - require.NoError(t, err) + Expect(err).ShouldNot(HaveOccurred()) } client.ClearActions() - c.run() + 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()) for bucket, backupDataSet := range test.cloudBuckets { // figure out which location this bucket is for; we need this for verification @@ -401,12 +428,12 @@ func TestBackupSyncControllerRun(t *testing.T) { break } } - require.NotNil(t, location) + Expect(location).NotTo(BeNil()) // process the cloud backups for _, cloudBackupData := range backupDataSet { obj, err := client.VeleroV1().Backups(test.namespace).Get(context.TODO(), cloudBackupData.backup.Name, metav1.GetOptions{}) - require.NoError(t, err) + Expect(err).To(BeNil()) // did this cloud backup already exist in the cluster? var existing *velerov1api.Backup @@ -425,23 +452,23 @@ func TestBackupSyncControllerRun(t *testing.T) { expected := existing.DeepCopy() expected.Spec.StorageLocation = location.Name - assert.Equal(t, expected, obj) + Expect(expected).To(BeEquivalentTo(obj)) } else { // verify that the storage location field and label are set properly - assert.Equal(t, location.Name, obj.Spec.StorageLocation) + Expect(location.Name).To(BeEquivalentTo(obj.Spec.StorageLocation)) locationName := location.Name if test.longLocationNameEnabled { locationName = label.GetValidName(locationName) } - assert.Equal(t, locationName, obj.Labels[velerov1api.StorageLocationLabel]) - assert.Equal(t, true, len(obj.Labels[velerov1api.StorageLocationLabel]) <= validation.DNS1035LabelMaxLength) + Expect(locationName).To(BeEquivalentTo(obj.Labels[velerov1api.StorageLocationLabel])) + Expect(len(obj.Labels[velerov1api.StorageLocationLabel]) <= validation.DNS1035LabelMaxLength).To(BeTrue()) } // 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{}) - require.NoError(t, err) + Expect(err).ShouldNot(HaveOccurred()) // did this cloud pod volume backup already exist in the cluster? var existingPodVolumeBackup *velerov1api.PodVolumeBackup @@ -456,134 +483,160 @@ func TestBackupSyncControllerRun(t *testing.T) { // 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() - assert.Equal(t, expected, objPodVolumeBackup) + Expect(expected).To(BeEquivalentTo(objPodVolumeBackup)) } } } } - }) - } -} + } + }) -func TestDeleteOrphanedBackups(t *testing.T) { - baseBuilder := func(name string) *builder.BackupBuilder { - return builder.ForBackup("ns-1", name).ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "default")) - } + It("Test deleting orphaned backups.", func() { + 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: "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(), + baseBuilder := func(name string) *builder.BackupBuilder { + return builder.ForBackup("ns-1", name).ObjectMeta(builder.WithLabels(velerov1api.StorageLocationLabel, "default")) + } - 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(), + 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"), }, - expectedDeletes: sets.NewString("backup-C"), - }, - } + { + 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(), - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { + 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 { 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 - "", - nil, // new plugin manager func - nil, // backupStoreGetter - velerotest.NewLogger(), - ).(*backupSyncController) + 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(), + } 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") + Expect(sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(backup)).ShouldNot(HaveOccurred()) // 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") + Expect(err).ShouldNot(HaveOccurred()) // if we expect this backup to be deleted, set up the expected DeleteAction if test.expectedDeletes.Has(backup.Name) { @@ -596,139 +649,49 @@ func TestDeleteOrphanedBackups(t *testing.T) { } } - c.deleteOrphanedBackups("default", 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 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) - } + bslName := "default" + if test.useLongBSLName { + bslName = longLabelName } + r.deleteOrphanedBackups(ctx, bslName, test.cloudBackups, velerotest.NewLogger()) - c.deleteOrphanedBackups(longLabelName, test.cloudBackups, velerotest.NewLogger()) - - numBackups, err := numBackups(t, client, c.namespace) - assert.NoError(t, err) + numBackups, err := numBackups(client, r.Namespace) + Expect(err).ShouldNot(HaveOccurred()) expected := len(test.k8sBackups) - len(test.expectedDeletes) - assert.Equal(t, expected, numBackups) + Expect(expected).To(BeEquivalentTo(numBackups)) - velerotest.CompareActions(t, expectedDeleteActions, getDeleteActions(client.Actions())) - }) - } -} + compareActions(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) +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) } } - 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 + 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) + } } - - 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 }