You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2024/05/10 19:41:34 UTC
[PR] Add timeout when watching pod events in k8s executor [airflow]
dstandish opened a new pull request, #39551:
URL: https://github.com/apache/airflow/pull/39551
If we don't set a timeout, it may hang indefinitely if there's a network issue.
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1597193472
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/94e42113a1fe5c580917decacdde879eab7406b3/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/b47caad922709350f477210317ac7f9574a72a97/kubernetes/client/api_client.py#L336-L339
Review Comment:
🤷
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1601932483
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/94e42113a1fe5c580917decacdde879eab7406b3/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/b47caad922709350f477210317ac7f9574a72a97/kubernetes/client/api_client.py#L336-L339
+ request_timeout = 30
+ server_conn_timeout = 3600
+ kwargs["_request_timeout"] = request_timeout
+ kwargs["timeout_seconds"] = server_conn_timeout
Review Comment:
i fixed the naming (request vs client)
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "collinmcnulty (via GitHub)" <gi...@apache.org>.
collinmcnulty commented on PR #39551:
URL: https://github.com/apache/airflow/pull/39551#issuecomment-2111127914
Should this be marked as closes #36998 ?
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "dirrao (via GitHub)" <gi...@apache.org>.
dirrao commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1597346417
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/v29.0.0/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/v29.0.0/kubernetes/client/api_client.py#L336-L339
+ request_timeout = 30
+ server_conn_timeout = 3600
+ kwargs["_request_timeout"] = request_timeout
Review Comment:
Why don't we use the configuration `kube_client_request_args`?
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1597188968
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/94e42113a1fe5c580917decacdde879eab7406b3/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/b47caad922709350f477210317ac7f9574a72a97/kubernetes/client/api_client.py#L336-L339
Review Comment:
Why are these permanent links pointing to different versions?
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/94e42113a1fe5c580917decacdde879eab7406b3/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/b47caad922709350f477210317ac7f9574a72a97/kubernetes/client/api_client.py#L336-L339
+ request_timeout = 30
+ server_conn_timeout = 3600
+ kwargs["_request_timeout"] = request_timeout
+ kwargs["timeout_seconds"] = server_conn_timeout
Review Comment:
```suggestion
kwargs["_request_timeout"] = 30 # client-side timeout
kwargs["timeout_seconds"] = 3600 # server-side timeout
```
nit
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1597369523
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/v29.0.0/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/v29.0.0/kubernetes/client/api_client.py#L336-L339
+ request_timeout = 30
+ server_conn_timeout = 3600
+ kwargs["_request_timeout"] = request_timeout
Review Comment:
this PR is about adding a default timeout for this specific call, not about letting the user optionally override the timeout; that is a separate issue / question IMHO
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #39551:
URL: https://github.com/apache/airflow/pull/39551#issuecomment-2111169953
> Should this be marked as closes #36998 ?
it's not obvious to me how this would be connected to that issue
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1597195303
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -165,6 +163,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/94e42113a1fe5c580917decacdde879eab7406b3/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/b47caad922709350f477210317ac7f9574a72a97/kubernetes/client/api_client.py#L336-L339
Review Comment:
```suggestion
# https://github.com/kubernetes-client/python/blob/v29.0.0/examples/watch/timeout-settings.md
# and https://github.com/kubernetes-client/python/blob/v29.0.0/kubernetes/client/api_client.py#L336-L339
```
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "brumi1024 (via GitHub)" <gi...@apache.org>.
brumi1024 commented on code in PR #39551:
URL: https://github.com/apache/airflow/pull/39551#discussion_r1666861403
##########
airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -150,6 +148,14 @@ def _run(
last_resource_version: str | None = None
+ # For info about k8s timeout settings see
+ # https://github.com/kubernetes-client/python/blob/v29.0.0/examples/watch/timeout-settings.md
+ # and https://github.com/kubernetes-client/python/blob/v29.0.0/kubernetes/client/api_client.py#L336-L339
+ client_timeout = 30
+ server_conn_timeout = 3600
+ kwargs["_request_timeout"] = client_timeout
+ kwargs["timeout_seconds"] = server_conn_timeout
Review Comment:
These settings were configurable using the AIRFLOW__KUBERNETES_EXECUTOR__KUBE_CLIENT_REQUEST_ARGS env variable, but now they are overridden if I'm not mistaken. Is this expected?
--
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] Add timeout when watching pod events in k8s executor [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #39551:
URL: https://github.com/apache/airflow/pull/39551
--
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