Skip to content

Mark ReductionPushdown and ReduceElision WMR-safe - #18554

Merged
ggevay merged 1 commit into
MaterializeInc:mainfrom
ggevay:wmr-reduction-pushdown
Apr 6, 2023
Merged

ggevay merged 1 commit into
MaterializeInc:mainfrom
ggevay:wmr-reduction-pushdown

Conversation

@ggevay

@ggevay ggevay commented Apr 3, 2023

Copy link
Copy Markdown
Contributor

This just marks ReductionPushdown and ReduceElision to be WMR-safe, and adds slts for both.

Note that unique key inference is not very smart yet for WMR, which is affecting ReduceElision, as shown by the last test in reduce_elision.slt. I opened a separate issue for that, and will work on this next. But there are already some situations when both ReductionPushdown and ReduceElision kick in, so we can merge this PR in itself.

Motivation

  • This PR adds a known-desirable feature:
    • MaterializeInc/database-issues#5339,
    • MaterializeInc/database-issues#5340

Tips for reviewer

I suggest reviewing the test for pushdown before elision, because the elision tests are similar, but more complicated.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

@ggevay ggevay added A-optimization Area: query optimization and transformation A-compute Area: compute labels Apr 3, 2023
@ggevay
ggevay requested review from a team and aalexandrov April 3, 2023 12:57
@aalexandrov

Copy link
Copy Markdown
Contributor

Tagging @MaterializeInc/qa for a review.

@aalexandrov
aalexandrov requested a review from a team April 3, 2023 16:01
@philip-stoev

Copy link
Copy Markdown
Contributor

I will work on this today. Apologies for the delay.

@philip-stoev
philip-stoev requested review from philip-stoev and removed request for a team April 5, 2023 06:12

@philip-stoev philip-stoev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No wrong results or panics with WMR. The only plan changes are the removal of Distinct, which I guess is expected.

@ggevay

ggevay commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

Great, thank you very much!

@aalexandrov aalexandrov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Note that ReduceElision will get smarter once we add a fixpoint loop
into the `LetRec` case of the unique key inference.

Fixes #18170, #18171
@ggevay
ggevay force-pushed the wmr-reduction-pushdown branch from 5338639 to 51dd430 Compare April 6, 2023 12:34
@ggevay
ggevay enabled auto-merge (rebase) April 6, 2023 12:34
@ggevay
ggevay merged commit c464ed6 into MaterializeInc:main Apr 6, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-compute Area: compute A-optimization Area: query optimization and transformation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants