Skip to content
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

KubernetesExecutor shadowing timeout_seconds and _request_seconds #45517

Open
2 tasks done
insomnes opened this issue Jan 9, 2025 · 5 comments · May be fixed by #45528
Open
2 tasks done

KubernetesExecutor shadowing timeout_seconds and _request_seconds #45517

insomnes opened this issue Jan 9, 2025 · 5 comments · May be fixed by #45528
Assignees
Labels
area:providers kind:bug This is a clearly a bug provider:cncf-kubernetes Kubernetes provider related issues

Comments

@insomnes
Copy link

insomnes commented Jan 9, 2025

Apache Airflow Provider(s)

cncf-kubernetes

Versions of Apache Airflow Providers

apache-airflow-providers-cncf-kubernetes==10.0.1

Apache Airflow version

2.10.4

Operating System

Debian Bookworm

Deployment

Other

Deployment details

It's deployment independent and related to code directly

What happened

kube client query options of timeout_seconds and _request_seconds provided via kube_client_request_args config option are re-written via hardcoded values in executor code of KubernetesJobWatcher.

What you think should happen instead

Values set via AIRFLOW__KUBERNETES_EXECUTOR__KUBE_CLIENT_REQUEST_ARGS for timeout_seconds and _request_seconds should be respected, hardcoded values should be provided only in case of missing keys in the provided JSON.

Simple if for checking if key is already present would be enough

How to reproduce

  1. Set the kube_client_request_args option with JSON: {"timeout_seconds": 1800, "_request_timeout": 60}
  2. Run airflow scheduler with kubernetes executor configured
  3. In logs you can see that without pod events the KubernetesJobWatcher will still die every 30 seconds instead of 60.

Anything else

The problem was introduced with:
610747d#diff-d884d637ab746b1301ce80b30ba2c1908299ba6f13edcad6535c837fb4d30938R151

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@insomnes insomnes added area:providers kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Jan 9, 2025
@dosubot dosubot bot added the provider:cncf-kubernetes Kubernetes provider related issues label Jan 9, 2025
@insomnes
Copy link
Author

insomnes commented Jan 9, 2025

I am happy to provide the PR fixing it.

I see this behavior as a 100% bug. But I want to be sure it's not intended in any way.

@RNHTTR
Copy link
Contributor

RNHTTR commented Jan 9, 2025

Is the idea to update these lines to default to 30/3600 but allow them to be overwritten via kube_client_request_args? If so, I think that makes sense.

@RNHTTR RNHTTR removed the needs-triage label for new issues that we didn't triage yet label Jan 9, 2025
@insomnes
Copy link
Author

insomnes commented Jan 9, 2025

Yes. I was thinking about checking the presence of these keys:

        if "_request_timeout" not in kwargs:
            client_timeout = 30
            kwargs["_request_timeout"] = client_timeout

@RNHTTR
Copy link
Contributor

RNHTTR commented Jan 9, 2025

Cool -- assigned you.

@Mizokuiam
Copy link

Thank you for raising this issue! I'll look into it and try to help if I can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers kind:bug This is a clearly a bug provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants