From 51a438a3587b5936e45897afd3ee31075749d6f8 Mon Sep 17 00:00:00 2001 From: "vitess-bot[bot]" <108069721+vitess-bot[bot]@users.noreply.github.com> Date: Thu, 7 Sep 2023 09:59:27 +0300 Subject: [PATCH 1/3] Cherry-pick f71583b6ef586faba07a2cf295831ccd47757fc8 with conflicts --- .../scheduler/onlineddl_scheduler_test.go | 3 ++ .../onlineddl/vrepl/onlineddl_vrepl_test.go | 3 ++ go/test/endtoend/onlineddl/vtgate_util.go | 43 +++++++++++++++++++ go/vt/vttablet/onlineddl/executor.go | 24 +++++++++++ go/vt/vttablet/onlineddl/schema.go | 5 ++- 5 files changed, 76 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go index 2911fbb80b2..2b068df643d 100644 --- a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go +++ b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go @@ -265,6 +265,9 @@ func TestSchemaChange(t *testing.T) { t.Run("summary: validate sequential migration IDs", func(t *testing.T) { onlineddl.ValidateSequentialMigrationIDs(t, &vtParams, shards) }) + t.Run("summary: validate completed_timestamp", func(t *testing.T) { + onlineddl.ValidateCompletedTimestamp(t, &vtParams) + }) } func testScheduler(t *testing.T) { diff --git a/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go b/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go index ec6eed14f17..fd4c9ada09f 100644 --- a/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go +++ b/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go @@ -888,6 +888,9 @@ func TestSchemaChange(t *testing.T) { t.Run("summary: validate sequential migration IDs", func(t *testing.T) { onlineddl.ValidateSequentialMigrationIDs(t, &vtParams, shards) }) + t.Run("summary: validate completed_timestamp", func(t *testing.T) { + onlineddl.ValidateCompletedTimestamp(t, &vtParams) + }) } func insertRow(t *testing.T) { diff --git a/go/test/endtoend/onlineddl/vtgate_util.go b/go/test/endtoend/onlineddl/vtgate_util.go index 75c35061da9..82ff6e68b8c 100644 --- a/go/test/endtoend/onlineddl/vtgate_util.go +++ b/go/test/endtoend/onlineddl/vtgate_util.go @@ -39,6 +39,21 @@ import ( "github.com/stretchr/testify/require" ) +<<<<<<< HEAD +======= +const ( + ThrottledAppsTimeout = 60 * time.Second +) + +var ( + testsStartupTime time.Time +) + +func init() { + testsStartupTime = time.Now() +} + +>>>>>>> f71583b6ef (OnlineDDL: fix nil 'completed_timestamp' for cancelled migrations (#13928)) // VtgateExecQuery runs a query on VTGate using given query params func VtgateExecQuery(t *testing.T, vtParams *mysql.ConnParams, query string, expectError string) *sqltypes.Result { t.Helper() @@ -428,3 +443,31 @@ func ValidateSequentialMigrationIDs(t *testing.T, vtParams *mysql.ConnParams, sh assert.Equalf(t, count, shardMax[shard]-shardMin[shard]+1, "mismatch: shared=%v, count=%v, min=%v, max=%v", shard, count, shardMin[shard], shardMax[shard]) } } + +// ValidateCompletedTimestamp ensures that any migration in `cancelled`, `completed`, `failed` statuses +// has a non-nil and valid `completed_timestamp` value. +func ValidateCompletedTimestamp(t *testing.T, vtParams *mysql.ConnParams) { + require.False(t, testsStartupTime.IsZero()) + r := VtgateExecQuery(t, vtParams, "show vitess_migrations", "") + + completedTimestampNumValidations := 0 + for _, row := range r.Named().Rows { + migrationStatus := row.AsString("migration_status", "") + require.NotEmpty(t, migrationStatus) + switch migrationStatus { + case string(schema.OnlineDDLStatusComplete), + string(schema.OnlineDDLStatusFailed), + string(schema.OnlineDDLStatusCancelled): + { + assert.False(t, row["completed_timestamp"].IsNull()) + // Also make sure the timestamp is "real", and that it is recent. + timestamp := row.AsString("completed_timestamp", "") + completedTime, err := time.Parse(sqltypes.TimestampFormat, timestamp) + assert.NoError(t, err) + assert.Greater(t, completedTime.Unix(), testsStartupTime.Unix()) + completedTimestampNumValidations++ + } + } + } + assert.NotZero(t, completedTimestampNumValidations) +} diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 413b5f012ee..7f26759a7e3 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -75,6 +75,7 @@ var ( ErrMigrationNotFound = errors.New("migration not found") ) +<<<<<<< HEAD var vexecUpdateTemplates = []string{ `update _vt.schema_migrations set migration_status='val1' where mysql_schema='val2'`, `update _vt.schema_migrations set migration_status='val1' where migration_uuid='val2' and mysql_schema='val3'`, @@ -99,6 +100,15 @@ var vexecInsertTemplates = []string{ 'val1', 'val2', 'val3', 'val4', 'val5', 'val6', 'val7', 'val8', 'val9', FROM_UNIXTIME(0), 'vala', 'valb' )`, } +======= +var ( + // fixCompletedTimestampDone fixes a nil `completed_tiemstamp` columns, see + // https://github.com/vitessio/vitess/issues/13927 + // The fix is in release-18.0 + // TODO: remove in release-19.0 + fixCompletedTimestampDone bool +) +>>>>>>> f71583b6ef (OnlineDDL: fix nil 'completed_timestamp' for cancelled migrations (#13928)) var emptyResult = &sqltypes.Result{} var acceptableDropTableIfExistsErrorCodes = []int{mysql.ERCantFindFile, mysql.ERNoSuchTable} @@ -3852,6 +3862,7 @@ func (e *Executor) gcArtifacts(ctx context.Context) error { e.migrationMutex.Lock() defer e.migrationMutex.Unlock() +<<<<<<< HEAD if _, err := e.execQuery(ctx, sqlFixCompletedTimestamp); err != nil { // This query fixes a bug where stale migrations were marked as 'failed' without updating 'completed_timestamp' // see https://github.com/vitessio/vitess/issues/8499 @@ -3859,6 +3870,19 @@ func (e *Executor) gcArtifacts(ctx context.Context) error { // This 'if' clause can be removed in version v13 return err } +======= + // v18 fix. Remove in v19 + if !fixCompletedTimestampDone { + if _, err := e.execQuery(ctx, sqlFixCompletedTimestamp); err != nil { + // This query fixes a bug where stale migrations were marked as 'cancelled' or 'failed' without updating 'completed_timestamp' + // Running this query retroactively sets completed_timestamp + // This fix is created in v18 and can be removed in v19 + return err + } + fixCompletedTimestampDone = true + } + +>>>>>>> f71583b6ef (OnlineDDL: fix nil 'completed_timestamp' for cancelled migrations (#13928)) query, err := sqlparser.ParseAndBind(sqlSelectUncollectedArtifacts, sqltypes.Int64BindVariable(int64((retainOnlineDDLTables).Seconds())), ) diff --git a/go/vt/vttablet/onlineddl/schema.go b/go/vt/vttablet/onlineddl/schema.go index adf550dabfe..c4a6d96953b 100644 --- a/go/vt/vttablet/onlineddl/schema.go +++ b/go/vt/vttablet/onlineddl/schema.go @@ -66,7 +66,8 @@ const ( migration_uuid=%a ` sqlUpdateMigrationStatusFailedOrCancelled = `UPDATE _vt.schema_migrations - SET migration_status=IF(cancelled_timestamp IS NULL, 'failed', 'cancelled') + SET migration_status=IF(cancelled_timestamp IS NULL, 'failed', 'cancelled'), + completed_timestamp=NOW(6) WHERE migration_uuid=%a ` @@ -349,7 +350,7 @@ const ( SET completed_timestamp=NOW(6) WHERE - migration_status='failed' + migration_status IN ('cancelled', 'failed') AND cleanup_timestamp IS NULL AND completed_timestamp IS NULL ` From 1de4a0e1d33b342c5efc371cfa7456d0cfdd7e59 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 7 Sep 2023 10:15:05 +0300 Subject: [PATCH 2/3] linter Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go | 10 +++++----- go/test/endtoend/onlineddl/vtgate_util.go | 9 +-------- go/test/endtoend/onlineddl/vttablet_util.go | 3 +-- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go b/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go index fd4c9ada09f..10902baf1d4 100644 --- a/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go +++ b/go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go @@ -213,7 +213,7 @@ func TestMain(m *testing.M) { if err != nil { fmt.Printf("%v\n", err) os.Exit(1) - } else { + } else { // nolint:revive os.Exit(exitcode) } @@ -537,11 +537,11 @@ func TestSchemaChange(t *testing.T) { onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning) }) t.Run("wait for vreplication to run on shard -80", func(t *testing.T) { - vreplStatus := onlineddl.WaitForVReplicationStatus(t, &vtParams, currentPrimaryTablet, uuid, normalMigrationWait, "Copying", "Running") + vreplStatus := onlineddl.WaitForVReplicationStatus(t, currentPrimaryTablet, uuid, normalMigrationWait, "Copying", "Running") require.Contains(t, []string{"Copying", "Running"}, vreplStatus) }) t.Run("wait for vreplication to run on shard 80-", func(t *testing.T) { - vreplStatus := onlineddl.WaitForVReplicationStatus(t, &vtParams, shards[1].Vttablets[0], uuid, normalMigrationWait, "Copying", "Running") + vreplStatus := onlineddl.WaitForVReplicationStatus(t, shards[1].Vttablets[0], uuid, normalMigrationWait, "Copying", "Running") require.Contains(t, []string{"Copying", "Running"}, vreplStatus) }) t.Run("check status again", func(t *testing.T) { @@ -646,11 +646,11 @@ func TestSchemaChange(t *testing.T) { onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusRunning) }) t.Run("wait for vreplication to run on shard -80", func(t *testing.T) { - vreplStatus := onlineddl.WaitForVReplicationStatus(t, &vtParams, currentPrimaryTablet, uuid, normalMigrationWait, "Copying", "Running") + vreplStatus := onlineddl.WaitForVReplicationStatus(t, currentPrimaryTablet, uuid, normalMigrationWait, "Copying", "Running") require.Contains(t, []string{"Copying", "Running"}, vreplStatus) }) t.Run("wait for vreplication to run on shard 80-", func(t *testing.T) { - vreplStatus := onlineddl.WaitForVReplicationStatus(t, &vtParams, shards[1].Vttablets[0], uuid, normalMigrationWait, "Copying", "Running") + vreplStatus := onlineddl.WaitForVReplicationStatus(t, shards[1].Vttablets[0], uuid, normalMigrationWait, "Copying", "Running") require.Contains(t, []string{"Copying", "Running"}, vreplStatus) }) t.Run("check status again", func(t *testing.T) { diff --git a/go/test/endtoend/onlineddl/vtgate_util.go b/go/test/endtoend/onlineddl/vtgate_util.go index 82ff6e68b8c..5052065082b 100644 --- a/go/test/endtoend/onlineddl/vtgate_util.go +++ b/go/test/endtoend/onlineddl/vtgate_util.go @@ -39,12 +39,6 @@ import ( "github.com/stretchr/testify/require" ) -<<<<<<< HEAD -======= -const ( - ThrottledAppsTimeout = 60 * time.Second -) - var ( testsStartupTime time.Time ) @@ -53,7 +47,6 @@ func init() { testsStartupTime = time.Now() } ->>>>>>> f71583b6ef (OnlineDDL: fix nil 'completed_timestamp' for cancelled migrations (#13928)) // VtgateExecQuery runs a query on VTGate using given query params func VtgateExecQuery(t *testing.T, vtParams *mysql.ConnParams, query string, expectError string) *sqltypes.Result { t.Helper() @@ -369,7 +362,7 @@ func WaitForThrottlerStatusEnabled(t *testing.T, tablet *cluster.Vttablet, timeo jsonPath := "IsEnabled" url := fmt.Sprintf("http://localhost:%d/throttler/status", tablet.HTTPPort) - ctx, cancel := context.WithTimeout(context.Background(), throttlerConfigTimeout) + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() ticker := time.NewTicker(time.Second) diff --git a/go/test/endtoend/onlineddl/vttablet_util.go b/go/test/endtoend/onlineddl/vttablet_util.go index 3d4ded89dac..b4669490f63 100644 --- a/go/test/endtoend/onlineddl/vttablet_util.go +++ b/go/test/endtoend/onlineddl/vttablet_util.go @@ -20,7 +20,6 @@ import ( "testing" "time" - "vitess.io/vitess/go/mysql" "vitess.io/vitess/go/sqltypes" "vitess.io/vitess/go/vt/sqlparser" @@ -31,7 +30,7 @@ import ( ) // WaitForVReplicationStatus waits for a vreplication stream to be in one of given states, or timeout -func WaitForVReplicationStatus(t *testing.T, vtParams *mysql.ConnParams, tablet *cluster.Vttablet, uuid string, timeout time.Duration, expectStatuses ...string) (status string) { +func WaitForVReplicationStatus(t *testing.T, tablet *cluster.Vttablet, uuid string, timeout time.Duration, expectStatuses ...string) (status string) { query, err := sqlparser.ParseAndBind("select state from _vt.vreplication where workflow=%a", sqltypes.StringBindVariable(uuid), From 90be65de29713fdc38454b6441fe0399a027d82a Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 7 Sep 2023 10:30:42 +0300 Subject: [PATCH 3/3] resolved conflict Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/vttablet/onlineddl/executor.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 7f26759a7e3..62e22cf3fda 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -75,7 +75,6 @@ var ( ErrMigrationNotFound = errors.New("migration not found") ) -<<<<<<< HEAD var vexecUpdateTemplates = []string{ `update _vt.schema_migrations set migration_status='val1' where mysql_schema='val2'`, `update _vt.schema_migrations set migration_status='val1' where migration_uuid='val2' and mysql_schema='val3'`, @@ -100,7 +99,6 @@ var vexecInsertTemplates = []string{ 'val1', 'val2', 'val3', 'val4', 'val5', 'val6', 'val7', 'val8', 'val9', FROM_UNIXTIME(0), 'vala', 'valb' )`, } -======= var ( // fixCompletedTimestampDone fixes a nil `completed_tiemstamp` columns, see // https://github.com/vitessio/vitess/issues/13927 @@ -108,7 +106,6 @@ var ( // TODO: remove in release-19.0 fixCompletedTimestampDone bool ) ->>>>>>> f71583b6ef (OnlineDDL: fix nil 'completed_timestamp' for cancelled migrations (#13928)) var emptyResult = &sqltypes.Result{} var acceptableDropTableIfExistsErrorCodes = []int{mysql.ERCantFindFile, mysql.ERNoSuchTable} @@ -3862,16 +3859,7 @@ func (e *Executor) gcArtifacts(ctx context.Context) error { e.migrationMutex.Lock() defer e.migrationMutex.Unlock() -<<<<<<< HEAD - if _, err := e.execQuery(ctx, sqlFixCompletedTimestamp); err != nil { - // This query fixes a bug where stale migrations were marked as 'failed' without updating 'completed_timestamp' - // see https://github.com/vitessio/vitess/issues/8499 - // Running this query retroactively sets completed_timestamp - // This 'if' clause can be removed in version v13 - return err - } -======= - // v18 fix. Remove in v19 + // v18 fix (backported to 16). Remove in v19 if !fixCompletedTimestampDone { if _, err := e.execQuery(ctx, sqlFixCompletedTimestamp); err != nil { // This query fixes a bug where stale migrations were marked as 'cancelled' or 'failed' without updating 'completed_timestamp' @@ -3882,7 +3870,6 @@ func (e *Executor) gcArtifacts(ctx context.Context) error { fixCompletedTimestampDone = true } ->>>>>>> f71583b6ef (OnlineDDL: fix nil 'completed_timestamp' for cancelled migrations (#13928)) query, err := sqlparser.ParseAndBind(sqlSelectUncollectedArtifacts, sqltypes.Int64BindVariable(int64((retainOnlineDDLTables).Seconds())), )