Make connection killing resilient to MySQL hangs #14500

arthurschreiber · 2023-11-09T10:17:13Z

Description

This fixes some issues around *Conn.Kill that can be encountered when MySQL "hangs" (e.g. either by sending it a SIGSTOP signal or when MySQL is writing a core dump file after a crash).

Kill has no context / timeout support at all. This means that when MySQL hangs, and some other code tries to send the KILL <id> statement to MySQL (e.g. due to setDeadline or because the code tries to call Kill directly), the Kill function is just going to hang as well.

I added a new function KillWithContext that allows passing in a Context object and correctly honors cancellation. I replaced setDeadline with direct calls to KillWithContext, with a 5 second timeout. I don't think a 5 second timeout actually makes any sense - but setDeadline intended to use a timeout of 2*elapsed + 5*time.Second (which never actually worked). Because KILL <id> on MySQL is supposed to return instantly, I think a much lower timeout would be sufficient.

In case the context passed to KillWithContext is canceled / timed out, there's nothing we can do - the client side connection on which we want to kill the executing statement was already closed anyway, and sending the KILL <id> statement is more or less best effort. So we just close the kill connection and return.

TODO

I want to go through all callers of Kill and either switch them over to pass a Context to higher level methods like Exec, or switch them to use KillWithContext instead.
This also requires more tests. Existing tests are all passing, but they didn't cover the "broken" cases.

Related Issue(s)

Fixes Bug Report: VTTablet & VTGate routing traffic to hung MySQL server #11884

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

Deployment Notes

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

vitess-bot · 2023-11-09T10:17:17Z

vitess-bot · 2023-11-20T05:31:01Z

Hello! 👋

This Pull Request is now handled by arewefastyet. The current HEAD and future commits will be benchmarked.

You can find the performance comparison on the arewefastyet website.

harshit-gangal · 2023-11-20T06:00:48Z

go/vt/vttablet/tabletserver/connpool/dbconn.go

+	resultChan := make(chan *sqltypes.Result, 1)
+	errChan := make(chan error, 1)

-	if done != nil {
-		close(done)
-		wg.Wait()
+	startTime := time.Now()


making channel on runtime for every call seems expensive to me.
I have added Benchmark me label to it as well.

This is no worse than the current code, which already creates a channel and a waitgroup. The current code, however, is very wasteful. So I think we can improve this to make this even less costly! Let me try...

vmg · 2023-11-20T09:26:53Z

@arthurschreiber: can you apply 996e652 to this PR? It significantly reduces all allocations we perform and should make us faster than the previous code, while still being correct

arthurschreiber · 2023-11-20T11:05:24Z

@vmg Thanks! I'm on it. 🙇‍♂️

arthurschreiber · 2023-11-20T11:07:56Z

@vmg Just for my understanding, the improvements come from not copying the current query string, and by creating one channel instead of two?

vmg · 2023-11-20T11:46:01Z

Yeah, the current string is obviously unrelated to your PR, I just noticed that we were using atomic.Value which is a very rough (type unsafe) API that has been superseded by atomic.Pointer.

The changes in the actual connection path boil down to always creating a single channel instead of two. I also noticed we were fetching the current time twice (one for the metrics and one for the potential Kill call) so I unified that. Overall I think this is a huge improvement. The previous setDeadline was expensive and definitely showing up in profiles, and with this new code we're gonna see half as many allocations, because we only have a channel per request now and previously we had a channel and a WaitGroup.

There are ways to make this zero allocation but I don't think we should attempt that for now.

Signed-off-by: Vicent Marti <vmg@strn.cat>

arthurschreiber · 2023-11-20T12:10:38Z

I worked on tests to fully cover this code, but the tests are using fakesqldb and that code has other problems that make it hard to write proper tests. 🫠 I will open another PR to fix the issues in fakesqldb, and then will add tests here.

github-actions · 2023-12-28T01:27:05Z

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2024-01-04T01:28:36Z

This PR was closed because it has been stale for 7 days with no activity.

Signed-off-by: Daniel Joos <danieljoos@github.com>

vmg

Looks great. Thanks for adding the tests!

vmg · 2024-03-04T09:12:45Z

I think the error in CI is related to the changes here, but looking at the test, it may just not be properly written/testing the right thing. Can you give it a look @arthurschreiber?

arthurschreiber · 2024-03-04T10:10:57Z

@vmg I kicked the tests off again. I could not see any relationship between the failures and the changes in this PR - did I miss something? 🤔

arthurschreiber · 2024-03-04T10:11:53Z

Oh, there is a different set of CI statuses on the commit and on the PR? Weird. I'm taking a look.

arthurschreiber · 2024-03-04T10:30:15Z

Ok, so from what I understand the failures in the mysql57 unit test build are all coming from the same test case?(TestSerializeTransactionsSameRow_ConcurrentTransactions). Digging into what's going on there.

…/fix-conn-kill-hang Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

arthurschreiber · 2024-03-04T11:07:25Z

I think I figured it out. The test case had an implicit dependency on the "incorrect" behaviour of fakesqldb.DB where only a single connection could be executing a query (and all other connections would have to wait implicitly for the callbacks of that first query to have finished executing). With that behaviour removed, the test case was failing.

I fixed it by making all 3 queries / transactions wait explicitly for the allQueriesPending channel.

vmg · 2024-03-04T12:04:51Z

Looks great! I figured the test was making wrong assumptions. Thanks again @arthurschreiber

GrahamCampbell · 2024-03-04T13:15:58Z

To be backported to v19?

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Daniel Joos <danieljoos@github.com> Co-authored-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Daniel Joos <danieljoos@github.com>

deepthi · 2024-03-06T16:33:28Z

To be backported to v19?

Sorry we missed this. The release is going out today. In future the way to get an auto-backport is to add a backport label before the PR is merged. I realize you don't have the permissions to do that, but @arthurschreiber does.

GrahamCampbell · 2024-03-06T16:50:09Z

We can probs just add the label to my 2nd PR, if it's wanted in 18.0 or 17.0. I don't personally care to see this backported earlier than 19.0. ;)

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com> Signed-off-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Daniel Joos <danieljoos@github.com> Co-authored-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Daniel Joos <danieljoos@github.com>

Make connection killing resilient to MySQL hangs (vitessio#14500)

arthurschreiber · 2024-05-30T09:01:41Z

Mh. I hoped adding backport labels will open backport PRs, but it doesn't look like that's working. :/ I'll open the backport PRs manually then.

GrahamCampbell · 2024-05-30T09:04:50Z

I don't think this was ever backported to v19 either?

GrahamCampbell · 2024-05-30T09:05:33Z

So I'm sure sure the vitess team will accept a v17 or v18 backport. My v19 backport was rejected: #15411.

arthurschreiber added 2 commits November 8, 2023 14:52

Add new KillWithContext function.

1bd34db

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

Remove setDeadline.

a0f8d66

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

arthurschreiber requested review from harshit-gangal, systay, shlomi-noach and rohit-nayak-ps as code owners November 9, 2023 10:17

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 9, 2023

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

arthurschreiber added Component: Query Serving Type: Bug and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Nov 9, 2023

harshit-gangal added the Benchmark me Add label to PR to run benchmarks label Nov 20, 2023

harshit-gangal requested a review from vmg November 20, 2023 05:31

harshit-gangal reviewed Nov 20, 2023

View reviewed changes

dbconn: reduce the number of channels

c4af426

Signed-off-by: Vicent Marti <vmg@strn.cat>

harshit-gangal approved these changes Nov 21, 2023

View reviewed changes

github-actions bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Dec 28, 2023

github-actions bot removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Feb 22, 2024

Fix detected race in dbconn_test.go

ee52f63

Signed-off-by: Daniel Joos <danieljoos@github.com>

danieljoos force-pushed the arthur/fix-conn-kill-hang branch from 38b8018 to ee52f63 Compare February 28, 2024 17:07

vmg approved these changes Mar 4, 2024

View reviewed changes

Merge branch 'main' of https://github.com/vitessio/vitess into arthur…

0f30e84

…/fix-conn-kill-hang Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

harshit-gangal approved these changes Mar 4, 2024

View reviewed changes

Fix test case to properly wait for all queries to be pending.

c1ca7b6

Signed-off-by: Arthur Schreiber <arthurschreiber@github.com>

vmg merged commit 7a2af6f into vitessio:main Mar 4, 2024
102 checks passed

arthurschreiber deleted the arthur/fix-conn-kill-hang branch March 4, 2024 12:06

GrahamCampbell mentioned this pull request Mar 6, 2024

[release-19.0] Make connection killing resilient to MySQL hangs #15411

Closed

frouioui modified the milestones: v19.0.0, v20.0.0 Mar 6, 2024

andyedison mentioned this pull request Apr 18, 2024

Make connection killing resilient to MySQL hangs (#14500) github/vitess-gh#104

Merged

3 tasks

arthurschreiber added a commit to github/vitess-gh that referenced this pull request Apr 23, 2024

Merge pull request #104 from github/backport-mysql-hangs

d8df704

Make connection killing resilient to MySQL hangs (vitessio#14500)

arthurschreiber added Backport to: release-17.0 Backport to: release-18.0 Needs to be back ported to release-18.0 labels May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make connection killing resilient to MySQL hangs #14500

Make connection killing resilient to MySQL hangs #14500

arthurschreiber commented Nov 9, 2023 •

edited by harshit-gangal

Loading

vitess-bot bot commented Nov 9, 2023

vitess-bot bot commented Nov 20, 2023

harshit-gangal Nov 20, 2023

vmg Nov 20, 2023

vmg commented Nov 20, 2023 •

edited

Loading

arthurschreiber commented Nov 20, 2023

arthurschreiber commented Nov 20, 2023

vmg commented Nov 20, 2023

arthurschreiber commented Nov 20, 2023

github-actions bot commented Dec 28, 2023

github-actions bot commented Jan 4, 2024

vmg left a comment

vmg commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024 •

edited

Loading

vmg commented Mar 4, 2024

GrahamCampbell commented Mar 4, 2024

deepthi commented Mar 6, 2024 •

edited

Loading

GrahamCampbell commented Mar 6, 2024

arthurschreiber commented May 30, 2024

GrahamCampbell commented May 30, 2024

GrahamCampbell commented May 30, 2024

Make connection killing resilient to MySQL hangs #14500

Make connection killing resilient to MySQL hangs #14500

Conversation

arthurschreiber commented Nov 9, 2023 • edited by harshit-gangal Loading

Description

TODO

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Nov 9, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

vitess-bot bot commented Nov 20, 2023

harshit-gangal Nov 20, 2023

Choose a reason for hiding this comment

vmg Nov 20, 2023

Choose a reason for hiding this comment

vmg commented Nov 20, 2023 • edited Loading

arthurschreiber commented Nov 20, 2023

arthurschreiber commented Nov 20, 2023

vmg commented Nov 20, 2023

arthurschreiber commented Nov 20, 2023

github-actions bot commented Dec 28, 2023

github-actions bot commented Jan 4, 2024

vmg left a comment

Choose a reason for hiding this comment

vmg commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024

arthurschreiber commented Mar 4, 2024 • edited Loading

vmg commented Mar 4, 2024

GrahamCampbell commented Mar 4, 2024

deepthi commented Mar 6, 2024 • edited Loading

GrahamCampbell commented Mar 6, 2024

arthurschreiber commented May 30, 2024

GrahamCampbell commented May 30, 2024

GrahamCampbell commented May 30, 2024

arthurschreiber commented Nov 9, 2023 •

edited by harshit-gangal

Loading

vmg commented Nov 20, 2023 •

edited

Loading

arthurschreiber commented Mar 4, 2024 •

edited

Loading

deepthi commented Mar 6, 2024 •

edited

Loading