qzsee opened a new pull request, #69642:
URL: https://github.com/apache/doris/pull/69642
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: None
Problem Summary:
A running FE runs bdbje `NetworkRestore` concurrently from several threads
into the same bdb directory. The restores corrupt the log files, and the FE can
never start again until the bdb directory is rebuilt by hand.
#### How it happens
The problem was hit on an observer during a rolling upgrade.
1. The old master sends some txns to the observer and is then stopped for
upgrade before a majority of electable nodes acks these txns. An observer
(`SECONDARY`) never acks, so it is not counted in the majority. Now the
observer has txns that the new master does not have.
2. While there is no master, the cleaner of the observer keeps running, and
a checkpoint with cleaned files is written after these txns.
3. A follower is elected as the new master. The observer syncs up with it
and has to roll back the extra txns. The matchpoint is before a checkpoint end,
so bdbje can not roll back in place. The replica thread invalidates the
environment with `InsufficientLogException`:
```
matchpointVLSN of xxx precedes a checkpoint end, needs network restore.
InsufficientLogException ... at ReplicaFeederSyncup.verifyRollback(...)
```
Up to here this is expected bdbje behavior.
4. From now on, **every** thread that touches the invalidated environment
gets the same `InsufficientLogException`. Three places run `NetworkRestore`
inside the running process when they catch it:
- `BDBJEJournal.getDatabaseNames()` -> `reSetupBdbEnvironment()`. It is
reached through `getMaxJournalId()`, which is called by the replayer thread and
also by http / rpc / metric threads (e.g. `HaController`,
`FrontendServiceImpl`, `MetricRepo`).
- `BDBJEJournal.open()` -> `reSetupBdbEnvironment()`.
- The `catch (InsufficientLogException)` of the replayer thread in `Env`.
As a result, several restores run at the same time on the same directory
(in the case above, two restores started within one second):
```
Started network restore <- restore 1
Started network restore <- restore 2
Rename from: xxx.jdb.tmp to xxx.jdb failed UNEXPECTED_STATE
Missing file: xxx.jdb UNEXPECTED_STATE
```
5. Why concurrent restores corrupt the directory:
- Each restore picks its own log provider. The `.jdb` files of different
nodes are independent, so files with the same name have different contents on
different nodes.
- `setRetainLogFiles(false)` deletes every local file that is not in the
file list of the chosen provider. One restore therefore deletes or overwrites
the `.tmp` and `.jdb` files that the other restore just downloaded.
- The invalidated environment is not closed during the restore, and its
bdbje daemon threads are still alive.
The directory ends up as a mix of files from different nodes, and some
referenced files are missing.
6. On the next start, recovery fails with `EnvironmentFailureException ...
LOG_FILE_NOT_FOUND` (or `LOG_CHECKSUM` if someone copies a single `.jdb` file
from another node). This is **not** an `InsufficientLogException`, so `setup()`
never triggers a restore again. The FE keeps failing to start until the bdb
directory is rebuilt by hand.
There is also a minor issue: if `setup()` still gets
`InsufficientLogException` in its last retry, the loop ends and `setup()`
returns with a `null` environment.
#### How to fix
The idea: **never run `NetworkRestore` in a running process. Exit, and
restore once, single-threaded, while opening the environment at startup.**
1. Add `BDBEnvironment.NETWORK_RESTORE_LOCK`.
2. `BDBEnvironment.setup()` is the only place that runs `NetworkRestore`.
When opening the environment throws `InsufficientLogException`, it first
`close()`s the environment, then runs the restore while holding the lock, then
retries the open. This is safe because the environment failed to open: there is
no environment handle and no bdbje daemon thread, and no other thread uses the
environment yet.
3. Add `BDBEnvironment.exitForNetworkRestore()`. While holding the lock, it
logs the reason and the thread name and exits the process. The three runtime
places above now call it instead of running a restore:
- only the first thread exits;
- the other threads block on the lock until the process is gone;
- the in-memory meta may already contain replayed journals that are now
rolled back, so it has to be rebuilt anyway.
4. After restart, opening the environment throws `InsufficientLogException`
again, because bdbje persists the "restore required" state in the log.
`setup()` then restores in a clean state. This is verified with a real
replication group in the new UT.
5. If `setup()` still can not open the environment after all retries, it
exits instead of returning a `null` environment.
6. `exitHook` (default `System::exit`) is added only so that UTs can replace
it.
`reSetupBdbEnvironment()` and the in-process `NetworkRestore` calls in
`BDBJEJournal` and `Env` are removed.
#### Tests
- `BDBNetworkRestoreTest` (mocked):
- 8 threads (replayer / http / rpc / metric) call `getMaxJournalId()` on
an invalidated environment at the same time. Asserts: no `NetworkRestore` runs,
exactly one thread exits while holding the lock, and the other 7 threads are
blocked on `NETWORK_RESTORE_LOCK` (checked with `ThreadMXBean`).
- When opening the environment throws `InsufficientLogException`,
`setup()` runs `NetworkRestore` exactly once, with the lock held and the
environment closed, and then opens a usable environment.
- If the environment still can not be opened after all retries, `setup()`
exits.
- `BDBNetworkRestoreClusterTest` (a real 4-node bdbje group: 3 electable
nodes + 1 secondary, no mocked exception):
1. All nodes are in sync.
2. The old master writes txns that only the observer receives.
3. The observer cleans log files and makes a checkpoint, then the old
master stops. The observer **keeps running**.
4. The other two followers elect a new master, which writes new txns.
5. The running observer is invalidated by its own replica thread.
`je.info` contains `precedes a checkpoint end, needs network restore`.
6. 8 FE threads call the real `BDBJEJournal.getMaxJournalId()`
concurrently. Asserts: no restore runs, exactly one thread exits, and `je.info`
has no `Started network restore` / `Missing file:` / `Rename from:` /
`LOG_FILE_NOT_FOUND` / `LOG_CHECKSUM`.
7. "Restart": opening the observer without a restore throws
`InsufficientLogException` again. `setup()` then runs the **real**
`NetworkRestore` exactly once. Afterwards the observer has the same data as the
new master: the uncommitted txns are rolled back, the new master's txns are
present, and `je.info` contains no corruption messages.
The exception stack in step 5 (`ReplicaFeederSyncup.verifyRollback` ->
`setupLogRefresh`) is the same as the one in the production case.
#### Note for a node that is already corrupted
This PR can not repair a directory that was already corrupted by concurrent
restores. To recover such a node:
1. Move `meta/bdb` away and keep `meta/image`.
2. Start the FE with this fix. Opening the empty environment triggers a
single restore from another member.
3. Alternatively, drop the node and add it again.
### Release note
Fix the bdb directory of an FE (usually an observer) being corrupted by
concurrent bdbje NetworkRestore after the master changed. Before this fix, the
FE could not start again until its bdb directory was rebuilt.
### Check List (For Author)
- Test
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test.
- Behavior changed:
- [ ] No.
- [x] Yes. When the bdb environment of a running FE gets
`InsufficientLogException`, the FE now exits immediately and restores the log
files the next time it starts. Before, it ran `NetworkRestore` inside the
running process. The FE needs to be restarted by its supervisor (or manually),
which is the same as the old replayer path, which also exited after the restore.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test coverage
- [ ] Confirm behavior changed
- [ ] Confirm whether need to document
--
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]
