bitflicker64 commented on PR #3132:
URL: https://github.com/apache/hugegraph/pull/3132#issuecomment-5612092139
## Correction and follow-up: the `/v1/ready` default is not safe on a
pre-1.8.0 PD image
In the campaign comment above I listed this as a gap:
> the claim that a pre-#3185 image would never turn Ready on `/v1/ready` is
derived, not
> shown, because no such image exists in this campaign.
I went and built one. **The claim is false, and the README says the opposite
of what
happens.** That seemed worth correcting quickly rather than leaving in place.
### The control
`bed2e457` is the parent of the #3185 merge, so it carries #3187 but neither
#3185 nor
#3189: a single variable. Built as `hugegraph/pd:pre3185` and swapped in for
PD only, chart
defaults untouched. A stamped revision label proves nothing about a jar, so
the control was
validated by the **absence of `hg_raft_*` metrics** on the running pod,
those being #3185's
own, and by its layer set differing from the merged build.
### What the README promises
> a PD image that predates the fix: `/v1/ready` does not exist there, so the
PD never turns
> Ready and Stores never leave Init. Set both values back to `/v1/health` on
such an image.
A loud, blocking, self-correcting failure.
### What happens
**All three PD pods reported `Ready=True`. Not one probe failed.**
| Path | pre-#3185 (`bed2e457`) | merged master (`60c8803d`) |
|---|---|---|
| `/v1/health` | 200, empty body, real handler | 200 |
| `/v1/ready` | **200**, body `{"error":"Unauthorized!","status":-1}` | 200,
real quorum-aware handler |
| `/v1/thispathdoesnotexist` | **200**, same Unauthorized body | **401** |
| `/v1/ready/alsobogus` | **200**, same body | **401** |
| `hg_raft_*` metrics | 0 | 3 |
`/v1/ready` on the old image is not an endpoint. It is **B5**: the pre-#3189
interceptor
writes its error body and returns without setting a status, so *every*
unmapped path under
`/v1/` answers 200. The arbitrary nonsense path answering 200 identically is
what proves it.
On merged master those same paths correctly return 401, which is #3189
working, and that is
the only reason this is invisible on current images.
**Why this is worse than the documented failure.** The documented one is
safe: nothing
starts, the operator reads Limitations, sets `/v1/health`. The real one is
silent: the
cluster comes up, every PD claims Ready unconditionally, and readiness
conveys nothing. It
is strictly worse than the `/v1/health` it replaced, which at least reflects
the listener,
and under a quorum loss such a PD keeps reporting Ready, which is #3183
reintroduced by the
new default on old images. This reaches released **1.7.0**, and this PR
defaults
`tag: latest`.
The remedy in the README works. The problem is that nothing tells an
operator to apply it.
Suggested wording, no code change: say that on a PD image predating #3189
every unmapped
`/v1/` path returns 200, so `/v1/ready` **silently always passes** and
readiness becomes
meaningless; therefore both paths **must** be set to `/v1/health` on such an
image, and
there is no symptom to warn you. The 1.8.x pin this PR already promises also
resolves it.
### The rest of the follow-up run
Same merged master, so nothing that already passed was re-run. Ten of the
eleven gaps I
listed are now closed.
| Gap | Now |
|---|---|
| **Clean single-node PD leader partition with a write oracle** |
**PASS-hardening.** All three earlier confounds fixed: victim from
`/v1/members`, cut inside the Pod netns between the leader and its peers only,
teardown via the Pod **sandbox** PID. `/v1/ready` went 503 at T0+2s;
**`/v1/health` stayed 200 on 200 of 200 samples**; leadership moved; 0
restarts; healed unaided |
| **EndpointSlice ready-condition membership** | **Closed.** The partitioned
PD's address flips to `ready=False` at T0+27s, tracking the pod condition
exactly |
| **`hg_raft_has_leader` reaching 0** | **Measured**, 1.0 to 0.0 at T0+3s.
No longer carried |
| **S11's last link** | **Measured.** `-t 10` takes exactly 10 s and `-t 30`
exactly 30 s, both printing `The operation timed out(Ns)`. A dead pid returns
in 0 s, so raising 120 s to 450 s does not slow crash detection |
| **kubescape** | **Run.** NSA 87.0 on the tested tree and 87.5 on this PR,
MITRE 100.0 on both. Three failing NSA controls, all already-documented
decisions |
| **Store majority loss** | **PASS-hardening.** 7 of 40 writes timed out,
full recovery at T+209s, data intact, PVCs rebound |
| **Cross-namespace REST** | **PASS.** Auth holds across the boundary; 401 /
401 / 200 on both Server and PD |
| **Node freeze** | **PASS-hardening.** NotReady at T+55s, 2 of 50 writes
lost, rejoined 10 s after unfreeze |
| **The N1 fix** | **Verified.** Lint clean, renders unchanged at 19/17/20,
63 tests pass, and one patch applies cleanly to both branches |
| Upgrade from a released chart version | **Still NOT-RUN.** Re-checked:
latest release is 1.7.0, no 1.8.x tag, Docker Hub has only `latest`,
`helm-dev`, `1.7.0` |
**One cost the partition exposed.** Recovery from a *partitioned* leader is
bounded by
`raft.rpc-timeout`, default 10000 ms (`PDConfig.java:187`, wired at
`RaftEngine.java:136-138`), not by the election timeout: a blackhole sends
no RST, so every
attempt to reach the unreachable ex-leader blocks for the full timeout.
Measured **67 s**
leaderless, against **5 s** for a `kill -9` of the leader, and for **47 s**
of it the
`pd-client` Service had zero ready endpoints because all three PDs correctly
answered 503.
Worth a docs line; the chart does not expose that value.
### Corrections to my own method, since two of them nearly became numbers
here
- My first scanner run measured **the wrong tree**. `git worktree add
feat/hstore-helm-chart`
resolved to an older branch of that name on a different remote, which
renders 13/11/13.
Caught only because it did not match the 16/14/16 I had measured earlier.
The right head
comes from `git fetch origin pull/3132/head`.
- A Store majority-loss sampler reported `3/3` ready throughout, which is
false: a
StatefulSet recreates deleted pods under the same names, so counting rows
never dips. Only
uid and start time show the replacement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
