Throttle Elastic migration batches to throttle_delay

John Masonrequested to merge
jmason-migration-worker-cadence into master

🤖 AI-authored change.

What does this MR do and why?

Elastic::MigrationWorker now runs at most one batch per the migration's declared throttle_delay, instead of one batch per concurrent chain. The worker re-enqueues itself after every batch and the */5 * * * * cron starts a fresh chain regardless, with nothing retiring the older ones. Unbatched migrations are unaffected — the gate only applies when batched? is true.

Deploy impact: any batched migration with several live chains will run its batches at the declared throttle_delay cadence after this ships, rather than at whatever pace the concurrent chains produced — in-flight backfills will slow down.

Reviewer focus: the choice of a short Redis lease over persisted migration state. A lease fails open — a dead chain costs one skipped window and the cron is still the recovery path — but it also means cadence is best-effort rather than guaranteed. If you would rather this be durable state on the migration record, say so and I will rework it.

References

How to set up and validate locally

  1. Run bundle exec rspec ee/spec/workers/elastic/migration_worker_spec.rb.
  2. The for batch cadence examples assert that three invocations inside one window produce one migrate call, that a second batch runs once the lease is released, that the lease TTL is short of throttle_delay, that a lease held elsewhere throttles without re-enqueueing, and that an unbatched migration is never throttled.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

🤖 Automated change — mention me to get a response. Feedback welcome. Ask to speak to John directly anytime.

Mechanism, rationale and verification (for agents / deep readers)

Mechanism

Elastic::MigrationWorker#perform gains an early throttled?(migration) guard ahead of execute_migration; when it fires the worker logs throttle_delay has not elapsed, skipping batch and breaks out of the loop without running a batch.

Elastic::MigrationWorker#throttled? returns false for any migration that is not batched?. Otherwise it checks (not takes) a Gitlab::ExclusiveLease keyed on the worker class plus the migration's name_for_key, and reports throttled when that key is held. The lease is taken in enqueue_next_batch, right after a batch finishes — not at the top of perform before the batch runs — with a TTL one second under throttle_delay. That timing is load-bearing: a lease taken before the batch would need to outlive both the batch's own duration and the wait for the next chain, so any batch at or above throttle_delay - 1s would let the lease expire mid-run and the migration would run unthrottled, back-to-back with the cron. Taking the lease after the batch means the TTL only has to cover the gap between chains, so batch duration stops mattering. A throttled invocation breaks before enqueue_next_batch, so it retires the chain rather than delaying a batch — that is what bounds the cadence, because surplus cron chains exit and only the lease holder keeps re-enqueueing.

Why a lease rather than migration state or scheduled-set dedupe: recording a last_batch_started_at on the migration record would cost a read-modify-write against the Elasticsearch migrations index on every invocation, on the hot path this change exists to make cheaper. Deduplicating against the Sidekiq scheduled set is racy under concurrent chains and does not bound cadence when a chain is mid-execution rather than scheduled. A lease fails open: if a chain dies, the cost is at most one skipped window, and the next cron tick makes progress, so nothing can wedge a migration permanently.

The spec file is tagged :clean_gitlab_redis_shared_state so the lease does not leak between examples.

A failed batch takes no lease either: execute_migration re-raising is caught by #perform's method-level rescue StandardError outside the in_lock block, so enqueue_next_batch (and therefore take_throttle_lease) never runs and the next cron tick retries immediately instead of waiting out a throttle window.

Verification

Ran ee/spec/workers/elastic/migration_worker_spec.rb in a GDK worktree at the new head: 41 examples, 0 failures. Removing the lease cancel from the elapsed-window example fails exactly that example, so it is sensitive to the behaviour it covers. RuboCop: 1 file inspected, no offenses.

The elapsed-window example cancels the lease between invocations instead of sleeping, so it has no wall-clock dependency.

What I did not verify

  • Behaviour under multiple Sidekiq processes against a real Redis in a production-like environment; only the single-process spec environment was exercised.
  • No measurement of the count-query and index-refresh cost this is expected to remove.
  • The margin is one second, so two invocations landing inside that second of the lease boundary could still race. That window is independent of batch duration, unlike taking the lease before the batch, which would expose the whole batch duration to the race.
Edited by John Mason

Merge request reports