You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jedcunningham (via GitHub)" <gi...@apache.org> on 2024/07/30 02:54:10 UTC
[PR] Fix ApiException handling when adopting completed pods [airflow]
jedcunningham opened a new pull request, #41109:
URL: https://github.com/apache/airflow/pull/41109
When trying to complete adopted pods, if we encounter an ApiException, we should assume we were unable to adopt the pod meaning we shouldn't add that to our `running` set. If we do, `running` can fill up over time with tasks the executor isn't actually watching.
cc @wolfier
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ApiException handling when adopting completed pods [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #41109:
URL: https://github.com/apache/airflow/pull/41109#discussion_r1696285950
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:
##########
@@ -694,8 +694,9 @@ def _adopt_completed_pods(self, kube_client: client.CoreV1Api) -> None:
)
except ApiException as e:
self.log.info("Failed to adopt pod %s. Reason: %s", pod.metadata.name, e)
- ti_id = annotations_to_key(pod.metadata.annotations)
- self.running.add(ti_id)
+ else:
Review Comment:
Nit: Is there a way maybe to distinguish temporary from permanent exceptions? I do not know the API well - but this one calls for (if transient -> retry) case.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ApiException handling when adopting completed pods [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham merged PR #41109:
URL: https://github.com/apache/airflow/pull/41109
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ApiException handling when adopting completed pods [airflow]
Posted by "wolfier (via GitHub)" <gi...@apache.org>.
wolfier commented on code in PR #41109:
URL: https://github.com/apache/airflow/pull/41109#discussion_r1696354408
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:
##########
@@ -694,8 +694,9 @@ def _adopt_completed_pods(self, kube_client: client.CoreV1Api) -> None:
)
except ApiException as e:
self.log.info("Failed to adopt pod %s. Reason: %s", pod.metadata.name, e)
- ti_id = annotations_to_key(pod.metadata.annotations)
- self.running.add(ti_id)
+ else:
Review Comment:
In [adopt_launched_task](https://github.com/apache/airflow/blob/2.9.3/airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py#L671-L679), the exception clause calls return.
```
try:
kube_client.patch_namespaced_pod(
name=pod.metadata.name,
namespace=pod.metadata.namespace,
body={"metadata": {"labels": {"airflow-worker": new_worker_id_label}}},
)
except ApiException as e:
self.log.info("Failed to adopt pod %s. Reason: %s", pod.metadata.name, e)
return
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ApiException handling when adopting completed pods [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #41109:
URL: https://github.com/apache/airflow/pull/41109#discussion_r1696901831
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:
##########
@@ -694,8 +694,9 @@ def _adopt_completed_pods(self, kube_client: client.CoreV1Api) -> None:
)
except ApiException as e:
self.log.info("Failed to adopt pod %s. Reason: %s", pod.metadata.name, e)
- ti_id = annotations_to_key(pod.metadata.annotations)
- self.running.add(ti_id)
+ else:
Review Comment:
Oh, and I switched to `continue` instead of `else`. Same behavior, easier to read imo.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix ApiException handling when adopting completed pods [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #41109:
URL: https://github.com/apache/airflow/pull/41109#discussion_r1696900146
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:
##########
@@ -694,8 +694,9 @@ def _adopt_completed_pods(self, kube_client: client.CoreV1Api) -> None:
)
except ApiException as e:
self.log.info("Failed to adopt pod %s. Reason: %s", pod.metadata.name, e)
- ti_id = annotations_to_key(pod.metadata.annotations)
- self.running.add(ti_id)
+ else:
Review Comment:
Even if it is transient, there isn't much harm in just waiting until the next cycle to try adopting it again.
There isn't a robust way to know if its transient, afaik. You can approach it like [this](https://github.com/apache/airflow/blob/41508f23ad87bdf410978ff96e62d8057d17d9d0/airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py#L411), but I don't think it's worth it in this case.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org