Fix restore from backup execution path to use context from caller by rsajwani · Pull Request #12828 · vitessio/vitess

rsajwani · 2023-04-04T15:31:58Z

Description

Mainly this PR is fixing a bug I identified during PR12703. VtctldClient calls 'RestoreFromBackup' with context. The context timeout value is set using action_timeout flag. But During executeRestoreFullBackup and executeRestoreIncrementalBackup, instead of passing the original context, we pass context.Background() to these methods. This results in restore to run in background even if client context get Canceled or Timed-out. To fix this issue we pass on that same context object.

Reference file: https://github.com/vitessio/vitess/blob/main/go/vt/mysqlctl/builtinbackupengine.go

There are few more fixes in this PR

1- Fixed typo in spelling Canceled
2- During restore, if the context get canceled, we report bug to backupHandler instead of rec structure. This can result in restore not throwing any error to its caller.
3- Reporting errors back to client incase of failure during backup or restore.

NOTE

During restore if context get timed-out or canceled, the tablet type remains 'RESTORE' and does not revert back to its original state. The reason is before we start restore, all the existing files first get deleted. Now if restore get aborted in the middle, the MySQL instance won't able to restart with incomplete set of restore files. This result in ChangeTabletType to fail.

Related Issue(s)

closes #12830.

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

vitess-bot · 2023-04-04T15:32:03Z

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani · 2023-04-05T12:37:38Z

 	})
 	require.NoError(t, err)

-	// Set up tm client


I think we don't need this.

rsajwani · 2023-04-05T21:41:42Z

spoke offline with @mattlord. He is suggesting to fix #12830 in this same PR... I will update the PR with additional changes once I research on #12830

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani · 2023-04-11T17:19:46Z

spoke offline with @mattlord. He is suggesting to fix #12830 in this same PR... I will update the PR with additional changes once I research on #12830

done

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

deepthi · 2023-04-13T19:33:32Z

+Prior to v17, this asynchronous process could run indefinitely in the background since it was called using an empty background. In v17 [PR#12830](https://github.com/vitessio/vitess/issues/12830),
+this behavior was changed to use the same context with which the client called the RestoreFromBackup command, which uses action_timeout to wait for any command to finish.
+If you are using VtctldClient to initiate a restore, make sure you provide an appropriate value for action_timeout to give enough time for the restore process to complete.
+Otherwise, the restore will throw an error if the context expires before it completes.


What happens to the tablet state? does it go back to its previous tablet_type?

Restore is tricky. It does not go back to original state and it keeps sentinel file which is indication that last restore failed. Since we cancel in the middle of copy , depends upon where we left it give error during the restore while calling ChangeTabletType()->updateLocked->ts.canServe

part of it is due to fact that during restore we first delete all the files.
logger.Infof("Restore: deleting existing files")
if err := removeExistingFiles(cnf); err != nil {
return err
}
This prevent us from being coming back to original state.

So I verified this again .. in topo we go back to 'REPLICA' state but in VTAdmin (which calls show vitess_tablets) will show RESTORE. Reason being as stated above mysql will be down due to incomplete set of files.

If this is really the behavior then there are two issues.

healthcheck: when the tablet goes back to REPLICA, it should be sending a healthcheck update to vtgate which updates its cache, which is what is used by show vitess_tablets.

We had agreed that if a RESTORE fails the tablet should not go back to REPLICA because there is a chance of it serving stale data.
Can you file a separate issue for this?

for number 2, since restore fail and we stop the MySql , therefore it lost .sock file so we weren't able to connect and do any change to db so it won't serve the data because tabletserver won't function.. the topo is refresh to replica value (therefore vtctlclient give you 'replica') and as I mentioned show vitess_tablet continue to show restore.

bug filed
#12945

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani · 2023-04-15T18:18:37Z

 func (be *BuiltinBackupEngine) executeRestoreIncrementalBackup(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle, bm builtinBackupManifest) error {
 	params.Logger.Infof("Restoring incremental backup to position: %v", bm.Position)
-	createdDir, err := be.restoreFiles(context.Background(), params, bh, bm)
+	createdDir, err := be.restoreFiles(ctx, params, bh, bm)


this is the bug fix.

deepthi · 2023-04-16T20:01:00Z

 		return nil, err
 	}

+	// if we got restore error then after restarting MYSQL we should return back with error


Does the test case validate that mysqld is indeed up after a timed-out restore?

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

shlomi-noach

Looks good, nice catch! There's one change I don't understand, please see inline comment.

shlomi-noach · 2023-04-19T11:38:31Z

+		bgCtx := context.Background()
 		// If anything failed, we should reset the original tablet type
-		if err := tm.tmState.ChangeTabletType(ctx, originalType, DBActionNone); err != nil {
+		if err := tm.tmState.ChangeTabletType(bgCtx, originalType, DBActionNone); err != nil {


I don't understand this change here. It looks like the opposite of the bugfix. Why would we not reuse ctx?

We need to set status back to 'REPLICA' (or whatever prior to backup/restore) in all cases. If we use 'ctx' then incase of it being canceled or timed out we won't able to set the status back to original value (#12701)

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

deepthi · 2023-04-20T22:08:17Z

I'm going to merge this now, but looks like we may need another follow up PR for the tablet type change.

fix error reporting

17d58a9

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

vitess-bot Bot added the NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work label Apr 4, 2023

vitess-bot Bot added the NeedsWebsiteDocsUpdate What it says label Apr 4, 2023

github-actions Bot added this to the v17.0.0 milestone Apr 4, 2023

adding test

194968d

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani commented Apr 4, 2023

View reviewed changes

Comment thread go/vt/mysqlctl/builtinbackupengine.go

rsajwani added Type: Bug Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Backup and Restore and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Apr 5, 2023

rsajwani commented Apr 5, 2023

View reviewed changes

rsajwani marked this pull request as ready for review April 5, 2023 12:39

rsajwani requested review from deepthi and mattlord as code owners April 5, 2023 12:39

rsajwani changed the title ~~Fix error reporting during restore while context concelled.~~ Fix error reporting during restore while context cancelled. Apr 5, 2023

rsajwani requested review from GuptaManan100 and frouioui April 5, 2023 19:55

rsajwani added 5 commits April 6, 2023 14:53

fixing bug vitessio#12830

f89442a

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing some error message

34df24b

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

remove unwanted comments

5590532

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

remove unwanted require in test

084fa58

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fix comments

d0ef154

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani changed the title ~~Fix error reporting during restore while context cancelled.~~ Fix restore from backup execution path to use context from caller Apr 11, 2023

deepthi reviewed Apr 11, 2023

View reviewed changes

Comment thread go/vt/mysqlctl/builtinbackupengine_test.go Outdated

Comment thread go/vt/mysqlctl/builtinbackupengine_test.go Outdated

deepthi added the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Apr 11, 2023

adding release-notes

daef1d3

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Merge remote-tracking branch 'upstream/main' into bugfixInPR12703

d629004

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

deepthi reviewed Apr 13, 2023

View reviewed changes

rsajwani added 2 commits April 14, 2023 15:43

feedback for summary.md

e8379e8

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

run tablet type with background context

a61ccd9

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani requested review from rohit-nayak-ps and shlomi-noach as code owners April 15, 2023 18:12

rsajwani commented Apr 15, 2023

View reviewed changes

deepthi reviewed Apr 16, 2023

View reviewed changes

rsajwani self-assigned this Apr 17, 2023

rsajwani added 2 commits April 18, 2023 18:04

Add check for tablet type

93c59a0

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

fixing typo

b20856e

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

rsajwani removed the release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) label Apr 19, 2023

shlomi-noach approved these changes Apr 19, 2023

View reviewed changes

fix test bug

9cc18c2

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

deepthi approved these changes Apr 20, 2023

View reviewed changes

deepthi merged commit a836318 into vitessio:main Apr 20, 2023

deepthi deleted the bugfixInPR12703 branch April 20, 2023 22:08

Conversation

rsajwani commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

NOTE

Related Issue(s)

Checklist

Deployment Notes

Uh oh!

vitess-bot Bot commented Apr 4, 2023 • edited by deepthi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsajwani commented Apr 5, 2023

Uh oh!

rsajwani commented Apr 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepthi Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsajwani Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepthi commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rsajwani commented Apr 4, 2023 •

edited

Loading

vitess-bot Bot commented Apr 4, 2023 •

edited by deepthi

Loading

deepthi Apr 20, 2023 •

edited

Loading

rsajwani Apr 20, 2023 •

edited

Loading