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

Keep pod name for k8s executor under 63 characters #28237

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

dstandish
Copy link
Contributor

Because of the way that the task log handler reads from running k8s executor pods, we must keep pod name <= 63 characters. The handler gets pod name from ti.hostname. TI hostname is derived from the container hostname, which is truncated to 63 characters. We could lift this limit by using label selectors instead of pod name to find the pod. But for now, easy enough to keep limited to 63.

Since we limit to 63 in the code, we can remove the logic to find the matching pod when length is >= 63.


Note:

Recently I made changes that allowed for longer pod name, and take up less space on the random part, and allow hyphens. At the time it wasn't clear why the pod id length had to be limited. But I think the reason is this task log issue. So we restore the length limit here, while still retaining the other improvements. And we make clear in the place where we limit what is the reason for the limit. And we remove the code to handle longer limit since it's not needed. For KPO, we can keep the longer name since hostname is not used for pod id. (there we use labels)

Because of the way that the task log handler reads from running k8s executor pods, we must keep pod name <= 63 characters.  The handler gets pod name from ti.hostname. TI hostname is derived from the container hostname, which is truncated to 63 characters. We could lift this limit by using label selectors instead of pod name to find the pod. But for now, easy enough to keep limited to 63.

Since we limit to 63 in the code, we can remove the logic to find the matching pod when length is >= 63.
@dstandish dstandish force-pushed the shorten-pod-id-for-kube-executor branch from b246b4f to af766e9 Compare December 8, 2022 20:27
@dstandish dstandish added this to the Airflow 2.6.0 milestone Dec 8, 2022
@dstandish dstandish merged commit bdc3d2e into apache:main Dec 8, 2022
@dstandish dstandish deleted the shorten-pod-id-for-kube-executor branch December 8, 2022 21:11
@ashb
Copy link
Member

ashb commented Dec 22, 2022

I am not sure this is the right change, given that we have had https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#hostname-callable for a while, and using airflow.utils.net.get_host_ip_address as the value for this config would entirely negate this.

@dstandish I vote we revert this PR

@dstandish
Copy link
Contributor Author

dstandish commented Dec 22, 2022

i am not sure it does.

how does it?

FTH gets the pod name from ti.hostname. If you store IP in ti.hostname, then now you are in even worse situation.

And note.... it was previously limited to 63, but then i lifted the limit (in code never released) when trying to make a "nicer pod id" -- shorter random suffix (8 instead of 36, allowing for more meaningful content), and allow hyphens for readability

But then when I was digging around task handler i discoverd why this is necessary, until / unless we switch to labels for "find pod" or store pod name in TI. So this PR in effect is reverting to the existing behavior.

WDYT?

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants