helm,ci,k8s-smoke: the chart stops encoding the leader — replicated renders candidates (#284) - #344
helm,ci,k8s-smoke: the chart stops encoding the leader — replicated renders candidates (#284)#344allamiro wants to merge 4 commits into
Conversation
…enders candidates (#284) The binary's role has followed the metadata lease since #343; this is the chart catching up, which values.yaml itself promised: 'the durable fix is for the role to follow the lease inside the binary, so the chart stops encoding it at all.' data.leaderOrdinal is RETIRED, not ignored: setting it fails the render with the migration story, because a value that used to steer roles deserves an explanation, not silence. The replicated topology now renders every pod as a CANDIDATE with one shared peer list (self included — the binary skips its own entry, and identical lists keep the configs diffable), and two more renders-then-cannot-work shapes fail at render time: replicated without the lease (no pod would ever lead) and a nonzero fencing floor (grants are minted from 1, and a floor at or above the first grant refuses the very grant that makes a candidate lead). The CI render check now asserts the symmetric shape — parsing, not pattern-matching, for the reason recorded beside it — and the refusal checks cover all three guards by their messages. The k8s smoke test stops assuming pod 0 leads, because nothing renders that anymore: it asks metadata for the holder, aims the produce there, and then does the thing the topology exists for — deletes the holder's pod gracefully (the #280 drain releases the lease rather than lapsing it), waits out the corpse for a live holder, produces thirty more records against whoever that is, and asserts every replica including the recreated pod converges on all ninety. A recreated pod that wins its own range back is a legitimate outcome of that race and is logged as such; scenario 14 already proves the takeover-by-a-survivor path with a kill no orchestrator softens. Chart version 0.2.0: a values-contract change earns the minor bump. Rendered and refusal-checked locally; the kind deploy job carries the live proof.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15ebe407f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {{- if eq $v.data.topology "replicated" }} | ||
| {{- /* Both of these render perfectly well and then cannot work, which is the | ||
| worst kind of configuration error: a leaderOrdinal outside the replica | ||
| set leaves EVERY pod a follower and the range with no leader at all, | ||
| and a single-replica replicated install gives a leader with no | ||
| followers, which is standalone wearing the wrong name. */ -}} | ||
| {{- if ge (int $v.data.leaderOrdinal) (int $v.replicaCount) }} | ||
| {{- fail (printf "\n\ndata.leaderOrdinal is %d but replicaCount is %d: the leader must be one of the pods, or the range has no leader at all." (int $v.data.leaderOrdinal) (int $v.replicaCount)) }} | ||
| {{- if hasKey $v.data "leaderOrdinal" }} | ||
| {{- fail "\n\ndata.leaderOrdinal is retired (#284): \"replicated\" now renders every pod as a CANDIDATE and the role follows the metadata lease, so failover no longer needs a re-render. Remove the value." }} |
There was a problem hiding this comment.
Move this retirement check outside the replicated branch. With data.topology=standalone, a values file can still set data.leaderOrdinal and render successfully, contrary to the new contract that any retained value must fail loudly; this leaves stale configuration unnoticed until a later switch to replicated unexpectedly fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real, fixed in c9a1bb3: the guard moves BEFORE the topology branch. Retired is retired in every shape — a standalone values file carrying the key rendered happily and would have ambushed its author the day they switched to replicated, which is the moment they can least afford a surprise. Both cases are now exercised in CI (standalone and replicated) by message.
| read -r NEW_HOLDER NEW_EPOCH <<< "$(await_holder)" \ | ||
| || fail "the range had no holder 180s after the pod delete" |
There was a problem hiding this comment.
When the best-effort graceful lease release times out or is refused, metadata legitimately retains the deleted holder and its old epoch until expiry, but this call accepts that stale pair immediately because it passes no exclusion. The recreated pod can then become Ready as a follower after another candidate wins, while r_produce still targets the old holder with the stale epoch and reports a false failover failure; accept the same UUID only after its epoch advances, or a different UUID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real, and the sharper form of it: my own helper took an exclusion parameter and the call site did not pass it. Fixed in c9a1bb3 with a stronger condition than exclusion — wait for a fencing epoch ABOVE the one observed before the delete. Metadata mints epochs monotonically, so 'epoch > old epoch' is exactly 'somebody has been granted the range since', and it holds whether a survivor took over or the recreated pod won its own range back (an exclusion would wrongly reject that legitimate outcome and turn it into a flake). The failure message now names the epoch it waited above.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…collector the code promised, and four review fixes The kind deploy failed on the new failover phase, and the failure was real: quorum produce succeeded on the holder, and every pod then reported nothing. Candidate mode registered NO role collector — the first slice said so in a comment and named the follow-up — so vtop_broker_local_committed_offset simply did not exist on a candidate pod. That is the metric this repo's own chaos harness reads to choose a promotion target, the metric the k8s smoke asserts, and the metric every dashboard draws. A replica nobody can measure is one nobody can operate, and the gap survived a ten-round review because nothing ever asked the question. The follow-up is now the fix. ONE collector, registered for the life of the process, reading through the switching view: registering a role-specific collector per transition never could have worked — the leader's and follower's collectors export the same descriptors, the registry refuses duplicates, and there is no unregister — so the first transition would either fail or leave the process exporting a role it no longer had. Reading through the view instead keeps the NAMES stable across every transition, which is what lets a panel built for a statically-rendered range keep working against a candidate deployment. It is deliberately not the promotion probe's trait: that one blocks on purpose because a probe must have the true offset, while a scrape must never block and can afford a stale gauge. A new broker_candidate_leading answers the one question candidate mode adds — who serves the range — from metrics alone. Scenario 14 now asks all of it of BOTH roles, because this suite is the composition proof and this walked straight through it. The four review findings, every one verified: data.leaderOrdinal was only refused under the replicated topology, so a standalone values file carrying it rendered happily and would ambush its author the day they switched. Retired is retired: the guard moves before the branch. The smoke's failover wait passed no exclusion, so it could accept the DELETED holder's still-unexpired lease and then produce at a stale epoch — my own helper offered the parameter and the call site did not use it. The fix is sharper than an exclusion: wait for an epoch above the one we saw. Metadata mints epochs monotonically, so that is exactly "somebody has been granted the range since", and it stays true whether a survivor took over or the recreated pod won its own range back. The CI render check proved three distinct ADDRESSES but never checked identities — three addresses under two UUIDs would render, deploy, and count a two-node quorum as three. Peer and pod identities are now checked against the configured set. And values.yaml said in one paragraph that automatic failover was not backed while the next promised failover without a re-render. The preamble is corrected, the lease block's documentation moved back onto the key it describes forty lines below it, and the fencing floor's render-time requirement stated where somebody would hit it. Scenario 14 PASS with the new pins; 68 vtop-node tests, clippy, fmt; every chart refusal exercised locally.
cac2345 to
c9a1bb3
Compare
Round one: five findings, all real, all fixed in c9a1bb3Four came from review; the fifth came from CI, and it was the most valuable thing on this PR. The kind deploy caught a real observability gap. The new failover phase failed with So the follow-up is now built: one collector for the life of the process, reading through the switching view. Per-transition role collectors could never have worked (same descriptors, registry refuses duplicates, no unregister), and reading through the view keeps the metric names stable across every transition — which is exactly what lets a dashboard built for a statically-rendered range keep working against a candidate deployment. It deliberately does not reuse the promotion probe's trait: that one blocks on purpose because a probe needs the true offset; a scrape must never block and can afford a stale gauge. New Scenario 14 now pins all of it against both roles, since the suite is the composition proof and this walked straight through it. The four review findings
ValidationScenario 14 PASS with the new observability pins; 68 vtop-node tests; clippy and fmt clean; every chart refusal (five of them) exercised locally; the CI render assertions transliterated and checked against the real render before pushing. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9a1bb34f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # that is following it. One collector answers for whichever role holds the | ||
| # range, and the names must not change when the role does — that is what | ||
| # lets a panel built for a statically-rendered range keep working here. | ||
| FOLLOWING=$(( NEW_LEADER == 2 ? 3 : 2 )) |
There was a problem hiding this comment.
When the original WINNER is candidate 2 and failover elects candidate 1 or 3, this expression selects candidate 2 even though it was just SIGKILLed; similarly, it selects the dead candidate 3 when candidate 3 was the winner and candidate 2 takes over. metric_value then curls a dead endpoint under set -euo pipefail, causing scenario 14 and the live-chaos CI job to fail despite a successful failover. Select the candidate distinct from both WINNER and NEW_LEADER.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real, fixed in 5c0e6c8 — and the joke writes itself: this landed in the suite whose entire purpose is refusing to assume which node won. A fixed ordinal selects the SIGKILLed candidate whenever the election comes out other than it did on my machine. The survivor is now derived: candidates are 1, 2 and 3, and await_lease_holder_changed guarantees NEW_LEADER != WINNER, so 6 - WINNER - NEW_LEADER is always the third, live one.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
… a corpse, and a one-shot produce Round two, and the fixes I shipped for round one brought their own findings. Every one was real. THE GAUGE THAT LIED. The candidate collector set broker_lease_active from the metadata snapshot alone, so it answered "is there a live lease for this range" when the name has always meant "is THIS replica the authorized leaseholder" — the leader's collector says so in its own comment, and computes `active && epoch == held` for exactly that reason. Copying that formula would not have been enough here: a FOLLOWING candidate adopts the holder's granted epoch and activates its view at it, because that is how it accepts replication, so every term is true on all three replicas at once. The gauge would have reported three leaseholders for one range, telling an operator the opposite of what two of them will do with a produce. The role is the missing term. The unit test now pins the following case, and scenario 14 pins both ends of it live. THE PROBE THAT COULD PICK A CORPSE. The new observability assertions chose the follower to scrape with a fixed ordinal, which selects the SIGKILLed candidate whenever the election comes out other than it did on my machine — curling a dead endpoint and failing the scenario for the crime of electing a different pod. The candidates are 1, 2 and 3, so the survivor is 6 minus the winner minus the new leader: derived, never guessed. That this landed in THIS suite, whose whole purpose is refusing to assume which node won, is the joke writing itself. And those assertions were one-shot and unguarded. metric_value is a pipeline, so under `set -o pipefail` a transient scrape failure aborted the scenario before the diagnostic it was carrying could ever print; and a follower that legitimately trailed the quorum by a record would have been reported as broken replication. Both are now deadline-polled through the shared helpers, which distinguish `absent` from "says something else" — the first means candidate mode exports no role collector at all, the second means replication is behind, and those send an operator to opposite places. await_metric_equals is new because some gauges are states rather than progress: "this replica leads" is 1 or 0, and a floor of 0 would accept the 1 that means the opposite. CI ALSO CAUGHT TWO OF MINE. The ruby render check died on a bash syntax error because a comment I added said "pod's" — inside a program that is one single-quoted shell argument, so the apostrophe closed the string. The block already warns that it cannot name a dollar-sigil for a related reason; it now warns about apostrophes too, and the step is extracted and parsed locally before pushing. And the kind deploy got further than ever — every pod reported its records, the holder was deleted, a survivor took the range at epoch 2 in place — then failed on a single-shot produce. A freshly granted holder has to establish its replication streams before any quorum write can land, which is why live-chaos retries every post-promotion produce under a deadline and says so. The smoke does now too, and it produces while the deleted pod is still returning: with three members a quorum is the holder plus one, so proving the range serves THROUGH the outage is the stronger claim and the one operators care about. The wait for the returning pod is polled rather than `kubectl wait`, which matched the pod still terminating under the same name and returned in four seconds while the replacement had not started. Scenario 14 PASS with the new gauge pins; 68 vtop-node tests, clippy, fmt; the CI step extracted and syntax-checked.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c0e6c888a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.held_fencing_epoch | ||
| .with_label_values(&range) | ||
| .set(held as i64); | ||
| if let Some((epoch, active)) = snapshot { |
There was a problem hiding this comment.
When a scrape occurs after SwitchingLocalView::clear() during demotion, try_meta_fencing_epoch() returns None, so this branch leaves the previous broker_lease_active value unchanged. If the pod was just leading, it therefore continues exporting lease_active=1 while broker_candidate_leading is already reset to 0; because the subsequent quiesce/reopen can stall on disk, that false leaseholder signal can persist indefinitely. Set lease-active to 0 when the view has no role rather than retaining the prior leader's authorization state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real, and it exposed a distinction the collector was missing: a contended read is IGNORANCE (keep the last value — the module doctrine, and right for progress gauges) while an empty switching view is KNOWLEDGE (this replica owns nothing right now). Fixed in 001bc32: the trait gains has_role(), defaulted true for concrete roles and false for a switching view between them, and broker_lease_active is written to 0 the moment no role owns the range — so it can no longer contradict broker_candidate_leading through a transition whose quiesce and reopen may stall on disk. Your finding also surfaced a neighbour: broker_held_fencing_epoch was written unconditionally and so REWOUND to 0 mid-transition, a monotonic gauge going backwards reading as a replica that lost its history; it now holds its last value too. New unit test pins both.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Round three. Five findings, all real, all in code from the two rounds before it. A REPLICA BETWEEN ROLES WAS STILL CLAIMING THE LEASE. The collector leaves a gauge standing when a read comes back contended — the module's own doctrine, and right for progress: a stalled disk should show an offset that stops advancing, not one that rewinds. But mid-transition the switching view holds no role at all, and that is a different thing entirely. A contended lock is ignorance; an empty view is knowledge. So broker_lease_active stayed 1 from the role that had just ended, beside a broker_candidate_leading of 0 that already said the opposite, and a transition that quiesces and reopens the range can stall on disk long enough for that contradiction to be what an operator sees. Authorization now fails closed the moment no role owns the range, while the progress gauges keep their last reading — including the held epoch, which used to be written unconditionally and so rewound to zero mid-transition, a monotonic gauge going backwards reading as a replica that lost its history. The rest are in the smoke test, and three of them are the retry loop I added yesterday to fix a one-shot produce. Every failed attempt left its port-forward alive, so sixty retries meant sixty kubectl processes and sixty API streams held open for the length of the wait. Each attempt now tears its own forward down — still a FRESH forward per attempt, because a pod that restarted takes its forward with it and a reused one only repeats "connection refused", which is the same reasoning the init retry loop above it records. The 180-second bound was also advisory: the deadline was checked between attempts, and the client waits on a socket, so a forward that accepted and then stopped answering would hang the job forever while the loop never got another chance to look at the clock. Attempts are bounded now. The readiness wait converted an API failure into "not ready", so four minutes of an unanswering API server would have been reported as a pod that never came back — accusing a component nobody observed doing anything. That rule is stated a hundred lines above in await_pods_exist, and this loop broke it on its first draft. Query failure and unreadiness now get the two different messages they deserve. And the phase claimed something it never checked. "Produce while the deleted pod is still returning" is only true if the replacement was actually still absent, which is a race this test does not control — a fast StatefulSet restart is not a failure. The member count is now captured and NARRATED: a run that says "2 of 3" proves the degraded path, one that says "3 of 3" says plainly that it did not, and a reading of zero is reported as the failed query it must be, since no cluster accepts a quorum write with nothing Ready. A log line nobody verified is worse than no log line. Scenario 14 PASS; 68 vtop-node tests including the new between-roles pin; clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 001bc3275a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # replicated ONE range across the pods, every pod a CANDIDATE (#284): the | ||
| # role follows the metadata lease inside the binary. Whichever | ||
| # pod acquires the range leads and the rest follow it; when the | ||
| # holder dies, a surviving candidate takes the range in place — | ||
| # no re-render, no helm upgrade, no restart. Requires | ||
| # `lease.enabled` (candidates acquire the range through the | ||
| # lease) and `fencingEpoch: 0` (grants are minted from 1); | ||
| # both are enforced at render time. |
There was a problem hiding this comment.
Update the chart's primary README alongside this new topology. helm/vtop/README.md:46-49 still explicitly says every pod is standalone and that the chart does not offer replication, its values table omits data.topology, and its verification section at lines 116-119 still claims every pod is an independent range. Users consulting the documented chart contract will therefore conclude this newly supported mode is unavailable and will not learn its required lease and fencing settings.
Useful? React with 👍 / 👎.
| members_at_write="$(kubectl -n "$REPLICATED_NS" get pods -l "app.kubernetes.io/instance=${REL}" \ | ||
| -o jsonpath='{range .items[*]}{.metadata.deletionTimestamp}{"|"}{.status.conditions[?(@.type=="Ready")].status}{"\n"}{end}' \ | ||
| 2>/dev/null | grep -c '^|True$' || true)" |
There was a problem hiding this comment.
Move this measurement to the attempt that actually succeeds. It is currently captured before a retry loop that can run for 180 seconds, so if two pods are Ready here, early produces fail, and the replacement becomes Ready before a later produce succeeds, lines 880-883 still report that the range served on a bare quorum. That creates false evidence that the degraded path was exercised even though the write may have landed only after all three members recovered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/k8s-smoke.sh">
<violation number="1" location="scripts/k8s-smoke.sh:867">
P2: The failover log can report the wrong quorum state because `members_at_write` is captured before potentially many produce retries, not when the write succeeds. Capturing readiness for each attempt, immediately before its produce call, would keep the degraded-quorum narration tied to the successful attempt.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # count is captured and NARRATED: a run that says "2 of 3" proves the | ||
| # degraded path, and one that says "3 of 3" says plainly that it did not, | ||
| # instead of printing a claim nobody checked (review). | ||
| members_at_write="$(kubectl -n "$REPLICATED_NS" get pods -l "app.kubernetes.io/instance=${REL}" \ |
There was a problem hiding this comment.
P2: The failover log can report the wrong quorum state because members_at_write is captured before potentially many produce retries, not when the write succeeds. Capturing readiness for each attempt, immediately before its produce call, would keep the degraded-quorum narration tied to the successful attempt.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/k8s-smoke.sh, line 867:
<comment>The failover log can report the wrong quorum state because `members_at_write` is captured before potentially many produce retries, not when the write succeeds. Capturing readiness for each attempt, immediately before its produce call, would keep the degraded-quorum narration tied to the successful attempt.</comment>
<file context>
@@ -843,6 +857,16 @@ R_TOTAL=$((R_EXPECTED + 30))
+# count is captured and NARRATED: a run that says "2 of 3" proves the
+# degraded path, and one that says "3 of 3" says plainly that it did not,
+# instead of printing a claim nobody checked (review).
+members_at_write="$(kubectl -n "$REPLICATED_NS" get pods -l "app.kubernetes.io/instance=${REL}" \
+ -o jsonpath='{range .items[*]}{.metadata.deletionTimestamp}{"|"}{.status.conditions[?(@.type=="Ready")].status}{"\n"}{end}' \
+ 2>/dev/null | grep -c '^|True$' || true)"
</file context>
1 participant
The chart slice of #284, and the last piece of it: #343 put the role inside the binary; this makes the chart stop encoding it, which values.yaml itself promised ('the chart stops encoding it at all').
What changes
data.leaderOrdinalis retired, loudly. Setting it fails the render with the migration story. A value that used to steer roles must not be silently ignored.replicatedrenders every pod as a candidate with one shared peer list (self included; the binary skips its own entry, and identical lists keep the rendered configs diffable). Failover is the binary's job now: whichever pod acquires the lease leads, and when the holder dies a survivor takes the range in place — no re-render, no upgrade, no restart.lease.enabled(no pod would ever lead) and a nonzerofencingEpoch(grants are minted from 1; a floor at or above the first grant refuses the very grant that makes a candidate lead).Validation
helm lint+ full render parse-verified locally (three identical leased candidates, epoch 0, distinct per-pod addrs); all three refusals fired with their messages.Closes #284.
Summary by cubic
Switch the
replicatedHelm topology to symmetric candidates that follow the metadata lease at runtime (#284). Retiresdata.leaderOrdinal, adds a non-blocking candidate metrics collector with a newvtop_broker_candidate_leadinggauge, hardens metrics semantics during role transitions, and tightens CI/smoke to verify election, in-place failover, and observability. Chart version 0.2.0.New Features
candidatewith one sharedpeerslist (self included); requiredata.lease.enabled=trueanddata.fencingEpoch=0; refuse anydata.leaderOrdinalin all topologies; NOTES updated.CandidateCollectorthat exports existing broker metrics across role changes; addvtop_broker_candidate_leading; makebroker_lease_activereflect the authorized holder (role + epoch) and fail closed between roles; keep progress gauges monotonic (no mid-transition rewind ofbroker_held_fencing_epoch); all reads non-blocking.Migration
data.leaderOrdinalfrom your values.replicated, setdata.lease.enabled=trueanddata.fencingEpoch=0.Written for commit 001bc32. Summary will update on new commits.