[AIRFLOW-5873] KubernetesPodOperator fixes and test #6523

ddelange · 2019-11-08T12:16:13Z

Make sure you have checked all steps below.

Jira

My PR addresses the following [Airflow Jira]
- https://issues.apache.org/jira/browse/AIRFLOW-5873

Description

KubernetesPodOperator kwarg resources is erroneously passed to base_operator, instead should only go to PodGenerator. The two have different syntax. (both on master and v1-10-test branches)
kubernetes/pod.py: Resources does not have __slots__ so accepts arbitrary values in setattr (present on v1-10-test branch https://github.com/apache/airflow/blame/50343040ff4679e32e01f138ead80bc4bcef4b47/airflow/contrib/operators/kubernetes_pod_operator.py#L166-L171)
KubernetesPodOperator kwarg in_cluster erroneously defaults to False in comparison to default_args.py, also default do_xcom_push was overwritten to False in contradiction to BaseOperator
v1-10-test is behind master with KubernetesPodOperator fixes and refactors (will not be addressed in this PR,)
- e.g. ~~move kubernetes folder one level up from /contrib~~ https://github.com/apache/airflow/blame/4dd24a2c595d4042ffe745aed947eaaea6abb652/airflow/contrib/operators/kubernetes_pod_operator.py#L21
- ~~fix xcom_push to do_xcom_push~~ https://github.com/apache/airflow/blame/4dd24a2c595d4042ffe745aed947eaaea6abb652/airflow/contrib/operators/kubernetes_pod_operator.py#L90

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
contrib/operators/test_kubernetes_pod_operator.py

ddelange · 2019-11-08T12:19:05Z

@ashb how should I cherry pick the relevant changes made to kubernetes in master onto v1-10-test? anything to add to UPDATING.md after we pull master and finish this PR? how/where is your branch structure documented (flow to pick commits for releases etc)?
cc #6506 (comment)

potiuk · 2019-11-08T12:27:53Z

Hey @ddelange. I saw your comment - while writing this. Cool that you are working on it :).
The process we have now is that we want to make sure 1.10.* does not diverge from master so we implement all the changes as PRs to master and cherry-pick them (committers do it usually) if we decide to back-port them to v1-10-test and then when tests pass we move then to v1-10-stable branch. However we must make sure that those changes are backwards compatible in 1.10 (unless we have very good reason to drop compatibility).

So if those changes are based on some existing commit in master it's better to pin-point this and cherry pick from there. If those are new changes, they should be added as PR to master and cherry-picked after it gets merged in master.

I think it's not described very well, but we are just in a process of updating contributor's documentation with Google Season of Docs and I think it's a good point to add to CONTRIBUTING.rst (@efedotova -> maybe we can add it to the workflows we were discussing about)

ddelange · 2019-11-08T12:48:52Z

Hi @potiuk, thanks for the swift reply. I can open a new PR based on master, but master is too different from this branch to test my v1.10 setup with these changes on our cluster (e.g. my pods won't even start because https://github.com/apache/airflow/blob/v1-10-test/airflow/models/user.py doesn't exist anymore in master) [?]

To me it seems they have already diverged (master __version__ is v2.0 after all) and I'm not sure how those cherry picks would turn out. E.g. I also added a fix (base_operator **resources) in this PR which is already (#6331 ) on master but not in v1-test, but should definitely be in v1.10.7

I could alternatively make two PR's, one main one based on master and merged separately, and make this PR a patch that keeps changes to a minimum (with compatibility in mind). Please advise :)

potiuk · 2019-11-08T12:50:25Z

Two PRs with the same JIRA Issue are also fine @ddelange ! Thanks for doing this!

ddelange · 2019-11-08T13:39:58Z

Opened up #6524 @potiuk
in the meantime, CI failed:

fatal: ambiguous argument 'v1-10-test...HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
The command "./scripts/ci/ci_before_install.sh" failed and exited with 128 during .

ddelange · 2019-11-10T12:34:37Z

On my local machine I get the same error as Travis. When I add origin/ to the command it works on my machine, but I get the same error as below on Travis. @potiuk how can I get Travis to run for this branch?

$ git diff --name-only v1-10-test...HEAD
fatal: ambiguous argument 'v1-10-test...HEAD': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

$ git diff --name-only origin/v1-10-test...HEAD
.pre-commit-config.yaml
.travis.yml
BREEZE.rst
Dockerfile
Dockerfile-checklicence
airflow/contrib/kubernetes/pod.py
airflow/contrib/operators/kubernetes_pod_operator.py
airflow/models/baseoperator.py
airflow/utils/helpers.py
breeze
common/_files_for_rebuild_check.sh
docs/howto/custom-operator.rst
hooks/build
scripts/ci/_utils.sh
scripts/ci/ci_check_license.sh
scripts/ci/ci_docs.sh
scripts/ci/ci_flake8.sh
scripts/ci/ci_mypy.sh
scripts/ci/ci_refresh_pylint_todo.sh
scripts/ci/ci_run_all_static_tests.sh
scripts/ci/ci_run_all_static_tests_except_licence.sh
scripts/ci/docker-compose.yml
scripts/ci/docker_build/ci_build_install_deps.sh
scripts/ci/in_container/entrypoint_ci.sh
scripts/ci/local_ci_build_ci_slim_image.sh
scripts/ci/local_ci_cleanup.sh
scripts/ci/pre_commit_check_license.sh
scripts/ci/pre_commit_ci_build.sh
scripts/ci/pre_commit_flake8.sh
scripts/ci/pre_commit_lint_dockerfile.sh
scripts/ci/pre_commit_mypy.sh
scripts/ci/pre_commit_pylint_main.sh
scripts/ci/pre_commit_pylint_tests.sh
tests/contrib/operators/test_kubernetes_pod_operator.py
tests/test_impersonation.py```

scripts/ci/_utils.sh

- `xcom_push` will be depracated, for now used as `do_xcom_push` - `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to False in comparison to `default_args.py`, also default `do_xcom_push` was overwritten to False in contradiction to `BaseOperator` - `KubernetesPodOperator` kwarg `resources` is erroneously passed to `base_operator`, instead should only go to `PodGenerator`. The two have different syntax. (both on `master` and `v1-10-test` branches) - `kubernetes/pod.py`: `Resources` does not have `__slots__` so accepts arbitrary values in `setattr`

potiuk · 2019-11-12T14:11:58Z

Thanks @ddelange !

- `xcom_push` will be depracated, for now used as `do_xcom_push` - `KubernetesPodOperator` kwarg `in_cluster` erroneously defaults to False in comparison to `default_args.py`, also default `do_xcom_push` was overwritten to False in contradiction to `BaseOperator` - `KubernetesPodOperator` kwarg `resources` is erroneously passed to `base_operator`, instead should only go to `PodGenerator`. The two have different syntax. (both on `master` and `v1-10-test` branches) - `kubernetes/pod.py`: `Resources` does not have `__slots__` so accepts arbitrary values in `setattr`

dsaiztc · 2020-05-08T18:00:08Z

airflow/contrib/operators/kubernetes_pod_operator.py

+        return Resources(**resources) if resources else Resources()
+
+    def _set_name(self, name):
+        validate_key(name, max_length=63)


Hi,

I've just upgraded from v.1.10.6 and thus this change has been included, breaking a lot of the DAGs we currently have setup due to this change.
I hugely appreciate the effort in this development, although fair warning of this breaking change would have been very appreciated.

Just leaving this here in case someone using KubernetesPodOperator finds the same issue when upgrading.

... File "/usr/local/lib/python3.6/site-packages/airflow/contrib/operators/kubernetes_pod_operator.py", line 167, in __init__ self.name = self._set_name(name) File "/usr/local/lib/python3.6/site-packages/airflow/contrib/operators/kubernetes_pod_operator.py", line 269, in _set_name validate_key(name, max_length=63) File "/usr/local/lib/python3.6/site-packages/airflow/utils/helpers.py", line 64, in validate_key "The key has to be less than {0} characters".format(max_length)) airflow.exceptions.AirflowException: The key has to be less than 63 characters

I understand Kubernetes has a character limit of 63 for its label's names but, at least in v1.10.6 (the latest version previous to this change) Airflow adds another 9 characters to the name, so I'm guessing the limit to set here would be 54, right?

Please let me know if I'm missing something.

Again, thanks a lot for the work on this, I mean this as constructive feedback.

Interesting that it's a breaking change, I was in the understanding that it would fail downstream and hence was a good check to move upstream to point of DAG creation. If this causes reproducible errors (do you have a MWE @dsaiztc with a breaking change?) that didn't occur before, it should either be loosened and for sure mentioned in UPDATING.md, or fixed to incorporate the 9 characters as mentioned above. What do you say @potiuk?

Sorry, I'm not an expert in Kubernetes.

I was creating the POD names in KubernetesPodOperator just concatenating the DAG name with the task name and haven't had any issue whatsoever. Suddenly when updating Airflow I started to see how many of my DAGs wouldn't be working as they didn't pass the newly-added validation.

Might we be confusing the limit on the labels (63 characters) and the subdomains (253 characters)?

I cannot get a clear view reading the docs:

Object Names and IDs

Identifiers and Names in Kubernetes

Also the unofficial Kubernetes mentions a limit of 253 characters.

Hey @ddelange, I can confirm that the limit for the POD name is 253 characters and not 63.

Tried out with one of my colleagues in our k8s cluster, only giving an error when trying to create a POD with more than 253 characters:

The Pod "eot5ahm9ua4ocahmahghoma1xoh7neihuvohz3oofaiz1iesaet2eibuwee5didietheet7faelohceyo1ci2eithu9nutie5fee5ahhohy0haegaikeizohngahzahteseifo5au2eb0aizi7reph9eibo5sahlee7hiequ2aeko6yiam8ieca4si6hodeiquoh6ceu9ienge2ooh4uo2umaijaec4aeli4ohfeodie3xahkove2iogieno2i" is invalid: metadata.name: Invalid value: "eot5ahm9ua4ocahmahghoma1xoh7neihuvohz3oofaiz1iesaet2eibuwee5didietheet7faelohceyo1ci2eithu9nutie5fee5ahhohy0haegaikeizohngahzahteseifo5au2eb0aizi7reph9eibo5sahlee7hiequ2aeko6yiam8ieca4si6hodeiquoh6ceu9ienge2ooh4uo2umaijaec4aeli4ohfeodie3xahkove2iogieno2i": must be no more than 253 characters

dsaiztc · 2020-05-11T14:48:22Z

airflow/contrib/operators/kubernetes_pod_operator.py

+    def _set_name(self, name):
+        validate_key(name, max_length=63)


So, with all that into consideration I'd suggest the following change:

Suggested change

def _set_name(self, name):

validate_key(name, max_length=63)

validate_key(name, max_length=244)

Considering a maximum char limit of 253 (244 plus the 9 characters mentioned in my first comment).

Thoughts?

Good catch! Agreed :) Do you wanna make a PR to master? Preferably with a small Jira ticket so it can be cherry-picked to 1-10?

I can give it a try... (never done this before)
Create a JIRA ticket + PR?
Should I base the PR in branch v1-10-stable?

Please base it on master (v2.0) - new releases for the v1 branches are mostly commits cherry-picked from master onto the 1-10-test and later the 1-10-stable branches

Not sure if it's complete enough, it's the first time I contribute (bear with me 🙈): #8829

Any feedback is welcome!

ddelange force-pushed the patch-2 branch from 8d495a5 to 2a1e5b3 Compare November 8, 2019 13:14

ddelange marked this pull request as ready for review November 8, 2019 13:15

ddelange mentioned this pull request Nov 8, 2019

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6524

Merged

2 tasks

ddelange force-pushed the patch-2 branch 4 times, most recently from e6cdd27 to 717dac6 Compare November 10, 2019 12:27

ddelange force-pushed the patch-2 branch 13 times, most recently from 40747f1 to b4a6c9c Compare November 11, 2019 15:32

ddelange commented Nov 11, 2019

View reviewed changes

scripts/ci/_utils.sh Show resolved Hide resolved

ddelange force-pushed the patch-2 branch 2 times, most recently from 0a772f2 to 762cdb9 Compare November 11, 2019 16:53

potiuk force-pushed the v1-10-test branch from 97ff92a to 10534a1 Compare November 11, 2019 18:47

ddelange force-pushed the patch-2 branch 2 times, most recently from 8e28275 to 265a42f Compare November 12, 2019 08:40

ddelange force-pushed the patch-2 branch from 265a42f to 331e9ad Compare November 12, 2019 09:37

potiuk approved these changes Nov 12, 2019

View reviewed changes

potiuk merged this pull request into apache:v1-10-test Nov 12, 2019

dsaiztc reviewed May 8, 2020

View reviewed changes

dsaiztc reviewed May 11, 2020

View reviewed changes

dsaiztc mentioned this pull request May 11, 2020

Fix KubernetesPodOperator pod name length validation #8829

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6523

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6523

ddelange commented Nov 8, 2019 •

edited

Loading

ddelange commented Nov 8, 2019 •

edited

Loading

potiuk commented Nov 8, 2019

ddelange commented Nov 8, 2019 •

edited

Loading

potiuk commented Nov 8, 2019

ddelange commented Nov 8, 2019 •

edited

Loading

ddelange commented Nov 10, 2019 •

edited

Loading

potiuk commented Nov 12, 2019

dsaiztc May 8, 2020 •

edited

Loading

ddelange May 8, 2020 •

edited

Loading

dsaiztc May 10, 2020

dsaiztc May 11, 2020

dsaiztc May 11, 2020

ddelange May 11, 2020

dsaiztc May 11, 2020

ddelange May 11, 2020

dsaiztc May 11, 2020

	def _set_name(self, name):
	validate_key(name, max_length=63)
	validate_key(name, max_length=244)

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6523

[AIRFLOW-5873] KubernetesPodOperator fixes and test #6523

Conversation

ddelange commented Nov 8, 2019 • edited Loading

Jira

Description

Tests

ddelange commented Nov 8, 2019 • edited Loading

potiuk commented Nov 8, 2019

ddelange commented Nov 8, 2019 • edited Loading

potiuk commented Nov 8, 2019

ddelange commented Nov 8, 2019 • edited Loading

ddelange commented Nov 10, 2019 • edited Loading

potiuk commented Nov 12, 2019

dsaiztc May 8, 2020 • edited Loading

Choose a reason for hiding this comment

ddelange May 8, 2020 • edited Loading

Choose a reason for hiding this comment

dsaiztc May 10, 2020

Choose a reason for hiding this comment

dsaiztc May 11, 2020

Choose a reason for hiding this comment

dsaiztc May 11, 2020

Choose a reason for hiding this comment

ddelange May 11, 2020

Choose a reason for hiding this comment

dsaiztc May 11, 2020

Choose a reason for hiding this comment

ddelange May 11, 2020

Choose a reason for hiding this comment

dsaiztc May 11, 2020

Choose a reason for hiding this comment

ddelange commented Nov 8, 2019 •

edited

Loading

ddelange commented Nov 8, 2019 •

edited

Loading

ddelange commented Nov 8, 2019 •

edited

Loading

ddelange commented Nov 8, 2019 •

edited

Loading

ddelange commented Nov 10, 2019 •

edited

Loading

dsaiztc May 8, 2020 •

edited

Loading

ddelange May 8, 2020 •

edited

Loading