feat(backup): modify setting and uninstall controllers

Remove adding `backup-target` back when node is updated. Remove backup targets when uninstalling. ref: longhorn/longhorn 5411 Signed-off-by: James Lu <james.lu@suse.com>
c3y1huang · Dec 16, 2024 · 913ec89 · 913ec89
1 parent c83b0d4
commit 913ec89
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 23 deletions.
diff --git a/controller/setting_controller.go b/controller/setting_controller.go
@@ -1275,14 +1275,6 @@ func (sc *SettingController) enqueueSettingForNode(obj interface{}) {
 	}
 
 	sc.queue.Add(sc.namespace + "/" + string(types.SettingNameGuaranteedInstanceManagerCPU))
-	sc.queue.Add(sc.namespace + "/" + string(types.SettingNameBackupTarget))
-}
-
-func (sc *SettingController) enqueueSettingForBackupTarget(obj interface{}) {
-	if _, ok := obj.(*longhorn.BackupTarget); !ok {
-		return
-	}
-	sc.queue.Add(sc.namespace + "/" + string(types.SettingNameBackupTarget))
 }
 
 // updateInstanceManagerCPURequest deletes all instance manager pods immediately with the updated CPU request.

diff --git a/controller/uninstall_controller.go b/controller/uninstall_controller.go
@@ -523,19 +523,12 @@ func (c *UninstallController) deleteCRs() (bool, error) {
 	// Unset backup target to prevent the remote backup target
 	// backup volume config, and backup config and it's data
 	// being deleted during uninstall process.
-	// TODO:
-	//   Remove the setting CRs and add OwnerReferences on BackupTarget CR.
-	//   After that when deleting setting CRs, the default BackupTarget CR
-	//   be cascading deleted automatically.
-	targetSetting, err := c.ds.GetSetting(types.SettingNameBackupTarget)
-	if err != nil {
+	// Delete the BackupTarget CRs
+	if backupTargets, err := c.ds.ListBackupTargets(); err != nil {
 		return true, err
-	}
-	if targetSetting.Value != "" {
-		targetSetting.Value = ""
-		if _, err := c.ds.UpdateSetting(targetSetting); err != nil {
-			return true, err
-		}
+	} else if len(backupTargets) > 0 {
+		c.logger.Infof("Found %d backuptargets remaining", len(backupTargets))
+		return true, c.deleteBackupTargets(backupTargets)
 	}
 
 	// Waits the BackupVolume CRs be clean up by backup_target_controller
@@ -808,9 +801,16 @@ func (c *UninstallController) deleteBackupTargets(backupTargets map[string]*long
 			bt.Annotations = make(map[string]string)
 		}
 		if bt.DeletionTimestamp == nil {
-			bt.Annotations[types.GetLonghornLabelKey(types.DeleteBackupTargetFromLonghorn)] = ""
-			if _, err := c.ds.UpdateBackupTarget(bt); err != nil {
-				return errors.Wrap(err, "failed to update backup target annotations to mark for deletion")
+			if isVolumeUpdateRequired(bt) {
+				// Annotations `DeleteBackupTargetFromLonghorn` is used for validator to delete default backup target only by Longhorn during uninstalling.
+				bt.Annotations[types.GetLonghornLabelKey(types.DeleteBackupTargetFromLonghorn)] = ""
+				// Clear the BackupTargetURL to prevent the data on the remote backup target from being unintentionally deleted.
+				bt.Spec.BackupTargetURL = ""
+				log.Info("Cleanup BackupTarget URL and add annotation to mark for deletion")
+				if _, err := c.ds.UpdateBackupTarget(bt); err != nil {
+					return errors.Wrap(err, "failed to update backup target annotations to mark for deletion")
+				}
+				continue
 			}
 			if errDelete := c.ds.DeleteBackupTarget(bt.Name); errDelete != nil {
 				if datastore.ErrorIsNotFound(errDelete) {
@@ -827,6 +827,11 @@ func (c *UninstallController) deleteBackupTargets(backupTargets map[string]*long
 	return
 }
 
+func isVolumeUpdateRequired(bt *longhorn.BackupTarget) bool {
+	_, ok := bt.Annotations[types.GetLonghornLabelKey(types.DeleteBackupTargetFromLonghorn)]
+	return bt.Spec.BackupTargetURL != "" || !ok
+}
+
 func (c *UninstallController) deleteEngineImages(engineImages map[string]*longhorn.EngineImage) (err error) {
 	defer func() {
 		err = errors.Wrapf(err, "failed to delete engine images")