Online DDL: support migration cut-over backoff and forced cut-over #14546

shlomi-noach · 2023-11-19T10:14:29Z

Description

This PR implements two functionalities relating to schema migration cut-over, relevant to ALTER TABLE in vitess strategy only. Both related to a scenario where the cut-over times out due to either excessive load on the migrated table, or due to some lock being placed on the table. The two functionalities are:

A backoff mechanism: if a cut-over fails, the next attempt is done not before 1min has passed; the next one not before additional 5min have passed, the next is 10min, 30min and from that point cut-overs are only attempted at 30min intervals. This is to avoid a scenario where the database, that is already is under heavy load, needs to cope with frequently recurring cut-over attempts, which themselves put additional locks on tables.
The complementing functionality: a force cut-over mechanism where, one the executer begins cut-over, it brutally kills any queries using the migrated table, as well as terminating any transactions that are holding locks on the table (by terminating their connections). This has high likelyhood to pave the way for the cut-over to succeed.

Backoff

The backoff mechanism is implemented as-is, and is not configurable.

Force cut-over

Forced cut-over can be controlled in these ways.

--force-cut-over-after DDL strategy flag

Example:

vtctldclient ApplySchema --ddl-strategy="vitess --force-cut-over-after=1h" --sql "alter table corder force" commerce

It's possible to preconfigure the maximum duration where we allow cut-overs to fail/timeout due to pending queries/transactions. --force-cut-over-after, if nonzero, applies starting the first cut-over attempt.

In the above example, --force-cut-over-after is set to 1 hour. The migration may run for as long as it needs, say 5 hours. Starting the first cut-over attempt, the clock starts ticking 1 hour. The cut-over may be successful, in which case all's well and nothing further happens. Or it may fail, in which case the backoff mechanism kicks in. The next attempt is done within 1m, then 5m, etc. But if these all keep failing, then 1h since the very first failed attempt, irrespective of backoff, and within a 1min resolution, the scheduler runs a cut-over with query&transaction termination. This is highly likely to succeed. But if it fails, then it continues to attempt forced cut-overs every minute.

`ALTER VITESS_MIGRATION ... FORCE_CUTOVER ...`

We introduce a new syntax:

ALTER VITESS_MIGRATION '9748c3b7_7fdb_11eb_ac2c_f875a4d24e90' FORCE_CUTOVER;
ALTER VITESS_MIGRATION FORCE_CUTOVER ALL;

The former forces cut-over for a specific migration, the latter for all pending migrations (queued, ready, running). All these command do is set the new schema_migrations.force_cutover column value to 1, much like ALTER VITESS_MIGRATION ... COMPLETE ....

The scheduler picks up this force_cutover column value on its next review of running migrations. If it's 1, then any backoff state is ignored, and the scheduler attempts a forced cut-over, terminating queries and transactions.

vtctldclient OnlineDDL force-cutover

These matching options are added to vtctldclient OnlineDDL command:

vtctldclient OnlineDDL force-cutover commerce 3ced2e5b_86b3_11ee_acea_0a43f95f28a3
vtctldclient OnlineDDL force-cutover commerce all

Notes

Only works on MySQL 8.0 (requires performance_schema.data_locks table. 5.7 does not provide reliable information).

To note the obvious, a forced cut-over is only relevant when the migration is actually eligible for cut-over. If the migration is incomplete, then it won't attempt a cut-over. Issuing a ALTER VITESS_MIGRATION ... FORCE_CUTOVER ... will set the column to 1, but it will only become relevant when the migration becomes ready to cut-over. Also, if the migration runs with --postpone-completion, then it will not be eligible to cut-over ; the user will first need to issue a ALTER VITESS_MIGRATION ... COMPLETE .... It's fine if the user runs a ALTER VITESS_MIGRATION ... FORCE_CUTOVER ... first, but this will only have the effect of setting force_cutover=1; it will not COMPLETE the migration.

Added unit and endtoend tests.

Documentation: vitessio/website#1641.

Related Issue(s)

Feature Request: Online DDL cut-over backoff + forced completion #14530

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: Shlomi Noach <[email protected]>

…irst invocation Signed-off-by: Shlomi Noach <[email protected]>

…actions holding a lock on a table Signed-off-by: Shlomi Noach <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]>

…illQueriesOnTable: kill queries on a table, and kill connections with transactions holding locks on table Signed-off-by: Shlomi Noach <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]>

…r and Online DDL executor Signed-off-by: Shlomi Noach <[email protected]>

…and the effect on 'force_cutover' column' Signed-off-by: Shlomi Noach <[email protected]>

…ransaction holding lock Signed-off-by: Shlomi Noach <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]>

…t, ForceCutOverSchemaMigrationResponse Signed-off-by: Shlomi Noach <[email protected]>

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach · 2023-11-20T06:57:34Z

Documentation PR: vitessio/website#1641

Signed-off-by: Shlomi Noach <[email protected]>

…tess into onlineddl-cutover-backoff Signed-off-by: Shlomi Noach <[email protected]>

mattlord

Looks good! My only major concern is that we have no tests for the KILL portion, which is particularly sensitive. Am I missing them? It would be ideally covered by unit tests with a lot of test cases with mock mysql results. But we probably don't have the framework in place for that so it may be difficult. Let me know what you think.

go/cmd/vtctldclient/command/onlineddl.go

mattlord · 2023-11-29T15:42:57Z

go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go

+		require.NotNil(t, rs)
+		for _, row := range rs.Named().Rows {
+			message := row.AsString("message", "")
+			if strings.Contains(message, messageSubstring) {


Any reason not to make it case insensitive?

I wonder why we should? If the test knows what to expect, then it should expect the exact string.

go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go

go/vt/schema/ddl_strategy_test.go

mattlord · 2023-11-29T16:16:22Z

go/vt/vtctl/grpcvtctldserver/server.go

@@ -673,6 +673,37 @@ func (s *VtctldServer) CleanupSchemaMigration(ctx context.Context, req *vtctldat
 	return resp, nil
 }

+// ForceCutOverSchemaMigration is part of the vtctlservicepb.VtctldServer interface.
+func (s *VtctldServer) ForceCutOverSchemaMigration(ctx context.Context, req *vtctldatapb.ForceCutOverSchemaMigrationRequest) (resp *vtctldatapb.ForceCutOverSchemaMigrationResponse, err error) {
+	span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigrationResponse")


This should just be the function name, not the response part.

I don't follow, can you please explain?

The trace span name should be the function name:

span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigration")

This is the last thing and it's minor. So I will approve and you can change this whenever you like.

go/vt/vttablet/onlineddl/executor.go

mattlord · 2023-11-29T16:27:30Z

go/vt/vttablet/onlineddl/executor.go

@@ -765,8 +766,93 @@ func (e *Executor) terminateVReplMigration(ctx context.Context, uuid string) err
 	return nil
 }

+func (e *Executor) killQueriesOnTable(ctx context.Context, tableName string) error {


This isn't really covered by any tests, is it? This feels like the most critical aspect as we're killing things off in the production system.

Things like this are actually easier in unit tests as you can mock the mysql query responses.

This is covered in https://github.com/vitessio/vitess/pull/14546/files#diff-e1236744a601a269891381b0ed09b7151d86f5a9b001d8c4e954211c2ae04a8d ; there's a test that holds an open transaction, we attempt a completion, the migration does not complete; we then force cut-over, and we see that the migration completes.

go/vt/vttablet/onlineddl/executor.go

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach · 2023-11-30T06:36:24Z

My only major concern is that we have no tests for the KILL portion, which is particularly sensitive. Am I missing them?

This is covered in https://github.com/vitessio/vitess/pull/14546/files#diff-e1236744a601a269891381b0ed09b7151d86f5a9b001d8c4e954211c2ae04a8d ; there's a test that holds an open transaction, we attempt a completion, the migration does not complete; we then force cut-over, and we see that the migration completes.

Signed-off-by: Shlomi Noach <[email protected]>

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

ajm188 · 2023-11-30T13:46:15Z

go/vt/vtctl/grpcvtctldserver/server.go

@@ -673,6 +673,37 @@ func (s *VtctldServer) CleanupSchemaMigration(ctx context.Context, req *vtctldat
 	return resp, nil
 }

+// ForceCutOverSchemaMigration is part of the vtctlservicepb.VtctldServer interface.
+func (s *VtctldServer) ForceCutOverSchemaMigration(ctx context.Context, req *vtctldatapb.ForceCutOverSchemaMigrationRequest) (resp *vtctldatapb.ForceCutOverSchemaMigrationResponse, err error) {
+	span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigrationResponse")


@mattlord is saying this, essentially

Suggested change

span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigrationResponse")

span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigration")

ajm188 · 2023-11-30T13:46:49Z

go/vt/vtctl/grpcvtctldserver/server.go

+		return nil, err
+	}
+
+	log.Info("Calling ApplySchema to force cut-over migration")


should we include the UUID in this log?

ajm188 · 2023-11-30T13:48:17Z

proto/vtctlservice.proto

@@ -66,6 +66,8 @@ service Vtctld {
  rpc CleanupSchemaMigration(vtctldata.CleanupSchemaMigrationRequest) returns (vtctldata.CleanupSchemaMigrationResponse) {};
  // CompleteSchemaMigration completes one or all migrations executed with --postpone-completion.
  rpc CompleteSchemaMigration(vtctldata.CompleteSchemaMigrationRequest) returns (vtctldata.CompleteSchemaMigrationResponse) {};
+  // ForceCutOverSchemaMigration marks a schema migration for forced cut-over.
+  rpc ForceCutOverSchemaMigration(vtctldata.ForceCutOverSchemaMigrationRequest) returns (vtctldata.ForceCutOverSchemaMigrationResponse) {};


nit: move this down below FindAllShardsInKeyspace (to keep them tidy)?

ajm188 · 2023-11-30T13:48:30Z

proto/vtctldata.proto

@@ -459,6 +458,15 @@ message CompleteSchemaMigrationResponse {
  map<string, uint64> rows_affected_by_shard = 1;
 }

+message ForceCutOverSchemaMigrationRequest {


nit: move these down below FindAllShardsInKeyspaceRequest/Response (to keep them tidy)?

Signed-off-by: Shlomi Noach <[email protected]>

…itessio#14546) Signed-off-by: Shlomi Noach <[email protected]> Co-authored-by: Matt Lord <[email protected]>

shlomi-noach added 21 commits October 24, 2023 11:15

Online DDL: backoff for vitess cut-over intervals

57667a7

Signed-off-by: Shlomi Noach <[email protected]>

resolve conflict

906e7fc

Signed-off-by: Shlomi Noach <[email protected]>

refactor reviewVReplRunningMigration()

100eb76

Signed-off-by: Shlomi Noach <[email protected]>

DDL Strategy: support --force-cut-over-after=<duration> flag

0c92e95

Signed-off-by: Shlomi Noach <[email protected]>

schema_migrations: support force_cutover column

08c83b3

Signed-off-by: Shlomi Noach <[email protected]>

Updte ready_to_complete: only update ready_to_complete_timestamp on f…

cb4bcf0

…irst invocation Signed-off-by: Shlomi Noach <[email protected]>

detect (heuristic) queries running on a given table, and detect trans…

abf999f

…actions holding a lock on a table Signed-off-by: Shlomi Noach <[email protected]>

validate --force-cut-over-after

bd2868b

Signed-off-by: Shlomi Noach <[email protected]>

shouldCutOverAccordingToBackoff(): backoff and force-cutover logic; k…

c3f3ccc

…illQueriesOnTable: kill queries on a table, and kill connections with transactions holding locks on table Signed-off-by: Shlomi Noach <[email protected]>

sqlparser: support ALTER VITESS_MIGRATION '...' FORCE_CUTOVER

8e7d8ac

Signed-off-by: Shlomi Noach <[email protected]>

Implement ALTER VITESS_MIGRATION '...' FORCE_CUTOVER in query executo…

c821fc3

…r and Online DDL executor Signed-off-by: Shlomi Noach <[email protected]>

endtoend: test 'ALTER VITESS_MIGRATION ... FORCE_CUTOVER' statement, …

a0d980f

…and the effect on 'force_cutover' column' Signed-off-by: Shlomi Noach <[email protected]>

endtoend: validate 'ALTER VITESS_MIGRATION ... FORCE_CUTOVER' kills t…

26d7809

…ransaction holding lock Signed-off-by: Shlomi Noach <[email protected]>

FORCE_CUTOVER triggers scheduler next checks

1c002c5

Signed-off-by: Shlomi Noach <[email protected]>

proto: ForceCutOverSchemaMigration, ForceCutOverSchemaMigrationReques…

9a0a82b

…t, ForceCutOverSchemaMigrationResponse Signed-off-by: Shlomi Noach <[email protected]>

nuance test changes

9eac122

Signed-off-by: Shlomi Noach <[email protected]>

rename function

738cd7b

Signed-off-by: Shlomi Noach <[email protected]>

vtctl gRPC: implement CleanupSchemaMigration

24da375

Signed-off-by: Shlomi Noach <[email protected]>

implement vtctldclient force_cutover

359c6fb

Signed-off-by: Shlomi Noach <[email protected]>

resolved conflict

0589d4a

Signed-off-by: Shlomi Noach <[email protected]>

support 'ALTER VITESS_MIGRATION FORCE_CUTOVER ALL'

804b684

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) labels Nov 19, 2023

shlomi-noach requested review from deepthi, harshit-gangal, mattlord, ajm188, notfelineit, systay and rohit-nayak-ps as code owners November 19, 2023 10:14

shlomi-noach removed the NeedsWebsiteDocsUpdate What it says label Nov 20, 2023

shlomi-noach added 3 commits November 26, 2023 08:11

resolved conflict

7995987

Signed-off-by: Shlomi Noach <[email protected]>

resolved conflict

7ed323c

Signed-off-by: Shlomi Noach <[email protected]>

Merge branch 'onlineddl-cutover-backoff' of github.com:planetscale/vi…

5240429

…tess into onlineddl-cutover-backoff Signed-off-by: Shlomi Noach <[email protected]>

mattlord reviewed Nov 29, 2023

View reviewed changes

shlomi-noach added 4 commits November 30, 2023 08:21

punctuation

baae9f2

Signed-off-by: Shlomi Noach <[email protected]>

validate: --force-cut-over-after is only valid in 'vitess' strategy.

c787863

Signed-off-by: Shlomi Noach <[email protected]>

simplify test expectations

71fb82b

Signed-off-by: Shlomi Noach <[email protected]>

-1 for maxrows

7b7413d

Signed-off-by: Shlomi Noach <[email protected]>

shlomi-noach and others added 5 commits November 30, 2023 08:42

rename as killTableLockHoldersAndAccessors, add comments

7192e96

Signed-off-by: Shlomi Noach <[email protected]>

endtoend: use context with timeout

92aeac1

Signed-off-by: Shlomi Noach <[email protected]>

added unit test

478fcad

Signed-off-by: Shlomi Noach <[email protected]>

OnlineDDL command named force-cutover

9c2929b

Signed-off-by: Shlomi Noach <[email protected]>

Update go/cmd/vtctldclient/command/onlineddl.go

10868b9

Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Shlomi Noach <[email protected]>

ajm188 reviewed Nov 30, 2023

View reviewed changes

mattlord approved these changes Nov 30, 2023

View reviewed changes

shlomi-noach added 5 commits December 4, 2023 07:40

span name

2fd3850

Signed-off-by: Shlomi Noach <[email protected]>

include UUID in log

848cafb

Signed-off-by: Shlomi Noach <[email protected]>

reorder proto alphabetically

849d8ae

Signed-off-by: Shlomi Noach <[email protected]>

resolved conflict

839f772

Signed-off-by: Shlomi Noach <[email protected]>

resolved conflict

892fd23

Signed-off-by: Shlomi Noach <[email protected]>

ajm188 approved these changes Dec 12, 2023

View reviewed changes

shlomi-noach merged commit 3b58bee into vitessio:main Dec 13, 2023
119 checks passed

shlomi-noach deleted the onlineddl-cutover-backoff branch December 13, 2023 06:56

ejortegau pushed a commit to slackhq/vitess that referenced this pull request Dec 13, 2023

Online DDL: support migration cut-over backoff and forced cut-over (v…

bebddba

…itessio#14546) Signed-off-by: Shlomi Noach <[email protected]> Co-authored-by: Matt Lord <[email protected]>

This was referenced Jan 4, 2024

Bug Report: FORCE_CUTOVER introduced in #14546 is identified as a keyword #14884

Closed

sqlparser: FORCE_CUTOVER is a non-reserved keyword #14885

Merged

shlomi-noach mentioned this pull request Jan 15, 2025

Online DDL forced cut-over: terminate transactions holding metadata locks on table #17535

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Online DDL: support migration cut-over backoff and forced cut-over #14546

Online DDL: support migration cut-over backoff and forced cut-over #14546

shlomi-noach commented Nov 19, 2023 •

edited

Loading

shlomi-noach commented Nov 20, 2023

mattlord left a comment

mattlord Nov 29, 2023

shlomi-noach Nov 30, 2023

mattlord Nov 29, 2023

shlomi-noach Nov 30, 2023

mattlord Nov 30, 2023

mattlord Nov 30, 2023

shlomi-noach Dec 4, 2023

mattlord Nov 29, 2023

shlomi-noach Nov 30, 2023

shlomi-noach commented Nov 30, 2023

ajm188 Nov 30, 2023

shlomi-noach Dec 4, 2023

ajm188 Nov 30, 2023

shlomi-noach Dec 4, 2023

ajm188 Nov 30, 2023

shlomi-noach Dec 4, 2023

ajm188 Nov 30, 2023

shlomi-noach Dec 4, 2023

	span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigrationResponse")
	span, ctx := trace.NewSpan(ctx, "VtctldServer.ForceCutOverSchemaMigration")

Online DDL: support migration cut-over backoff and forced cut-over #14546

Online DDL: support migration cut-over backoff and forced cut-over #14546

Conversation

shlomi-noach commented Nov 19, 2023 • edited Loading

Description

Backoff

Force cut-over

--force-cut-over-after DDL strategy flag

ALTER VITESS_MIGRATION ... FORCE_CUTOVER ...

vtctldclient OnlineDDL force-cutover

Notes

Related Issue(s)

Checklist

Deployment Notes

shlomi-noach commented Nov 20, 2023

mattlord 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach commented Nov 30, 2023

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

shlomi-noach commented Nov 19, 2023 •

edited

Loading

`ALTER VITESS_MIGRATION ... FORCE_CUTOVER ...`