feat: data protection support gc controller to delete expired backups

apis/dataprotection/v1alpha1/backup_types.go

@@ -26,10 +26,12 @@ type BackupSpec struct {
 	// Which backupPolicy is applied to perform this backup.
 	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:Pattern:=`^[a-z0-9]([a-z0-9\.\-]*[a-z0-9])?$`
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="forbidden to update spec.backupPolicyName"
 	BackupPolicyName string `json:"backupPolicyName"`
 
 	// backupMethod specifies the backup method name that is defined in backupPolicy.
 	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="forbidden to update spec.backupMethod"
 	BackupMethod string `json:"backupMethod"`
 
 	// deletionPolicy determines whether the backup contents stored in backup repository
@@ -43,22 +45,23 @@ type BackupSpec struct {
 	DeletionPolicy BackupDeletionPolicy `json:"deletionPolicy,omitempty"`
 
 	// retentionPeriod determines a duration up to which the backup should be kept.
-	// controller will remove all backups that are older than the RetentionPeriod.
+	// Controller will remove all backups that are older than the RetentionPeriod.
 	// For example, RetentionPeriod of `30d` will keep only the backups of last 30 days.
 	// Sample duration format:
 	// - years: 	2y
 	// - months: 	6mo
 	// - days: 		30d
 	// - hours: 	12h
 	// - minutes: 	30m
-	// You can also combine the above durations. For example: 30d12h30m
-	// +kubebuilder:default="7d"
+	// You can also combine the above durations. For example: 30d12h30m.
+	// If not set, the backup will be kept forever.
 	// +optional
 	RetentionPeriod RetentionPeriod `json:"retentionPeriod,omitempty"`
 
 	// parentBackupName determines the parent backup name for incremental or
 	// differential backup.
 	// +optional
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="forbidden to update spec.parentBackupName"
 	ParentBackupName string `json:"parentBackupName,omitempty"`
 }
 

cmd/dataprotection/main.go

@@ -50,6 +50,7 @@ import (
 	dpcontrollers "github.com/apecloud/kubeblocks/controllers/dataprotection"
 	"github.com/apecloud/kubeblocks/internal/constant"
 	intctrlutil "github.com/apecloud/kubeblocks/internal/controllerutil"
+	dptypes "github.com/apecloud/kubeblocks/internal/dataprotection/types"
 	viper "github.com/apecloud/kubeblocks/internal/viperx"
 )
 
@@ -97,6 +98,7 @@ func init() {
 	viper.SetDefault("KUBEBLOCKS_SERVICEACCOUNT_NAME", "kubeblocks")
 	viper.SetDefault(constant.CfgKeyCtrlrMgrNS, "default")
 	viper.SetDefault(constant.KubernetesClusterDomainEnv, constant.DefaultDNSDomain)
+	viper.SetDefault(dptypes.CfgKeyGCFrequencySeconds, dptypes.DefaultGCFrequencySeconds)
 }
 
 func main() {
@@ -260,6 +262,11 @@ func main() {
 		os.Exit(1)
 	}
 
+	if err = dpcontrollers.NewGCReconciler(mgr).SetupWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create controller", "controller", "GarbageCollection")
+		os.Exit(1)
+	}
+
 	// +kubebuilder:scaffold:builder
 
 	if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {

config/crd/bases/dataprotection.kubeblocks.io_backups.yaml

@@ -69,10 +69,16 @@ spec:
                 description: backupMethod specifies the backup method name that is
                   defined in backupPolicy.
                 type: string
+                x-kubernetes-validations:
+                - message: forbidden to update spec.backupMethod
+                  rule: self == oldSelf
               backupPolicyName:
                 description: Which backupPolicy is applied to perform this backup.
                 pattern: ^[a-z0-9]([a-z0-9\.\-]*[a-z0-9])?$
                 type: string
+                x-kubernetes-validations:
+                - message: forbidden to update spec.backupPolicyName
+                  rule: self == oldSelf
               deletionPolicy:
                 allOf:
                 - enum:
@@ -93,15 +99,17 @@ spec:
                 description: parentBackupName determines the parent backup name for
                   incremental or differential backup.
                 type: string
+                x-kubernetes-validations:
+                - message: forbidden to update spec.parentBackupName
+                  rule: self == oldSelf
               retentionPeriod:
-                default: 7d
                 description: "retentionPeriod determines a duration up to which the
-                  backup should be kept. controller will remove all backups that are
+                  backup should be kept. Controller will remove all backups that are
                   older than the RetentionPeriod. For example, RetentionPeriod of
                   `30d` will keep only the backups of last 30 days. Sample duration
                   format: - years: \t2y - months: \t6mo - days: \t\t30d - hours: \t12h
                   - minutes: \t30m You can also combine the above durations. For example:
-                  30d12h30m"
+                  30d12h30m. If not set, the backup will be kept forever."
                 type: string
             required:
             - backupMethod

controllers/apps/cluster_controller_test.go

@@ -388,7 +388,10 @@ var _ = Describe("Cluster Controller", func() {
 		backup := testdp.NewBackupFactory(testCtx.DefaultNamespace, backupName).
 			SetBackupPolicyName(backupPolicyName).
 			SetBackupMethod(backupMethod).
-			SetLabels(map[string]string{constant.AppInstanceLabelKey: clusterKey.Name, constant.BackupProtectionLabelKey: constant.BackupRetain}).
+			SetLabels(map[string]string{
+				constant.AppInstanceLabelKey:      clusterKey.Name,
+				constant.BackupProtectionLabelKey: constant.BackupRetain,
+			}).
 			WithRandomName().
 			Create(&testCtx).GetObject()
 		backupKey := client.ObjectKeyFromObject(backup)

controllers/dataprotection/backup_controller.go

@@ -352,7 +352,7 @@ func (r *BackupReconciler) patchBackupStatus(
 	if err != nil {
 		return fmt.Errorf("failed to parse retention period %s, %v", original.Spec.RetentionPeriod, err)
 	}
-	if original.Spec.RetentionPeriod != "" {
+	if duration.Seconds() > 0 {
 		request.Status.Expiration = &metav1.Time{
 			Time: request.Status.StartTimestamp.Add(duration),
 		}

controllers/dataprotection/backupschedule_controller.go

@@ -22,7 +22,6 @@ package dataprotection
 import (
 	"context"
 	"reflect"
-	"strings"
 	"time"
 
 	batchv1 "k8s.io/api/batch/v1"
@@ -84,12 +83,6 @@ func (r *BackupScheduleReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return *res, err
 	}
 
-	// try to remove expired or oldest backups, triggered by cronjob controller
-	// TODO(ldm): another garbage collection controller to remove expired backups
-	if err = r.removeExpiredBackups(reqCtx); err != nil {
-		return r.patchStatusFailed(reqCtx, backupSchedule, "RemoveExpiredBackupsFailed", err)
-	}
-
 	if err = r.handleSchedule(reqCtx, backupSchedule); err != nil {
 		return r.patchStatusFailed(reqCtx, backupSchedule, "HandleBackupScheduleFailed", err)
 	}
@@ -197,34 +190,6 @@ func (r *BackupScheduleReconciler) patchStatusFailed(reqCtx intctrlutil.RequestC
 	return intctrlutil.RequeueWithError(err, reqCtx.Log, "")
 }
 
-func (r *BackupScheduleReconciler) removeExpiredBackups(reqCtx intctrlutil.RequestCtx) error {
-	backups := dpv1alpha1.BackupList{}
-	if err := r.Client.List(reqCtx.Ctx, &backups,
-		client.InNamespace(reqCtx.Req.Namespace)); err != nil {
-		return err
-	}
-
-	now := metav1.Now()
-	for _, item := range backups.Items {
-		// ignore retained backup.
-		if strings.EqualFold(item.GetLabels()[constant.BackupProtectionLabelKey], constant.BackupRetain) {
-			continue
-		}
-
-		// ignore backup which is not expired.
-		if item.Status.Expiration == nil || !item.Status.Expiration.Before(&now) {
-			continue
-		}
-
-		// delete expired backup.
-		if err := intctrlutil.BackgroundDeleteObject(r.Client, reqCtx.Ctx, &item); err != nil {
-			// failed delete backups, return error info.
-			return err
-		}
-	}
-	return nil
-}
-
 // handleSchedule handles backup schedules for different backup method.
 func (r *BackupScheduleReconciler) handleSchedule(
 	reqCtx intctrlutil.RequestCtx,

controllers/dataprotection/backupschedule_controller_test.go

@@ -21,12 +21,11 @@ package dataprotection
 
 import (
 	"fmt"
-	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
 	batchv1 "k8s.io/api/batch/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	dpv1alpha1 "github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1"
@@ -92,13 +91,6 @@ var _ = Describe("Backup Schedule Controller", func() {
 			}
 		}
 
-		getJobKey := func(backup *dpv1alpha1.Backup) client.ObjectKey {
-			return client.ObjectKey{
-				Name:      dpbackup.GenerateBackupJobName(backup, dpbackup.BackupDataJobNamePrefix),
-				Namespace: backup.Namespace,
-			}
-		}
-
 		BeforeEach(func() {
 			By("creating an actionSet")
 			actionSet := testdp.NewFakeActionSet(&testCtx)
@@ -118,7 +110,6 @@ var _ = Describe("Backup Schedule Controller", func() {
 
 		Context("creates a backup schedule", func() {
 			var (
-				backupNamePrefix  = "schedule-test-backup-"
 				backupSchedule    *dpv1alpha1.BackupSchedule
 				backupScheduleKey client.ObjectKey
 			)
@@ -152,86 +143,6 @@ var _ = Describe("Backup Schedule Controller", func() {
 					g.Expect(*fetched.Spec.StartingDeadlineSeconds).To(Equal(getStartingDeadlineSeconds(backupSchedule)))
 				})).Should(Succeed())
 			})
-
-			It("delete expired backups", func() {
-				now := metav1.Now()
-				backupStatus := dpv1alpha1.BackupStatus{
-					Phase:               dpv1alpha1.BackupPhaseCompleted,
-					Expiration:          &now,
-					StartTimestamp:      &now,
-					CompletionTimestamp: &now,
-				}
-
-				autoBackupLabel := map[string]string{
-					dataProtectionLabelAutoBackupKey:   "true",
-					dataProtectionLabelBackupPolicyKey: testdp.BackupPolicyName,
-					dataProtectionLabelBackupMethodKey: testdp.BackupMethodName,
-				}
-
-				createBackup := func(name string) *dpv1alpha1.Backup {
-					return testdp.NewBackupFactory(testCtx.DefaultNamespace, name).
-						WithRandomName().AddLabelsInMap(autoBackupLabel).
-						SetBackupPolicyName(testdp.BackupPolicyName).
-						SetBackupMethod(testdp.BackupMethodName).
-						Create(&testCtx).GetObject()
-				}
-
-				checkBackupCompleted := func(key client.ObjectKey) {
-					Eventually(testapps.CheckObj(&testCtx, key,
-						func(g Gomega, fetched *dpv1alpha1.Backup) {
-							g.Expect(fetched.Status.Phase).To(Equal(dpv1alpha1.BackupPhaseCompleted))
-						})).Should(Succeed())
-				}
-
-				By("create an expired backup")
-				backupExpired := createBackup(backupNamePrefix + "expired")
-
-				By("create 1st backup")
-				backupOutLimit1 := createBackup(backupNamePrefix + "1")
-
-				By("create 2nd backup")
-				backupOutLimit2 := createBackup(backupNamePrefix + "2")
-
-				By("waiting expired backup completed")
-				expiredKey := client.ObjectKeyFromObject(backupExpired)
-				testdp.PatchK8sJobStatus(&testCtx, getJobKey(backupExpired), batchv1.JobComplete)
-				checkBackupCompleted(expiredKey)
-
-				By("mock update expired backup status to expire")
-				backupStatus.Expiration = &metav1.Time{Time: now.Add(-time.Hour * 24)}
-				backupStatus.StartTimestamp = backupStatus.Expiration
-				testdp.PatchBackupStatus(&testCtx, client.ObjectKeyFromObject(backupExpired), backupStatus)
-
-				By("waiting 1st backup completed")
-				outLimit1Key := client.ObjectKeyFromObject(backupOutLimit1)
-				testdp.PatchK8sJobStatus(&testCtx, getJobKey(backupOutLimit1), batchv1.JobComplete)
-				checkBackupCompleted(outLimit1Key)
-
-				By("mock 1st backup not to expire")
-				backupStatus.Expiration = &metav1.Time{Time: now.Add(time.Hour * 24)}
-				backupStatus.StartTimestamp = &metav1.Time{Time: now.Add(time.Hour)}
-				testdp.PatchBackupStatus(&testCtx, client.ObjectKeyFromObject(backupOutLimit1), backupStatus)
-
-				By("waiting 2nd backup completed")
-				outLimit2Key := client.ObjectKeyFromObject(backupOutLimit2)
-				testdp.PatchK8sJobStatus(&testCtx, getJobKey(backupOutLimit2), batchv1.JobComplete)
-				checkBackupCompleted(outLimit2Key)
-
-				By("mock 2nd backup not to expire")
-				backupStatus.Expiration = &metav1.Time{Time: now.Add(time.Hour * 24)}
-				backupStatus.StartTimestamp = &metav1.Time{Time: now.Add(time.Hour * 2)}
-				testdp.PatchBackupStatus(&testCtx, client.ObjectKeyFromObject(backupOutLimit2), backupStatus)
-
-				By("patch backup schedule to trigger the controller to delete expired backup")
-				Eventually(testapps.GetAndChangeObj(&testCtx, backupScheduleKey, func(fetched *dpv1alpha1.BackupSchedule) {
-					fetched.Spec.Schedules[0].RetentionPeriod = "1d"
-				})).Should(Succeed())
-
-				By("retain the latest backup")
-				Eventually(testapps.List(&testCtx, generics.BackupSignature,
-					client.MatchingLabels(autoBackupLabel),
-					client.InNamespace(backupPolicy.Namespace))).Should(HaveLen(2))
-			})
 		})
 
 		Context("creates a backup schedule with empty schedule", func() {