Skip to content

Commit

Permalink
Relax singleton-context constraint for pending non-singleton-context …
Browse files Browse the repository at this point in the history 
…reverts (#10114)

* relax singleton-context constraint with regard REVERT migrations

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* fixed logic; added tests

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* release notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* long options: double dashes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* double dashes in ddl_strategy

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* no error is returned

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

* check error instead of uuid value

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
  • Loading branch information
@shlomi-noach
shlomi-noach committed May 3, 2022
1 parent f08ba5b commit bb2230e
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 31 deletions.
6 changes: 5 additions & 1 deletion doc/releasenotes/14_0_0_summary.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,13 @@ 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

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`.
- `vtctl ApplySchema --uuid_list='...'` now rejects a migration if an existing migration has the same UUID but with different `migration_context`.

### Table lifecycle

Expand Down
85 changes: 57 additions & 28 deletions go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ var (
cell = "zone1"
schemaChangeDirectory = ""
tableName = `stress_test`
onlineSingletonDDLStrategy = "online -singleton"
onlineSingletonContextDDLStrategy = "online -singleton-context"
onlineSingletonDDLStrategy = "online --singleton"
onlineSingletonContextDDLStrategy = "online --singleton-context"
createStatement = `
CREATE TABLE stress_test (
id bigint(20) not null,
Expand Down 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)
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
27 changes: 25 additions & 2 deletions go/vt/vttablet/onlineddl/executor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3586,6 +3586,28 @@ 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 {
if pendingOnlineDDL.MigrationContext == submittedMigration.MigrationContext {
// same migration context. this is obviously allowed
return false
}
// Let's see if the pending migration is a revert:
if _, err := pendingOnlineDDL.GetRevertUUID(); err != nil {
// Not a revert. So the pending migration definitely conflicts with our migration.
return true
}

// 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
}
return true
}

// SubmitMigration inserts a new migration request
func (e *Executor) SubmitMigration(
ctx context.Context,
Expand Down Expand Up @@ -3667,9 +3689,10 @@ 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)
if e.submittedMigrationConflictsWithPendingMigrationInSingletonContext(ctx, onlineDDL, pendingOnlineDDL) {
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

0 comments on commit bb2230e

Please sign in to comment.