Add opt-in concurrent pod creation to KubernetesExecutor - #68480
Conversation
d17fb0e to
22102a4
Compare
22102a4 to
8a19e1e
Compare
jscheffl
left a comment
There was a problem hiding this comment.
I am not a KubernetesExecutor expert as not using it but as I am an asyncio fanboy I really understand and like the improvement.
I hope that this feature matures over time and then the sync and async code streams can be consolidated (meaning: drop the sync option because in the way implemented and K8s API client restricts it needs two parallel implementation streams to maintain).
As not being expert requesting another pair of eyes as review, my comments are just from reading not from running myself.
|
Please resolve conflicts |
8a19e1e to
4718057
Compare
|
@1fanwang — this PR has 1 unresolved review thread(s) that still look like they need your attention. Once you've addressed them (push changes and/or reply in-thread), please resolve the threads or reply to confirm, and give the reviewer a nudge for another look. Thanks! See the PR quality criteria. Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
thanks for the ping, resolved |
|
@1fanwang Could you please resolve conflicts? |
d25fe89 to
191adaa
Compare
|
@1fanwang could you please resolve conflicts again? |
2381f78 to
ea6d76e
Compare
The KubernetesExecutor creates worker pods one at a time within each scheduler loop: run_next() issues a synchronous create_namespaced_pod and blocks on the API response before starting the next. When per-create latency is high (API round-trip, mutating admission webhooks), this serializes the scheduler loop and caps pod-creation throughput per loop. Add an opt-in concurrent path: when [kubernetes_executor] async_pod_creation is True, the pods dequeued in a loop are built synchronously (pod-mutation hook and reconciliation unchanged), then their create calls are issued concurrently via the asynchronous Kubernetes client, bounded by pod_creation_max_concurrency. The sequential path stays the default and is unchanged. A new pod_creation_batch_duration metric (emitted by both paths) measures per-loop batch creation time.
Move the request-timeout ApiClient wrappers into kube_client.py and build the executor's async client from _TimeoutAsyncK8sApiClient, so async creates carry the same client-side request timeout as the rest of the provider and the timeout logic lives in one place. Drop the config-parse test that only restated values already exercised by the async-creation tests.
Replace the str(e.status) == "NNN" comparisons in the pod-publish error handler with http.HTTPStatus constants; type the timeout-wrapper call_api signatures; and return the concurrent batch's per-pod errors as a position-aligned list instead of a dict keyed on id(job).
…ync type The shared pod-publish error handler now matches both the sync and async client ApiException (they expose the same status/body/reason/headers), mirroring _should_retry_api. The concurrent path re-raises its native exception, so the async-to-sync conversion helper is gone.
Trim the batch-metric and concurrent-create docstrings to the why, drop the refactor-history reference in _build_pod_request, and condense the positional result-mapping comment.
Add explicit type hints to the remaining locals in the new code (batch counters and timings, the dequeued job, the error-handler body, and the gather outcomes) for consistency with the rest of the diff. Verified with mypy.
Following review: rather than KPO's in-line generic_api_retry decorator (its blocking backoff would stall the scheduler loop, and the whole run_until_complete batch on the concurrent path), broaden the shared _handle_pod_publish_error so both creation paths re-queue the transient failures it previously dropped. 502/503/504 join the re-queue set, with 503's Retry-After honored like 429. Connection-level errors (urllib3 on the sync client, aiohttp on the async) — which previously crashed the sequential sync loop or failed the task on the concurrent path — are now re-queued too, via a TRANSIENT_CONNECTION_ERRORS set shared with generic_api_retry so the two agree on what counts as transient.
Trim redundant second sentences; keep the load-bearing notes.
MaxRetryError's first arg is typed ConnectionPool; pass a real pool instead of None.
KubernetesExecutor.task_queue is Optional (lazily created in start()), so the sequential and concurrent creation paths and the shared publish-error handler need the same TYPE_CHECKING assert the rest of the executor already uses before dereferencing it. Signed-off-by: 1fanwang <1fannnw@gmail.com>
ea6d76e to
d9776b5
Compare
…e DAGs A 5-minute getting-started (install, flagd, a DAG, ramp, kill switch). Rewrite use-cases around the standard toggle taxonomy mapped to Airflow, with the deployment-canary-vs-flag positioning (Argo/ Flagger don't support queue workers) and copy-paste recipes. Add example DAGs for 2->3 migration, a KubernetesExecutor canary (apache/airflow#68480), and A/B-a-model with exposure, plus a DagBag test that keeps them importable. Signed-off-by: 1fanwang <1fannnw@gmail.com>
A flag routes a DAG cohort to the kubernetes executor; each routed task launches a real pod on a kind cluster, and ramping the flag 25% -> 50% grows the cohort live via flagd hot-reload. The reliable core of the KubernetesExecutor-canary use case (cf. apache/airflow#68480) without a full executor deployment. Signed-off-by: 1fanwang <1fannnw@gmail.com>
Why
KubernetesExecutorcreates worker pods one at a time — each create call blocks before the nextstarts. At production create latencies (~150–500ms of round-trip plus admission webhooks), the
executor, not the cluster, caps task startup. The existing batch size dequeues more pods per loop
but still creates them serially, so per-create latency re-serializes them.
The cost lands on
task.queued_duration: it grows with a task's position in the batch, so afan-out's average and p99 inflate while the first task is unaffected. Dynamic task mapping makes
large fan-outs routine.
What
Opt-in, off by default. The pods dequeued in a scheduler loop are created concurrently instead of
serially, bounded by a configurable in-flight limit. The serial path stays the default and
unchanged (retry, exceeded-quota, rate-limit backoff). A new metric reports per-loop batch
creation time on both paths.
Benchmark
Live multi-node
kind, per-create latency injected by an admission webhook.Create serialization (webhook admission timestamps, which isolate the create call from pod start),
30-pod batch, concurrency 32:
Tail (last task) across the production band — serial scales with latency and position, async stays flat:
End-to-end
task.queued_duration, read from the metadata DB (start_date − queued_dttm) per mappedtask — the actual metric, not derived. 30-way fan-out; per-create latency raised to 2s so the create
serialization dominates the single-host kubelet-start floor (a small-cluster artifact — a real
multi-node cluster starts the batch in parallel):
task.queued_durationAverage falls 1.9×, p99 and tail 3.1×. It is opt-in because at low latency or small batches the gap
shrinks toward zero and async carries a small fixed overhead.
Verification
Unit: serial path unchanged; new coverage for concurrent creation, the concurrency bound, error
handling and backoff on the concurrent path, per-pod failure isolation, and the Airflow 3 path.
E2E: live
kind, real apiserver — the concurrent path creates pods as measured above.Hardening (from review)
_handle_pod_publish_errornow re-queues transient failures it previously dropped, on both paths:502/503/504(503'sRetry-Afterhonored like429) and connection-level errors on the sync(
urllib3) and async (aiohttp) clients — the latter previously crashed the sequential loop.E2E — error injected on the first CREATE per task,
task_publish_max_retries=3503+Retry-Afteraiohttp)0 / 6)urllib3)Open question: a connection error can arrive after the pod was created (reply lost), so re-queue can
double-create — same risk as today's
500re-queue. Gate to connect-phase failures?