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

Limit pod_name length to HOST_NAME_MAX #36332

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

csp33
Copy link
Contributor

@csp33 csp33 commented Dec 20, 2023

Closes #36326

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues labels Dec 20, 2023
@csp33 csp33 force-pushed the bugfix/36326-shorten-pod-name branch 2 times, most recently from 0821ff8 to 72022f4 Compare December 20, 2023 14:57
@csp33 csp33 changed the title Make pod_name length equal to HOST_NAME_MAX Limit pod_name length to HOST_NAME_MAX Dec 20, 2023
@csp33 csp33 force-pushed the bugfix/36326-shorten-pod-name branch 4 times, most recently from 5b3c57c to f71207f Compare December 20, 2023 15:45
@potiuk
Copy link
Member

potiuk commented Dec 20, 2023

How do you plan to tackle backwards compatibilty. Imagine you are upgrading Airflow and thera are long running Pods still running being adopted by the new Airflow with limited length/ What is your plan to do with it?

@dirrao
Copy link
Contributor

dirrao commented Dec 21, 2023

How do you plan to tackle backwards compatibilty. Imagine you are upgrading Airflow and thera are long running Pods still running being adopted by the new Airflow with limited length/ What is your plan to do with it?

Pod adoption is seamless. As we are not using pod name to deduce anything. Its just a reference to identify the dag and task from pod while debugging.

@dirrao
Copy link
Contributor

dirrao commented Dec 21, 2023

@csp33
Nice work. LGTM. we have seen problem where K8 cluster not able to resolve/update DNS because of when they exceeds max length 63.

@csp33 csp33 force-pushed the bugfix/36326-shorten-pod-name branch from f71207f to c547b16 Compare December 21, 2023 07:26
@csp33
Copy link
Contributor Author

csp33 commented Dec 21, 2023

How do you plan to tackle backwards compatibilty. Imagine you are upgrading Airflow and thera are long running Pods still running being adopted by the new Airflow with limited length/ What is your plan to do with it?

K8s executor does not use the pod name to adopt previous TIs, it uses the queued_by_job_id field instead:

f"airflow-worker={scheduler_job_id},{POD_EXECUTOR_DONE_KEY}!=True"

Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work. LGTM.

@potiuk
Copy link
Member

potiuk commented Dec 21, 2023

How do you plan to tackle backwards compatibilty. Imagine you are upgrading Airflow and thera are long running Pods still running being adopted by the new Airflow with limited length/ What is your plan to do with it?

K8s executor does not use the pod name to adopt previous TIs, it uses the queued_by_job_id field instead:

f"airflow-worker={scheduler_job_id},{POD_EXECUTOR_DONE_KEY}!=True"

Cool

@csp33 csp33 force-pushed the bugfix/36326-shorten-pod-name branch from c547b16 to aa80afd Compare December 21, 2023 14:37
@csp33 csp33 requested a review from potiuk December 21, 2023 14:38
@potiuk potiuk merged commit 381922f into apache:main Dec 21, 2023
57 checks passed
@csp33 csp33 deleted the bugfix/36326-shorten-pod-name branch December 22, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task hostname does not reflect pod name when it's too long
3 participants