Repository navigation
environmentd: Let bootstrap finish before DDL-triggered restarts - #39151
Conversation
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
Some comments.
(We'll soon be able to run CI)
|
The GHCR issue should be resolved now. I've clicked rebuild on all those PRs of the stack that needed it. |
QA LLM Review1. MEDIUM -- Force-promote and
|
|
@ggevay I reworked this, could you please take another look! 🙏 |
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM, but I'm not totally sure whether we want to accept the issue that the QA bot pointed out. See some more details below.
| // NOTE: There _is_ a window where DDL can happen in the old | ||
| // environment, between checking above, us announcing as ready to | ||
| // promote, and cloud giving us the go-ahead signal. Its size | ||
| // depends on how quickly cloud will trigger promotion once we | ||
| // report as ready. | ||
| // | ||
| // We could add another check here, right before cutting over, but I | ||
| // think this requires changes in Cloud: with this additional check, | ||
| // it can now happen that cloud gives us the promote signal but we | ||
| // then notice there were changes and restart. Could would have to | ||
| // notice this and give us the promote signal again, once we're | ||
| // ready again. |
There was a problem hiding this comment.
Nit on the moved NOTE: the second paragraph is outdated, since orchestratord keeps re-sending promote until it sees IsLeader. Maybe:
| // NOTE: There _is_ a window where DDL can happen in the old | |
| // environment, between checking above, us announcing as ready to | |
| // promote, and cloud giving us the go-ahead signal. Its size | |
| // depends on how quickly cloud will trigger promotion once we | |
| // report as ready. | |
| // | |
| // We could add another check here, right before cutting over, but I | |
| // think this requires changes in Cloud: with this additional check, | |
| // it can now happen that cloud gives us the promote signal but we | |
| // then notice there were changes and restart. Could would have to | |
| // notice this and give us the promote signal again, once we're | |
| // ready again. | |
| // NOTE: DDL the old leader commits between the final check above and | |
| // the fencing `open` below only costs hydration progress: we reboot as | |
| // leader from the full catalog, and those objects have hydrated on the | |
| // old leader for at most that window. Don't re-check here: a restart | |
| // after the promote signal can, under continuous DDL, hold the rollout | |
| // in orchestratord's `Promoting` state, which never times out and | |
| // cannot be cancelled. |
Let orphaned replica cleanup finish before preflight can restart the read-only generation, including on the catch-up timeout path. Preserve the administrative skip-catchup escape hatch. Addresses SQL-729. Amp-Thread-ID: https://ampcode.com/threads/T-01a0d85f-8cd1-7078-afa3-21a9cce44857 Co-authored-by: Amp <[email protected]>
The catch-up task starts right after preflight again, so an administrator can force-promote a read-only deployment whose bootstrap is stuck. The ID baseline reaches the task over a oneshot once adapter bootstrap succeeds, and only then do the DDL checks and the catch-up timeout start.
7a204c1 to
89d301e
Compare
| ) | ||
| .await | ||
| .expect("incompatible catalog/persist version"); | ||
| /// Returns the next user item and replica IDs based on existing catalog objects. |
There was a problem hiding this comment.
Nit: the check_ddl_changes doc above (outside this diff) still says it looks for IDs "allocated after the preflight check began". Since the baseline now comes from this snapshot, maybe: "We do this by checking whether items or replicas with IDs above the highest ones in the catalog snapshot this deployment bootstrapped from were committed."
| tokio::time::advance(2 * caught_up_max_wait).await; | ||
| tokio::task::yield_now().await; |
There was a problem hiding this comment.
Nit: advance plus one yield_now only gives the spawned task a bounded number of polls, so it's not certain a wrong implementation would reach its first catalog transaction before the asserts. A paused-clock sleep resolves only once the runtime is idle, which makes the check unambiguous:
| tokio::time::advance(2 * caught_up_max_wait).await; | |
| tokio::task::yield_now().await; | |
| tokio::time::sleep(2 * caught_up_max_wait).await; |
Describe the DDL check against the bootstrap snapshot baseline, and let the paused clock idle the runtime before asserting that catch-up waits for bootstrap.
Motivation
Closes: SQL-729
Continuous DDL can repeatedly restart a read-only deployment before it cleans up orphaned replicas, leaving them running and retaining resources.
Description
Preflight selects the boot mode and starts the catch-up task. Once adapter bootstrap succeeds, the task receives an ID baseline captured from the same catalog savepoint, and only then do DDL checks and the catch-up timeout start. An administrative skip is accepted throughout and promotes without waiting for bootstrap.
Orphan cleanup waits for queued delete requests to be processed before returning. It does not wait for pods to terminate.
Verification
Unit tests cover deferred catch-up startup, skipping before bootstrap, an early caught-up notification, and a bootstrap baseline that excludes subsequent catalog changes without consuming pending bootstrap updates.