feat: improve and refactor backup and restore #4917

ldming · 2023-08-30T06:17:23Z

This PR has redesign the backup/restore and its related CRDs, the relevant controllers have also been refactored. So I'd like to apologize to the reviewers that this is too big of a PR. Below I will explain specifically what has been done and what still needs to be done in the future(Not in this PR).

Refactor backup/restore CRD

refactor backup CRD, using backupMethod to replace backupType, add other fileds
split backupSchedule out of backupPolicy, using backupPolicy to configure backup policy that can be used for both manual and automatic backups, and using backupSchedule to configure automatic scheduling strategies
for BackupPolicy, remove datafile and snapshot fileds, use more flexible backupMethod array to specify the backup methods, the backup type is only one attribute of backupMethod
using actionSet to replace backupTool, with actionSet defining the specific actions required for backup and restore, delete fields like logicalCommand and physicalCommand.
refactor backupPolicyTemplate
add restore CRD by @wangyelei that is used to restore a backup
modify all backupPolicyTempldate and backupTool to use new CRD

Refactor controller

split some code from controller to internal/dataprotection
backup controller build some actions to execute and check the actions status
split backupPolicy controller to backupPolicy controller and backupSchedule controller
refactor backupPolicyTemplate transformer build backupPolicy and backupSchedule
support restore controller that prepare the backed up data to a PV, move the restore logic from cluster controller to this controller, the cluster controller only need to build a restore CR and check its status. by @wangyelei

kbcli

kbcli use new CRD

needs to be done in the future PR

add more test cases to improve the coverage
add Error Code for returned error
add Conditions in CR status
use the builder to build the kubernetes object
kbcli supports to show all backup methods
support to backup and restore kubernetes resources
more kbcli work
and so on

In summary, this PR refactored backup and restore, including refactoring the CRD and controller code, but there is still much work remaining going forward.

local env test

TODO:

refactor CRDs, use backupMethods instead of datafile/logfile/snapshot
backup controller
backup schedule controller
backuppolicytemplate
kbcli use new CRD
test cases
cluster controller hscale based on backup
cluster controller restore from backup
modify all existed data protection CR

Signed-off-by: L.Dongming <l.dongming@apecloud.com>

sonarcloud · 2023-09-13T11:08:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
17 Code Smells

No Coverage information
0.1% Duplication

apis/apps/v1alpha1/clusterdefinition_webhook.go

apis/dataprotection/v1alpha1/actionset_types.go

zjx20 · 2023-09-27T09:34:08Z

apis/dataprotection/v1alpha1/backup_types.go

-	// Backup total size.
-	// A string with capacity units in the form of "1Gi", "1Mi", "1Ki".
+	// totalSize is the total size of backed up data size.
+	// A string with capacity units in the format of "1Gi", "1Mi", "1Ki".
 	// +optional
 	TotalSize string `json:"totalSize,omitempty"`


Maybe add another field to record the exact size in bytes, for example totalSizeBytes.

I prefer to use one filed to record the data size, maybe totalSizeBytes is a good choice. And other client can convert it to more human-readable format.

I plan to improve it in subsequent PRs.

apis/dataprotection/v1alpha1/backuppolicy_types.go

controllers/dataprotection/cronjob_controller.go

deploy/apecloud-mysql-cluster/Chart.yaml

deploy/apecloud-mysql-cluster/extension.yaml

deploy/apecloud-mysql/config/mysql8-config.tpl

internal/cli/cmd/cluster/create.go

ldming · 2023-09-28T03:27:03Z

/approve to trigger test

zjx20

There are too many changes and I haven't done my review of the controller logic yet.

zjx20 · 2023-09-28T02:23:18Z

internal/cli/cmd/cluster/dataprotection.go

-	SourceCluster  string     `json:"sourceCluster,omitempty"`
+	RestoreTime             *time.Time `json:"restoreTime,omitempty"`
+	RestoreTimeStr          string     `json:"restoreTimeStr,omitempty"`
+	RestoreManagementPolicy string     `json:"volumeRestorePolicy,omitempty"`


RestoreManagementPolicy is hard to understand, maybe ParallelRestoreVolumes bool.

internal/testutil/dataprotection/restore_factory.go

test/integration/controller_suite_test.go

internal/testutil/apps/common_util.go

internal/testutil/apps/constant.go

internal/dataprotection/types/types.go

internal/dataprotection/restore/builder.go

ldming · 2023-09-28T07:21:58Z

There are too many changes and I haven't done my review of the controller logic yet.

Ok, I will fix review comment in another PR.

zjx20

I have finished my review.

internal/dataprotection/action/action_create_vs.go

zjx20 · 2023-09-28T14:31:57Z

internal/dataprotection/action/types.go

+type ErrorMode string
+
+const (
+	ErrorModeContinue ErrorMode = "Continue"


zjx20 · 2023-09-29T13:39:37Z

controllers/dataprotection/backup_controller.go

-			return intctrlutil.CheckedRequeueWithError(err, reqCtx.Log, "")
-		}
+	if len(targetPods) > 1 {
+		return nil, fmt.Errorf("do not support more than one target pods")


It seems possible to hit this error when PodSelectionStrategyAll is specified in the backup policy.

internal/dataprotection/backup/scheduler.go

internal/dataprotection/restore/utils.go

zjx20 · 2023-10-02T15:59:13Z

internal/dataprotection/restore/manager.go

+	// if reaches full backup, sort the BackupActionSets and return
+	sortBackupSets := func(backupSets []BackupActionSet, reverse bool) []BackupActionSet {
+		sort.Slice(backupSets, func(i, j int) bool {
+			if reverse {


reverse is always false.

zjx20 · 2023-10-02T16:17:18Z

internal/dataprotection/restore/manager.go

+	var (
+		restoreActionCount  int
+		finishedActionCount int
+		exitFailedAction    bool


existFailedAction? According to the Recalculation().

nashtsai · 2023-11-08T11:07:15Z

apis/apps/v1alpha1/opsrequest_types.go

@@ -372,11 +372,9 @@ type BackupSpec struct {
 	// +optional
 	BackupPolicyName string `json:"backupPolicyName"`

-	// Backup Type. datafile or logfile or snapshot. If not set, datafile is the default type.
-	// +kubebuilder:default=datafile
-	// +kubeBuilder:validation:Enum={datafile,logfile,snapshot}


I would expect this an enum string type?

nashtsai · 2023-11-08T11:45:12Z

deploy/apecloud-mysql/templates/actionset.yaml

+metadata:
+  name: xtrabackup-for-apecloud-mysql
+  labels:
+    clusterdefinition.kubeblocks.io/name: apecloud-mysql


using label isn't ideal for hard constraint/bounding with clusterdefinition, what is the reason for this design?

nashtsai · 2023-11-08T11:49:38Z

apis/dataprotection/v1alpha1/actionset_types.go

+	// +kubebuilder:validation:Enum={Full,Incremental,Differential,Continuous}
+	// +kubebuilder:default=Full
+	// +kubebuilder:validation:Required
+	BackupType BackupType `json:"backupType"`


Since you have refactored to a generic name (ActionSet), and I would suggest have this attribute as "type" to accommodate future extensible design? Or otherwise this API would be named as "BackupRestoreActionSet".

Signed-off-by: L.Dongming <l.dongming@apecloud.com> Co-authored-by: wangyelei <yelei.wang@apecloud.com>

apecloud-bot added refactor feature labels Aug 30, 2023

github-actions bot added size/L Denotes a PR that changes 100-499 lines. size/XL Denotes a PR that changes 500-999 lines. size/XXL Denotes a PR that changes 1000+ lines. and removed size/L Denotes a PR that changes 100-499 lines. size/XL Denotes a PR that changes 500-999 lines. labels Aug 30, 2023

ldming added 7 commits September 1, 2023 18:35

feat: refactor and improve bakcup

9521ef5

Signed-off-by: L.Dongming <l.dongming@apecloud.com>

backup policy

7f2413a

fix backup status

d567594

backuppolicy

fd5ceff

backupschedule

cd72df5

actionset

3e9b0ab

backup controller

2cd0d03

ldming force-pushed the feature/refactor-backup branch from b1cd9d5 to 2cd0d03 Compare September 1, 2023 10:36

ldming added 10 commits September 4, 2023 10:09

support action

0bf7a64

backup controller

268b5d2

backup controller

2bb782b

add actions

17be925

test backup controller using mysql xtrabackup

3da10f7

create volume snapshot action

1e45622

inject init container

69a2845

delete unused code

8a73c75

disable continuous backup

e11d90a

backup schedule controller

4a4f269

ldming added 4 commits September 14, 2023 17:50

support scheduler and remove cue

6545bab

fix backup path and env

f6946c9

backup policy template

12b9a74

kbcli and test case use new data protection crd

a9501a0

ldming added the area/data-protection label Sep 27, 2023

make manifests

7ff1d65

zjx20 reviewed Sep 27, 2023

View reviewed changes

ldming added 2 commits September 27, 2023 18:59

Merge branch 'main' into feature/refactor-backup

2673207

fix review comment

026c954

ldming force-pushed the feature/refactor-backup branch from 23b16c4 to f87c975 Compare September 27, 2023 15:14

Merge branch 'main' into feature/refactor-backup

3c6a197

ldming force-pushed the feature/refactor-backup branch from 3d98952 to 3c6a197 Compare September 27, 2023 15:21

ldming and others added 3 commits September 27, 2023 15:24

chore: auto update cli doc changes

bfd1e8d

remove cron job controller

27f02de

rename OrderedReady to Serial

df6693f

nayutah approved these changes Sep 28, 2023

View reviewed changes

apecloud-bot approved these changes Sep 28, 2023

View reviewed changes

ldming merged commit f7c18e0 into main Sep 28, 2023
94 of 102 checks passed

ldming deleted the feature/refactor-backup branch September 28, 2023 03:49

zjx20 reviewed Sep 28, 2023

View reviewed changes

This was referenced Sep 28, 2023

[BUG]full restore failed on GKE for pod CrashLoopBackOff #2924

Closed

[Improvement] support specifying backup and restore image separately in BackupTool #3569

Closed

[Improvement] BackupTool should support passing arguments #4576

Closed

ldming mentioned this pull request Sep 28, 2023

chore: improve the code refactored for backup #5306

Merged

zjx20 reviewed Oct 3, 2023

View reviewed changes

fengluodb mentioned this pull request Oct 13, 2023

[BUG] wrong example about backup operation. #5433

Closed

nashtsai reviewed Nov 8, 2023

View reviewed changes

ldming added a commit that referenced this pull request Nov 21, 2023

feat: improve and refactor backup and restore (#4917)

aebd503

Signed-off-by: L.Dongming <l.dongming@apecloud.com> Co-authored-by: wangyelei <yelei.wang@apecloud.com>

weicao mentioned this pull request Jun 25, 2024

[BUG] no matches for kind "BackupTool" in version "dataprotection.kubeblocks.io/v1alpha1" #7619

Open

shanshanying mentioned this pull request Oct 8, 2024

[Improvement] kubectl create log backup, the failed message is unclear #4486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve and refactor backup and restore #4917

feat: improve and refactor backup and restore #4917

ldming commented Aug 30, 2023 •

edited

Loading

sonarcloud bot commented Sep 13, 2023

zjx20 Sep 27, 2023

ldming Sep 28, 2023

ldming commented Sep 28, 2023

zjx20 left a comment

zjx20 Sep 28, 2023

ldming commented Sep 28, 2023

zjx20 left a comment

zjx20 Sep 28, 2023

zjx20 Sep 29, 2023

zjx20 Oct 2, 2023

zjx20 Oct 2, 2023

nashtsai Nov 8, 2023

nashtsai Nov 8, 2023

nashtsai Nov 8, 2023

feat: improve and refactor backup and restore #4917

feat: improve and refactor backup and restore #4917

Conversation

ldming commented Aug 30, 2023 • edited Loading

Refactor backup/restore CRD

Refactor controller

kbcli

needs to be done in the future PR

local env test

sonarcloud bot commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldming commented Sep 28, 2023

zjx20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldming commented Sep 28, 2023

zjx20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldming commented Aug 30, 2023 •

edited

Loading