Fix RemoveTablet during TabletExternallyReparented causing connection issues

[arthurschreiber]

Description

This pull request fixes an issue in the health checking logic where an external primary failover in a keyspace/shard can cause two primaries to be marked as healthy. This can occur when a tablet is removed during an external reparenting operation.

This bug can lead to queries being sent to the old primary, or in case the demoted primary vttablet process is restarted, will lead to vttablet: Connection closed errors (which are mostly invisible because queries will be retried automatically on the "actual" primary).

When RemoveTablet was called, the list of healthy tablets was recomputed, but the logic did not account for the fact that there can only be a single entry in the list of healthy primary tablets. This PR changes the logic in deleteTablet to mirror the special handling for PRIMARY tablets that also exists in updateHealth. This ensures that we can never end up in a situation where there is more than one healthy primary tablet for a given shard.

Related Issue(s)

Fixes: #16373

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

All modified and coverable lines are covered by tests ✅

Project coverage is 69.05%. Comparing base (eb29999) to head (4052306).
Report is 76 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16371      +/-   ##
==========================================
+ Coverage   68.69%   69.05%   +0.35%     
==========================================
  Files        1547     1556       +9     
  Lines      198297   202409    +4112     
==========================================
+ Hits       136228   139781    +3553     
- Misses      62069    62628     +559     

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

[arthurschreiber]

Added the code that fixes this issue, and updated the PR body description accordingly.

[deepthi]

Why does removing an unrelated tablet trigger the bug?

[deepthi]

Oh I see this

When RemoveTablet was called, the list of healthy tablets was recomputed, but the logic did not account for the fact that there can only be a single entry in the list of healthy primary tablets.

[arthurschreiber]

This comment is wrong, it should be

Suggested change

	// tablet 1 is the primary now
	// `secondTablet` should be the primary now

[deepthi]

This is an interesting bug. We have safeguards in updateHealth which make sure that there is exactly one element in healthy for the PRIMARY tablet type, but the recomputation here had no such checks.
While the fix seems correct, what do you think about pushing it into recomputeHealthy instead of creating a new function call here?
Right now this is the only call site where recomputeHealthy is called for the PRIMARY tablet type, but it seems more compact and future proof to change recomputeHealthy to maintain the invariant.

[arthurschreiber]

While the fix seems correct, what do you think about pushing it into recomputeHealthy instead of creating a new function call here?

I feel like having two separate methods helps highlighting the difference between the recomputation logic for primaries vs any other replica type more clearly.

I was thinking that maybe having a recomputeHealthy method that then calls out to either recomputeHealthyReplicas and recomputeHealthyPrimaries would probably be the best for clarity (and the go compiler should just inline all of these calls anyway, so not a performance issue either).

But then I noticed that pushing down the tablet type check into recomputeHealth would require passing down the key and tabletType as arguments, which seems redundant, because key contains the tablet type already, just as a string. Not passing down key would require passing down other arguments to be able to build the key, and then things just start to become more messy.

As I mentioned in Slack, I'm very interested giving the healthcheck code a polishing pass separately from this fix, so I'd suggest we punt on this for now. 🙇‍♂️ What do you think?

[deepthi]

I can see that instead of passing the "key", you would now have to pass the target. When this was written, it just seemed convenient to use the key because it has already been computed. So we have

			hc.recomputeHealthy(targetKey)

However, later on, we are doing a key computation for the second call

			oldTargetKey := KeyFromTarget(prevTarget)
			hc.recomputeHealthy(oldTargetKey)

It does not seem too invasive to change recomputeHealthy to be

func (hc *HealthCheckImpl) recomputeHealthy(target *query.Target) {
			key := KeyFromTarget(target)
			if target.TabletType == ..._PRIMARY {
			...
}

[deepthi]

Love it. Nice targeted fix to a long-standing issue.

[deepthi]

@arthurschreiber can you edit the description to match the final implementation?

[mattlord]

Thanks, @arthurschreiber! I only had a few minor questions and a nit.

I'll let you adjust the title and merge.

[mattlord]

Should we not allow it?

[arthurschreiber]

Unfortunately key is a string that concats a bunch of values, so checking whether the type is PRIMARY is not straightforward.

@deepthi and I discussed various options for refactoring this, but I feel like that shouldn't be part of this PR.

[mattlord]

Should we check and handle cases where healthy has a length > 1 just to be safe? I'm not sure, but the thought came up.

[arthurschreiber]

I think maintaining the invariant that healthy only ever contains a single entry for PRIMARY (and verifying this in the tests) seems "good enough" to me? I'm also not sure what the correct behaviour would be if we detect more than one entry in the list, because that means the invariant is broken and all guarantees are invalid.

[mattlord]

I'm curious why we do this rather than removing the key? Not implying it's somehow wrong. 🙂

[arthurschreiber]

Just mirroring what happens in updateHealth. 🤷