vtorc: treat any /healthz response as reachable for IncapacitatedPrimary#19514
vtorc: treat any /healthz response as reachable for IncapacitatedPrimary#19514mhamza15 wants to merge 2 commits intovitessio:mainfrom
/healthz response as reachable for IncapacitatedPrimary#19514Conversation
`IncapacitatedPrimary` currently considers only an HTTP status code 200 as "primary reachable", and will only perform a recovery in that case (to avoid acting during a network partition). Tablets' `/healthz` can return a 500 in the case they are not serving, which prevents a recovery from happening. This changes the condition so that it considers any response as "primary reachable" and not just a 200. There is a case we've seen where a primary that could've been recovered through `IncapacitatedPrimary` would have likely been ignored as it was not serving at the time. Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
mhamza15
changed the title
/healthz response as reachable/healthz response as reachable for IncapacitatedPrimary
vitess-bot
bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
mhamza15
added
Type: Enhancement
| } | ||
| defer resp.Body.Close() | ||
| return resp.StatusCode == http.StatusOK, nil | ||
| return true, nil |
There was a problem hiding this comment.
Do we want to treat all of them as success? https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status Even 404?
There was a problem hiding this comment.
My opinion is that the main thing we're validating here is that the instance is reachable over the network, i.e. there is no network partition. From that lens, I don't think we should differentiate between any HTTP status codes. I think ideally, we would even switch to a TCP dial. But I think this is a simple middle-ground for now.
What do you think?
There was a problem hiding this comment.
Yeah, that's reasonable. That did align with my original thinking for making the call.
There was a problem hiding this comment.
@mhamza15 / @mattlord until I guess recent PRs, VTOrc talked only gRPCs to tablets (pooled for efficiency)
I'm a bit confused why we're talking to a 2nd non-gRPC port on tablets, ie: /healthz, etc. We can cross/use-both ports if there's a strong reason, but I'm not following what it is
If we want to know if a tablet is up, I suggest we use PingTablet and not a 2nd port without pooled connections
There was a problem hiding this comment.
I don't have a strong opinion here, but my thinking was that this simple HTTP request is a simpler and more direct way to confirm that we can simply talk to the process over the network. Fewer things involved.
Signed-off-by: Mohamed Hamza <mhamza@fastmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19514 +/- ##
===========================================
- Coverage 69.67% 0.00% -69.68%
===========================================
Files 1614 3 -1611
Lines 216793 116 -216677
===========================================
- Hits 151044 0 -151044
+ Misses 65749 116 -65633
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Description
IncapacitatedPrimarycurrently considers only an HTTP status code 200 as "primary reachable", and will only perform a recovery in that case (to avoid acting during a network partition). Tablets'/healthzcan return a 500 in the case they are not serving, which prevents a recovery from happening.This changes the condition so that it considers any response as "primary reachable" and not just a 200. There is a case we've seen where a primary that could've been recovered through
IncapacitatedPrimarywould have likely been ignored as it was not serving at the time.Edit:
This also makes another change to fix a flaky test: when I added
--log-format textas the default for tests, I removed some logic in a vtorc test that was parsing vtorc logs to calculate the time it took for a recovery to happen. That caused the test to be flaky, so I've made two changes:2026-02-27 17:46:41.123 UTCfor two reasons: one is to make it more global friendly instead of the American-oriented old format5:05 PM, and two is to add subsecond resolution for more accurate duration calculation.TestDeadPrimaryRecoversImmediately(the flaky test) so that it parses the time from the logs again and calculated recovery duration that way.Related Issue(s)
Checklist
Deployment Notes
AI Disclosure