From b32190b0b593b060400ae3367dac0744e5de57ae Mon Sep 17 00:00:00 2001 From: James Lu Date: Wed, 9 Oct 2024 14:30:15 +0800 Subject: [PATCH] fix(backup): only delete backups' resources when synchronization Only delete backups' resources (BackupVolume, BackupBackingImage, SystemBackup, and Backup) when synchronization. Add the label `DeleteCustomResourceOnly`, and delete remote backup data when deleting the corresponding backup resource in the cluster and the label does not exist. ref: longhorn/longhorn 9530 Signed-off-by: James Lu --- controller/backup_backing_image_controller.go | 6 +- controller/backup_controller.go | 8 +- controller/backup_target_controller.go | 29 ++++- controller/backup_volume_controller.go | 10 +- controller/system_backup_controller.go | 14 ++- controller/system_backup_controller_test.go | 2 +- controller/utils.go | 19 ++- datastore/longhorn.go | 115 ++++++++++++++++++ types/types.go | 2 + webhook/resources/backupvolume/mutator.go | 31 +++++ 10 files changed, 221 insertions(+), 15 deletions(-) diff --git a/controller/backup_backing_image_controller.go b/controller/backup_backing_image_controller.go index c8cc99db07..2af5aabca8 100644 --- a/controller/backup_backing_image_controller.go +++ b/controller/backup_backing_image_controller.go @@ -214,7 +214,11 @@ func (bc *BackupBackingImageController) reconcile(backupBackingImageName string) // Examine DeletionTimestamp to determine if object is under deletion if !bbi.DeletionTimestamp.IsZero() { - if backupTarget.Spec.BackupTargetURL != "" { + needsCleanupRemoteData, err := checkIfRemoteDataCleanupIsNeeded(bbi, backupTarget) + if err != nil { + return errors.Wrap(err, "failed to check if it needs to delete remote backup backing image data") + } + if needsCleanupRemoteData { backupTargetClient, err := newBackupTargetClientFromDefaultEngineImage(bc.ds, backupTarget) if err != nil { log.WithError(err).Warn("Failed to init backup target clients") diff --git a/controller/backup_controller.go b/controller/backup_controller.go index 04c0c7bd09..84fd526482 100644 --- a/controller/backup_controller.go +++ b/controller/backup_controller.go @@ -280,8 +280,12 @@ func (bc *BackupController) reconcile(backupName string) (err error) { return err } - if backupTarget.Spec.BackupTargetURL != "" && - backupVolume != nil && backupVolume.DeletionTimestamp == nil { + needsCleanupRemoteData, err := checkIfRemoteDataCleanupIsNeeded(backup, backupTarget) + if err != nil { + return errors.Wrap(err, "failed to check if it needs to delete remote backup data") + } + + if needsCleanupRemoteData && backupVolume != nil && backupVolume.DeletionTimestamp == nil { backupTargetClient, err := newBackupTargetClientFromDefaultEngineImage(bc.ds, backupTarget) if err != nil { log.WithError(err).Warn("Failed to init backup target clients") diff --git a/controller/backup_target_controller.go b/controller/backup_target_controller.go index b1adc8c9d6..03789b55e0 100644 --- a/controller/backup_target_controller.go +++ b/controller/backup_target_controller.go @@ -506,9 +506,12 @@ func (btc *BackupTargetController) syncBackupVolume(backupStoreBackupVolumeNames log.Infof("Found %d backup volumes in the backup target that do not exist in the cluster and need to be deleted from the cluster", count) } for backupVolumeName := range backupVolumesToDelete { - log.WithField("backupVolume", backupVolumeName).Info("Deleting backup volume from cluster") + log.WithField("backupVolume", backupVolumeName).Info("Deleting BackupVolume not exist in backupstore") + if err = datastore.AddBackupVolumeDeleteCustomResourceOnlyLabel(btc.ds, backupVolumeName); err != nil { + return errors.Wrapf(err, "failed to add label delete-custom-resource-only to Backupvolume %s", backupVolumeName) + } if err = btc.ds.DeleteBackupVolume(backupVolumeName); err != nil { - return errors.Wrapf(err, "failed to delete backup volume %s from cluster", backupVolumeName) + return errors.Wrapf(err, "failed to delete BackupVolume %s not exist in backupstore", backupVolumeName) } } @@ -561,9 +564,12 @@ func (btc *BackupTargetController) syncBackupBackingImage(backupStoreBackingImag log.Infof("Found %d backup backing images in the cluster that do not exist in the backup target and need to be deleted", count) } for backupBackingImageName := range backupBackingImagesToDelete { - log.WithField("backupBackingImage", backupBackingImageName).Info("Deleting backup backing image from cluster") + log.WithField("backupBackingImage", backupBackingImageName).Info("Deleting BackupBackingImage not exist in backupstore") + if err = datastore.AddBackupBackingImageDeleteCustomResourceOnlyLabel(btc.ds, backupBackingImageName); err != nil { + return errors.Wrapf(err, "failed to add label delete-custom-resource-only to BackupBackingImage %s", backupBackingImageName) + } if err = btc.ds.DeleteBackupBackingImage(backupBackingImageName); err != nil { - return errors.Wrapf(err, "failed to delete backup backing image %s from cluster", backupBackingImageName) + return errors.Wrapf(err, "failed to delete BackupBackingImage %s not exist in backupstore", backupBackingImageName) } } @@ -617,6 +623,9 @@ func (btc *BackupTargetController) syncSystemBackup(backupStoreSystemBackups sys delSystemBackupsInCluster := clusterReadySystemBackupNames.Difference(backupstoreSystemBackupNames) for name := range delSystemBackupsInCluster { log.WithField("systemBackup", name).Info("Deleting SystemBackup not exist in backupstore") + if err = datastore.AddSystemBackupDeleteCustomResourceOnlyLabel(btc.ds, name); err != nil { + return errors.Wrapf(err, "failed to add label delete-custom-resource-only to SystemBackup %v", name) + } if err = btc.ds.DeleteSystemBackup(name); err != nil { return errors.Wrapf(err, "failed to delete SystemBackup %v not exist in backupstore", name) } @@ -666,6 +675,10 @@ func (btc *BackupTargetController) cleanupBackupVolumes() error { var errs []string for backupVolumeName := range clusterBackupVolumes { + if err = datastore.AddBackupVolumeDeleteCustomResourceOnlyLabel(btc.ds, backupVolumeName); err != nil { + errs = append(errs, err.Error()) + continue + } if err = btc.ds.DeleteBackupVolume(backupVolumeName); err != nil && !apierrors.IsNotFound(err) { errs = append(errs, err.Error()) continue @@ -686,6 +699,10 @@ func (btc *BackupTargetController) cleanupBackupBackingImages() error { var errs []string for backupBackingImageName := range clusterBackupBackingImages { + if err = datastore.AddBackupBackingImageDeleteCustomResourceOnlyLabel(btc.ds, backupBackingImageName); err != nil { + errs = append(errs, err.Error()) + continue + } if err = btc.ds.DeleteBackupBackingImage(backupBackingImageName); err != nil && !apierrors.IsNotFound(err) { errs = append(errs, err.Error()) continue @@ -706,6 +723,10 @@ func (btc *BackupTargetController) cleanupSystemBackups() error { var errs []string for systemBackup := range systemBackups { + if err = datastore.AddSystemBackupDeleteCustomResourceOnlyLabel(btc.ds, systemBackup); err != nil { + errs = append(errs, err.Error()) + continue + } if err = btc.ds.DeleteSystemBackup(systemBackup); err != nil && !apierrors.IsNotFound(err) { errs = append(errs, err.Error()) continue diff --git a/controller/backup_volume_controller.go b/controller/backup_volume_controller.go index cf64b39152..c668d94957 100644 --- a/controller/backup_volume_controller.go +++ b/controller/backup_volume_controller.go @@ -223,13 +223,16 @@ func (bvc *BackupVolumeController) reconcile(backupVolumeName string) (err error // Examine DeletionTimestamp to determine if object is under deletion if !backupVolume.DeletionTimestamp.IsZero() { - if err := bvc.ds.DeleteAllBackupsForBackupVolume(backupVolumeName); err != nil { return errors.Wrap(err, "failed to delete backups") } + needsCleanupRemoteData, err := checkIfRemoteDataCleanupIsNeeded(backupVolume, backupTarget) + if err != nil { + return errors.Wrap(err, "failed to check if it needs to delete remote backup volume data") + } // Delete the backup volume from the remote backup target - if backupTarget.Spec.BackupTargetURL != "" { + if needsCleanupRemoteData { engineClientProxy, backupTargetClient, err := getBackupTarget(bvc.controllerID, backupTarget, bvc.ds, log, bvc.proxyConnCounter) if err != nil || engineClientProxy == nil { log.WithError(err).Error("Failed to init backup target clients") @@ -354,6 +357,9 @@ func (bvc *BackupVolumeController) reconcile(backupVolumeName string) (err error log.Infof("Found %d backups in the backup target that do not exist in the cluster and need to be deleted from the cluster", count) } for backupName := range backupsToDelete { + if err = datastore.AddBackupDeleteCustomResourceOnlyLabel(bvc.ds, backupName); err != nil { + return errors.Wrapf(err, "failed to add label delete-custom-resource-only to backup %s", backupName) + } if err = bvc.ds.DeleteBackup(backupName); err != nil { return errors.Wrapf(err, "failed to delete backup %s from cluster", backupName) } diff --git a/controller/system_backup_controller.go b/controller/system_backup_controller.go index 3b6245088f..77d4816116 100644 --- a/controller/system_backup_controller.go +++ b/controller/system_backup_controller.go @@ -275,7 +275,7 @@ func (c *SystemBackupController) syncSystemBackup(key string) (err error) { return err } - return c.reconcile(name, backupTargetClient) + return c.reconcile(name, backupTargetClient, backupTarget) } func getLoggerForSystemBackup(logger logrus.FieldLogger, systemBackup *longhorn.SystemBackup) logrus.FieldLogger { @@ -318,7 +318,7 @@ func (c *SystemBackupController) updateSystemBackupRecord(record *systemBackupRe record.message = message } -func (c *SystemBackupController) reconcile(name string, backupTargetClient engineapi.SystemBackupOperationInterface) (err error) { +func (c *SystemBackupController) reconcile(name string, backupTargetClient engineapi.SystemBackupOperationInterface, backupTarget *longhorn.BackupTarget) (err error) { systemBackup, err := c.ds.GetSystemBackup(name) if err != nil { if !apierrors.IsNotFound(err) { @@ -457,7 +457,7 @@ func (c *SystemBackupController) reconcile(name string, backupTargetClient engin cleanupLocalSystemBackupFiles(tempBackupArchivePath, tempBackupDir, log) case longhorn.SystemBackupStateDeleting: - cleanupRemoteSystemBackupFiles(systemBackup, backupTargetClient, log) + cleanupRemoteSystemBackupFiles(systemBackup, backupTargetClient, backupTarget, log) cleanupLocalSystemBackupFiles(tempBackupArchivePath, tempBackupDir, log) @@ -592,11 +592,17 @@ func (c *SystemBackupController) UploadSystemBackup(systemBackup *longhorn.Syste } } -func cleanupRemoteSystemBackupFiles(systemBackup *longhorn.SystemBackup, backupTargetClient engineapi.SystemBackupOperationInterface, log logrus.FieldLogger) { +func cleanupRemoteSystemBackupFiles(systemBackup *longhorn.SystemBackup, backupTargetClient engineapi.SystemBackupOperationInterface, backupTarget *longhorn.BackupTarget, log logrus.FieldLogger) { if systemBackup.Status.Version == "" { // The backup store sync might not have finished return } + if needsCleanupRemoteData, err := checkIfRemoteDataCleanupIsNeeded(systemBackup, backupTarget); err != nil { + log.WithError(err).Warn("failed to check if it needs to delete remote system backup data") + return + } else if !needsCleanupRemoteData { + return + } systemBackupsFromBackupTarget, err := backupTargetClient.ListSystemBackup() if err != nil { diff --git a/controller/system_backup_controller_test.go b/controller/system_backup_controller_test.go index f7a0fa72ba..7d8e39118a 100644 --- a/controller/system_backup_controller_test.go +++ b/controller/system_backup_controller_test.go @@ -278,7 +278,7 @@ func (s *TestSuite) TestReconcileSystemBackup(c *C) { systemBackupController.UploadSystemBackup(systemBackup, archievePath, tempDir, backupTargetClient) default: - err = systemBackupController.reconcile(tc.systemBackupName, backupTargetClient) + err = systemBackupController.reconcile(tc.systemBackupName, backupTargetClient, nil) if tc.expectError { c.Assert(err, NotNil) } else { diff --git a/controller/utils.go b/controller/utils.go index 0535d5ff9f..1c27ce1f64 100644 --- a/controller/utils.go +++ b/controller/utils.go @@ -1,10 +1,15 @@ package controller import ( - "github.com/longhorn/longhorn-manager/types" "github.com/sirupsen/logrus" + + "k8s.io/apimachinery/pkg/runtime" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "github.com/longhorn/longhorn-manager/datastore" + "github.com/longhorn/longhorn-manager/types" + longhorn "github.com/longhorn/longhorn-manager/k8s/pkg/apis/longhorn/v1beta2" ) @@ -90,3 +95,15 @@ func isRegularRWXVolume(v *longhorn.Volume) bool { } return v.Spec.AccessMode == longhorn.AccessModeReadWriteMany && !v.Spec.Migratable } + +func checkIfRemoteDataCleanupIsNeeded(obj runtime.Object, bt *longhorn.BackupTarget) (bool, error) { + if obj == nil || bt == nil { + return false, nil + } + exists, err := datastore.IsLabelLonghornDeleteCustomResourceOnlyExisting(obj) + if err != nil { + return false, err + } + + return !exists && bt.Spec.BackupTargetURL != "", nil +} diff --git a/datastore/longhorn.go b/datastore/longhorn.go index e18d67a985..9799d3b571 100644 --- a/datastore/longhorn.go +++ b/datastore/longhorn.go @@ -3255,6 +3255,121 @@ func labelBackupVolume(backupVolumeName string, obj k8sruntime.Object) error { return nil } +// labelLonghornDeleteCustomResourceOnly labels the object with the label `longhorn.io/delete-custom-resource-only: true` +func labelLonghornDeleteCustomResourceOnly(obj k8sruntime.Object) error { + metadata, err := meta.Accessor(obj) + if err != nil { + return err + } + + labels := metadata.GetLabels() + if labels == nil { + labels = map[string]string{} + } + labels[types.GetLonghornLabelKey(types.DeleteCustomResourceOnly)] = "true" + metadata.SetLabels(labels) + return nil +} + +// IsLabelLonghornDeleteCustomResourceOnlyExisting check if the object label `longhorn.io/delete-custom-resource-only` exists +func IsLabelLonghornDeleteCustomResourceOnlyExisting(obj k8sruntime.Object) (bool, error) { + metadata, err := meta.Accessor(obj) + if err != nil { + return false, err + } + + labels := metadata.GetLabels() + if labels == nil { + return false, nil + } + _, ok := labels[types.GetLonghornLabelKey(types.DeleteCustomResourceOnly)] + return ok, nil +} + +// AddBackupVolumeDeleteCustomResourceOnlyLabel adds the label `longhorn.io/delete-custom-resource-only: true` to the BackupVolume +func AddBackupVolumeDeleteCustomResourceOnlyLabel(ds *DataStore, backupVolumeName string) error { + bv, err := ds.GetBackupVolume(backupVolumeName) + if err != nil { + return err + } + if exists, err := IsLabelLonghornDeleteCustomResourceOnlyExisting(bv); err != nil { + return err + } else if exists { + return nil + } + if err := labelLonghornDeleteCustomResourceOnly(bv); err != nil { + return err + } + if _, err = ds.UpdateBackupVolume(bv); err != nil { + return err + } + + return nil +} + +// AddBackupDeleteCustomResourceOnlyLabel adds the label `longhorn.io/delete-custom-resource-only: true` to the Backup +func AddBackupDeleteCustomResourceOnlyLabel(ds *DataStore, backupName string) error { + backup, err := ds.GetBackup(backupName) + if err != nil { + return err + } + if exists, err := IsLabelLonghornDeleteCustomResourceOnlyExisting(backup); err != nil { + return err + } else if exists { + return nil + } + if err := labelLonghornDeleteCustomResourceOnly(backup); err != nil { + return err + } + if _, err = ds.UpdateBackup(backup); err != nil { + return err + } + + return nil +} + +// AddBackupBackingImageDeleteCustomResourceOnlyLabel adds the label `longhorn.io/delete-custom-resource-only: true` to the BackupBackingImage +func AddBackupBackingImageDeleteCustomResourceOnlyLabel(ds *DataStore, backupBackingImageName string) error { + bbi, err := ds.GetBackupBackingImage(backupBackingImageName) + if err != nil { + return err + } + if exists, err := IsLabelLonghornDeleteCustomResourceOnlyExisting(bbi); err != nil { + return err + } else if exists { + return nil + } + if err := labelLonghornDeleteCustomResourceOnly(bbi); err != nil { + return err + } + if _, err = ds.UpdateBackupBackingImage(bbi); err != nil { + return err + } + + return nil +} + +// AddSystemBackupDeleteCustomResourceOnlyLabel adds the label `longhorn.io/delete-custom-resource-only: true` to the SystemBackup +func AddSystemBackupDeleteCustomResourceOnlyLabel(ds *DataStore, systemBackupName string) error { + sb, err := ds.GetSystemBackup(systemBackupName) + if err != nil { + return err + } + if exists, err := IsLabelLonghornDeleteCustomResourceOnlyExisting(sb); err != nil { + return err + } else if exists { + return nil + } + if err := labelLonghornDeleteCustomResourceOnly(sb); err != nil { + return err + } + if _, err = ds.UpdateSystemBackup(sb); err != nil { + return err + } + + return nil +} + func FixupRecurringJob(v *longhorn.Volume) error { if err := labelRecurringJobDefault(v); err != nil { return err diff --git a/types/types.go b/types/types.go index 8acd372c58..c8178c07e0 100644 --- a/types/types.go +++ b/types/types.go @@ -128,6 +128,8 @@ const ( ConfigMapResourceVersionKey = "configmap-resource-version" UpdateSettingFromLonghorn = "update-setting-from-longhorn" + DeleteCustomResourceOnly = "delete-custom-resource-only" + KubernetesStatusLabel = "KubernetesStatus" KubernetesReplicaSet = "ReplicaSet" KubernetesStatefulSet = "StatefulSet" diff --git a/webhook/resources/backupvolume/mutator.go b/webhook/resources/backupvolume/mutator.go index 489d89d79a..e54152adfa 100644 --- a/webhook/resources/backupvolume/mutator.go +++ b/webhook/resources/backupvolume/mutator.go @@ -45,9 +45,40 @@ func (b *backupVolumeMutator) Create(request *admission.Request, newObj runtime. } func (b *backupVolumeMutator) Update(request *admission.Request, oldObj runtime.Object, newObj runtime.Object) (admission.PatchOps, error) { + backupVolume, ok := newObj.(*longhorn.BackupVolume) + if !ok { + return nil, werror.NewInvalidError(fmt.Sprintf("%v is not a *longhorn.BackupVolume", newObj), "") + } + + deleteCustomResourceOnlyLabelExists, err := datastore.IsLabelLonghornDeleteCustomResourceOnlyExisting(backupVolume) + if err != nil { + err := errors.Wrap(err, "failed to check if the label longhorn.io/delete-custom-resource-only exists") + return nil, werror.NewInvalidError(err.Error(), "") + } + if deleteCustomResourceOnlyLabelExists { + if err := b.addBackupsDeleteCustomResourceLabel(backupVolume); err != nil { + err := errors.Wrapf(err, "failed to add the label longhorn.io/delete-custom-resource-only to backups of the backup volume %v", backupVolume.Name) + return nil, werror.NewInvalidError(err.Error(), "") + } + } + return mutate(newObj) } +func (b *backupVolumeMutator) addBackupsDeleteCustomResourceLabel(bv *longhorn.BackupVolume) error { + backups, err := b.ds.ListBackupsWithBackupVolumeName(bv.Name) + if err != nil { + return errors.Wrap(err, "failed to list backups of the backup volume") + } + for _, backup := range backups { + if err = datastore.AddBackupDeleteCustomResourceOnlyLabel(b.ds, backup.Name); err != nil { + return errors.Wrapf(err, "failed to add the label longhorn.io/delete-custom-resource-only to backup %s", backup.Name) + } + } + + return nil +} + // mutate contains functionality shared by Create and Update. func mutate(newObj runtime.Object) (admission.PatchOps, error) { backupVolume, ok := newObj.(*longhorn.BackupVolume)