Repository navigation
Fix KubernetesPodOperator 404 on pod preemption (#59626) #60626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4eef285
38410d2
5650b1b
1daf68f
9c319d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |
|
|
||
| from airflow.configuration import conf | ||
| from airflow.providers.cncf.kubernetes.backcompat import get_logical_date_key | ||
| from airflow.providers.cncf.kubernetes.exceptions import PodPreemptedException | ||
| from airflow.providers.common.compat.sdk import AirflowException | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -64,7 +65,10 @@ class KubernetesApiException(AirflowException): | |
|
|
||
|
|
||
| def _should_retry_api(exc: BaseException) -> bool: | ||
| """Retry on selected ApiException status codes, plus plain HTTP/timeout errors.""" | ||
| """Retry on transient API errors, HTTP errors, and PodPreemptedException.""" | ||
| if isinstance(exc, PodPreemptedException): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is wrong. The function is used to retry calling API if e.g. a HTTP 500 or 403 is returned - so on call level. I assume you wanted to make the Pod execution retryable - but this is not defined with this method. |
||
| # Pod was preempted before running - safe to retry | ||
| return True | ||
| if isinstance(exc, (SyncApiException, AsyncApiException)): | ||
| return exc.status in TRANSIENT_STATUS_CODES | ||
| return isinstance(exc, (HTTPError, KubernetesApiException)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ | |
| from urllib3.exceptions import HTTPError, TimeoutError | ||
|
|
||
| from airflow.providers.cncf.kubernetes.callbacks import ExecutionMode, KubernetesPodOperatorCallback | ||
| from airflow.providers.cncf.kubernetes.exceptions import PodNotFoundException, PodPreemptedException | ||
| from airflow.providers.cncf.kubernetes.kubernetes_helper_functions import ( | ||
| KubernetesApiException, | ||
| PodLaunchFailedException, | ||
|
|
@@ -208,8 +209,28 @@ class PodLaunchTimeoutException(AirflowException): | |
| """When pod does not leave the ``Pending`` phase within specified timeout.""" | ||
|
|
||
|
|
||
| class PodNotFoundException(AirflowException): | ||
| """Expected pod does not exist in kube-api.""" | ||
| @dataclass | ||
| class PodPhaseTracker: | ||
| """ | ||
| Track whether a pod ever reached Running state for safe 404 retry handling. | ||
|
|
||
| If pod never reached Running: safe to retry (likely preemption). | ||
| If pod was Running: NOT safe to retry (may cause duplicate execution). | ||
| """ | ||
|
|
||
| ever_reached_running: bool = False | ||
| last_observed_phase: str | None = None | ||
|
|
||
| def update_from_pod(self, pod: V1Pod) -> None: | ||
| """Update tracker state from observed pod status.""" | ||
| if pod.status and pod.status.phase: | ||
| self.last_observed_phase = pod.status.phase | ||
| if pod.status.phase == PodPhase.RUNNING: | ||
| self.ever_reached_running = True | ||
|
Comment on lines
+228
to
+229
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about the race condition if the Pod was so fast that it is already failed or completed when checking? I think it might be a bit dangerous checking just for running.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for pods running very quickly, (Pending → Running → Succeeded/Failed), with this we can safely consider, any phase beyond Pending means pod was scheduled and not preempted. |
||
|
|
||
| def is_safe_to_retry_on_404(self) -> bool: | ||
| """Return True if pod never reached Running (safe to retry).""" | ||
| return not self.ever_reached_running | ||
|
|
||
|
|
||
| class PodLogsConsumer: | ||
|
|
@@ -811,13 +832,54 @@ def read_pod_events(self, pod: V1Pod, resource_version: str | None = None) -> Co | |
| raise KubernetesApiException(f"There was an error reading the kubernetes API: {e}") | ||
|
|
||
| @generic_api_retry | ||
| def read_pod(self, pod: V1Pod) -> V1Pod: | ||
| """Read POD information.""" | ||
| def read_pod(self, pod: V1Pod, *, phase_tracker: PodPhaseTracker | None = None) -> V1Pod: | ||
| """ | ||
| Read POD information. | ||
|
|
||
| :param pod: The pod object to read. | ||
| :param phase_tracker: Optional tracker to monitor pod lifecycle state. | ||
| When provided, enables state-aware retry behavior on 404 errors | ||
| to handle pod preemption during node bootstrap. | ||
| """ | ||
| try: | ||
| return self._client.read_namespaced_pod(pod.metadata.name, pod.metadata.namespace) | ||
| result = self._client.read_namespaced_pod(pod.metadata.name, pod.metadata.namespace) | ||
| if phase_tracker: | ||
| phase_tracker.update_from_pod(result) | ||
| return result | ||
| except ApiException as e: | ||
| if e.status == 404: | ||
| self._handle_pod_not_found(pod, phase_tracker) | ||
| raise | ||
| except HTTPError as e: | ||
| raise KubernetesApiException(f"There was an error reading the kubernetes API: {e}") | ||
|
|
||
| def _handle_pod_not_found(self, pod: V1Pod, phase_tracker: PodPhaseTracker | None) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code here is redundant (and some more) here in the utility and the hook. please consolidate to one place and copy only.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, will consolidate under PodPhaseTracker. |
||
| """ | ||
| Handle 404 error with state-aware exception raising. | ||
|
|
||
| :raises PodNotFoundException: If pod was previously running. | ||
| :raises PodPreemptedException: If pod never reached running. | ||
| """ | ||
| pod_name = pod.metadata.name | ||
| namespace = pod.metadata.namespace | ||
|
|
||
| if phase_tracker and not phase_tracker.is_safe_to_retry_on_404(): | ||
| # Pod was running - don't retry to prevent duplicate execution | ||
| raise PodNotFoundException( | ||
| f"Pod '{pod_name}' in namespace '{namespace}' not found. " | ||
| f"The pod was previously observed in Running state (last phase: " | ||
| f"{phase_tracker.last_observed_phase}), so this is treated as a " | ||
| f"terminal failure to prevent duplicate execution of non-idempotent workloads." | ||
| ) | ||
| # Pod never ran - likely preempted during node bootstrap | ||
| raise PodPreemptedException( | ||
| f"Pod '{pod_name}' in namespace '{namespace}' not found. " | ||
| f"The pod was never observed in Running state, which may indicate " | ||
| f"preemption by higher-priority pods (e.g., daemonsets) during node bootstrap. " | ||
| f"Check Kubernetes events: kubectl get events --field-selector " | ||
| f"reason=Preempted -n {namespace}" | ||
| ) | ||
|
|
||
| def await_xcom_sidecar_container_start( | ||
| self, pod: V1Pod, timeout: int = 900, log_interval: int = 30 | ||
| ) -> None: | ||
|
|
@@ -1007,12 +1069,23 @@ def __init__( | |
| self._callbacks = callbacks or [] | ||
| self.stop_watching_events = False | ||
|
|
||
| async def read_pod(self, pod: V1Pod) -> V1Pod: | ||
| """Read POD information.""" | ||
| return await self._hook.get_pod( | ||
| async def read_pod(self, pod: V1Pod, *, phase_tracker: PodPhaseTracker | None = None) -> V1Pod: | ||
| """ | ||
| Read POD information. | ||
|
|
||
| :param pod: The pod object to read. | ||
| :param phase_tracker: Optional tracker to monitor pod lifecycle state. | ||
| When provided, enables state-aware retry behavior on 404 errors | ||
| to handle pod preemption during node bootstrap. | ||
| """ | ||
| result = await self._hook.get_pod( | ||
| pod.metadata.name, | ||
| pod.metadata.namespace, | ||
| phase_tracker=phase_tracker, | ||
| ) | ||
| if phase_tracker and result: | ||
| phase_tracker.update_from_pod(result) | ||
| return result | ||
|
|
||
| async def read_pod_events(self, pod: V1Pod, resource_version: str | None = None) -> CoreV1EventList: | ||
| """Get pod's events.""" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not clear to me. There might be cases where a logic is only allowed to start only once and then a broken state must not retried - and there are cases where a failed execution still should retry.
I understand that you want to handle it differently but the exceptions are just different exceptions, but it is not changing anything in retry handling. Also the text is mis-leading because besides the exception types the "normal" retry would happen if configured?
For me this PR only makes sense if a configurable setting based on Exception type skips or makes the retry based on preemption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jscheffl, thanks for the valuable feedback.
As task-level retries will, in turn, try pod re-creation, I am wondering if it is worth adding more complexity for internal retries. I will rely on your discretion here.
If I were to continue implementing - configurable part: