Fix KubernetesPodOperator pod name length validation #8829

dsaiztc · 2020-05-11T22:28:07Z

In #6523 a validation was added to KubernetesPodOperator that verifies the provided name conforms to the maximum number of characters for Kubernetes pods.

The provided character limit in such PR is not correct as it's using the maximum number of characters for a Kubernetes label, which is 63, instead of the maximum number of characters for Kubernetes name, which is 253 [1].

However, the char limit set in this module depends on the implementation of airflow.kubernetes.pod_generator.PodGenerator. Upon the commit this PR is based, whatever the length of the given name, such name will be shortened to conform to the norm (253 characters). In the referenced commit an additional 33 characters are added (- + 32 random), which in turns means that this should validate that the name is no longer that 220 characters (the value proposed).

Bear also in mind that current v.10.10 does not have the implementation of the commit mentioned but a different one where the name is not shortened but directly concatenated to another 9 characters (- + 8 random) and thus the name this limit could be set to 244 (instead of the 220 characters proposed).

[1] Identifiers and Names in Kubernetes

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Unit tests coverage for changes (not needed for documentation changes)
Target Github ISSUE in description if exists
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

boring-cyborg · 2020-05-11T22:28:11Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://apache-airflow-slack.herokuapp.com/

ddelange · 2020-05-12T08:19:12Z

airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

@@ -322,5 +322,5 @@ def _set_resources(resources):
    def _set_name(self, name):
        if self.pod_template_file or self.full_pod_spec:
            return None
-        validate_key(name, max_length=63)
+        validate_key(name, max_length=220)


LGTM, we can keep it on 220 also for v1.10, as preparation for the longer suffix added in v2.0

Any further action needed from my side? 🤔

I'm not sure what else is needed for this fix to get on the v1.10 merge train 🚂

merging is blocked as long as there are unticked TODOs in the PR description btw.

maybe @kaxil: anything else?

Can we add tests too, please?

@dimberman Can you take a look please at this PR?

LGTM just would like to see some tests.

dimberman · 2020-05-14T22:32:06Z

airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

@@ -322,5 +322,5 @@ def _set_resources(resources):
    def _set_name(self, name):
        if self.pod_template_file or self.full_pod_spec:
            return None
-        validate_key(name, max_length=63)
+        validate_key(name, max_length=220)


LGTM just would like to see some tests.

dimberman · 2020-05-15T13:19:39Z

thank you @ddelange! Once tests pass I'll merge.

dsaiztc · 2020-05-15T15:40:31Z

All tests passed @dimberman =)

Any further feedback welcome.

boring-cyborg · 2020-05-15T15:41:55Z

Awesome work, congrats on your first merged pull request!

dimberman · 2020-05-15T15:42:23Z

Thank you @dsaiztc !

* Fix KubernetesPodOperator pod name length validation * Add test, verify Exception is raised (cherry picked from commit f82ad45)

Fix KubernetesPodOperator pod name length validation

660d042

boring-cyborg bot added the k8s label May 11, 2020

dsaiztc mentioned this pull request May 11, 2020

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6523

Merged

2 tasks

ddelange reviewed May 12, 2020

View reviewed changes

kaxil added this to the Airflow 1.10.11 milestone May 14, 2020

kaxil requested a review from dimberman May 14, 2020 22:02

kaxil removed this from the Airflow 1.10.11 milestone May 14, 2020

dimberman requested changes May 14, 2020

View reviewed changes

Add test, verify Exception is raised

a83b9ef

dsaiztc force-pushed the feature/k8s-operator-pod-name-length branch from 2dd9a6f to a83b9ef Compare May 15, 2020 14:09

dsaiztc requested a review from dimberman May 15, 2020 15:41

dimberman approved these changes May 15, 2020

View reviewed changes

dimberman merged commit f82ad45 into apache:master May 15, 2020

dimberman added this to the Airflow 1.10.11 milestone May 15, 2020

dimberman pushed a commit that referenced this pull request Jun 17, 2020

Fix KubernetesPodOperator pod name length validation (#8829)

5e16a12

* Fix KubernetesPodOperator pod name length validation * Add test, verify Exception is raised (cherry picked from commit f82ad45)

potiuk pushed a commit that referenced this pull request Jun 29, 2020

Fix KubernetesPodOperator pod name length validation (#8829)

7ef7315

* Fix KubernetesPodOperator pod name length validation * Add test, verify Exception is raised (cherry picked from commit f82ad45)

kaxil pushed a commit that referenced this pull request Jul 1, 2020

Fix KubernetesPodOperator pod name length validation (#8829)

c9f0c2e

* Fix KubernetesPodOperator pod name length validation * Add test, verify Exception is raised (cherry picked from commit f82ad45)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix KubernetesPodOperator pod name length validation #8829

Fix KubernetesPodOperator pod name length validation #8829

dsaiztc commented May 11, 2020 •

edited

Loading

boring-cyborg bot commented May 11, 2020

ddelange May 12, 2020

dsaiztc May 12, 2020

ddelange May 12, 2020

kaxil May 14, 2020

dimberman May 14, 2020

dimberman May 14, 2020

dimberman commented May 15, 2020

dsaiztc commented May 15, 2020

boring-cyborg bot commented May 15, 2020

dimberman commented May 15, 2020

Fix KubernetesPodOperator pod name length validation #8829

Fix KubernetesPodOperator pod name length validation #8829

Conversation

dsaiztc commented May 11, 2020 • edited Loading

boring-cyborg bot commented May 11, 2020

ddelange May 12, 2020

Choose a reason for hiding this comment

dsaiztc May 12, 2020

Choose a reason for hiding this comment

ddelange May 12, 2020

Choose a reason for hiding this comment

kaxil May 14, 2020

Choose a reason for hiding this comment

dimberman May 14, 2020

Choose a reason for hiding this comment

dimberman May 14, 2020

Choose a reason for hiding this comment

dimberman commented May 15, 2020

dsaiztc commented May 15, 2020

boring-cyborg bot commented May 15, 2020

dimberman commented May 15, 2020

dsaiztc commented May 11, 2020 •

edited

Loading