Control backup gc by completed and failed bakcups number #4480

just1900 · 2022-03-17T02:59:26Z

Signed-off-by: just1900 [email protected]

What problem does this PR solve?

fix #3660

What is changed and how does it work?

adding MaxCompletedBackups and MaxFailedBackups fields to control backup gc.

Code changes

Has Go code change
Has CI related scripts change

Tests

Unit test
E2E test
Manual test
No code

Side effects

Breaking backward compatibility
Other side effects:

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release Notes

Please refer to Release Notes Language Style Guide before writing the release note.

control backup gc by completed and failed bakcups number

Signed-off-by: just1900 <[email protected]>

ti-chi-bot · 2022-03-17T02:59:36Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

KanShiori
csuzhangxc

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: just1900 <[email protected]>

just1900 · 2022-03-17T03:26:42Z

pkg/backup/backupschedule/backup_schedule_manager.go

 		klog.Infof("backup schedule %s/%s gc backup %s success", ns, bsName, backup.GetName())
 	}
-
-	if deleteCount == len(backupsList) && deleteCount > 0 {


Remove this condition, because only if .spec.maxBackups is 0 will this condition be true, which is impossible.

Why is this impossible?

codecov-commenter · 2022-03-17T04:07:36Z

Codecov Report

Merging #4480 (8a025a8) into master (0d13639) will increase coverage by 9.56%.
The diff coverage is 69.69%.

@@            Coverage Diff             @@
##           master    #4480      +/-   ##
==========================================
+ Coverage   62.79%   72.35%   +9.56%     
==========================================
  Files         184      188       +4     
  Lines       19875    22310    +2435     
==========================================
+ Hits        12480    16142    +3662     
+ Misses       6262     5043    -1219     
+ Partials     1133     1125       -8

Flag	Coverage Δ
e2e	`60.09% <0.00%> (?)`
unittest	`62.80% <71.87%> (+0.01%)`	⬆️

Signed-off-by: just1900 <[email protected]>

just1900 · 2022-03-23T03:59:34Z

seems like a issue exists when backupGCByPolicy and backupGCByMaxBackups concurrently delete backups.

… effect immediately Signed-off-by: just1900 <[email protected]>

pkg/apis/pingcap/v1alpha1/types.go

pkg/backup/backupschedule/backup_schedule_manager_test.go

Signed-off-by: just1900 <[email protected]>

just1900 · 2022-03-30T03:14:00Z

/test pull-e2e-kind-serial

KanShiori · 2022-04-12T02:39:07Z

/test pull-e2e-kind pull-e2e-kind-br pull-e2e-kind-tngm

ti-chi-bot · 2022-05-11T22:19:15Z

@just1900: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

DanielZhangQD · 2022-06-21T09:33:40Z

pkg/apis/pingcap/v1alpha1/types.go

+	// if both MaxCompletedBackups and MaxBackups are set, operator will keep no more than MaxCompletedBackups completed backups,
+	// and no more than MaxBackups backups in total.
+	// +kubebuilder:default:=3
+	// +kubebuilder:validation:Minimum:=1


For the old backupSchedule, if the user does not configure MaxBackups, which means no clean-up for the backups, with this change, after upgrading the TiDB Operator, only 3 completed backups will be retained, and the others will be cleaned, right? If yes, this is unexpected behavior.

DanielZhangQD · 2022-06-21T09:34:11Z

pkg/apis/pingcap/v1alpha1/types.go

+	// and no more than MaxBackups backups in total.
+	// +kubebuilder:default:=1
+	// +optional
+	MaxFailedBackups *int32 `json:"maxFailedBackups,omitempty"`


DanielZhangQD · 2022-06-21T09:38:25Z

pkg/backup/backupschedule/backup_schedule_manager.go

 	// if MaxBackups and MaxReservedTime are set at the same time, MaxReservedTime is preferred.
 	if bs.Spec.MaxReservedTime != nil {
 		bm.backupGCByMaxReservedTime(bs)
 		return
 	}

-	if bs.Spec.MaxBackups != nil && *bs.Spec.MaxBackups > 0 {
+	if (bs.Spec.MaxBackups != nil && *bs.Spec.MaxBackups > 0) || bs.Spec.MaxCompletedBackups != nil || bs.Spec.MaxFailedBackups != nil {


Check the *bs.Spec.MaxCompletedBackups > 0 and *bs.Spec.MaxFailedBackups > 0

DanielZhangQD · 2022-06-21T11:43:39Z

pkg/backup/backupschedule/backup_schedule_manager.go

-		// All backups have been deleted, so the last backup information in the backupSchedule should be reset
-		bm.resetLastBackup(bs)
+		if bs.Spec.MaxBackups != nil && int(*bs.Spec.MaxBackups) > 0 && totalCnt > int(*bs.Spec.MaxBackups) {
+			totalCnt--


Should we also adjust the value of complete count and failed count here?

control backup gc by completed and failed bakcups number

b610000

Signed-off-by: just1900 <[email protected]>

ti-chi-bot requested review from DanielZhangQD and july2993 March 17, 2022 02:59

add validation for maxCompletedBackups

e751206

Signed-off-by: just1900 <[email protected]>

just1900 commented Mar 17, 2022

View reviewed changes

just1900 changed the title ~~WIP: control backup gc by completed and failed bakcups number~~ Control backup gc by completed and failed bakcups number Mar 17, 2022

just1900 changed the title ~~Control backup gc by completed and failed bakcups number~~ WIP: Control backup gc by completed and failed bakcups number Mar 17, 2022

DanielZhangQD requested review from KanShiori and csuzhangxc March 18, 2022 11:34

add uts

b6f2ce9

Signed-off-by: just1900 <[email protected]>

just1900 changed the title ~~WIP: Control backup gc by completed and failed bakcups number~~ Control backup gc by completed and failed bakcups number Mar 23, 2022

just1900 added 3 commits March 23, 2022 10:01

Merge branch 'master' into refine-backup-schedule

74a1b61

fix ut

14e585e

Signed-off-by: just1900 <[email protected]>

remove fields

b5d2f1e

Signed-off-by: just1900 <[email protected]>

just1900 added 2 commits March 23, 2022 17:59

fix the issue that backup wrongly gced because of deletion won't take…

9f70372

… effect immediately Signed-off-by: just1900 <[email protected]>

Merge branch 'master' into refine-backup-schedule

697160b

csuzhangxc reviewed Mar 28, 2022

View reviewed changes

pkg/apis/pingcap/v1alpha1/types.go Show resolved Hide resolved

pkg/apis/pingcap/v1alpha1/types.go Show resolved Hide resolved

pkg/backup/backupschedule/backup_schedule_manager_test.go Outdated Show resolved Hide resolved

add fields description

0c033b0

Signed-off-by: just1900 <[email protected]>

just1900 requested a review from csuzhangxc March 28, 2022 06:48

csuzhangxc approved these changes Mar 28, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 label Mar 28, 2022

Merge branch 'master' into refine-backup-schedule

2f26ace

KanShiori approved these changes Apr 12, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Apr 12, 2022

KanShiori and others added 2 commits April 13, 2022 14:03

Merge branch 'master' into refine-backup-schedule

69c6a21

Merge branch 'master' into refine-backup-schedule

8a025a8

ti-chi-bot added the needs-rebase label May 11, 2022

DanielZhangQD reviewed Jun 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control backup gc by completed and failed bakcups number #4480

Control backup gc by completed and failed bakcups number #4480

just1900 commented Mar 17, 2022 •

edited

Loading

ti-chi-bot commented Mar 17, 2022 •

edited

Loading

just1900 Mar 17, 2022

DanielZhangQD Jun 21, 2022

codecov-commenter commented Mar 17, 2022 •

edited

Loading

just1900 commented Mar 23, 2022

just1900 commented Mar 30, 2022

KanShiori commented Apr 12, 2022

ti-chi-bot commented May 11, 2022

DanielZhangQD Jun 21, 2022

DanielZhangQD Jun 21, 2022

DanielZhangQD Jun 21, 2022

DanielZhangQD Jun 21, 2022

Control backup gc by completed and failed bakcups number #4480

Are you sure you want to change the base?

Control backup gc by completed and failed bakcups number #4480

Conversation

just1900 commented Mar 17, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Code changes

Tests

Side effects

Related changes

Release Notes

ti-chi-bot commented Mar 17, 2022 • edited Loading

just1900 Mar 17, 2022

Choose a reason for hiding this comment

DanielZhangQD Jun 21, 2022

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2022 • edited Loading

Codecov Report

just1900 commented Mar 23, 2022

just1900 commented Mar 30, 2022

KanShiori commented Apr 12, 2022

ti-chi-bot commented May 11, 2022

DanielZhangQD Jun 21, 2022

Choose a reason for hiding this comment

DanielZhangQD Jun 21, 2022

Choose a reason for hiding this comment

DanielZhangQD Jun 21, 2022

Choose a reason for hiding this comment

DanielZhangQD Jun 21, 2022

Choose a reason for hiding this comment

just1900 commented Mar 17, 2022 •

edited

Loading

ti-chi-bot commented Mar 17, 2022 •

edited

Loading

codecov-commenter commented Mar 17, 2022 •

edited

Loading