Allow for passing in the MySQL shutdown timeout #14568

dbussink · 2023-11-21T16:48:09Z

We have a hardcoded timeout today of 300 seconds when we call mysqladmin during a MySQL shutdown. This means that for example when we have fast shutdown disabled for an upgrade, we still kill after 5 minutes and don't allow for a longer shutdown.

Large tablets with a lot of data can definitely take longer than 5 minutes, so this hardcoded timeout leads to failures in this case which means we can't easily upgrade the tablet and need to do a full replacement from a backup.

By allowing this to be configured, we can make sure that we can give it longer timeouts when we want to do an upgrade.

Related Issue(s)

Fixes #14567

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

vitess-bot · 2023-11-21T16:48:12Z

dbussink · 2023-11-21T16:48:44Z

go/cmd/mysqlctld/cli/mysqlctld.go

@@ -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")


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.

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?

@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.

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].

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.

mattlord

Thanks, @dbussink ! I only had some minor comments/suggestions so approving and we can discuss if/as needed.

mattlord · 2023-11-29T14:46:27Z

go/cmd/mysqlctld/cli/mysqlctld.go

@@ -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")


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].

mattlord · 2023-11-29T14:52:05Z

go/test/endtoend/utils/mysql.go

@@ -96,7 +97,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, 30*time.Second)


Worth adding a default const for e2e tests too, IMO.

If we hit this value, will the test and workflow fail? If so, might be worth doubling it to avoid failures when CI is just really slow.

30 seconds seems long for our tests, but we can increase it if you think that's appropriate? I'll also move it to a constant.

We have a hardcoded timeout today of 300 seconds when we call mysqladmin during a MySQL shutdown. This means that for example when we have fast shutdown disabled for an upgrade, we still kill after 5 minutes and don't allow for a longer shutdown. Large tablets with a lot of data can definitely take longer than 5 minutes, so this hardcoded timeout leads to failures in this case which means we can't easily upgrade the tablet and need to do a full replacement from a backup. By allowing this to be configured, we can make sure that we can give it longer timeouts when we want to do an upgrade. Signed-off-by: Dirkjan Bussink <[email protected]>

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink requested review from deepthi, harshit-gangal, mattlord, rohit-nayak-ps, GuptaManan100, shlomi-noach and ajm188 as code owners November 21, 2023 16:48

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Nov 21, 2023

dbussink commented Nov 21, 2023

View reviewed changes

github-actions bot added this to the v19.0.0 milestone Nov 21, 2023

mattlord approved these changes Nov 29, 2023

View reviewed changes

dbussink added 2 commits November 29, 2023 16:48

Move to constants for test timeouts

1db6254

Signed-off-by: Dirkjan Bussink <[email protected]>

dbussink force-pushed the dbussink/allow-configuring-mysql-shutdown-timeout branch from 7c50a4e to 1db6254 Compare November 29, 2023 16:23

frouioui approved these changes Nov 30, 2023

View reviewed changes

deepthi approved these changes Nov 30, 2023

View reviewed changes

deepthi merged commit 5655236 into vitessio:main Nov 30, 2023
119 checks passed

deepthi deleted the dbussink/allow-configuring-mysql-shutdown-timeout branch November 30, 2023 17:03

dbussink mentioned this pull request Dec 4, 2023

vtbackup: Fix copy pasta typo in option description #14664

Merged

4 tasks

ejortegau pushed a commit to slackhq/vitess that referenced this pull request Dec 13, 2023

Allow for passing in the MySQL shutdown timeout (vitessio#14568)

4aebf1c

Signed-off-by: Dirkjan Bussink <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for passing in the MySQL shutdown timeout #14568

Allow for passing in the MySQL shutdown timeout #14568

dbussink commented Nov 21, 2023

vitess-bot bot commented Nov 21, 2023

dbussink Nov 21, 2023

deepthi Nov 21, 2023

dbussink Nov 22, 2023

mattlord Nov 29, 2023

dbussink Nov 29, 2023 •

edited

Loading

mattlord left a comment

mattlord Nov 29, 2023

mattlord Nov 29, 2023

dbussink Nov 29, 2023

Allow for passing in the MySQL shutdown timeout #14568

Allow for passing in the MySQL shutdown timeout #14568

Conversation

dbussink commented Nov 21, 2023

Related Issue(s)

Checklist

vitess-bot bot commented Nov 21, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

dbussink Nov 21, 2023

Choose a reason for hiding this comment

deepthi Nov 21, 2023

Choose a reason for hiding this comment

dbussink Nov 22, 2023

Choose a reason for hiding this comment

mattlord Nov 29, 2023

Choose a reason for hiding this comment

dbussink Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

mattlord Nov 29, 2023

Choose a reason for hiding this comment

mattlord Nov 29, 2023

Choose a reason for hiding this comment

dbussink Nov 29, 2023

Choose a reason for hiding this comment

dbussink Nov 29, 2023 •

edited

Loading