Skip to content

Relax singleton-context constraint for pending non-singleton-context reverts #10114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shlomi-noach merged 7 commits into from
May 3, 2022
Merged

Relax singleton-context constraint for pending non-singleton-context reverts #10114

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4481b48
relax singleton-context constraint with regard REVERT migrations
shlomi-noach Apr 14, 2022
e6ff7c2
fixed logic; added tests
shlomi-noach Apr 20, 2022
676a08c
release notes
shlomi-noach Apr 20, 2022
4eef9d6
long options: double dashes
shlomi-noach May 3, 2022
481e328
double dashes in ddl_strategy
shlomi-noach May 3, 2022
f58a187
no error is returned
shlomi-noach May 3, 2022
ff8d9c1
check error instead of uuid value
shlomi-noach May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_0_summary.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ Example:
vtctlclient ApplySchema -skip_preflight -ddl_strategy='vitess' -sql "alter table my_table add column my_val int not null default 0" commerce
```

### -singleton-context and REVERT migrations
Comment thread
mattlord marked this conversation as resolved.
Outdated

It is now possible to submit a migration with `-singleton-context` strategy flag, while there's a pending (queued or running) `REVERT` migration that does not have a `-singleton-context` flag.

#### Behavior changes

- `vtctl ApplySchema -uuid_list='...'` now rejects a migration if an existing migration has the same UUID but with different `migration_context`.
Expand Down
81 changes: 55 additions & 26 deletions go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ var (
dropStatement = `
DROP TABLE stress_test
`
dropNonexistentTableStatement = `
DROP TABLE IF EXISTS t_non_existent
`
multiDropStatements = `DROP TABLE IF EXISTS t1; DROP TABLE IF EXISTS t2; DROP TABLE IF EXISTS t3;`
)

Expand Down Expand Up @@ -151,41 +154,41 @@ func TestSchemaChange(t *testing.T) {
// CREATE
t.Run("CREATE TABLE", func(t *testing.T) {
// The table does not exist
uuid := testOnlineDDLStatement(t, createStatement, onlineSingletonDDLStrategy, "vtgate", "", "", false)
uuid := testOnlineDDLStatement(t, createStatement, onlineSingletonDDLStrategy, "vtgate", "", "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, true)
})
t.Run("revert CREATE TABLE", func(t *testing.T) {
// The table existed, so it will now be dropped (renamed)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", "", false)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", onlineSingletonDDLStrategy, "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, false)
})
t.Run("revert revert CREATE TABLE", func(t *testing.T) {
// Table was dropped (renamed) so it will now be restored
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", "", false)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", onlineSingletonDDLStrategy, "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, true)
})

var throttledUUID string
t.Run("throttled migration", func(t *testing.T) {
throttledUUID = testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtgate", "hint_col", "", false)
throttledUUID = testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtgate", "", "hint_col", "", false)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, throttledUUID, schema.OnlineDDLStatusRunning)
})
t.Run("failed singleton migration, vtgate", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtgate", "hint_col", "rejected", true)
uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtgate", "", "hint_col", "rejected", true)
assert.Empty(t, uuid)
})
t.Run("failed singleton migration, vtctl", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtctl", "hint_col", "rejected", true)
uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtctl", "", "hint_col", "rejected", true)
assert.Empty(t, uuid)
})
t.Run("failed revert migration", func(t *testing.T) {
uuid := testRevertMigration(t, throttledUUID, "vtgate", "rejected", true)
uuid := testRevertMigration(t, throttledUUID, "vtgate", onlineSingletonDDLStrategy, "", "rejected", true)
assert.Empty(t, uuid)
})
t.Run("terminate throttled migration", func(t *testing.T) {
Expand All @@ -195,20 +198,20 @@ func TestSchemaChange(t *testing.T) {
onlineddl.CheckMigrationStatus(t, &vtParams, shards, throttledUUID, schema.OnlineDDLStatusFailed)
})
t.Run("successful gh-ost alter, vtctl", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtctl", "hint_col", "", false)
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtctl", "", "hint_col", "", false)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false)
onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, false)
})
t.Run("successful gh-ost alter, vtgate", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "hint_col", "", false)
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "", "hint_col", "", false)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false)
onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, false)
})

t.Run("successful online alter, vtgate", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, onlineSingletonDDLStrategy, "vtgate", "hint_col", "", false)
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, onlineSingletonDDLStrategy, "vtgate", "", "hint_col", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false)
Expand All @@ -217,7 +220,7 @@ func TestSchemaChange(t *testing.T) {
})
t.Run("revert ALTER TABLE, vttablet", func(t *testing.T) {
// The table existed, so it will now be dropped (renamed)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vttablet", "", false)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtctl", onlineSingletonDDLStrategy, "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, true)
Expand All @@ -226,15 +229,15 @@ func TestSchemaChange(t *testing.T) {
var throttledUUIDs []string
// singleton-context
t.Run("throttled migrations, singleton-context", func(t *testing.T) {
uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "", false)
uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "", "hint_col", "", false)
throttledUUIDs = strings.Split(uuidList, "\n")
assert.Equal(t, 3, len(throttledUUIDs))
for _, uuid := range throttledUUIDs {
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusQueued, schema.OnlineDDLStatusReady, schema.OnlineDDLStatusRunning)
}
})
t.Run("failed migrations, singleton-context", func(t *testing.T) {
_ = testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "rejected", false)
_ = testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "", "hint_col", "rejected", false)
})
t.Run("terminate throttled migrations", func(t *testing.T) {
for _, uuid := range throttledUUIDs {
Expand All @@ -249,7 +252,7 @@ func TestSchemaChange(t *testing.T) {
})

t.Run("successful multiple statement, singleton-context, vtctl", func(t *testing.T) {
uuidList := testOnlineDDLStatement(t, multiDropStatements, onlineSingletonContextDDLStrategy, "vtctl", "", "", false)
uuidList := testOnlineDDLStatement(t, multiDropStatements, onlineSingletonContextDDLStrategy, "vtctl", "", "", "", false)
uuidSlice := strings.Split(uuidList, "\n")
assert.Equal(t, 3, len(uuidSlice))
for _, uuid := range uuidSlice {
Expand All @@ -261,34 +264,59 @@ func TestSchemaChange(t *testing.T) {
//DROP

t.Run("online DROP TABLE", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, dropStatement, onlineSingletonDDLStrategy, "vtgate", "", "", false)
uuid := testOnlineDDLStatement(t, dropStatement, onlineSingletonDDLStrategy, "vtgate", "", "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, false)
})
t.Run("revert DROP TABLE", func(t *testing.T) {
// This will recreate the table (well, actually, rename it back into place)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vttablet", "", false)
uuid := testRevertMigration(t, uuids[len(uuids)-1], "vttablet", onlineSingletonDDLStrategy, "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, true)
})

// Last two tests (we run an incomplete migration)
t.Run("submit successful migration, no wait, vtgate", func(t *testing.T) {
_ = testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "hint_col", "", true)
t.Run("fail concurrent singleton, vtgate", func(t *testing.T) {
uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "vitess --postpone-completion -singleton", "vtgate", "", "hint_col", "", true)
Comment thread
mattlord marked this conversation as resolved.
Outdated
uuids = append(uuids, uuid)
_ = testOnlineDDLStatement(t, dropNonexistentTableStatement, "vitess -singleton", "vtgate", "", "hint_col", "rejected", true)
onlineddl.CheckCompleteMigration(t, &vtParams, shards, uuid, true)
status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, uuid, 20*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed)
fmt.Printf("# Migration status (for debug purposes): <%s>\n", status)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
})
t.Run("fail submit migration, no wait, vtgate", func(t *testing.T) {
_ = testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "hint_col", "rejected", true)
t.Run("fail concurrent singleton-context with revert", func(t *testing.T) {
revertUUID := testRevertMigration(t, uuids[len(uuids)-1], "vtctl", "vitess --allow-concurrent --postpone-completion -singleton-context", "rev:ctx", "", false)
onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusRunning)
// revert is running
_ = testOnlineDDLStatement(t, dropNonexistentTableStatement, "vitess --allow-concurrent -singleton-context", "vtctl", "migrate:ctx", "", "rejected", true)
onlineddl.CheckCancelMigration(t, &vtParams, shards, revertUUID, true)
status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed)
fmt.Printf("# Migration status (for debug purposes): <%s>\n", status)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, revertUUID, schema.OnlineDDLStatusFailed)
})
t.Run("success concurrent singleton-context with no-context revert", func(t *testing.T) {
revertUUID := testRevertMigration(t, uuids[len(uuids)-1], "vtctl", "vitess --allow-concurrent --postpone-completion", "rev:ctx", "", false)
onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusRunning)
// revert is running but has no -singleton-context. Our next migration should be able to run.
uuid := testOnlineDDLStatement(t, dropNonexistentTableStatement, "vitess --allow-concurrent -singleton-context", "vtctl", "migrate:ctx", "", "", false)
uuids = append(uuids, uuid)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
onlineddl.CheckCancelMigration(t, &vtParams, shards, revertUUID, true)
status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed)
fmt.Printf("# Migration status (for debug purposes): <%s>\n", status)
onlineddl.CheckMigrationStatus(t, &vtParams, shards, revertUUID, schema.OnlineDDLStatusFailed)
})
}

// testOnlineDDLStatement runs an online DDL, ALTER statement
func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy string, executeStrategy string, expectHint string, expectError string, skipWait bool) (uuid string) {
func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy string, executeStrategy string, migrationContext string, expectHint string, expectError string, skipWait bool) (uuid string) {
strategySetting, err := schema.ParseDDLStrategy(ddlStrategy)
require.NoError(t, err)

if executeStrategy == "vtgate" {
assert.Empty(t, migrationContext, "explicit migration context not supported in vtgate. Test via vtctl")
result := onlineddl.VtgateExecDDL(t, &vtParams, ddlStrategy, alterStatement, expectError)
if result != nil {
row := result.Named().Row()
Expand All @@ -297,7 +325,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str
}
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, MigrationContext: migrationContext})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand All @@ -322,18 +350,19 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str
}

// testRevertMigration reverts a given migration
func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string, expectError string, skipWait bool) (uuid string) {
func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string, ddlStrategy string, migrationContext string, expectError string, skipWait bool) (uuid string) {
revertQuery := fmt.Sprintf("revert vitess_migration '%s'", revertUUID)
if executeStrategy == "vtgate" {
result := onlineddl.VtgateExecDDL(t, &vtParams, onlineSingletonDDLStrategy, revertQuery, expectError)
assert.Empty(t, migrationContext, "explicit migration context not supported in vtgate. Test via vtctl")
result := onlineddl.VtgateExecDDL(t, &vtParams, ddlStrategy, revertQuery, expectError)
if result != nil {
row := result.Named().Row()
if row != nil {
uuid = row.AsString("uuid", "")
}
}
} else {
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: onlineSingletonDDLStrategy, SkipPreflight: true})
output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, MigrationContext: migrationContext})
if expectError == "" {
assert.NoError(t, err)
uuid = output
Expand Down
32 changes: 30 additions & 2 deletions go/vt/vttablet/onlineddl/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3586,6 +3586,29 @@ func (e *Executor) CompleteMigration(ctx context.Context, uuid string) (result *
return e.execQuery(ctx, query)
}

func (e *Executor) submittedMigrationConflictsWithPendingMigrationInSingletonContext(
ctx context.Context, submittedMigration, pendingOnlineDDL *schema.OnlineDDL,
) (bool, error) {
if pendingOnlineDDL.MigrationContext == submittedMigration.MigrationContext {
// same migration context. this is obviously allowed
return false, nil
}
// Let's see if the pending migration is a revert:
revertedUUID, _ := pendingOnlineDDL.GetRevertUUID()
if revertedUUID == "" {

@mattlord mattlord May 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like an opportunity for undefined behavior in the future. Any reason not to do this instead?

if _, err := pendingOnlineDDL.GetRevertUUID(); errors.Is(err, vtrpcpb.Code_INVALID_ARGUMENT) {

@shlomi-noach shlomi-noach May 3, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to if _, err := pendingOnlineDDL.GetRevertUUID(); err != nil ; I prefer not to rely on a specific error code.

// Not a revert. So the pending migration definitely conflicts with our migration.
return true, nil
}

// The pending migration is a revert
if !pendingOnlineDDL.StrategySetting().IsSingletonContext() {
// Aha! So, our "conflict" is with a REVERT migration, which does _not_ have a -singleton-context
// flag. Because we want to allow REVERT migrations to run as concurrently as possible, we allow this scenario.
return false, nil
}
return true, nil
Comment thread
mattlord marked this conversation as resolved.
Outdated
}

// SubmitMigration inserts a new migration request
func (e *Executor) SubmitMigration(
ctx context.Context,
Expand Down Expand Up @@ -3667,9 +3690,14 @@ func (e *Executor) SubmitMigration(
if err != nil {
return err
}
if pendingOnlineDDL.MigrationContext != onlineDDL.MigrationContext {
return vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.MigrationContext)
conflictFound, err := e.submittedMigrationConflictsWithPendingMigrationInSingletonContext(ctx, onlineDDL, pendingOnlineDDL)
if err != nil {
return err
}
if conflictFound {
return vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "singleton-context migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.MigrationContext)
}
// no conflict? continue looking for other pending migrations
}
}
// OK to go! We can submit the migration:
Expand Down