kartik00052 commented on PR #73395: URL: https://github.com/apache/airflow/pull/73395#issuecomment-5772913600
Reading through the retry/failure paths after seeing this issue and #73395 — a couple of things worth pointing at directly with code references, plus a possible way to close the remaining gap. **A bounded, separately-counted infra retry already exists for one case.** `KubernetesExecutor._is_pre_execution_failure()` (in `kubernetes_executor.py`) already distinguishes a pod that failed *before* the task instance reached `RUNNING` from one that failed after — it checks `ti_state == QUEUED` at failure time, and if that holds, the pod is requeued on its own counter (`pod_launch_failure_max_retries`) without ever touching `task.retries`. That's exactly the "infra failure shouldn't burn the application's retry budget" behavior this issue is asking for — it just only covers pod-launch failures. **The mid-execution case — the one in this issue's screenshot — doesn't get that treatment.** Once a task instance is `RUNNING` and gets killed externally (pod eviction, node drain, SIGTERM), that's the case #73395 is fixing: right now on `main`, `_on_term` in the task runner doesn't raise anything, so `execute()` silently resumes and the task can end up reported `SUCCESS` even though the work was interrupted. #73395 fixes that by raising `AirflowTaskTerminated`. But looking at how `task_runner.py` handles that exception (the except-chain around `except AirflowTaskTerminated`), it goes straight to `state = TaskInstanceState.FAILED` — it never calls `_handle_current_task_failed`, which is the path that checks `TaskInstance.is_eligible_to_retry()`. So after #73395 merges, a mid-execution infra kill won't silently succeed anymore, but it *will* permanently fail with zero retries even if the task has `retries=5` set. #73395's own PR description flags this directly: *"Whether an infrastructure kill should instead be retry-eligible is a real question — it is what #73238 is about... changing that classification also changes the UI 'mark failed' path, so it is left alone here."* So the gap this issue is really about, narrowed down: **`AirflowTaskTerminated` needs the same kind of separate, bounded retry budget that pod-launch failures already get — not the application's `task.retries`, and not zero.** **A possible shape for that**, modeled on the existing pattern rather than inventing something new: - A new config, e.g. `infrastructure_failure_max_retries`, parallel to `pod_launch_failure_max_retries`. - The real complication: `pod_launch_failure_max_retries` is tracked in `KubernetesExecutor.pod_launch_attempts`, an in-memory counter on the scheduler process. `AirflowTaskTerminated` is raised inside the task runner process itself — it doesn't have access to that. So this can't just reuse the same storage; the counter would need to live somewhere both sides can see it (task instance / task instance history row seems like the natural place, but I haven't traced whether there's already a suitable field, or whether this needs a new column/attempt-tracking concept). - Rather than making `AirflowTaskTerminated` unconditionally terminal, it would need to go through something like `_handle_current_task_failed`'s retry-eligibility check, but against the new infra-budget instead of (or in addition to) `task.retries`. - I'd explicitly leave the UI "mark failed" path alone, same as #73395 did — that's a separate behavior change and probably deserves its own discussion if anyone wants to touch it. Given this touches task-sdk (where `AirflowTaskTerminated` is raised) and core retry semantics (not just the K8s executor), I'd guess this needs at least a short AIP rather than going straight to a PR, but I'm not certain where the line is for a change this size — happy to be corrected. Open questions I don't have good answers to yet: - Where should the infra-retry counter actually live given the cross-process constraint above? - Should this budget be K8s-executor-specific (like today's pod-launch one) or executor-agnostic, since `AirflowTaskTerminated` itself is raised in executor-agnostic code? - Does this want to be one unified "infrastructure failure retry budget" covering both pre-execution and mid-execution cases, or should they stay as separate mechanisms since they're detected completely differently? Happy to take a pass at drafting this out further (or an AIP if that's the right next step) if this direction seems reasonable — wanted to sanity-check the approach here first rather than show up with a PR that reopens a decision #73395 intentionally deferred. -- 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]
