br: a more straight forward operator interface #5710

RidRisR · 2024-08-15T06:38:30Z

What problem does this PR solve?

Use a more straight forward operator interface to replace the old one.
support pause and resume pitr task

What is changed and how does it work?

Now, the crd to control log backup would loops like

  backupMode: log
  subcommand: log-start/log-stop/log-pause

(If no subcommand is provided, the default value is log-start)

If the task is not created before:
log-start: would start the task
log-pause/log-stop: would not run until start finished

If the task is running:
log-start: would skip and do nothing
log-pause: would pause the task
log-stop:would stop the task

If the task is paused:
log-start: would resume the task
log-pause: would skip and do nothing
log-stop: would stop the task

If the task is stopped:
log-start/log-pause/log-stop: would do nothing

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.

1. Use a more straight forward operator interface to replace the old one.
Now, the crd to control log backup would loops like
`spec:
  backupMode: log
  subcommand: `log-start`/`log-stop`/`log-pause`
`
(If no subcommand is provided, the default value is `log-start`)

2. Support pause and resume pitr task
Now, users could pause and resume pitr task by using `log-pause` and `log-start`

ti-chi-bot · 2024-08-15T06:38:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign charleszheng44 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sre-bot · 2024-08-15T06:38:37Z

All committers have signed the CLA.

codecov-commenter · 2024-08-27T05:27:36Z

Codecov Report

Attention: Patch coverage is 20.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 61.47%. Comparing base (9ef26f8) to head (9419c07).
Report is 16 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #5710    +/-   ##
========================================
  Coverage   61.47%   61.47%            
========================================
  Files         235      235            
  Lines       30653    30825   +172     
========================================
+ Hits        18843    18951   +108     
- Misses       9920     9963    +43     
- Partials     1890     1911    +21

Flag	Coverage Δ
unittest	`61.47% <20.00%> (+<0.01%)`	⬆️

csuzhangxc · 2024-09-02T02:53:26Z

pkg/apis/pingcap/v1alpha1/backup.go

-	if backup.Spec.LogStop {
+	switch backup.Spec.LogSubcommand {
+	case "log-start":
+		if IsLogBackupAlreadyPaused(backup) {
+			return LogResumeCommand
+		}
+		return LogStartCommand
+	case "log-stop":
 		return LogStopCommand
-	}
-	if backup.Spec.LogTruncateUntil != "" {


Is it needed to compatible with the old fields?

No, user shouldn't mix up these two interface to avoid mistakes

So the old existing Backup user MUST update their pipeline to use these new fields, right?

Yes, we don't want handle corner case like:

Spec: logSubcommand: log-start logStop: true ``` BTW, I am fresh to operator. Would there be any problem? I tested updating operator online while a pitr task is running. Seems there wouldn't be any problem.

We need to keep compatible with the old fields, because tidbcloud depends on current implementation. I think we can make old fields have higher priority. For example:

if backup.Spec.LogStop return LogStopCommand if backup.Spec.LogTruncateUntil != "" && backup.Spec.LogTruncateUntil != backup.Status.LogSuccessTruncateUntil return LogTruncateCommand switch backup.Spec.LogSubcommand ... // default command is log start return LogStartCommand

pkg/apis/pingcap/v1alpha1/types.go

WangLe1321 · 2024-09-14T03:42:20Z

pkg/controller/backup/backup_controller.go

@@ -145,6 +145,7 @@ func (c *Controller) sync(key string) error {
 		return err
 	}

+	klog.Infof("Syncing Backup %s/%s", ns, name)


This log should be debug level.

WangLe1321 · 2024-09-14T06:39:38Z

cmd/backup-manager/app/backup/backup.go

+		"resume",
+		fmt.Sprintf("--task-name=%s", backup.Name),
+	}
+	// if bo.CommitTS != "" && bo.CommitTS != "0" {


suggest to remove these comment codes. They seem useless.

WangLe1321 · 2024-09-14T07:13:11Z

pkg/apis/pingcap/v1alpha1/backup.go

-	if backup.Spec.LogStop {
+	switch backup.Spec.LogSubcommand {
+	case "log-start":
+		if IsLogBackupAlreadyPaused(backup) {
+			return LogResumeCommand
+		}
+		return LogStartCommand
+	case "log-stop":
 		return LogStopCommand
-	}
-	if backup.Spec.LogTruncateUntil != "" {


We need to keep compatible with the old fields, because tidbcloud depends on current implementation. I think we can make old fields have higher priority. For example:

if backup.Spec.LogStop return LogStopCommand if backup.Spec.LogTruncateUntil != "" && backup.Spec.LogTruncateUntil != backup.Status.LogSuccessTruncateUntil return LogTruncateCommand switch backup.Spec.LogSubcommand ... // default command is log start return LogStartCommand

pkg/apis/pingcap/v1alpha1/backup.go

WangLe1321 · 2024-09-14T07:27:08Z

pkg/apis/pingcap/v1alpha1/backup.go

+
+func IsBackupPaused(backup *Backup) bool {
+	if backup.Spec.Mode == BackupModeLog {
+		return IsLogBackupSubCommandOntheCondition(backup, BackupPaused)


Seems log backup sub command condition has no BackupPaused status

WangLe1321 · 2024-09-14T07:29:41Z

pkg/controller/backup/backup_controller.go

@@ -230,6 +230,10 @@ func (c *Controller) updateBackup(cur interface{}) {
 		return
 	}

+	if newBackup.Spec.Mode == v1alpha1.BackupModeLog && newBackup.Spec.LogSubcommand == string(v1alpha1.LogStartCommand) && v1alpha1.IsBackupPaused(newBackup) {
+		newBackup.Spec.LogSubcommand = string(v1alpha1.LogResumeCommand)


I don't know why add this hack logic. We shouldn't modify the spec fields even the user inputs wrong value.

WangLe1321 · 2024-09-14T07:33:49Z

pkg/apis/pingcap/v1alpha1/backup.go

+		return IsLogBackupSubCommandOntheCondition(backup, BackupPaused)
+	}
+	_, condition := GetBackupCondition(&backup.Status, BackupPaused)
+	return condition != nil && condition.Status == corev1.ConditionTrue


This only means the log backup is ever paused, not means it is paused. I guess you want to check if the log backup is paused. Only backup.Status.Phase represents the current status.

WangLe1321 · 2024-09-14T07:34:26Z

pkg/apis/pingcap/v1alpha1/backup.go

@@ -392,3 +416,13 @@ func IsLogBackupAlreadyTruncate(backup *Backup) bool {
 func IsLogBackupAlreadyStop(backup *Backup) bool {
 	return backup.Spec.Mode == BackupModeLog && backup.Status.Phase == BackupStopped
 }
+
+// IsLogBackupAlreadyStop return whether log backup has already paused.


Suggested change

// IsLogBackupAlreadyStop return whether log backup has already paused.

// IsLogBackupAlreadyPaused return whether log backup has already paused.

WangLe1321 · 2024-09-14T07:34:35Z

pkg/apis/pingcap/v1alpha1/backup.go

+	return backup.Spec.Mode == BackupModeLog && backup.Status.Phase == BackupPaused
+}
+
+// IsLogBackupAlreadyRestart return whether log backup has already resumed.


Suggested change

// IsLogBackupAlreadyRestart return whether log backup has already resumed.

// IsLogBackupAlreadyRunning return whether log backup has already resumed.

BornChanger · 2024-09-17T17:58:57Z

For the state machine figure, please also add the scenarios where the state change due to gc-ttl. @RidRisR

Co-authored-by: WangLe1321 <[email protected]>

ti-chi-bot bot added first-time-contributor do-not-merge/work-in-progress labels Aug 15, 2024

ti-chi-bot bot requested a review from howardlau1999 August 15, 2024 06:38

ti-chi-bot bot added the size/XXL label Aug 15, 2024

ti-chi-bot bot added the needs-rebase label Aug 28, 2024

RidRisR marked this pull request as ready for review August 29, 2024 09:35

ti-chi-bot bot requested a review from shonge August 29, 2024 09:35

ti-chi-bot bot removed the do-not-merge/work-in-progress label Aug 29, 2024

RidRisR added 11 commits August 30, 2024 05:13

add command

e5fde7c

add more

49e78da

add truncate task

f7ff6ce

add pause(p1)

90fb9c8

task didn't apply

3159c11

done

ce46516

handel skip

745be1e

before bm

4e59630

add resume

30dbcb8

add pause

0c5848d

change states

52532e5

RidRisR force-pushed the subcommand branch from 3b00e5e to 52532e5 Compare August 30, 2024 03:13

RidRisR added 2 commits August 30, 2024 08:51

fix

b8858d2

fix resume failed

bac5fca

ti-chi-bot bot removed the needs-rebase label Aug 30, 2024

RidRisR added 3 commits August 30, 2024 10:40

support k3s for local-up

d2e5ac0

separate irrelevant changes (local-up-operator.sh)

e1cd583

lint

47ad565

RidRisR changed the title ~~Subcommand~~ br: a more straight forward operator interface Aug 30, 2024

BornChanger requested review from csuzhangxc and WangLe1321 and removed request for howardlau1999 and shonge August 30, 2024 10:24

csuzhangxc reviewed Sep 2, 2024

View reviewed changes

add test

cac450c

RidRisR requested a review from csuzhangxc September 3, 2024 09:59

WangLe1321 reviewed Sep 14, 2024

View reviewed changes

RidRisR and others added 2 commits September 19, 2024 17:30

Update pkg/apis/pingcap/v1alpha1/types.go

9635b64

Co-authored-by: WangLe1321 <[email protected]>

Update pkg/apis/pingcap/v1alpha1/backup.go

5910ac2

Co-authored-by: WangLe1321 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: a more straight forward operator interface #5710

br: a more straight forward operator interface #5710

RidRisR commented Aug 15, 2024 •

edited by BornChanger

Loading

ti-chi-bot bot commented Aug 15, 2024

sre-bot commented Aug 15, 2024 •

edited

Loading

codecov-commenter commented Aug 27, 2024

csuzhangxc Sep 2, 2024

RidRisR Sep 2, 2024

csuzhangxc Sep 3, 2024

RidRisR Sep 3, 2024 •

edited

Loading

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

WangLe1321 Sep 14, 2024

BornChanger commented Sep 17, 2024 •

edited

Loading

	// IsLogBackupAlreadyStop return whether log backup has already paused.
	// IsLogBackupAlreadyPaused return whether log backup has already paused.

	// IsLogBackupAlreadyRestart return whether log backup has already resumed.
	// IsLogBackupAlreadyRunning return whether log backup has already resumed.

br: a more straight forward operator interface #5710

Are you sure you want to change the base?

br: a more straight forward operator interface #5710

Conversation

RidRisR commented Aug 15, 2024 • edited by BornChanger 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 bot commented Aug 15, 2024

sre-bot commented Aug 15, 2024 • edited Loading

codecov-commenter commented Aug 27, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RidRisR Sep 3, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BornChanger commented Sep 17, 2024 • edited Loading

RidRisR commented Aug 15, 2024 •

edited by BornChanger

Loading

sre-bot commented Aug 15, 2024 •

edited

Loading

RidRisR Sep 3, 2024 •

edited

Loading

BornChanger commented Sep 17, 2024 •

edited

Loading