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

Expand KPO template_fields, fix Spark k8s operator tests #46268

Merged
merged 6 commits into from
Feb 1, 2025

Conversation

insomnes
Copy link
Contributor

@insomnes insomnes commented Jan 30, 2025

Adding name and hostname to KubernetesPodOperator template_fields

  • Move name validation and normalization inside execute method
  • Expand KubernetesPodOperator tests
  • Refactor create application SparkKubernetesOperator due to bug in tests
  • Fix kubernetes_tests/test_kubernetes_pod_operator.py too long name testing flow and add this case to unit tests

closes: #43480
Based on previously staled PR


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@insomnes
Copy link
Contributor Author

insomnes commented Jan 30, 2025

I was interested in this feature so I've bring back staled PR with some extra tests, maybe we can succeed with it this time

@insomnes
Copy link
Contributor Author

I think, I've found the bug in spark k8s operator test code. Will check and try to fix for affected test and bring the bug issue to fix any other occurrences

@insomnes
Copy link
Contributor Author

insomnes commented Jan 31, 2025

The mentioned bug in the spark k8s operator test is related to mocking and new changes.

Previously, tests were failed because the new KPO execute() validates the name set for a pod on an operator by calling _set_name (because of templating changes, we need to have context applied to templates before name validation).
Thus the tests in k8s spark operator with mock_create_job_name could not succeed. MagickMock is not a str instance, so validation failed.

This led me to understand that calls to assert op.name.startswith("...") are broken in these tests, otherwise, they would fail too (this is checked after operator execution). My investigation showed that changing prefix to anything like name.startswith("ABRA-CADABRA") didn't lead to test fails. Functions with applied unittest.patch actually return MagicMock object, so the op.name was a MagicMock instance in these tests.

Any method call on instance of MagickMock doesn't fail by design and returns another MagicMock object. They lead to positive assertions. That's why I have refactored affected spark k8s operator tests. I've extracted create_application tests to a separate class where create_job_name is not patched.

I believe that all affected tests are fixed now. So there is no need for a new issue.

@insomnes insomnes changed the title Add name and hostname to KPO template_fields Add name to KPO template_fields Jan 31, 2025
@insomnes insomnes changed the title Add name to KPO template_fields Add name and hostname to KPO template_fields Jan 31, 2025
@insomnes insomnes changed the title Add name and hostname to KPO template_fields Expand KPO template_fields, fix Spark k8s operator tests Jan 31, 2025
@shahar1
Copy link
Contributor

shahar1 commented Feb 1, 2025

LGTM!

@shahar1 shahar1 merged commit 6235002 into apache:main Feb 1, 2025
71 checks passed
@insomnes insomnes deleted the kpo-name-template branch February 1, 2025 20:51
amoghrajesh pushed a commit to astronomer/airflow that referenced this pull request Feb 3, 2025
* Add name and hostname to KPO template_fields

* Add fetch container mock to name normalization test

* Fix bugged tests

* Run execute in test_pod_name to get validation fail

* Add long name case to KPO unit tests
dabla pushed a commit to dabla/airflow that referenced this pull request Feb 3, 2025
* Add name and hostname to KPO template_fields

* Add fetch container mock to name normalization test

* Fix bugged tests

* Run execute in test_pod_name to get validation fail

* Add long name case to KPO unit tests
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.

Add KubernetesPodOperator's name in templated fields
2 participants