Skip to content
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.

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

Allow for passing in the MySQL shutdown timeout #14568

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 2 additions & 3 deletions go/cmd/mysqlctl/command/shutdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ var Shutdown = &cobra.Command{
Use: "shutdown",
Short: "Shuts down mysqld, without removing any files.",
Long: "Stop a `mysqld` instance that was previously started with `init` or `start`.\n\n" +

"For large `mysqld` instances, you may need to extend the `wait_time` to shutdown cleanly.",
Example: `mysqlctl --tablet_uid 101 --alsologtostderr shutdown`,
Args: cobra.NoArgs,
Expand All @@ -51,9 +50,9 @@ func commandShutdown(cmd *cobra.Command, args []string) error {
}
defer mysqld.Close()

ctx, cancel := context.WithTimeout(context.Background(), shutdownArgs.WaitTime)
ctx, cancel := context.WithTimeout(context.Background(), shutdownArgs.WaitTime+10*time.Second)
defer cancel()
if err := mysqld.Shutdown(ctx, cnf, true); err != nil {
if err := mysqld.Shutdown(ctx, cnf, true, shutdownArgs.WaitTime); err != nil {
return fmt.Errorf("failed shutdown mysql: %v", err)
}
return nil
Expand Down
5 changes: 2 additions & 3 deletions go/cmd/mysqlctl/command/teardown.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ var Teardown = &cobra.Command{
Long: "{{< warning >}}\n" +
"This is a destructive operation.\n" +
"{{</ warning >}}\n\n" +

"Shuts down a `mysqld` instance and removes its data directory.",
Example: `mysqlctl --tablet_uid 101 --alsologtostderr teardown`,
Args: cobra.NoArgs,
Expand All @@ -54,9 +53,9 @@ func commandTeardown(cmd *cobra.Command, args []string) error {
}
defer mysqld.Close()

ctx, cancel := context.WithTimeout(context.Background(), teardownArgs.WaitTime)
ctx, cancel := context.WithTimeout(context.Background(), teardownArgs.WaitTime+10*time.Second)
defer cancel()
if err := mysqld.Teardown(ctx, cnf, teardownArgs.Force); err != nil {
if err := mysqld.Teardown(ctx, cnf, teardownArgs.Force, teardownArgs.WaitTime); err != nil {
return fmt.Errorf("failed teardown mysql (forced? %v): %v", teardownArgs.Force, err)
}
return nil
Expand Down
13 changes: 8 additions & 5 deletions go/cmd/mysqlctld/cli/mysqlctld.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ var (
mysqlSocket string

// mysqlctl init flags
waitTime = 5 * time.Minute
initDBSQLFile string
waitTime = 5 * time.Minute
shutdownWaitTime = 5 * time.Minute
initDBSQLFile string

Main = &cobra.Command{
Use: "mysqlctld",
Expand Down Expand Up @@ -84,8 +85,9 @@ func init() {
Main.Flags().IntVar(&mysqlPort, "mysql_port", mysqlPort, "MySQL port")
Main.Flags().Uint32Var(&tabletUID, "tablet_uid", tabletUID, "Tablet UID")
Main.Flags().StringVar(&mysqlSocket, "mysql_socket", mysqlSocket, "Path to the mysqld socket file")
Main.Flags().DurationVar(&waitTime, "wait_time", waitTime, "How long to wait for mysqld startup or shutdown")
Main.Flags().DurationVar(&waitTime, "wait_time", waitTime, "How long to wait for mysqld startup")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun fact, this config value was not used at all for shutdown, only for startup so call it out as such. Also we want these to really be separate.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of fixing this to actually be used during both startup and shutdown? Then you could just pass the desired higher value. Are there downsides to increasing the startup wait time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deepthi I think they are very different concepts? How long you want to wait for startup can be very different I think than for shutdown.

Especially in the case that is relevant here for disabling fast shutdown on upgrades, you want to wait a lot longer (say 30 minutes) for shutdown, but waiting that long for startup is entirely unneeded and likely not desired since you want to know earlier if you have a problem starting up MySQL.

That's why I think they must be separate values really and assuming the same value for both doesn't make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's effectively a wait time for the mysqlctl client command / mysqlctld RPC, no? In that case I also think we should just have a single wait timeout flag. I'm OK with adding another flag, if it's truly necessary. Looks like this is also relevant for other programs though such as vttablet, so having an additional flag there is required and there's IMO less reason to avoid that for mysqlctl[d].

Copy link
Contributor Author

@dbussink dbussink Nov 29, 2023

Choose a reason for hiding this comment

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

But it's effectively a wait time for the mysqlctl client command / mysqlctld RPC, no?

Not really I think. The only operation that uses it is the start command. Also I would not classify it as an RPC, because this is for the internal "starting MySQL" phase and it's not used for RPC to mysqlctld.

I'm OK with adding another flag, if it's truly necessary.

I don't see a way without it. Startup is a fundamentally different thing from shutdown and therefore I think they need separate controls.

Main.Flags().StringVar(&initDBSQLFile, "init_db_sql_file", initDBSQLFile, "Path to .sql file to run after mysqld initialization")
Main.Flags().DurationVar(&shutdownWaitTime, "shutdown-wait-time", shutdownWaitTime, "How long to wait for mysqld shutdown")

acl.RegisterFlags(Main.Flags())
}
Expand Down Expand Up @@ -154,8 +156,9 @@ func run(cmd *cobra.Command, args []string) error {
// Take mysqld down with us on SIGTERM before entering lame duck.
servenv.OnTermSync(func() {
log.Infof("mysqlctl received SIGTERM, shutting down mysqld first")
ctx := context.Background()
if err := mysqld.Shutdown(ctx, cnf, true); err != nil {
ctx, cancel := context.WithTimeout(context.Background(), shutdownWaitTime+10*time.Second)
defer cancel()
if err := mysqld.Shutdown(ctx, cnf, true, shutdownWaitTime); err != nil {
log.Errorf("failed to shutdown mysqld: %v", err)
}
})
Expand Down
68 changes: 36 additions & 32 deletions go/cmd/vtbackup/cli/vtbackup.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,14 @@ var (
incrementalFromPos string

// mysqlctld-like flags
mysqlPort = 3306
mysqlSocket string
mysqlTimeout = 5 * time.Minute
initDBSQLFile string
detachedMode bool
keepAliveTimeout time.Duration
disableRedoLog bool
mysqlPort = 3306
mysqlSocket string
mysqlTimeout = 5 * time.Minute
mysqlShutdownTimeout = 5 * time.Minute
initDBSQLFile string
detachedMode bool
keepAliveTimeout time.Duration
disableRedoLog bool

// Deprecated, use "Phase" instead.
deprecatedDurationByPhase = stats.NewGaugesWithSingleLabel(
Expand Down Expand Up @@ -207,6 +208,7 @@ func init() {
Main.Flags().IntVar(&mysqlPort, "mysql_port", mysqlPort, "mysql port")
Main.Flags().StringVar(&mysqlSocket, "mysql_socket", mysqlSocket, "path to the mysql socket")
Main.Flags().DurationVar(&mysqlTimeout, "mysql_timeout", mysqlTimeout, "how long to wait for mysqld startup")
Main.Flags().DurationVar(&mysqlShutdownTimeout, "mysql-shutdown-timeout", mysqlShutdownTimeout, "how long to wait for mysqld startup")
Main.Flags().StringVar(&initDBSQLFile, "init_db_sql_file", initDBSQLFile, "path to .sql file to run after mysql_install_db")
Main.Flags().BoolVar(&detachedMode, "detach", detachedMode, "detached mode - run backups detached from the terminal")
Main.Flags().DurationVar(&keepAliveTimeout, "keep-alive-timeout", keepAliveTimeout, "Wait until timeout elapses after a successful backup before shutting down.")
Expand Down Expand Up @@ -340,9 +342,9 @@ func takeBackup(ctx context.Context, topoServer *topo.Server, backupStorage back
defer func() {
// Be careful not to use the original context, because we don't want to
// skip shutdown just because we timed out waiting for other things.
mysqlShutdownCtx, mysqlShutdownCancel := context.WithTimeout(context.Background(), 30*time.Second)
mysqlShutdownCtx, mysqlShutdownCancel := context.WithTimeout(context.Background(), mysqlShutdownTimeout+10*time.Second)
defer mysqlShutdownCancel()
if err := mysqld.Shutdown(mysqlShutdownCtx, mycnf, false); err != nil {
if err := mysqld.Shutdown(mysqlShutdownCtx, mycnf, false, mysqlShutdownTimeout); err != nil {
log.Errorf("failed to shutdown mysqld: %v", err)
}
}()
Expand All @@ -356,18 +358,19 @@ func takeBackup(ctx context.Context, topoServer *topo.Server, backupStorage back
}

backupParams := mysqlctl.BackupParams{
Cnf: mycnf,
Mysqld: mysqld,
Logger: logutil.NewConsoleLogger(),
Concurrency: concurrency,
IncrementalFromPos: incrementalFromPos,
HookExtraEnv: extraEnv,
TopoServer: topoServer,
Keyspace: initKeyspace,
Shard: initShard,
TabletAlias: topoproto.TabletAliasString(tabletAlias),
Stats: backupstats.BackupStats(),
UpgradeSafe: upgradeSafe,
Cnf: mycnf,
Mysqld: mysqld,
Logger: logutil.NewConsoleLogger(),
Concurrency: concurrency,
IncrementalFromPos: incrementalFromPos,
HookExtraEnv: extraEnv,
TopoServer: topoServer,
Keyspace: initKeyspace,
Shard: initShard,
TabletAlias: topoproto.TabletAliasString(tabletAlias),
Stats: backupstats.BackupStats(),
UpgradeSafe: upgradeSafe,
MysqlShutdownTimeout: mysqlShutdownTimeout,
}
// In initial_backup mode, just take a backup of this empty database.
if initialBackup {
Expand Down Expand Up @@ -416,16 +419,17 @@ func takeBackup(ctx context.Context, topoServer *topo.Server, backupStorage back
log.Infof("Restoring latest backup from directory %v", backupDir)
restoreAt := time.Now()
params := mysqlctl.RestoreParams{
Cnf: mycnf,
Mysqld: mysqld,
Logger: logutil.NewConsoleLogger(),
Concurrency: concurrency,
HookExtraEnv: extraEnv,
DeleteBeforeRestore: true,
DbName: dbName,
Keyspace: initKeyspace,
Shard: initShard,
Stats: backupstats.RestoreStats(),
Cnf: mycnf,
Mysqld: mysqld,
Logger: logutil.NewConsoleLogger(),
Concurrency: concurrency,
HookExtraEnv: extraEnv,
DeleteBeforeRestore: true,
DbName: dbName,
Keyspace: initKeyspace,
Shard: initShard,
Stats: backupstats.RestoreStats(),
MysqlShutdownTimeout: mysqlShutdownTimeout,
}
backupManifest, err := mysqlctl.Restore(ctx, params)
var restorePos replication.Position
Expand Down Expand Up @@ -583,7 +587,7 @@ func takeBackup(ctx context.Context, topoServer *topo.Server, backupStorage back
return fmt.Errorf("Could not prep for full shutdown: %v", err)
}
// Shutdown, waiting for it to finish
if err := mysqld.Shutdown(ctx, mycnf, true); err != nil {
if err := mysqld.Shutdown(ctx, mycnf, true, mysqlShutdownTimeout); err != nil {
return fmt.Errorf("Something went wrong during full MySQL shutdown: %v", err)
}
// Start MySQL, waiting for it to come up
Expand Down
14 changes: 11 additions & 3 deletions go/cmd/vtcombo/cli/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ func startMysqld(uid uint32) (mysqld *mysqlctl.Mysqld, cnf *mysqlctl.Mycnf, err
return mysqld, cnf, nil
}

const mysqlShutdownTimeout = 5 * time.Minute

func run(cmd *cobra.Command, args []string) (err error) {
// Stash away a copy of the topology that vtcombo was started with.
//
Expand Down Expand Up @@ -195,7 +197,9 @@ func run(cmd *cobra.Command, args []string) (err error) {
return err
}
servenv.OnClose(func() {
mysqld.Shutdown(context.TODO(), cnf, true)
ctx, cancel := context.WithTimeout(context.Background(), mysqlShutdownTimeout+10*time.Second)
defer cancel()
mysqld.Shutdown(ctx, cnf, true, mysqlShutdownTimeout)
})
// We want to ensure we can write to this database
mysqld.SetReadOnly(false)
Expand All @@ -217,7 +221,9 @@ func run(cmd *cobra.Command, args []string) (err error) {
if err != nil {
// ensure we start mysql in the event we fail here
if startMysql {
mysqld.Shutdown(context.TODO(), cnf, true)
ctx, cancel := context.WithTimeout(context.Background(), mysqlShutdownTimeout+10*time.Second)
defer cancel()
mysqld.Shutdown(ctx, cnf, true, mysqlShutdownTimeout)
}

return fmt.Errorf("initTabletMapProto failed: %w", err)
Expand Down Expand Up @@ -264,7 +270,9 @@ func run(cmd *cobra.Command, args []string) (err error) {
err := topotools.RebuildKeyspace(context.Background(), logutil.NewConsoleLogger(), ts, ks.GetName(), tpb.Cells, false)
if err != nil {
if startMysql {
mysqld.Shutdown(context.TODO(), cnf, true)
ctx, cancel := context.WithTimeout(context.Background(), mysqlShutdownTimeout+10*time.Second)
defer cancel()
mysqld.Shutdown(ctx, cnf, true, mysqlShutdownTimeout)
}

return fmt.Errorf("Couldn't build srv keyspace for (%v: %v). Got error: %w", ks, tpb.Cells, err)
Expand Down
3 changes: 2 additions & 1 deletion go/flags/endtoend/mysqlctld.txt
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ Flags:
--replication_connect_retry duration how long to wait in between replica reconnect attempts. Only precise to the second. (default 10s)
--security_policy string the name of a registered security policy to use for controlling access to URLs - empty means allow all for anyone (built-in policies: deny-all, read-only)
--service_map strings comma separated list of services to enable (or disable if prefixed with '-') Example: grpc-queryservice
--shutdown-wait-time duration How long to wait for mysqld shutdown (default 5m0s)
--socket_file string Local unix socket file to listen on
--stderrthreshold severityFlag logs at or above this threshold go to stderr (default 1)
--table-refresh-interval int interval in milliseconds to refresh tables in status page with refreshRequired class
Expand All @@ -118,4 +119,4 @@ Flags:
--v Level log level for V logs
-v, --version print binary version
--vmodule vModuleFlag comma-separated list of pattern=N settings for file-filtered logging
--wait_time duration How long to wait for mysqld startup or shutdown (default 5m0s)
--wait_time duration How long to wait for mysqld startup (default 5m0s)
1 change: 1 addition & 0 deletions go/flags/endtoend/vtbackup.txt
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ Flags:
--mycnf_slow_log_path string mysql slow query log path
--mycnf_socket_file string mysql socket file
--mycnf_tmp_dir string mysql tmp directory
--mysql-shutdown-timeout duration how long to wait for mysqld startup (default 5m0s)
--mysql_port int mysql port (default 3306)
--mysql_server_version string MySQL server version to advertise. (default "8.0.30-Vitess")
--mysql_socket string path to the mysql socket
Expand Down
1 change: 1 addition & 0 deletions go/flags/endtoend/vtcombo.txt
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ Flags:
--mycnf_tmp_dir string mysql tmp directory
--mysql-server-keepalive-period duration TCP period between keep-alives
--mysql-server-pool-conn-read-buffers If set, the server will pool incoming connection read buffers
--mysql-shutdown-timeout duration timeout to use when MySQL is being shut down. (default 5m0s)
--mysql_allow_clear_text_without_tls If set, the server will allow the use of a clear text password over non-SSL connections.
--mysql_auth_server_impl string Which auth server implementation to use. Options: none, ldap, clientcert, static, vault. (default "static")
--mysql_default_workload string Default session workload (OLTP, OLAP, DBA) (default "OLTP")
Expand Down
1 change: 1 addition & 0 deletions go/flags/endtoend/vttablet.txt
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ Flags:
--mycnf_slow_log_path string mysql slow query log path
--mycnf_socket_file string mysql socket file
--mycnf_tmp_dir string mysql tmp directory
--mysql-shutdown-timeout duration timeout to use when MySQL is being shut down. (default 5m0s)
--mysql_server_version string MySQL server version to advertise. (default "8.0.30-Vitess")
--mysqlctl_mycnf_template string template file to use for generating the my.cnf file during server init
--mysqlctl_socket string socket file to use for remote mysqlctl actions (empty for local actions)
Expand Down
5 changes: 4 additions & 1 deletion go/test/endtoend/utils/mysql.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"os"
"path"
"testing"
"time"

"github.com/stretchr/testify/assert"

Expand All @@ -35,6 +36,8 @@ import (
"vitess.io/vitess/go/vt/mysqlctl"
)

const mysqlShutdownTimeout = 1 * time.Minute

// NewMySQL creates a new MySQL server using the local mysqld binary. The name of the database
// will be set to `dbName`. SQL queries that need to be executed on the new MySQL instance
// can be passed through the `schemaSQL` argument.
Expand Down Expand Up @@ -96,7 +99,7 @@ func NewMySQLWithMysqld(port int, hostname, dbName string, schemaSQL ...string)
}
return params, mysqld, func() {
ctx := context.Background()
_ = mysqld.Teardown(ctx, mycnf, true)
_ = mysqld.Teardown(ctx, mycnf, true, mysqlShutdownTimeout)
}, nil
}

Expand Down
2 changes: 1 addition & 1 deletion go/vt/mysqlctl/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ func Restore(ctx context.Context, params RestoreParams) (*BackupManifest, error)
// The MySQL manual recommends restarting mysqld after running mysql_upgrade,
// so that any changes made to system tables take effect.
params.Logger.Infof("Restore: restarting mysqld after mysql_upgrade")
if err := params.Mysqld.Shutdown(context.Background(), params.Cnf, true); err != nil {
if err := params.Mysqld.Shutdown(context.Background(), params.Cnf, true, params.MysqlShutdownTimeout); err != nil {
return nil, err
}
if err := params.Mysqld.Start(context.Background(), params.Cnf); err != nil {
Expand Down
Loading
Loading