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
Allow longer pod names for k8s executor / KPO #27736
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boring-cyborg
bot
added
the
provider:cncf-kubernetes
Kubernetes provider related issues
label
Nov 17, 2022
eladkal
reviewed
Nov 17, 2022
dstandish
force-pushed
the
allow-longer-k8s-pod-ids
branch
2 times, most recently
from
November 18, 2022 07:08
c4894d4
to
ba90692
Compare
o-nikolas
reviewed
Nov 19, 2022
dstandish
force-pushed
the
allow-longer-k8s-pod-ids
branch
2 times, most recently
from
November 20, 2022 07:44
3f70863
to
fe06764
Compare
Errors |
dstandish
force-pushed
the
allow-longer-k8s-pod-ids
branch
from
December 2, 2022 22:51
fe06764
to
3e6efc8
Compare
Previously this limited pod length to 63 but that's the limit for label values not pod ids. I assume that's just an atavism. Meanwhile, a UUID was previously used which takes up 32 characters, much more space than necessary. We use 8 random alphanum chars now which should be plenty.
dstandish
force-pushed
the
allow-longer-k8s-pod-ids
branch
from
December 3, 2022 06:49
3e6efc8
to
da7d833
Compare
potiuk
approved these changes
Dec 5, 2022
84 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
provider:cncf-kubernetes
Kubernetes provider related issues
type:improvement
Changelog: Improvements
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously this limited pod length to 63 but that's the limit for label values not pod ids.
Meanwhile, a UUID was previously used which occupies 32 characters, much more space than necessary. This left you with a short and not very readable pod name.
Here we update the logic to use a suffix of 8 random alphanum chars, and set a longer max length of 80 characters.
One qualification on that. We set a max length of 80 when we control the pod name -- e.g. when using kube executor or KPO when pod name not supplied. Since we're generating it, 80 is a reasonable max. But, with KPO, when pod_id, and user asks for random suffix, we set max length to 253, to give user most control.
Along the way, we consolidate the name building logic into one function. Previously, one function would create the "pod_id" and then, only later in the process would a random suffix be added. Here, all of that is done in create_pod_id, and its behavior is controllable with params. cc @o-nikolas
Why 80 characters: that's about as much as can reasonably fit on a normal-sized terminal. Any more doesn't help with readability.
Why 8 character suffix: 8 random characters is more than enough. Probably fewer would be fine too.
Finally, a note.... I copied the core code to provider and we must keep it there until min airflow version catches up. And instead of duplicating test code, i parameterized the core test to also handle provider.
Now, an example.
Calling with
We would previously get this:
And now we'd get this: