Add a way to know if DemotePrimary is blocked and send it in the health stream

[GuptaManan100]

Description

This PR adds the feature requested in #17288.

We spawn a new goroutine when we start DemotePrimary and when enough time has passed such that DemotePrimary hasn't finished despite context cancellation, we deem it to be blocked. In this case we send an error in the health stream that the users can track and use to restart MySQL and vttablet.

Related Issue(s)

• Fixes Feature Request: Way to detect DemotePrimary is blocked #17288

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]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: Patch coverage is 44.82759% with 16 lines in your changes missing coverage. Please review.

Project coverage is 67.52%. Comparing base (8648264) to head (0b8c3f4).
Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vttablet/tabletmanager/rpc_replication.go	43.75%	9 Missing ⚠️
go/vt/vttablet/tabletserver/tabletserver.go	0.00%	5 Missing ⚠️
go/vt/vttablet/tabletservermock/controller.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17289      +/-   ##
==========================================
+ Coverage   67.40%   67.52%   +0.11%     
==========================================
  Files        1574     1581       +7     
  Lines      253205   253969     +764     
==========================================
+ Hits       170676   171492     +816     
+ Misses      82529    82477      -52     

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

[deepthi]

We don't seem to ever reset this. Is that because once it is stalled the only solution is to restart the tablet?

[GuptaManan100]

Yes, exactly!

[maxenglander]

if we read the end of demotePrimary and we have called SetDemotePrimaryStalled, what is the correct course of action? it seems like we're assuming this will never happen. should we do something like block forever without returning, to ensure that the tablet doesn't accidentally make forward progress or re-enter the set of serving tablets?

[GuptaManan100]

Yes, there is an inherent race between the finishCtx completing (DemotePrimary finishing) and the timeout triggering. For that matter, DemotePrimary can unblock and finish, after we've marked the tablet as Stalled. If it is successful, even then I don't really see an issue with the tablet rejoining the serving tablets, until it is eventually restarted by the operator.

[maxenglander]

even then I don't really see an issue with the tablet rejoining the serving tablets, until it is eventually restarted by the operator.

hm I think that is potentially a problem. because if the operator gets notified that a tablet is stalled, it's going to forcefully throw that tablet away with the assumption that (a) there is another tablet that is the real primary and (b) the stalled primary is not serving any traffic. if the stalled primary is able to rejoin the set of serving tablets, both of those assumptions go out the window, and it is unsafe for the operator to safely throw it away.

[GuptaManan100]

Yes, that is true, this would trigger an ERS. Let me see if we can make the tablet not become serving ever again if it is stalled.

[GuptaManan100]

Okay! I added few more safeties to ensure nothing goes wrong -

1. After we set DemotePrimaryStalled we immediately trigger a health check update, which would make vtgate mark this tablet not-serving and not send it any requests ever again, because we never clear the field.
2. For any requests already sent, if DemotePrimaryStalled is set, we won't process it on vttablet and instead just return an error.

I think with these safeguards we can be sure htat a vttablet is not going to accept any new writes once we mark it as stalled.

WDYT @maxenglander? Let's also wait for @deepthi to be able to take a look.

[maxenglander]

looks good!