From 904918e2d3a52f7411599e8670e90e6c59e17de5 Mon Sep 17 00:00:00 2001 From: William Lu Date: Fri, 8 Sep 2023 19:40:25 +0000 Subject: [PATCH] Fix: convertBoolToSemiSyncAction method to account for all semi sync actions (#13075) * Fix convertBoolToSemiSyncAction method to account for all semi sync actions Signed-off-by: William Lu Signed-off-by: William Lu Signed-off-by: Arthur Schreiber --- go/cmd/vtcombo/main.go | 5 ++ .../reparent/newfeaturetest/reparent_test.go | 37 +++++++++++++++ .../fakemysqldaemon/fakemysqldaemon.go | 5 ++ go/vt/mysqlctl/mysql_daemon.go | 1 + go/vt/mysqlctl/replication.go | 13 ++++++ go/vt/vttablet/tabletmanager/rpc_actions.go | 31 +++++++++++-- .../vttablet/tabletmanager/rpc_replication.go | 46 +++++++++++++++---- go/vt/vttablet/tabletmanager/tm_init.go | 9 +++- 8 files changed, 133 insertions(+), 14 deletions(-) diff --git a/go/cmd/vtcombo/main.go b/go/cmd/vtcombo/main.go index acbea8ff490..ff52c284216 100644 --- a/go/cmd/vtcombo/main.go +++ b/go/cmd/vtcombo/main.go @@ -346,3 +346,8 @@ func (mysqld *vtcomboMysqld) StopReplication(hookExtraEnv map[string]string) err func (mysqld *vtcomboMysqld) SetSemiSyncEnabled(source, replica bool) error { return nil } + +// SemiSyncExtensionLoaded implements the MysqlDaemon interface +func (mysqld *vtcomboMysqld) SemiSyncExtensionLoaded() (bool, error) { + return true, nil +} diff --git a/go/test/endtoend/reparent/newfeaturetest/reparent_test.go b/go/test/endtoend/reparent/newfeaturetest/reparent_test.go index 4fb2b827539..394f9f71226 100644 --- a/go/test/endtoend/reparent/newfeaturetest/reparent_test.go +++ b/go/test/endtoend/reparent/newfeaturetest/reparent_test.go @@ -17,6 +17,8 @@ limitations under the License. package newfeaturetest import ( + "context" + "fmt" "strconv" "testing" @@ -182,3 +184,38 @@ func TestTabletRestart(t *testing.T) { err := tablets[1].VttabletProcess.Setup() require.NoError(t, err) } + +// Tests ensures that ChangeTabletType works even when semi-sync plugins are not loaded. +func TestChangeTypeWithoutSemiSync(t *testing.T) { + defer cluster.PanicHandler(t) + clusterInstance := utils.SetupReparentCluster(t, "none") + defer utils.TeardownCluster(clusterInstance) + tablets := clusterInstance.Keyspaces[0].Shards[0].Vttablets + + ctx := context.Background() + + primary, replica := tablets[0], tablets[1] + + // Unload semi sync plugins + for _, tablet := range tablets[0:4] { + qr := utils.RunSQL(ctx, t, "select @@global.super_read_only", tablet) + result := fmt.Sprintf("%v", qr.Rows[0][0].ToString()) + if result == "1" { + utils.RunSQL(ctx, t, "set global super_read_only = 0", tablet) + } + + utils.RunSQL(ctx, t, "UNINSTALL PLUGIN rpl_semi_sync_slave;", tablet) + utils.RunSQL(ctx, t, "UNINSTALL PLUGIN rpl_semi_sync_master;", tablet) + } + + utils.ValidateTopology(t, clusterInstance, true) + utils.CheckPrimaryTablet(t, clusterInstance, primary) + + // Change replica's type to rdonly + err := clusterInstance.VtctlclientProcess.ExecuteCommand("ChangeTabletType", replica.Alias, "rdonly") + require.NoError(t, err) + + // Change tablets type from rdonly back to replica + err = clusterInstance.VtctlclientProcess.ExecuteCommand("ChangeTabletType", replica.Alias, "replica") + require.NoError(t, err) +} diff --git a/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go b/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go index 5831d817d40..3effa9309c2 100644 --- a/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go +++ b/go/vt/mysqlctl/fakemysqldaemon/fakemysqldaemon.go @@ -642,6 +642,11 @@ func (fmd *FakeMysqlDaemon) SemiSyncClients() uint32 { return 0 } +// SemiSyncExtensionLoaded is part of the MysqlDaemon interface. +func (fmd *FakeMysqlDaemon) SemiSyncExtensionLoaded() (bool, error) { + return true, nil +} + // SemiSyncSettings is part of the MysqlDaemon interface. func (fmd *FakeMysqlDaemon) SemiSyncSettings() (timeout uint64, numReplicas uint32) { return 10000000, 1 diff --git a/go/vt/mysqlctl/mysql_daemon.go b/go/vt/mysqlctl/mysql_daemon.go index c4c76224b3b..ec96eee7b2e 100644 --- a/go/vt/mysqlctl/mysql_daemon.go +++ b/go/vt/mysqlctl/mysql_daemon.go @@ -57,6 +57,7 @@ type MysqlDaemon interface { GetGTIDPurged(ctx context.Context) (mysql.Position, error) SetSemiSyncEnabled(source, replica bool) error SemiSyncEnabled() (source, replica bool) + SemiSyncExtensionLoaded() (bool, error) SemiSyncStatus() (source, replica bool) SemiSyncClients() (count uint32) SemiSyncSettings() (timeout uint64, numReplicas uint32) diff --git a/go/vt/mysqlctl/replication.go b/go/vt/mysqlctl/replication.go index 670a9b4e1ec..3c866019c63 100644 --- a/go/vt/mysqlctl/replication.go +++ b/go/vt/mysqlctl/replication.go @@ -656,3 +656,16 @@ func (mysqld *Mysqld) SemiSyncReplicationStatus() (bool, error) { } return false, nil } + +// SemiSyncExtensionLoaded returns whether semi-sync plugins are loaded. +func (mysqld *Mysqld) SemiSyncExtensionLoaded() (bool, error) { + qr, err := mysqld.FetchSuperQuery(context.Background(), "SELECT COUNT(*) > 0 AS plugin_loaded FROM information_schema.plugins WHERE plugin_name LIKE 'rpl_semi_sync%'") + if err != nil { + return false, err + } + pluginPresent, err := qr.Rows[0][0].ToBool() + if err != nil { + return false, err + } + return pluginPresent, nil +} diff --git a/go/vt/vttablet/tabletmanager/rpc_actions.go b/go/vt/vttablet/tabletmanager/rpc_actions.go index 1093c331a1a..d51f5d2f84a 100644 --- a/go/vt/vttablet/tabletmanager/rpc_actions.go +++ b/go/vt/vttablet/tabletmanager/rpc_actions.go @@ -30,6 +30,7 @@ import ( tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata" topodatapb "vitess.io/vitess/go/vt/proto/topodata" + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" ) // DBAction is used to tell ChangeTabletType whether to call SetReadOnly on change to @@ -82,7 +83,13 @@ func (tm *TabletManager) ChangeType(ctx context.Context, tabletType topodatapb.T return err } defer tm.unlock() - return tm.changeTypeLocked(ctx, tabletType, DBActionNone, convertBoolToSemiSyncAction(semiSync)) + + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return err + } + + return tm.changeTypeLocked(ctx, tabletType, DBActionNone, semiSyncAction) } // ChangeType changes the tablet type @@ -142,9 +149,23 @@ func (tm *TabletManager) RunHealthCheck(ctx context.Context) { tm.QueryServiceControl.BroadcastHealth() } -func convertBoolToSemiSyncAction(semiSync bool) SemiSyncAction { - if semiSync { - return SemiSyncActionSet +func (tm *TabletManager) convertBoolToSemiSyncAction(semiSync bool) (SemiSyncAction, error) { + semiSyncExtensionLoaded, err := tm.MysqlDaemon.SemiSyncExtensionLoaded() + if err != nil { + return SemiSyncActionNone, err + } + + if semiSyncExtensionLoaded { + if semiSync { + return SemiSyncActionSet, nil + } else { + return SemiSyncActionUnset, nil + } + } else { + if semiSync { + return SemiSyncActionNone, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "semi-sync plugins are not loaded") + } else { + return SemiSyncActionNone, nil + } } - return SemiSyncActionUnset } diff --git a/go/vt/vttablet/tabletmanager/rpc_replication.go b/go/vt/vttablet/tabletmanager/rpc_replication.go index 55fbd8c195f..79f6ef7361e 100644 --- a/go/vt/vttablet/tabletmanager/rpc_replication.go +++ b/go/vt/vttablet/tabletmanager/rpc_replication.go @@ -294,7 +294,12 @@ func (tm *TabletManager) StartReplication(ctx context.Context, semiSync bool) er } }() - if err := tm.fixSemiSync(tm.Tablet().Type, convertBoolToSemiSyncAction(semiSync)); err != nil { + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return err + } + + if err := tm.fixSemiSync(tm.Tablet().Type, semiSyncAction); err != nil { return err } return tm.MysqlDaemon.StartReplication(tm.hookExtraEnv()) @@ -377,16 +382,21 @@ func (tm *TabletManager) InitPrimary(ctx context.Context, semiSync bool) (string return "", err } + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return "", err + } + // Set the server read-write, from now on we can accept real // client writes. Note that if semi-sync replication is enabled, // we'll still need some replicas to be able to commit transactions. - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_PRIMARY, DBActionSetReadWrite, convertBoolToSemiSyncAction(semiSync)); err != nil { + if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_PRIMARY, DBActionSetReadWrite, semiSyncAction); err != nil { return "", err } // Enforce semi-sync after changing the tablet)type to PRIMARY. Otherwise, the // primary will hang while trying to create the database. - if err := tm.fixSemiSync(topodatapb.TabletType_PRIMARY, convertBoolToSemiSyncAction(semiSync)); err != nil { + if err := tm.fixSemiSync(topodatapb.TabletType_PRIMARY, semiSyncAction); err != nil { return "", err } @@ -423,11 +433,16 @@ func (tm *TabletManager) InitReplica(ctx context.Context, parent *topodatapb.Tab } defer tm.unlock() + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return err + } + // If we were a primary type, switch our type to replica. This // is used on the old primary when using InitShardPrimary with // -force, and the new primary is different from the old primary. if tm.Tablet().Type == topodatapb.TabletType_PRIMARY { - if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_REPLICA, DBActionNone, convertBoolToSemiSyncAction(semiSync)); err != nil { + if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_REPLICA, DBActionNone, semiSyncAction); err != nil { return err } } @@ -450,7 +465,7 @@ func (tm *TabletManager) InitReplica(ctx context.Context, parent *topodatapb.Tab if tt == topodatapb.TabletType_PRIMARY { tt = topodatapb.TabletType_REPLICA } - if err := tm.fixSemiSync(tt, convertBoolToSemiSyncAction(semiSync)); err != nil { + if err := tm.fixSemiSync(tt, semiSyncAction); err != nil { return err } @@ -601,8 +616,13 @@ func (tm *TabletManager) UndoDemotePrimary(ctx context.Context, semiSync bool) e } defer tm.unlock() + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return err + } + // If using semi-sync, we need to enable source-side. - if err := tm.fixSemiSync(topodatapb.TabletType_PRIMARY, convertBoolToSemiSyncAction(semiSync)); err != nil { + if err := tm.fixSemiSync(topodatapb.TabletType_PRIMARY, semiSyncAction); err != nil { return err } @@ -670,9 +690,14 @@ func (tm *TabletManager) SetReplicationSource(ctx context.Context, parentAlias * } defer tm.unlock() + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return err + } + // setReplicationSourceLocked also fixes the semi-sync. In case the tablet type is primary it assumes that it will become a replica if SetReplicationSource // is called, so we always call fixSemiSync with a non-primary tablet type. This will always set the source side replication to false. - return tm.setReplicationSourceLocked(ctx, parentAlias, timeCreatedNS, waitPosition, forceStartReplication, convertBoolToSemiSyncAction(semiSync)) + return tm.setReplicationSourceLocked(ctx, parentAlias, timeCreatedNS, waitPosition, forceStartReplication, semiSyncAction) } func (tm *TabletManager) setReplicationSourceRepairReplication(ctx context.Context, parentAlias *topodatapb.TabletAlias, timeCreatedNS int64, waitPosition string, forceStartReplication bool) (err error) { @@ -962,8 +987,13 @@ func (tm *TabletManager) PromoteReplica(ctx context.Context, semiSync bool) (str return "", err } + semiSyncAction, err := tm.convertBoolToSemiSyncAction(semiSync) + if err != nil { + return "", err + } + // If using semi-sync, we need to enable it before going read-write. - if err := tm.fixSemiSync(topodatapb.TabletType_PRIMARY, convertBoolToSemiSyncAction(semiSync)); err != nil { + if err := tm.fixSemiSync(topodatapb.TabletType_PRIMARY, semiSyncAction); err != nil { return "", err } diff --git a/go/vt/vttablet/tabletmanager/tm_init.go b/go/vt/vttablet/tabletmanager/tm_init.go index cf1ea9f3d62..f4e1f702794 100644 --- a/go/vt/vttablet/tabletmanager/tm_init.go +++ b/go/vt/vttablet/tabletmanager/tm_init.go @@ -909,8 +909,15 @@ func (tm *TabletManager) initializeReplication(ctx context.Context, tabletType t } // If using semi-sync, we need to enable it before connecting to primary. // We should set the correct type, since it is used in replica semi-sync + tablet.Type = tabletType - if err := tm.fixSemiSync(tabletType, convertBoolToSemiSyncAction(reparentutil.IsReplicaSemiSync(durability, currentPrimary.Tablet, tablet))); err != nil { + + semiSyncAction, err := tm.convertBoolToSemiSyncAction(reparentutil.IsReplicaSemiSync(durability, currentPrimary.Tablet, tablet)) + if err != nil { + return nil, err + } + + if err := tm.fixSemiSync(tabletType, semiSyncAction); err != nil { return nil, err }