Repository navigation
Conversation
benesch
force-pushed
the
source-reader-no-panic
branch
from
April 1, 2023 15:35
557c41b to
257f742
Compare
Instead, log the error to the source status collection and request a `SuspendAndRestart`. This requires some tricky capability handling to avoid bricking the remap shard. This commit also introduces a mandatory 30s backoff before a `SuspendAndResume` takes place, to limit hot retry loops. This comes at the expense of latency in the case of a transient error; unfortunately implementing a smarter retry policy (like exponential backoff) is difficult. Fix #18491.
benesch
force-pushed
the
source-reader-no-panic
branch
from
April 1, 2023 15:47
257f742 to
0d22a3f
Compare
Contributor
|
If you don't mind I'll incorporate this PR in this one #18055 which is close to merging and deletes the |
Contributor
Author
|
I don’t mind! I’d love to not be responsible for this! :D
…On Sun, Apr 2, 2023 at 1:18 PM Petros Angelatos ***@***.***> wrote:
If you don't mind I'll incorporate this PR in this one MaterializeInc/materialize#18055
<#18055> which is close
to merging and deletes the SourceReader trait
—
Reply to this email directly, view it on GitHub
<#18545 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGXSIDHDRR5YOSWJF6CJKTW7GYGVANCNFSM6AAAAAAWPNKV54>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
5 tasks
petrosagg
approved these changes
Apr 2, 2023
petrosagg
left a comment
Contributor
There was a problem hiding this comment.
Great! I cherry-picked your commit primarily for the tests and adapted the rest of the logic. I'm leaving my approval here for posterity. I have enabled auto-merge on the other PR
Contributor
|
Incorporated in #18055 |
aljoscha
pushed a commit
that referenced
this pull request
Oct 21, 2026
…40061) This reverts #40029, because its in-container restart loop lets a fenced environmentd reboot inside a pod that Kubernetes has already replaced, take a new catalog epoch and fence out the live leader. ### Motivation The nightly job `K8s recovery: envd on failing node` fails on main since #40029 ([SQL-807](https://linear.app/materializeinc/issue/SQL-807)). * Failing: [nightly #18553](https://buildkite.com/materialize/nightly/builds/18553#01a15ba6-bf60-4bb3-b6ff-db71cf56c89a) (main, all three attempts), [#18550](https://buildkite.com/materialize/nightly/builds/18550) and [#18551](https://buildkite.com/materialize/nightly/builds/18551) (branches that contain #40029). * Passing: every nightly without #40029 ([#18543](https://buildkite.com/materialize/nightly/builds/18543), [#18545](https://buildkite.com/materialize/nightly/builds/18545), [#18546](https://buildkite.com/materialize/nightly/builds/18546), [#18548](https://buildkite.com/materialize/nightly/builds/18548), [#18552](https://buildkite.com/materialize/nightly/builds/18552), [#18556](https://buildkite.com/materialize/nightly/builds/18556)). One branch build with #40029, [#18554](https://buildkite.com/materialize/nightly/builds/18554), passed, so the failure is a race and not deterministic. * Signature: `Failed to achieve 'no issues after node recovery' ... within 10s`. The replacement `environmentd-0` panics with `unable to advance catalog upper: Durable(Fence(Epoch { current_epoch: 3, fence_epoch: 4 }))` (exit 134) right after the old envd node is unpaused. Timeline from #18553 attempt 3 (kail, clusterd logs, k8s events): * 00:53:35 UTC: the original environmentd boots on `mzcloud-worker9` (pod IP 10.244.5.2) and initializes the catalog (epoch 2). 00:54:22: the test pauses worker9. * 01:00:14.6: the replacement `environmentd-0` boots on worker8 (10.244.2.2), opens the catalog writable (epoch 3) and serves queries. * 01:00:20.41: the test unpauses worker9. * 01:00:20.98: clusterds accept new connections from 10.244.5.2 and drop the replacement's. * 01:00:21.28: the replacement panics on the fence with `fence_epoch: 4`. * 01:00:21.44: the 10.244.5.2 connections close; a `Killing` event (`Stopping container environmentd`) follows. The replacement restarts and the test query hits it. Only a writable catalog open bumps the epoch (`Mode::Writable` in `src/catalog/src/durable/persist.rs`), and the replacement booted once in that window. So epoch 4 came from a new environmentd process started inside the old, already deleted pod on worker9. The events show no second `Started` for the old pod; its only container start is the original one at 00:53. That fits the wrapper's new behavior: * The resumed old environmentd is fenced and halts with exit code 166, for example via `fenced by envd` in `storage_collections.rs` or the txn-wal fence. * Before #40029, `entrypoint.sh` exited the container on 166. For a pod that is already deleted, the kubelet does not restart it. * With #40029, the wrapper restarts environmentd immediately. The new process opens the catalog, takes the next epoch and fences the legitimate leader until the kubelet kills the container. The same can happen outside the test whenever an environmentd that lost leadership halts while its pod is being replaced, for example after a node failure. The restart loop bypasses the kubelet's decision about whether that container should run again. ### Description `git revert e6e3e21`: restore `tini` without `-g` and the `entrypoint.sh` that exits on any nonzero status. Avoiding crash backoff for repeated catch-up halts during zero-downtime upgrades ([SQL-807](https://linear.app/materializeinc/issue/SQL-807)) still needs a fix. A restart that only covers halts that are safe to retry in place, such as a distinct exit code for the preflight reboots in `src/environmentd/src/deployment/preflight.rs`, would avoid this. That is left to the owner. ### Tests * No local run: reproducing it needs the cloudtest kind cluster and a full image build. * Please run `K8s recovery: envd on failing node` from the nightly pipeline against this branch before merging. Since the failure is a race, a pass is only meaningful together with the three failing attempts on #18553. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
bosconi
added a commit
that referenced
this pull request
Oct 26, 2026
) This re-lands the in-container restart of #40029 ([SQL-807](https://linear.app/materializeinc/issue/SQL-807)) for the read-only catch-up halts only, so a fenced environmentd in a replaced pod no longer restarts and fences the live leader. ### Motivation #40029 restarted environmentd inside its container on every `halt!` (exit 166), to avoid Kubernetes crash backoff for repeated catch-up halts during zero-downtime upgrades. #40061 reverted it, and this PR is based on main after that revert, because #40029 broke the nightly job `K8s recovery: envd on failing node`. * Failing with #40029: [nightly #18553](https://buildkite.com/materialize/nightly/builds/18553#01a15ba6-bf60-4bb3-b6ff-db71cf56c89a) (main, all three attempts), [#18550](https://buildkite.com/materialize/nightly/builds/18550), [#18551](https://buildkite.com/materialize/nightly/builds/18551). One branch build with #40029, [#18554](https://buildkite.com/materialize/nightly/builds/18554), passed, so the failure is a race. * Passing without it: [#18543](https://buildkite.com/materialize/nightly/builds/18543), [#18545](https://buildkite.com/materialize/nightly/builds/18545), [#18546](https://buildkite.com/materialize/nightly/builds/18546), [#18548](https://buildkite.com/materialize/nightly/builds/18548), [#18552](https://buildkite.com/materialize/nightly/builds/18552), [#18556](https://buildkite.com/materialize/nightly/builds/18556). * Signature: the replacement `environmentd-0` panics with `unable to advance catalog upper: Durable(Fence(Epoch { current_epoch: 3, fence_epoch: 4 }))` right after the old envd node is unpaused. * Mechanism: the old environmentd, resumed after its pod was replaced, is fenced and halts with 166. The kubelet would not restart a deleted pod, but the #40029 wrapper did. The new process opened the catalog writable, took epoch 4 and fenced the replacement until the kubelet killed the container. The halts that [SQL-807](https://linear.app/materializeinc/issue/SQL-807) is about are the DDL reboots in `check_ddl_changes`, which a read-only deployment can hit many times while it catches up. ### Description * `mz_ore::process`: add `RESTART_IN_PLACE_EXIT_CODE` (75, `EX_TEMPFAIL`) and the `restart_in_place!` macro, which behaves like `halt!` but exits with 75. Name the existing halt code `HALT_EXIT_CODE` (166). Shells, tini and Kubernetes report death by signal N as 128 + N, so a code below 128 can only come from an explicit exit. * `src/environmentd/ci/entrypoint.sh`: restart environmentd in place only on 75, and export `MZ_RESTARTED_IN_PLACE=true` before doing so. Every other exit, including 166, ends the container as before #40029. A graceful exit still sleeps forever. * `preflight_0dt`: when `MZ_RESTARTED_IN_PLACE` is set (the hidden `--restarted-in-place` flag) and the catalog is at this process's own generation, halt with 166 instead of booting writable ([`preflight.rs#L70-L82`](https://github.com/MaterializeInc/materialize/blob/a5f2ab0c41e844fc109211a460fb76a53c1282ca/src/environmentd/src/deployment/preflight.rs#L70-L82)). * Only read-only code paths restart in place, so that combination means a process of this generation has been promoted: one in a pod that replaced this one, or, after a skipped catch-up, this container's previous process, whose read-only bootstrap restarted after the takeover. Booting writable would fence a replacement, and halting is correct in both cases. * A kubelet restart starts a new container without the marker, so a pod that wasn't replaced still boots as leader. * `check_ddl_changes`: use `restart_in_place!` only while the catalog's deployment generation is still older than this process's, so the restarted process boots read-only. Once a process of this generation has been promoted, halt with 166 (`deployment generation N has been promoted; halting`). The generation is read from the openable catalog that the check already builds, before `open_savepoint` consumes it, so the check reads one catalog snapshot, not two. * Storage controller: the read-only halt in `create_collections_for_bootstrap` for a dependency deleted during bootstrap uses `restart_in_place!` ([`lib.rs#L1031-L1047`](https://github.com/MaterializeInc/materialize/blob/a5f2ab0c41e844fc109211a460fb76a53c1282ca/src/storage-controller/src/lib.rs#L1031-L1047)). It keeps `halt!` when writable. * The takeover halt after promotion (`fenced out old deployment; rebooting as leader`) keeps `halt!`. That process has just opened the catalog writable, and a restart in place would only reach the marker check one boot later. * `tini -g` and the entrypoint's TERM/INT traps are restored as #40029 had them: environmentd receives SIGTERM itself, and the shell exits after it instead of restarting or sleeping. * `bin/environmentd` (`misc/python/materialize/cli/run.py`) restarts on 75 as it does on 166. Residual: a stale read-only process in a replaced pod still restarts in place on these halts until the kubelet kills its container. The marker keeps it from booting writable, but it competes with the replacement for the generation's replicas during that window. ### Tests * `cargo check` and `cargo clippy --all-targets -- -D warnings` for `mz-ore`, `mz-environmentd`, `mz-storage-controller` and `mz-sqllogictest` with Rust 1.98.1: clean. `rustfmt --check` on the changed Rust files: clean. * `shellcheck -s sh src/environmentd/ci/entrypoint.sh` (koalaman/shellcheck): clean. * Entrypoint loop under busybox `sh` with a stub `environmentd`: * exits 75 then 0: restarted once with the same arguments and `MZ_RESTARTED_IN_PLACE=true`, then sleeps. * exits 75, 75, 166: restarted twice, then the container exits 166. * exits 166, 1 or 134: the container exits with that code, no restart. * `docker stop` under tini with `-g`: environmentd gets SIGTERM, and the container exits 143 within 0.1 s. * `test/0dt` workflow `restarted-in-place-after-promotion`: promotes `mz_new`, then starts a second process of its generation with `MZ_RESTARTED_IN_PLACE=true`, and asserts that it exits 166 with the marker halt while `mz_new` keeps serving as leader. Without the marker check, that process would fence `mz_new`. * Not run: `K8s recovery: envd on failing node` needs the cloudtest kind cluster, and it and the `test/0dt` workflows, including the new one, need image builds. Please run the nightly job against this branch before merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead, log the error to the source status collection and request a
SuspendAndRestart. This requires some tricky capability handling to avoid bricking the remap shard.This commit also introduces a mandatory 30s backoff before a
SuspendAndResumetakes place, to limit hot retry loops. This comes at the expense of latency in the case of a transient error; unfortunately implementing a smarter retry policy (like exponential backoff) is difficult.Fix MaterializeInc/database-issues#5461.
Motivation
Tips for reviewer
Please pay extra close attention to this PR! I'm quite unfamiliar with this code, and the semantics of this change are quite subtle.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.mz_source_status_history, particularly when SSH tunnels are in use.