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 }