Concurrent migrations #9192

shlomi-noach · 2021-11-11T13:12:41Z

Description

Ongoing work towards supporting concurrent REVERT migrations.

Up till now, Vitess would only ever run a single migration at a time, for a given keyspace.

This isn't 100% correct, as a tablet failure could end up resurrecting an already vreplication stream.

Anyway, we wish to support the following:

REVERT migrations, which are by nature high priority migrations, should be able to run concurrently to any other migration
We will require an additional flag in ddl_strategy, something like -allow-concurrent or similar
There can be an unlimited number of concurrent REVERT migrations with this flag
But still only one single "normal" migration at a time.

This changes the scheduler logic, which up till now assumed that if any migration was running, then no need to schedule the next one. From now on, it will need to consider these scenarios:

if a migration is queued, it can be made ready if nothing else is running
if a "allowed concurrent" migration is queued, it can be made ready is no other migration operates on the same table
if any migration is queued, it can be made ready if only other running migrations are "allowed concurrent" and none of them operates on same table

This means a more programmatic approach to scheduling next migration (where today some of the logic is a simple SQL)

Related Issue(s)

This PR extends OnlineDDL: implementing -postpone-completion, ALTER VITESS_MIGRATION ... COMPLETE #9171 (unmerged yet)
tracking issue: Online DDL: tracking issue #6926

Checklist

Should this PR be backported?
Tests were added or are not required
Documentation was added or is not required

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

shlomi-noach · 2021-11-14T07:21:41Z

While #9171 is still unmerged, this is the diff for this branch: planetscale/vitess@online-ddl-postpone-completion...planetscale:online-ddl-concurrent-revert-migrations

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

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

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

…ting migrations on table Signed-off-by: Shlomi Noach <[email protected]>

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

…s() before runNextMigration() can actually run a new migration Signed-off-by: Shlomi Noach <[email protected]>

…er than 'queued') Signed-off-by: Shlomi Noach <[email protected]>

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

…e reason this is required is that there could be a migration in 'ready' state, which conflicts with some running migration. But we still want to be able to run _other_ -allow-concurrent migrations that do not conflict with running migrations Signed-off-by: Shlomi Noach <[email protected]>

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

shlomi-noach · 2021-11-15T09:39:53Z

This PR has evolved a bit. We now support concurrent migrations as follows:

A migration needs to opt in to run concurrently via -allow-concurrent flag in ddl_strategy
A migration is scheduled and executed concurrently if:
- It specifies -allow-concurrent
- Is CREATE, or DROP, or REVERT (REVERT works for CREATE, DROP and for online/vrplication ALTER migrations)
- There is no other running migration operating on same table.

With the new scheduler:

There can be as many concurrent migrations running at any given time
And there can be at most one non-concurrent ("regular") migration running at any time
And never two migrations operating on the same table

Also:

There can be multiple migrations in ready state (previously only one)
A migration can be "blocked" from running, if it conflicts with a running migration (operating on same table, or non-concurrent)
While some ready migrations can be blocked, new migrations are still able to proceed to ready state an then to running, i.e. actually execute.

Just some possible scenarios:

If all migrations are non-concurrent, i.e. are regular, then they can only run sequentially.
If a regular migration is running, it's possible for a -allow-concurrent migration to also kick in and run
- and then it's also possible for yet another N -allow-concurrent migrations to kick in and run
If one or more -allow-concurrent migrations are running, it is possible for a regular migration to kick in and run

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

shlomi-noach · 2021-11-21T15:15:19Z

This is now ready to review, given #9171 is merged.

shlomi-noach · 2021-11-22T08:02:54Z

go/vt/vttablet/onlineddl/executor.go

@@ -2179,45 +2235,67 @@ func (e *Executor) executeMigration(ctx context.Context, onlineDDL *schema.Onlin
 	return nil
 }

+// runNextMigration picks one 'ready' migration that is able to run, and executes it.


This is at the heart of the PR: the scheduler here gets to decide whether there's a migration thta can be executed, concurrently or not concurrently, based on existence or non-existence of running migrations.

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

deepthi

Feedback is all on code comments.
May merge after fixing them.

deepthi · 2021-12-09T23:28:27Z

go/vt/vttablet/onlineddl/executor.go

+// isAnyNonConcurrentMigrationRunning sees if there's any migration running right now
+// that does not have -allow-concurrent.
+// such a running migration will for example prevent a new non-concurrent migration from running.


copy-pasta. Need the correct description of this function instead.

deepthi · 2021-12-09T23:29:16Z

go/vt/vttablet/onlineddl/executor.go

@@ -1561,36 +1634,15 @@ func (e *Executor) CancelPendingMigrations(ctx context.Context, message string)
 }

 // scheduleNextMigration attemps to schedule a single migration to run next.
-// possibly there's no migrations to run. Possibly there's a migration running right now,
-// in which cases nothing happens.
+// possibly there's no migrations to run.


nit: there are

deepthi · 2021-12-09T23:29:28Z

go/vt/vttablet/onlineddl/executor.go

+// possibly there's no migrations to run.
+// The effect of this function is to move a migration from 'queued' state to 'ready' state, is all.
+// Notice that the query sqlScheduleSingleMigration embeds some logic inside. We may choose
+// to refactor the logic into the app


hmm.. which app is this referring to?

Clarified, moved explanation into the function itself. This is more a developer's note for future me/us.

deepthi · 2021-12-09T23:31:13Z

go/vt/vttablet/onlineddl/executor.go

+// Possible scenarios:
+// - no migration is in 'ready' state -- nothing to be done
+// - a migration is 'ready', but conflicts with other running migrations -- try another 'ready' migration
+// It is therefore possible that there is a 'ready' migration, and still this function only handles one.


This sentence is confusing. I would have thought that it is possible that there is a "ready" migration and still this function chooses none.

Oh wow the wording was really bad. Changed to:

// Note that per the above breakdown, and due to potential conflicts, it is possible to have one or // more 'ready' migration, and still none is executed.

deepthi · 2021-12-09T23:32:02Z

go/vt/vttablet/onlineddl/executor.go

+		// a vreplication migration from a pre-PRS/ERS that we still need to learn about?
+		// We're going to be careful here, and avoid running new migrations until we have
+		// a better picture. It will likely take a couple seconds till next iteration.
+		// execution. This delay ony takes place shortly after Open().


Suggested change

// execution. This delay ony takes place shortly after Open().

// This delay only takes place shortly after Open().

deepthi · 2021-12-09T23:32:45Z

go/vt/vttablet/onlineddl/executor.go

-			RequestContext: row["migration_context"].ToString(),
+	// getNonConflictingMigration finds a single 'ready' migration which does not conflict with running migrations.
+	// Conflicts are:
+	// - a migration is 'ready' but is not set to run _concurrnetly_, and there's a running migration that is also non-concurrent


Suggested change

// - a migration is 'ready' but is not set to run _concurrnetly_, and there's a running migration that is also non-concurrent

// - a migration is 'ready' but is not set to run _concurrently_, and there's a running migration that is also non-concurrent

deepthi · 2021-12-09T23:33:34Z

go/vt/vttablet/onlineddl/executor.go

-		if i == 0 {
-			break
+		// no non-conflicting migration found...
+		// Either all ready migrations are conflicting, or there's no ready migrations...


there's no ready migration OR there are no ready migrations.

shlomi-noach · 2021-12-12T05:16:53Z

Addressed all comments.

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

shlomi-noach · 2021-12-12T06:09:39Z

Woohoo! That's an important one merged.

reading all ready migrations, running one

cb45d58

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

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) labels Nov 11, 2021

shlomi-noach self-assigned this Nov 11, 2021

shlomi-noach added 2 commits November 11, 2021 15:25

check nil value

db69952

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

select required columns

ea3200f

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

shlomi-noach added 16 commits November 14, 2021 13:08

support -allow-concurrent flag in ddl_strategy

2e979d5

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

onlineddl scheduler allows running concurrent, non-conflicting migrat…

766e764

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

fix column name

451e018

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

ignore UUID of the very reverting migration when checking for conflic…

f80f515

…ting migrations on table Signed-off-by: Shlomi Noach <[email protected]>

better naming

2fa94c0

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

adding endtoend test: onlineddl_scheduler

78de043

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

more testing

eacbc47

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

Wrapping in sub tests

c5a9b8d

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

safeguard: require one successful execution of reviewRunningMigration…

c5b4daf

…s() before runNextMigration() can actually run a new migration Signed-off-by: Shlomi Noach <[email protected]>

safeguard: require one successful execution of reviewRunningMigration…

70995df

…s() before runNextMigration() can actually run a new migration Signed-off-by: Shlomi Noach <[email protected]>

updated singleton tests with new logic (migration can be 'ready' rath…

1d44d80

…er than 'queued') Signed-off-by: Shlomi Noach <[email protected]>

more tests: check blocked migration does not stand in the way of new …

c6ba988

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

code comment

74490dc

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

faster singleton tests

bf5c8c5

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

endtoend: test for empty uuid

0935b7d

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

This was referenced Nov 15, 2021

Document -allow-concurrent in ddl_strategy #9230

Closed

Document OnlineDDL concurrent migrations (scheduler.md) #9231

Closed

unit test for -allow-concurrent

25c9093

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

shlomi-noach changed the title ~~WIP: concurrent REVERT migrations~~ WIP: concurrent migrations Nov 15, 2021

more endtoend checks

8dcfe0d

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

shlomi-noach requested review from deepthi and sougou November 15, 2021 13:10

shlomi-noach changed the title ~~WIP: concurrent migrations~~ Concurrent migrations Nov 16, 2021

Merge branch 'main' into online-ddl-concurrent-revert-migrations

782579f

shlomi-noach marked this pull request as ready for review November 21, 2021 15:09

shlomi-noach requested review from harshit-gangal and systay as code owners November 21, 2021 15:09

merged main; resolved conflict

0143e7d

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

shlomi-noach commented Nov 22, 2021

View reviewed changes

shlomi-noach added 3 commits December 5, 2021 11:17

merged main; resolved conflict

eebdf20

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

merge conflict resolved

d4582a9

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

merged main; resolved conflict

82c2eaf

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

deepthi approved these changes Dec 9, 2021

View reviewed changes

Language, grammar, typos and clarity, per review

9dee14e

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

shlomi-noach merged commit 395ffc4 into vitessio:main Dec 12, 2021

shlomi-noach deleted the online-ddl-concurrent-revert-migrations branch December 12, 2021 06:09

shlomi-noach mentioned this pull request Jun 7, 2022

Concurrent vitess migrations #10410

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Concurrent migrations #9192

Concurrent migrations #9192

shlomi-noach commented Nov 11, 2021 •

edited

Loading

shlomi-noach commented Nov 14, 2021

shlomi-noach commented Nov 15, 2021 •

edited

Loading

shlomi-noach commented Nov 21, 2021

shlomi-noach Nov 22, 2021

deepthi left a comment

deepthi Dec 9, 2021

deepthi Dec 9, 2021

deepthi Dec 9, 2021

shlomi-noach Dec 12, 2021

deepthi Dec 9, 2021

shlomi-noach Dec 12, 2021

deepthi Dec 9, 2021

deepthi Dec 9, 2021

deepthi Dec 9, 2021

shlomi-noach commented Dec 12, 2021

shlomi-noach commented Dec 12, 2021

	// execution. This delay ony takes place shortly after Open().
	// This delay only takes place shortly after Open().

	// - a migration is 'ready' but is not set to run _concurrnetly_, and there's a running migration that is also non-concurrent
	// - a migration is 'ready' but is not set to run _concurrently_, and there's a running migration that is also non-concurrent

Concurrent migrations #9192

Concurrent migrations #9192

Conversation

shlomi-noach commented Nov 11, 2021 • edited Loading

Description

Related Issue(s)

Checklist

shlomi-noach commented Nov 14, 2021

shlomi-noach commented Nov 15, 2021 • edited Loading

shlomi-noach commented Nov 21, 2021

Choose a reason for hiding this comment

deepthi 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 Dec 12, 2021

shlomi-noach commented Dec 12, 2021

shlomi-noach commented Nov 11, 2021 •

edited

Loading

shlomi-noach commented Nov 15, 2021 •

edited

Loading