Add StalledDiskPrimary analysis and recovery by vtorc #16050

joekelley · 2024-06-04T15:33:56Z

Description

At HubSpot we have had a handful of incidents where a primary becomes impaired due to disk issues. When this happens, we observe that vtorc assigns an UnreachablePrimary analysis and does nothing because the FullStatus
call it makes to the tablet times out. We monitor for these cases outside of Vitess and resolve them by running ERS, but it would be ideal if vtorc could detect and address these cases itself.

This change adds support for a StalledDiskPrimary analysis and recovery by vtorc. To detect when a tablet has a stalled disk we add a FileSystemManager that attempts to write a file to vt data root every five seconds and expose a method that the tablet manager invokes in FullStatus to report whether the disk is stalled. Vtorc is modified to check for the stalled disk error from FullStatus and record the result in the Cleanup block.

We are in the process of testing a change to this effect in lower environments at HubSpot. We aren't running the latest version of Vitess, so our internal patch is a bit different than what is presented here and this exact implementation hasn't been tested. This is my first Vitess PR. Any and all feedback is greatly appreciated 🙂

Related Issue(s)

Fixes: Feature Request: Add support for StalledDiskPrimary recoveries by vtorc #16375

Slack discussion from this time last year: https://vitess.slack.com/archives/C02GSRZ8XAN/p1685456224040299
PR that came from that discussion but didn't get merged: #13207

Note that this implementation is heavily inspired by the comments on #13207.

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

vitess-bot · 2024-06-04T15:34:00Z

codecov · 2024-06-04T23:04:56Z

Codecov Report

Attention: Patch coverage is 72.63158% with 26 lines in your changes missing coverage. Please review.

Project coverage is 68.66%. Comparing base (f4591fb) to head (abd5444).
Report is 330 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vttablet/tabletmanager/filesystemmanager.go	78.78%	14 Missing ⚠️
go/vt/vtorc/inst/instance_dao.go	54.54%	5 Missing ⚠️
go/vt/vttablet/tabletmanager/tm_init.go	25.00%	3 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_replication.go	0.00%	2 Missing ⚠️
go/vt/vtorc/config/config.go	66.66%	1 Missing ⚠️
go/vt/vtorc/logic/topology_recovery.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16050      +/-   ##
==========================================
+ Coverage   68.23%   68.66%   +0.43%     
==========================================
  Files        1541     1549       +8     
  Lines      197254   199172    +1918     
==========================================
+ Hits       134597   136763    +2166     
+ Misses      62657    62409     -248

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

GuptaManan100 · 2024-06-19T10:28:26Z

go/vt/vttablet/tabletmanager/rpc_replication.go

+	// Return error if the disk is stalled or rejecting writes.
+	// Noop by default, must be enabled with the flag "enable_stalled_disk_check".
+	if tm.fsManager.IsDiskStalled() {
+		return nil, errors.New("stalled disk")
+	}


I think it's better to have a boolean field or an error field here. Even if the disk is stalled, we do get all the other field information back in FullStatus that can be used. Also, an error in full status indicates to vtorc that it couldn't reach the vttablet.

I developed this change by using fsfreeze -f /vt on the primary instance in a test keyspace. In my testing before making any code changes, I found that vtorc's invocation of FullStatus would timeout and return a context deadline exceeded error. Simple queries like select @@global.server_id; would hang until the filesystem was unfrozen.

We could add a boolean or error field to model this, but I think we'd want to return here either way if the check fails, and it seems a bit cleaner to me to return an error rather than a response message with mostly nil/zero values.

I don't think it's a good architectural design to rely on the error message containing stalled disk. It is better to have that as a boolean field in the FullStatus output. The way the code is written now, we will always have to have the error message have stalled disk for backward compatibility. We can leave all other fields empty if this new boolean is set, but in my opinion we shouldn't rely on the contents of the error message.

I agree with @GuptaManan100 here, a boolean in FullStatus would also allow users of vtctldclient to see the status using GetFullStatus

Makes sense, I'll make this change 👍

GuptaManan100 · 2024-06-19T10:31:24Z

go/vt/vtorc/inst/instance_dao.go

 	if err != nil {
+		if config.Config.EnableStalledDiskPrimaryAnalysis && strings.Contains(err.Error(), "stalled disk") {
+			stalledDisk = true
+		}
 		goto Cleanup


If we don't send the stalled disk as an error back as suggested ☝️, we can set the value in the normal flow below and it would be written as part of mkInsertOdkuInstances, and we won't need to change UpdateInstanceLastChecked.

timvaillancourt · 2024-06-21T22:18:05Z

go/vt/vttablet/tabletmanager/filesystemmanager.go

+type writeFunction func() error
+
+func attemptFileWrite() error {
+	file, err := os.Create(path.Join(env.VtDataRoot(), ".stalled_disk_check"))


Are there any cases where VTDATAROOT isn't the same disk as MySQL datadir? For our deployment it IS the same disk - our datadir is one level below VTDATAROOT. But theoretically datadir could be on it's own disk 🤔

It might be more flexible to have a flag like --stalled_disk_check_dir / stalled_disk_check_root enable this feature vs a boolean "enable" flag

go/vt/vttablet/tabletmanager/tm_init.go

timvaillancourt · 2024-06-21T22:26:27Z

@joekelley this is awesome, thanks a lot for moving this forward!

joekelley · 2024-07-10T18:21:54Z

Thanks for the suggestions here! I pushed a few changes to improve the readability of the file system manager and fixed a bug where it would attempt to run concurrent file writes in the event that a write exceeded the timeout. Now it will skip attempting to write a file if there is already a slow write in progress.

Additionally I increased the default write timeout to 30s. We found in our testing that 2s was far too low and would lead to many false positives and unnecessary failovers. By increasing to 30s we hope to only capture disk stalls that would lead to prolonged downtime rather than transient blips in the storage layer.

deepthi · 2024-07-11T00:11:45Z

@joekelley I'll get this re-reviewed, but in the meantime, can you fix the failing DCO check? We won't be able to merge without that. Once that is fixed, we can re-run CI as well.

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

…cess failovers Signed-off-by: Joe Kelley <jkelley@hubspot.com>

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

deepthi · 2024-07-11T23:13:40Z

Looks like there isn't a linked issue. That is something we require for anything other than trivial doc changes or code cleanup type of PRs. It will be good if you can create one, and also look at the test failures in order to get those out of the way before reviewers come back.

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

timvaillancourt · 2024-07-12T16:05:29Z

Following this change, something I'd to explore is MySQL itself providing the signal that the disk is stalled. I wasn't able to find this easily in #13207 and opted for a similar approach as here, but I imagine there is a way now or in the future, with some sort of performance_schema query or metric that doesn't exist yet (blocked/stalled writes?) - for now MySQL just waits forever

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

joekelley · 2024-07-16T14:10:31Z

Looks like the most recent build was successful except for the PR Labels check. I added a linked issue but don't have permission to remove the NeedsIssue label, could someone take care of that for me?

rvrangel · 2024-07-17T17:30:35Z

go/vt/vttablet/tabletmanager/filesystemmanager.go

+	return fs
+}
+
+func (fs *pollingFileSystemManager) poll(ctx context.Context) {


this function seem to work well for preventing stalling itself in case the disk is frozen, but I was wondering if there are other opportunities for parts of vitess being stalled and still causing an issue depending on how the environment is setup.

for example, we don't log directly to disk but rely on rsyslog to capture and the STDERR and route the logs appropriately. if the disk is stalled, rsyslog will be stalled, not the vttablet. but perhaps if you log directly to disk via --log_dir and don't have a separate thread handling the logging mechanism, whenever you log a message the particular thread will try to perform an I/O operation and be blocked (we saw this in a different system, not vitess, because we were logging directly to disk).

so I would be curious if you could test with --logdir (or maybe you already are!) and it allows vtorc to properlly detect and failover the faulty host.

Interesting -- I don't see why this check wouldn't work if you set --stalled-disk-write-dir to the same directory as --log_dir in the event that directory could not be written to. Is your suggestion to run the stalled disk check against the --log_dir in addition to whatever data directory the user provides with --stalled-disk-write-dir? Or even to be able to provide multiple directories as arguments and poll each separately?

yeah, I was wondering if other threads that might be writing to disk directly could cause other parts of vitess to be I/O blocked, specially if we are logging something. setting both --log_dir and --stalled-disk-write-dir to directories in the same filesystem and freezing it would be a good additional test, in case you are only logging to STDERR right now. if you are already writing to the same filesystem and this is working, great!

I think the main goal for VTOrc detection/remediations is to prevent availability/durability problems, so if the log dir being unwritable doesn't impact query serving I'm on the fence if that would be a reason to failover, but an unwritable data dir is always a good reason if we are sure

I'm curious what @GuptaManan100 thinks about this detail?

I agree with @timvaillancourt. Not being able to log probably shouldn't trigger an automatic failover. If that is a use-case we want to address, I think it would be better to add a metric around not being able to log, and let the users see the metric on vttablet page and if they deem it worthwhile, they can run the ERS manually.

I agree with that, my question is specifically towards if by enabling --logdir and having other threads (including the ones that can impact availability) be blocked because they are trying to log something and the flags makes it go to disk instead (or in addition) to STDERR. if the disk is stalled, they might be blocked as well. I am not worried if we stop logging because the disk is stalled but VTOrc still does the right thing.

I see. If some tablet thread(s) can't flush logs to disk then some critical work could be blocked.

I had forgotten this but early in my iterations on the feature I believe I encountered this phenomenon. I had many debug logs around my changes in filesystemmanager.go and I found that the filesystem manager was not reaching the line set stalled = true because it was blocked on a log line, presumably because the logs were unable to flush to disk. I have not seen this issue in my testing since removing the log lines from the critical section but it may be possible that a log line elsewhere the tablet code could be breaking in this way.

IMO if we can establish confidence that the StalledDiskPrimary feature is not broken by a blocking log statement we should proceed with this change as-is. Separately it could be worthwhile to audit which (or if any) critical tablet functionality is broken when logs can't be flushed to disk.

yeah, I don't mean to block this at all, I think it is a good change to have (we had to have our automation to handle this particular case). But just wanted to mention it might still stall and prevent some failover under some conditions, and it would be good to audit those (not necessarily as part of this PR)

joekelley · 2024-07-29T17:53:32Z

We've noticed in our testing that when we run fsfreeze on the primary's vtdataroot to trigger the StalledDiskPrimary recovery, we are left with serving co-primaries as in #14637. We have automation that quickly kills the old primary tablet but I wanted to call attention to that here.

github-actions · 2024-08-29T01:32:25Z

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.

GuptaManan100 · 2024-08-29T03:19:59Z

Hello @joekelley, just a couple of review comments are left to be addressed - #16050 (comment), before we can merge this PR.

timvaillancourt · 2024-08-29T14:36:27Z

@joekelley totally optional/deferrable: something I've wondered since #13207 was written (during 5.7 days with older kernels): is there a more elegant way now for us to detect a stalled disk? Something about writing a file to check the disk has never sat right with me

The two areas of thought I had were:

Is there a signal in performance_schema, show status, etc we're not thinking of?
Is there a value in /prod/$PID/* that already tells us we are blocked on writes?

I'll try to find the time to validate those ideas but I was curious what you thought. Of course, writing a temp file is valid if required

joekelley · 2024-08-29T15:02:41Z

Thanks for the feedback! I'll address asap.

(Commenting here to satisfy the stale check because I may not get to the requested changes this week and I can't update labels on the PR)

rbranson · 2024-08-30T23:37:17Z

I'm extremely curious why this causes an UnreachablePrimary and not a DeadPrimary. It is surprising that the tablet cannot perform basic status queries against MySQL but there are intact replicating replicas.

erikhalperin-hub · 2024-09-24T22:38:23Z

Hey @GuptaManan100 - sorry for the delayed response here, but we've put this PR down and have stopped using StalledDiskPrimary for the time being because of a rare and complicated series of events that caused data loss. What happened went something like:

The disk of the primary stalled
Vtorc detected an SDP and ran an ERS
The old primary demotion got stuck and didn't finish because of what Joe described above. Importantly, demotion did not yet get to the part where it killed the in flight queries
Shortly after, the old primary's disk became healthy again, and there was a race condition between demotion and vtorc that caused semi-sync to be turned off on the old primary before the in-flight queries were killed, so those in flight queries completed and returned success to the client. However, these queries were now errants on a replica, which got wiped. The details on the race condition are a bit hazy.

There is a fix to #14637 but we haven't yet had a chance to try it because we're fairly far behind and there are some pre-requisites to getting that fix out. Ultimately, our plan is to get vitess up-to-date (and keep it there) and then return to the DemotePrimary fix as well as this StalledDiskPrimary.

erikhalperin-hub · 2024-09-24T22:42:54Z

I'm extremely curious why this causes an UnreachablePrimary and not a DeadPrimary. It is surprising that the tablet cannot perform basic status queries against MySQL but there are intact replicating replicas.

That's a good question. What we've observed when testing freezing the filesystem with fsfreeze is that mysql "locks up" in many ways but it keeps running. I'm not familiar with replication internals but my guess would be that replicas are querying the primary at a pretty basic networking level which is still responsive here.

erikhalperin-hub · 2024-09-24T22:49:46Z

@timvaillancourt

is there a more elegant way now for us to detect a stalled disk? Something about writing a file to check the disk has never sat right with me

The two areas of thought I had were:

Is there a signal in performance_schema, show status, etc we're not thinking of?

Is there a value in /prod/$PID/* that already tells us we are blocked on writes?

I'll try to find the time to validate those ideas but I was curious what you thought. Of course, writing a temp file is valid if required

Writing to disk has been our best signal. Mysql in our tests with fsfreeze is basically completely unqueriable, even for things like show status. We considered using an external metric such as amazon CloudWatch or linux kernel-level metrics but writing to disk seemed to be the simplest solution.

timvaillancourt · 2024-10-03T00:32:01Z

go/vt/vttablet/tabletmanager/filesystemmanager.go

@@ -0,0 +1,131 @@
+package tabletmanager


Suggested change

package tabletmanager

/*

Copyright 2024 The Vitess Authors.

Licensed under the Apache License, Version 2.0 (the "License");

you may not use this file except in compliance with the License.

You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software

distributed under the License is distributed on an "AS IS" BASIS,

WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and

limitations under the License.

*/

package tabletmanager

timvaillancourt · 2024-10-03T00:32:18Z

go/vt/vttablet/tabletmanager/filesystemmanager_test.go

@@ -0,0 +1,103 @@
+package tabletmanager


Suggested change

package tabletmanager

/*

Copyright 2024 The Vitess Authors.

Licensed under the Apache License, Version 2.0 (the "License");

you may not use this file except in compliance with the License.

You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software

distributed under the License is distributed on an "AS IS" BASIS,

WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

See the License for the specific language governing permissions and

limitations under the License.

*/

package tabletmanager

joekelley · 2024-10-03T15:29:02Z

Closing this per Erik's comment #16050 (comment)

Thanks for all the feedback and discussion here.

joekelley requested review from deepthi, GuptaManan100, rohit-nayak-ps and shlomi-noach as code owners June 4, 2024 15:33

github-actions bot added this to the v21.0.0 milestone Jun 4, 2024

GuptaManan100 reviewed Jun 19, 2024

View reviewed changes

joekelley requested a review from timvaillancourt as a code owner June 21, 2024 18:25

timvaillancourt reviewed Jun 21, 2024

View reviewed changes

go/vt/vttablet/tabletmanager/tm_init.go Outdated Show resolved Hide resolved

Joe Kelley added 6 commits July 11, 2024 13:45

Add StalledDiskPrimary analysis and recovery

e04dc10

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

update expected help text txt files

13bc7a7

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

address lint issue, update naming

9753c01

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

make write timeout and interval configurable, much longer to avoid ex…

8568cdd

…cess failovers Signed-off-by: Joe Kelley <jkelley@hubspot.com>

refactor to prevent concurrent writes, improve readability

1be0fde

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

clean up init flags with improved semantics, naming, and defaults

0f4af18

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

joekelley force-pushed the stalled-disk-primary branch from 00da2af to 0f4af18 Compare July 11, 2024 17:46

Joe Kelley added 2 commits July 12, 2024 11:21

add type to expected flag help text

160dd7e

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

address data race in mock writer

720699c

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

reorder stalled disk flags, add default values to expected help outputs

abd5444

Signed-off-by: Joe Kelley <jkelley@hubspot.com>

joekelley force-pushed the stalled-disk-primary branch from b7659d2 to abd5444 Compare July 12, 2024 16:46

timvaillancourt removed the NeedsIssue A linked issue is missing for this Pull Request label Jul 16, 2024

rvrangel reviewed Jul 17, 2024

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 Aug 29, 2024

deepthi removed the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Aug 29, 2024

timvaillancourt reviewed Oct 3, 2024

View reviewed changes

joekelley closed this Oct 3, 2024

-package tabletmanager
+/*
+Copyright 2024 The Vitess Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package tabletmanager

Add StalledDiskPrimary analysis and recovery by vtorc #16050

Add StalledDiskPrimary analysis and recovery by vtorc #16050

Conversation

joekelley commented Jun 4, 2024 • edited by mattlord Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jun 4, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvaillancourt Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

timvaillancourt commented Jun 21, 2024

joekelley commented Jul 10, 2024

deepthi commented Jul 11, 2024

deepthi commented Jul 11, 2024

timvaillancourt commented Jul 12, 2024 • edited Loading

joekelley commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvaillancourt Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joekelley commented Jul 29, 2024

github-actions bot commented Aug 29, 2024

GuptaManan100 commented Aug 29, 2024

timvaillancourt commented Aug 29, 2024 • edited Loading

joekelley commented Aug 29, 2024

rbranson commented Aug 30, 2024

erikhalperin-hub commented Sep 24, 2024

erikhalperin-hub commented Sep 24, 2024

erikhalperin-hub commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joekelley commented Oct 3, 2024

joekelley commented Jun 4, 2024 •

edited by mattlord

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

timvaillancourt Jun 21, 2024 •

edited

Loading

timvaillancourt commented Jul 12, 2024 •

edited

Loading

timvaillancourt Jul 17, 2024 •

edited

Loading

timvaillancourt commented Aug 29, 2024 •

edited

Loading